Re: [PATCH] firewire-ohci: work around oversized DMA reads on JMicron controllers

2017-11-03 Thread Clemens Ladisch
Hector Martin wrote:
> At least some JMicron controllers issue buggy oversized DMA reads when
> fetching context descriptors, always fetching 0x20 bytes at once for
> descriptors which are only 0x10 bytes long. This is often harmless, but
> can cause page faults on modern systems with IOMMUs:
>
> DMAR: [DMA Read] Request device [05:00.0] fault addr fff56000 [fault reason 
> 06] PTE Read access is not set
> firewire_ohci :05:00.0: DMA context IT0 has stopped, error code: 
> evt_descriptor_read
>
> This works around the problem by always leaving 0x10 padding bytes at
> the end of descriptor buffer pages, which should be harmless to do
> unconditionally for controllers in case others have the same behavior.
>
> Signed-off-by: Hector Martin <mar...@marcan.st>

Reviewed-by: Clemens Ladisch <clem...@ladisch.de>

> ---
>  drivers/firewire/ohci.c | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
> index 8bf89267dc25..d731b413cb2c 100644
> --- a/drivers/firewire/ohci.c
> +++ b/drivers/firewire/ohci.c
> @@ -1130,7 +1130,13 @@ static int context_add_buffer(struct context *ctx)
>   return -ENOMEM;
>
>   offset = (void *)>buffer - (void *)desc;
> - desc->buffer_size = PAGE_SIZE - offset;
> + /*
> +  * Some controllers, like JMicron ones, always issue 0x20-byte DMA reads
> +  * for descriptors, even 0x10-byte ones. This can cause page faults when
> +  * an IOMMU is in use and the oversized read crosses a page boundary.
> +  * Work around this by always leaving at least 0x10 bytes of padding.
> +  */
> + desc->buffer_size = PAGE_SIZE - offset - 0x10;
>   desc->buffer_bus = bus_addr + offset;
>   desc->used = 0;
>


Re: [PATCH] firewire-ohci: work around oversized DMA reads on JMicron controllers

2017-11-03 Thread Clemens Ladisch
Hector Martin wrote:
> At least some JMicron controllers issue buggy oversized DMA reads when
> fetching context descriptors, always fetching 0x20 bytes at once for
> descriptors which are only 0x10 bytes long. This is often harmless, but
> can cause page faults on modern systems with IOMMUs:
>
> DMAR: [DMA Read] Request device [05:00.0] fault addr fff56000 [fault reason 
> 06] PTE Read access is not set
> firewire_ohci :05:00.0: DMA context IT0 has stopped, error code: 
> evt_descriptor_read
>
> This works around the problem by always leaving 0x10 padding bytes at
> the end of descriptor buffer pages, which should be harmless to do
> unconditionally for controllers in case others have the same behavior.
>
> Signed-off-by: Hector Martin 

Reviewed-by: Clemens Ladisch 

> ---
>  drivers/firewire/ohci.c | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c
> index 8bf89267dc25..d731b413cb2c 100644
> --- a/drivers/firewire/ohci.c
> +++ b/drivers/firewire/ohci.c
> @@ -1130,7 +1130,13 @@ static int context_add_buffer(struct context *ctx)
>   return -ENOMEM;
>
>   offset = (void *)>buffer - (void *)desc;
> - desc->buffer_size = PAGE_SIZE - offset;
> + /*
> +  * Some controllers, like JMicron ones, always issue 0x20-byte DMA reads
> +  * for descriptors, even 0x10-byte ones. This can cause page faults when
> +  * an IOMMU is in use and the oversized read crosses a page boundary.
> +  * Work around this by always leaving at least 0x10 bytes of padding.
> +  */
> + desc->buffer_size = PAGE_SIZE - offset - 0x10;
>   desc->buffer_bus = bus_addr + offset;
>   desc->used = 0;
>


Re: [alsa-devel] [PATCH] opl3: Fix a possible sleep-in-atomic bug in snd_opl3_find_patch

2017-10-09 Thread Clemens Ladisch
Jia-Ju Bai wrote:
> The driver may sleep under a spinlock, and the function call path is:
> snd_opl3_note_on (acquire the spinlock)
>   snd_opl3_find_patch
> kzalloc(GFP_KERNEL) --> may sleep

This call path is not possible because create_patch is not set.


Regards,
Clemens


Re: [alsa-devel] [PATCH] opl3: Fix a possible sleep-in-atomic bug in snd_opl3_find_patch

2017-10-09 Thread Clemens Ladisch
Jia-Ju Bai wrote:
> The driver may sleep under a spinlock, and the function call path is:
> snd_opl3_note_on (acquire the spinlock)
>   snd_opl3_find_patch
> kzalloc(GFP_KERNEL) --> may sleep

This call path is not possible because create_patch is not set.


Regards,
Clemens


Re: [alsa-devel] [PATCH] ALSA: oxygen: Xonar DG(X): make model_xonar_dg const

2017-09-14 Thread Clemens Ladisch
Bhumika Goyal wrote:
> Make this const as it not modified anywhere. It is only used during a
> copy operation. Also, add const to the declaration in header.
>
> Signed-off-by: Bhumika Goyal <bhumi...@gmail.com>

Acked-by: Clemens Ladisch <clem...@ladisch.de>

> ---
>  sound/pci/oxygen/xonar_dg.h   | 2 +-
>  sound/pci/oxygen/xonar_dg_mixer.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/sound/pci/oxygen/xonar_dg.h b/sound/pci/oxygen/xonar_dg.h
> index d461df3..5a07cda 100644
> --- a/sound/pci/oxygen/xonar_dg.h
> +++ b/sound/pci/oxygen/xonar_dg.h
> @@ -51,6 +51,6 @@ void dump_cs4245_registers(struct oxygen *chip,
>  void dg_resume(struct oxygen *chip);
>  void dg_cleanup(struct oxygen *chip);
>
> -extern struct oxygen_model model_xonar_dg;
> +extern const struct oxygen_model model_xonar_dg;
>
>  #endif
> diff --git a/sound/pci/oxygen/xonar_dg_mixer.c 
> b/sound/pci/oxygen/xonar_dg_mixer.c
> index b885dac..d22fbe8 100644
> --- a/sound/pci/oxygen/xonar_dg_mixer.c
> +++ b/sound/pci/oxygen/xonar_dg_mixer.c
> @@ -449,7 +449,7 @@ static int dg_mixer_init(struct oxygen *chip)
>   return 0;
>  }
>
> -struct oxygen_model model_xonar_dg = {
> +const struct oxygen_model model_xonar_dg = {
>   .longname = "C-Media Oxygen HD Audio",
>   .chip = "CMI8786",
>   .init = dg_init,


Re: [alsa-devel] [PATCH] ALSA: oxygen: Xonar DG(X): make model_xonar_dg const

2017-09-14 Thread Clemens Ladisch
Bhumika Goyal wrote:
> Make this const as it not modified anywhere. It is only used during a
> copy operation. Also, add const to the declaration in header.
>
> Signed-off-by: Bhumika Goyal 

Acked-by: Clemens Ladisch 

> ---
>  sound/pci/oxygen/xonar_dg.h   | 2 +-
>  sound/pci/oxygen/xonar_dg_mixer.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/sound/pci/oxygen/xonar_dg.h b/sound/pci/oxygen/xonar_dg.h
> index d461df3..5a07cda 100644
> --- a/sound/pci/oxygen/xonar_dg.h
> +++ b/sound/pci/oxygen/xonar_dg.h
> @@ -51,6 +51,6 @@ void dump_cs4245_registers(struct oxygen *chip,
>  void dg_resume(struct oxygen *chip);
>  void dg_cleanup(struct oxygen *chip);
>
> -extern struct oxygen_model model_xonar_dg;
> +extern const struct oxygen_model model_xonar_dg;
>
>  #endif
> diff --git a/sound/pci/oxygen/xonar_dg_mixer.c 
> b/sound/pci/oxygen/xonar_dg_mixer.c
> index b885dac..d22fbe8 100644
> --- a/sound/pci/oxygen/xonar_dg_mixer.c
> +++ b/sound/pci/oxygen/xonar_dg_mixer.c
> @@ -449,7 +449,7 @@ static int dg_mixer_init(struct oxygen *chip)
>   return 0;
>  }
>
> -struct oxygen_model model_xonar_dg = {
> +const struct oxygen_model model_xonar_dg = {
>   .longname = "C-Media Oxygen HD Audio",
>   .chip = "CMI8786",
>   .init = dg_init,


Re: [PATCH] hwmon: (k10temp) Add support for family 17h

2017-09-05 Thread Clemens Ladisch
Guenter Roeck wrote:
> On Tue, Sep 05, 2017 at 04:12:07PM +0200, Clemens Ladisch wrote:
>> Guenter Roeck wrote:
>>> What we should do then, as we did for coretemp, would be to collect the 
>>> various
>>> temperature offsets (and temperature limits, for that matter) and apply 
>>> per-CPU
>>> adjustments. Are the offsets documented somewhere ?
>>
>> AMD says:
>>  "Tctl is the processor temperature control value, used by the platform to
>>   control cooling systems. Tctl is a non-physical temperature on an
>>   arbitrary scale measured in degrees. It does _not_ represent an actual
>>   physical temperature like die or case temperature. Instead, it specifies
>>   the processor temperature relative to the point at which the system must
>>   supply the maximum cooling for the processor's specified maximum case
>>   temperature and maximum thermal power dissipation."
>
> Pretty much the same as Intel. That doesn't mean we should not (try to) report
> the real temperature as good as we can, as at least most of the BIOSes do,

AFAIK the BIOSes use the thermal diode (with external circuitry) for that.

> and as all the Windows tools do, and as users expect us to do.
>
> Do we really have to argue about this ?

Looking at coretemp, this is going to be a maintenance nightmare.

Oh well.  If you insist, please add a proper chip-to-offset database, and
apply the offset to all four values.


Regards,
Clemens


Re: [PATCH] hwmon: (k10temp) Add support for family 17h

2017-09-05 Thread Clemens Ladisch
Guenter Roeck wrote:
> On Tue, Sep 05, 2017 at 04:12:07PM +0200, Clemens Ladisch wrote:
>> Guenter Roeck wrote:
>>> What we should do then, as we did for coretemp, would be to collect the 
>>> various
>>> temperature offsets (and temperature limits, for that matter) and apply 
>>> per-CPU
>>> adjustments. Are the offsets documented somewhere ?
>>
>> AMD says:
>>  "Tctl is the processor temperature control value, used by the platform to
>>   control cooling systems. Tctl is a non-physical temperature on an
>>   arbitrary scale measured in degrees. It does _not_ represent an actual
>>   physical temperature like die or case temperature. Instead, it specifies
>>   the processor temperature relative to the point at which the system must
>>   supply the maximum cooling for the processor's specified maximum case
>>   temperature and maximum thermal power dissipation."
>
> Pretty much the same as Intel. That doesn't mean we should not (try to) report
> the real temperature as good as we can, as at least most of the BIOSes do,

AFAIK the BIOSes use the thermal diode (with external circuitry) for that.

> and as all the Windows tools do, and as users expect us to do.
>
> Do we really have to argue about this ?

Looking at coretemp, this is going to be a maintenance nightmare.

Oh well.  If you insist, please add a proper chip-to-offset database, and
apply the offset to all four values.


Regards,
Clemens


Re: [PATCH] hwmon: (k10temp) Add support for family 17h

2017-09-05 Thread Clemens Ladisch
Guenter Roeck wrote:
> On 09/04/2017 11:47 PM, Clemens Ladisch wrote:
>> Guenter Roeck wrote:
>>> Some of this is guesswork, but afaics it is working. No idea if there
>>> is a better way to determine the temperature offset.
>>
>> The reported value is not an absolute temperature on any CPU.
>>
>> As far as I know, the offset is not guaranteed to be fixed for any model,
>> i.e., it would be pointless to apply the offset observed on one specific
>> chip.
>
> What we should do then, as we did for coretemp, would be to collect the 
> various
> temperature offsets (and temperature limits, for that matter) and apply 
> per-CPU
> adjustments. Are the offsets documented somewhere ?

AMD says:
 "Tctl is the processor temperature control value, used by the platform to
  control cooling systems. Tctl is a non-physical temperature on an
  arbitrary scale measured in degrees. It does _not_ represent an actual
  physical temperature like die or case temperature. Instead, it specifies
  the processor temperature relative to the point at which the system must
  supply the maximum cooling for the processor's specified maximum case
  temperature and maximum thermal power dissipation."

(That point is defined as 70.)


Regards,
Clemens


Re: [PATCH] hwmon: (k10temp) Add support for family 17h

2017-09-05 Thread Clemens Ladisch
Guenter Roeck wrote:
> On 09/04/2017 11:47 PM, Clemens Ladisch wrote:
>> Guenter Roeck wrote:
>>> Some of this is guesswork, but afaics it is working. No idea if there
>>> is a better way to determine the temperature offset.
>>
>> The reported value is not an absolute temperature on any CPU.
>>
>> As far as I know, the offset is not guaranteed to be fixed for any model,
>> i.e., it would be pointless to apply the offset observed on one specific
>> chip.
>
> What we should do then, as we did for coretemp, would be to collect the 
> various
> temperature offsets (and temperature limits, for that matter) and apply 
> per-CPU
> adjustments. Are the offsets documented somewhere ?

AMD says:
 "Tctl is the processor temperature control value, used by the platform to
  control cooling systems. Tctl is a non-physical temperature on an
  arbitrary scale measured in degrees. It does _not_ represent an actual
  physical temperature like die or case temperature. Instead, it specifies
  the processor temperature relative to the point at which the system must
  supply the maximum cooling for the processor's specified maximum case
  temperature and maximum thermal power dissipation."

(That point is defined as 70.)


Regards,
Clemens


Re: [alsa-devel] [PATCH RESEND1 00/12] ALSA: vsnd: Add Xen para-virtualized frontend driver

2017-09-05 Thread Clemens Ladisch
Oleksandr Andrushchenko wrote:
>>> We understand that emulated interrupt on the frontend side is completely not
>>> acceptable

Allow me to expand on that:  Proper synchronization requires that the
exact position is communicated, not estimated.  Just because the nominal
rate of the stream is known does not imply that you know the actual rate.
Forget for the moment that there even is a nominal rate; assume that it
works like, e.g., a storage controller, and that you can know that a DMA
buffer was consumed by the device only after it has told you.

It's possible and likely that there is a latency when reporting the
stream position, but that is still better than guessing what the DMA
is doing.  (You would never just try to guess when writing data to
disk, would you?)

>>> and definitely we need to provide some feedback mechanism from
>>> Dom0 to DomU.
>>>
>>> In our case it is technically impossible to provide precise period interrupt
>>> (mostly because our backend is a user space application).

As far as I can see, all audio APIs (ALSA, PulseAudio, etc.) have poll()
or callbacks or similar mechanisms to inform you when new data can be
written, and always allow to query the current position.

> [...]
> ok, so the main concern here is that we cannot properly synchronize Dom0-DomU.
> If we put this apart for a second are there any other concerns on having ALSA
> frontend driver? If not, can we have the driver with timer implementation 
> upstreamed
> as experimental until we have some acceptable synchronization solution?
> This will allow broader audience to try and feel the solution and probably 
> contribute?

I doubt that the driver architecture will stay completely the same, so I
do not think that this experimental driver would demonstrate how the
solution would feel.

As the first step, I would suggest creating a driver with proper
synchronization, even if it has high latency.  Reducing the latency
would then be 'just' an optimization.


Regards,
Clemens


Re: [alsa-devel] [PATCH RESEND1 00/12] ALSA: vsnd: Add Xen para-virtualized frontend driver

2017-09-05 Thread Clemens Ladisch
Oleksandr Andrushchenko wrote:
>>> We understand that emulated interrupt on the frontend side is completely not
>>> acceptable

Allow me to expand on that:  Proper synchronization requires that the
exact position is communicated, not estimated.  Just because the nominal
rate of the stream is known does not imply that you know the actual rate.
Forget for the moment that there even is a nominal rate; assume that it
works like, e.g., a storage controller, and that you can know that a DMA
buffer was consumed by the device only after it has told you.

It's possible and likely that there is a latency when reporting the
stream position, but that is still better than guessing what the DMA
is doing.  (You would never just try to guess when writing data to
disk, would you?)

>>> and definitely we need to provide some feedback mechanism from
>>> Dom0 to DomU.
>>>
>>> In our case it is technically impossible to provide precise period interrupt
>>> (mostly because our backend is a user space application).

As far as I can see, all audio APIs (ALSA, PulseAudio, etc.) have poll()
or callbacks or similar mechanisms to inform you when new data can be
written, and always allow to query the current position.

> [...]
> ok, so the main concern here is that we cannot properly synchronize Dom0-DomU.
> If we put this apart for a second are there any other concerns on having ALSA
> frontend driver? If not, can we have the driver with timer implementation 
> upstreamed
> as experimental until we have some acceptable synchronization solution?
> This will allow broader audience to try and feel the solution and probably 
> contribute?

I doubt that the driver architecture will stay completely the same, so I
do not think that this experimental driver would demonstrate how the
solution would feel.

As the first step, I would suggest creating a driver with proper
synchronization, even if it has high latency.  Reducing the latency
would then be 'just' an optimization.


Regards,
Clemens


Re: [PATCH] hwmon: (k10temp) Add support for family 17h

2017-09-05 Thread Clemens Ladisch
Guenter Roeck wrote:
> Add support for temperature sensors on Family 17h (Ryzen) processors.
>
> Signed-off-by: Guenter Roeck 
> ---
> Some of this is guesswork, but afaics it is working. No idea if there
> is a better way to determine the temperature offset.

The reported value is not an absolute temperature on any CPU.

As far as I know, the offset is not guaranteed to be fixed for any model,
i.e., it would be pointless to apply the offset observed on one specific
chip.

> @@ -25,6 +25,10 @@
>  #include 
>  #include 
>
> +#ifndef PCI_DEVICE_ID_AMD_17H_DF_F3
> +#define PCI_DEVICE_ID_AMD_17H_DF_F3  0x1463
> +#endif
> +

Please move this down to the other symbols.


Regards,
Clemens


Re: [PATCH] hwmon: (k10temp) Add support for family 17h

2017-09-05 Thread Clemens Ladisch
Guenter Roeck wrote:
> Add support for temperature sensors on Family 17h (Ryzen) processors.
>
> Signed-off-by: Guenter Roeck 
> ---
> Some of this is guesswork, but afaics it is working. No idea if there
> is a better way to determine the temperature offset.

The reported value is not an absolute temperature on any CPU.

As far as I know, the offset is not guaranteed to be fixed for any model,
i.e., it would be pointless to apply the offset observed on one specific
chip.

> @@ -25,6 +25,10 @@
>  #include 
>  #include 
>
> +#ifndef PCI_DEVICE_ID_AMD_17H_DF_F3
> +#define PCI_DEVICE_ID_AMD_17H_DF_F3  0x1463
> +#endif
> +

Please move this down to the other symbols.


Regards,
Clemens


Re: [alsa-devel] [PATCH] ALSA: USB-MIDI: Use common error handling code in __snd_usbmidi_create()

2017-08-23 Thread Clemens Ladisch
SF Markus Elfring wrote:
> Add jump targets so that a bit of exception handling can be better reused
> at the end of this function.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <elfr...@users.sourceforge.net>

Acked-by: Clemens Ladisch <clem...@ladisch.de>

> ---
>  sound/usb/midi.c | 22 +++---
>  1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/sound/usb/midi.c b/sound/usb/midi.c
> index a35f41467237..bd9d02268724 100644
> --- a/sound/usb/midi.c
> +++ b/sound/usb/midi.c
> @@ -2435,10 +2435,8 @@ int __snd_usbmidi_create(struct snd_card *card,
>   err = -ENXIO;
>   break;
>   }
> - if (err < 0) {
> - kfree(umidi);
> - return err;
> - }
> + if (err < 0)
> + goto free_midi;
>
>   /* create rawmidi device */
>   out_ports = 0;
> @@ -2448,23 +2446,25 @@ int __snd_usbmidi_create(struct snd_card *card,
>   in_ports += hweight16(endpoints[i].in_cables);
>   }
>   err = snd_usbmidi_create_rawmidi(umidi, out_ports, in_ports);
> - if (err < 0) {
> - kfree(umidi);
> - return err;
> - }
> + if (err < 0)
> + goto free_midi;
>
>   /* create endpoint/port structures */
>   if (quirk && quirk->type == QUIRK_MIDI_MIDIMAN)
>   err = snd_usbmidi_create_endpoints_midiman(umidi, 
> [0]);
>   else
>   err = snd_usbmidi_create_endpoints(umidi, endpoints);
> - if (err < 0) {
> - return err;
> - }
> + if (err < 0)
> + goto exit;
>
>   usb_autopm_get_interface_no_resume(umidi->iface);
>
>   list_add_tail(>list, midi_list);
>   return 0;
> +
> +free_midi:
> + kfree(umidi);
> +exit:
> + return err;
>  }
>  EXPORT_SYMBOL(__snd_usbmidi_create);
>


Re: [alsa-devel] [PATCH] ALSA: USB-MIDI: Use common error handling code in __snd_usbmidi_create()

2017-08-23 Thread Clemens Ladisch
SF Markus Elfring wrote:
> Add jump targets so that a bit of exception handling can be better reused
> at the end of this function.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring 

Acked-by: Clemens Ladisch 

> ---
>  sound/usb/midi.c | 22 +++---
>  1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/sound/usb/midi.c b/sound/usb/midi.c
> index a35f41467237..bd9d02268724 100644
> --- a/sound/usb/midi.c
> +++ b/sound/usb/midi.c
> @@ -2435,10 +2435,8 @@ int __snd_usbmidi_create(struct snd_card *card,
>   err = -ENXIO;
>   break;
>   }
> - if (err < 0) {
> - kfree(umidi);
> - return err;
> - }
> + if (err < 0)
> + goto free_midi;
>
>   /* create rawmidi device */
>   out_ports = 0;
> @@ -2448,23 +2446,25 @@ int __snd_usbmidi_create(struct snd_card *card,
>   in_ports += hweight16(endpoints[i].in_cables);
>   }
>   err = snd_usbmidi_create_rawmidi(umidi, out_ports, in_ports);
> - if (err < 0) {
> - kfree(umidi);
> - return err;
> - }
> + if (err < 0)
> + goto free_midi;
>
>   /* create endpoint/port structures */
>   if (quirk && quirk->type == QUIRK_MIDI_MIDIMAN)
>   err = snd_usbmidi_create_endpoints_midiman(umidi, 
> [0]);
>   else
>   err = snd_usbmidi_create_endpoints(umidi, endpoints);
> - if (err < 0) {
> - return err;
> - }
> + if (err < 0)
> + goto exit;
>
>   usb_autopm_get_interface_no_resume(umidi->iface);
>
>   list_add_tail(>list, midi_list);
>   return 0;
> +
> +free_midi:
> + kfree(umidi);
> +exit:
> + return err;
>  }
>  EXPORT_SYMBOL(__snd_usbmidi_create);
>


Re: [alsa-devel] [PATCH 08/11] ALSA: vsnd: Add timer for period interrupt emulation

2017-08-07 Thread Clemens Ladisch
Oleksandr Andrushchenko wrote:
> On 08/07/2017 04:11 PM, Clemens Ladisch wrote:
>> How does that interface work?
>
> For the buffer received in .copy_user/.copy_kernel we send
> a request to the backend and get response back (async) when it has copied
> the bytes into HW/mixer/etc, so the buffer at frontend side can be reused.

So if the frontend sends too many (too large) requests, does the
backend wait until there is enough free space in the buffer before
it does the actual copying and then acks?

If yes, then these acks can be used as interrupts.  (You still
have to count frames, and call snd_pcm_period_elapsed() exactly
when a period boundary was reached or crossed.)

Splitting a large read/write into smaller requests to the backend
would improve the granularity of the known stream position.

The overall latency would be the sum of the sizes of the frontend
and backend buffers.


Why is the protocol designed this way?  Wasn't the goal to expose
some 'real' sound card?


Regards,
Clemens


Re: [alsa-devel] [PATCH 08/11] ALSA: vsnd: Add timer for period interrupt emulation

2017-08-07 Thread Clemens Ladisch
Oleksandr Andrushchenko wrote:
> On 08/07/2017 04:11 PM, Clemens Ladisch wrote:
>> How does that interface work?
>
> For the buffer received in .copy_user/.copy_kernel we send
> a request to the backend and get response back (async) when it has copied
> the bytes into HW/mixer/etc, so the buffer at frontend side can be reused.

So if the frontend sends too many (too large) requests, does the
backend wait until there is enough free space in the buffer before
it does the actual copying and then acks?

If yes, then these acks can be used as interrupts.  (You still
have to count frames, and call snd_pcm_period_elapsed() exactly
when a period boundary was reached or crossed.)

Splitting a large read/write into smaller requests to the backend
would improve the granularity of the known stream position.

The overall latency would be the sum of the sizes of the frontend
and backend buffers.


Why is the protocol designed this way?  Wasn't the goal to expose
some 'real' sound card?


Regards,
Clemens


Re: [alsa-devel] [PATCH 08/11] ALSA: vsnd: Add timer for period interrupt emulation

2017-08-07 Thread Clemens Ladisch
Oleksandr Andrushchenko wrote:
> On 08/07/2017 01:27 PM, Clemens Ladisch wrote:
>> You have to implement period interrupts (and the .pointer callback)
>> based on when the samples are actually moved from/to the backend.
>
> Do you think I can implement this in a slightly different way,
> without a timer at all, by updating
> substream->runtime->hw_ptr_base explicitly in the frontend driver?

As far as I am aware, hw_ptr_base is an internal field that drivers
are not supposed to change.

Just use your own variable, and return it from the .pointer callback.

> So, that way, whenever I get an ack/response from the backend that it has
> successfully played the buffer

That response should come after every period.

How does that interface work?  Is it possible to change the period size,
or at least to detect what it is?


Regards,
Clemens


Re: [alsa-devel] [PATCH 08/11] ALSA: vsnd: Add timer for period interrupt emulation

2017-08-07 Thread Clemens Ladisch
Oleksandr Andrushchenko wrote:
> On 08/07/2017 01:27 PM, Clemens Ladisch wrote:
>> You have to implement period interrupts (and the .pointer callback)
>> based on when the samples are actually moved from/to the backend.
>
> Do you think I can implement this in a slightly different way,
> without a timer at all, by updating
> substream->runtime->hw_ptr_base explicitly in the frontend driver?

As far as I am aware, hw_ptr_base is an internal field that drivers
are not supposed to change.

Just use your own variable, and return it from the .pointer callback.

> So, that way, whenever I get an ack/response from the backend that it has
> successfully played the buffer

That response should come after every period.

How does that interface work?  Is it possible to change the period size,
or at least to detect what it is?


Regards,
Clemens


Re: [alsa-devel] [PATCH 08/11] ALSA: vsnd: Add timer for period interrupt emulation

2017-08-07 Thread Clemens Ladisch
Oleksandr Andrushchenko wrote:
> Front sound driver has no real interrupts, so
> playback/capture period passed interrupt needs to be emulated:
> this is done via timer. Add required timer operations,
> this is based on sound/drivers/dummy.c.

A 'real' sound card use the interrupt to synchronize the stream position
between the hardware and the driver.  The hardware triggers an interrupt
immediately after a period has been completely read (for playback) from
the ring buffer by the DMA unit; this tells the driver that it is now
again allowed to write to that part of the buffer.

The dummy driver has no hardware that accesses the buffer, so the period
interrupts are not synchronized to anything.  This is not a suitable
implementation when the samples are actually used.

If you issue interrupts based on the system timer, the position reported
by the .pointer callback and the position where the hardware (backend)
actually accesses the buffer will diverge, which will eventually corrupt
data.

You have to implement period interrupts (and the .pointer callback)
based on when the samples are actually moved from/to the backend.


Regards,
Clemens


Re: [alsa-devel] [PATCH 08/11] ALSA: vsnd: Add timer for period interrupt emulation

2017-08-07 Thread Clemens Ladisch
Oleksandr Andrushchenko wrote:
> Front sound driver has no real interrupts, so
> playback/capture period passed interrupt needs to be emulated:
> this is done via timer. Add required timer operations,
> this is based on sound/drivers/dummy.c.

A 'real' sound card use the interrupt to synchronize the stream position
between the hardware and the driver.  The hardware triggers an interrupt
immediately after a period has been completely read (for playback) from
the ring buffer by the DMA unit; this tells the driver that it is now
again allowed to write to that part of the buffer.

The dummy driver has no hardware that accesses the buffer, so the period
interrupts are not synchronized to anything.  This is not a suitable
implementation when the samples are actually used.

If you issue interrupts based on the system timer, the position reported
by the .pointer callback and the position where the hardware (backend)
actually accesses the buffer will diverge, which will eventually corrupt
data.

You have to implement period interrupts (and the .pointer callback)
based on when the samples are actually moved from/to the backend.


Regards,
Clemens


Re: [PATCH 18/18] ALSA: opl4: Move inline before return type

2017-07-05 Thread Clemens Ladisch
Joe Perches wrote:
> Make the code like the rest of the kernel.
>
> Signed-off-by: Joe Perches <j...@perches.com>

Acked-by: Clemens Ladisch <clem...@ladisch.de>

> ---
>  sound/drivers/opl4/opl4_lib.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sound/drivers/opl4/opl4_lib.c b/sound/drivers/opl4/opl4_lib.c
> index bc345d564f8d..db76a5bf2bd2 100644
> --- a/sound/drivers/opl4/opl4_lib.c
> +++ b/sound/drivers/opl4/opl4_lib.c
> @@ -29,7 +29,7 @@ MODULE_AUTHOR("Clemens Ladisch <clem...@ladisch.de>");
>  MODULE_DESCRIPTION("OPL4 driver");
>  MODULE_LICENSE("GPL");
>
> -static void inline snd_opl4_wait(struct snd_opl4 *opl4)
> +static inline void snd_opl4_wait(struct snd_opl4 *opl4)
>  {
>   int timeout = 10;
>   while ((inb(opl4->fm_port) & OPL4_STATUS_BUSY) && --timeout > 0)


Re: [PATCH 18/18] ALSA: opl4: Move inline before return type

2017-07-05 Thread Clemens Ladisch
Joe Perches wrote:
> Make the code like the rest of the kernel.
>
> Signed-off-by: Joe Perches 

Acked-by: Clemens Ladisch 

> ---
>  sound/drivers/opl4/opl4_lib.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sound/drivers/opl4/opl4_lib.c b/sound/drivers/opl4/opl4_lib.c
> index bc345d564f8d..db76a5bf2bd2 100644
> --- a/sound/drivers/opl4/opl4_lib.c
> +++ b/sound/drivers/opl4/opl4_lib.c
> @@ -29,7 +29,7 @@ MODULE_AUTHOR("Clemens Ladisch ");
>  MODULE_DESCRIPTION("OPL4 driver");
>  MODULE_LICENSE("GPL");
>
> -static void inline snd_opl4_wait(struct snd_opl4 *opl4)
> +static inline void snd_opl4_wait(struct snd_opl4 *opl4)
>  {
>   int timeout = 10;
>   while ((inb(opl4->fm_port) & OPL4_STATUS_BUSY) && --timeout > 0)


Re: HPET enabled in BIOS, not presented as available_clocksource -- config, kernel code, &/or BIOS?

2017-05-13 Thread Clemens Ladisch
Randy Dunlap wrote:
> On 05/12/17 19:30, PGNet Dev wrote:
>>  dmesg | grep -i hpet
>>  [8.491738] hpet_acpi_add: no address or irqs in _CRS
>
> Above line marks a big failure. 
> ^^^
>
>> Disassembling the firmware acpi tables
>>
>>  cat /sys/firmware/acpi/tables/HPET > /var/tmp/hpet.out
>>  iasl -d /var/tmp/hpet.out

That table is not used by hpet_acpi_add; you have to check for the device
that mentions "PNP0103" in the DSDT table.

But anyway, as far as I can tell from my own machine, the _CRS in the
DSDT table never lists the HPET interrupts, and the HPET registration is
always done by hpet_reserve_platform_timers() in arch/x86/kernel/hpet.c.
Try adding logging to hpet_late_init() to find out why it aborts.


Regards,
Clemens


Re: HPET enabled in BIOS, not presented as available_clocksource -- config, kernel code, &/or BIOS?

2017-05-13 Thread Clemens Ladisch
Randy Dunlap wrote:
> On 05/12/17 19:30, PGNet Dev wrote:
>>  dmesg | grep -i hpet
>>  [8.491738] hpet_acpi_add: no address or irqs in _CRS
>
> Above line marks a big failure. 
> ^^^
>
>> Disassembling the firmware acpi tables
>>
>>  cat /sys/firmware/acpi/tables/HPET > /var/tmp/hpet.out
>>  iasl -d /var/tmp/hpet.out

That table is not used by hpet_acpi_add; you have to check for the device
that mentions "PNP0103" in the DSDT table.

But anyway, as far as I can tell from my own machine, the _CRS in the
DSDT table never lists the HPET interrupts, and the HPET registration is
always done by hpet_reserve_platform_timers() in arch/x86/kernel/hpet.c.
Try adding logging to hpet_late_init() to find out why it aborts.


Regards,
Clemens


Re: [PATCH v2] usb: core: Warn if an URB's transfer_buffer is on stack

2017-04-24 Thread Clemens Ladisch
Florian Fainelli wrote:
> We see a large number of fixes to several drivers to remove the usage of
> on-stack buffers feeding into USB transfer functions. Make it easier to spot
> the offenders by adding a warning in usb_hcd_map_urb_for_dma() checking that
> urb->transfer_buffer is not a stack object.

This description is incomplete.

> + } else if (object_is_on_stack(urb->transfer_buffer)) {
> + WARN_ONCE(1, "transfer buffer is on stack\n");
> + ret = -EAGAIN;
>   } else {
>   urb->transfer_dma = dma_map_single(

Not only is there a warning, but the check also forces all those URBs
to abort with an error.

Well, that makes it even easier to spot the offenders ...


Regards,
Clemens


Re: [PATCH v2] usb: core: Warn if an URB's transfer_buffer is on stack

2017-04-24 Thread Clemens Ladisch
Florian Fainelli wrote:
> We see a large number of fixes to several drivers to remove the usage of
> on-stack buffers feeding into USB transfer functions. Make it easier to spot
> the offenders by adding a warning in usb_hcd_map_urb_for_dma() checking that
> urb->transfer_buffer is not a stack object.

This description is incomplete.

> + } else if (object_is_on_stack(urb->transfer_buffer)) {
> + WARN_ONCE(1, "transfer buffer is on stack\n");
> + ret = -EAGAIN;
>   } else {
>   urb->transfer_dma = dma_map_single(

Not only is there a warning, but the check also forces all those URBs
to abort with an error.

Well, that makes it even easier to spot the offenders ...


Regards,
Clemens


Re: [PATCH v2 5/6] hpet: removing unused variable m in hpet_interrupt

2017-04-03 Thread Clemens Ladisch
Corentin Labbe wrote:
> This patch fix the following warning:
> drivers/char/hpet.c:146:17: attention : variable ‘m’ set but not used 
> [-Wunused-but-set-variable]
> by removing the unused variable m in hpet_interrupt

This patch might silence the warning, but it leaves the bug that
actually caused the warning.

As far as I can see, the computation of "base" should use "m".

But the entire algorithm is completely bogus because it does not
actually remove the race condition; the counter is likely to have
advanced beyond the "mc" value when the new comparator value is
written.  Also see arch/x86/kernel/hpet.c for how hpet_next_event()
handles this.

And why a non-periodic timer should generate periodic interrupts is
another question.

And nobody uses this crap.
So I'm really not sure what to do about this ...


Regards,
Clemens


Re: [PATCH v2 5/6] hpet: removing unused variable m in hpet_interrupt

2017-04-03 Thread Clemens Ladisch
Corentin Labbe wrote:
> This patch fix the following warning:
> drivers/char/hpet.c:146:17: attention : variable ‘m’ set but not used 
> [-Wunused-but-set-variable]
> by removing the unused variable m in hpet_interrupt

This patch might silence the warning, but it leaves the bug that
actually caused the warning.

As far as I can see, the computation of "base" should use "m".

But the entire algorithm is completely bogus because it does not
actually remove the race condition; the counter is likely to have
advanced beyond the "mc" value when the new comparator value is
written.  Also see arch/x86/kernel/hpet.c for how hpet_next_event()
handles this.

And why a non-periodic timer should generate periodic interrupts is
another question.

And nobody uses this crap.
So I'm really not sure what to do about this ...


Regards,
Clemens


Re: [PATCH] ALSA: oxygen - Fix snd_oxygen module not loading for some (new?) Xonar DG SI cards.

2017-03-29 Thread Clemens Ladisch
Eugene Ganeev wrote:
> My Xonar DG SI card is showing up in lspci but no module is loaded for
> it.
>
> The patch just adds a new value with card's PCI ID to oxygen_ids array.

Is the hardware identical?  Do all the inputs and outputs work?

> Signed-off-by: Eugene Ganeev 
> ---
>  sound/pci/oxygen/oxygen.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/sound/pci/oxygen/oxygen.c b/sound/pci/oxygen/oxygen.c
> index ada6c256378e..99ba0354d4cc 100644
> --- a/sound/pci/oxygen/oxygen.c
> +++ b/sound/pci/oxygen/oxygen.c
> @@ -110,6 +110,7 @@ static DEFINE_PCI_DEVICE_TABLE(oxygen_ids) = {
>   { OXYGEN_PCI_SUBID(0x1a58, 0x0910), .driver_data = MODEL_CMEDIA_REF },
>   /* Asus Xonar DG */
>   { OXYGEN_PCI_SUBID(0x1043, 0x8467), .driver_data = MODEL_XONAR_DG },
> + { OXYGEN_PCI_SUBID(0x1043, 0x855e), .driver_data = MODEL_XONAR_DG },

Please add the correct name to the names[] array.


Regards.
Clemens


Re: [PATCH] ALSA: oxygen - Fix snd_oxygen module not loading for some (new?) Xonar DG SI cards.

2017-03-29 Thread Clemens Ladisch
Eugene Ganeev wrote:
> My Xonar DG SI card is showing up in lspci but no module is loaded for
> it.
>
> The patch just adds a new value with card's PCI ID to oxygen_ids array.

Is the hardware identical?  Do all the inputs and outputs work?

> Signed-off-by: Eugene Ganeev 
> ---
>  sound/pci/oxygen/oxygen.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/sound/pci/oxygen/oxygen.c b/sound/pci/oxygen/oxygen.c
> index ada6c256378e..99ba0354d4cc 100644
> --- a/sound/pci/oxygen/oxygen.c
> +++ b/sound/pci/oxygen/oxygen.c
> @@ -110,6 +110,7 @@ static DEFINE_PCI_DEVICE_TABLE(oxygen_ids) = {
>   { OXYGEN_PCI_SUBID(0x1a58, 0x0910), .driver_data = MODEL_CMEDIA_REF },
>   /* Asus Xonar DG */
>   { OXYGEN_PCI_SUBID(0x1043, 0x8467), .driver_data = MODEL_XONAR_DG },
> + { OXYGEN_PCI_SUBID(0x1043, 0x855e), .driver_data = MODEL_XONAR_DG },

Please add the correct name to the names[] array.


Regards.
Clemens


Re: [PATCH 2/6] hpet: remove unused writeq/readq function definitions

2017-03-27 Thread Clemens Ladisch
Corentin Labbe wrote:
> On Mon, Mar 27, 2017 at 07:49:34AM +0800, kbuild test robot wrote:
>>drivers//char/hpet.c: In function 'hpet_timer_set_irq':
 drivers//char/hpet.c:207:7: error: implicit declaration of function 
 'readq' [-Werror=implicit-function-declaration]
>
> Wrongly believed that x86 and x86_64 shared writeq/readq.
> Sorry, I will drop this patch
>
> Since the writeq/readq redefined is present in lots of other file, perhaps 
> adding it to i386 could be done.

Just use  instead.


Regards,
Clemens


Re: [PATCH 2/6] hpet: remove unused writeq/readq function definitions

2017-03-27 Thread Clemens Ladisch
Corentin Labbe wrote:
> On Mon, Mar 27, 2017 at 07:49:34AM +0800, kbuild test robot wrote:
>>drivers//char/hpet.c: In function 'hpet_timer_set_irq':
 drivers//char/hpet.c:207:7: error: implicit declaration of function 
 'readq' [-Werror=implicit-function-declaration]
>
> Wrongly believed that x86 and x86_64 shared writeq/readq.
> Sorry, I will drop this patch
>
> Since the writeq/readq redefined is present in lots of other file, perhaps 
> adding it to i386 could be done.

Just use  instead.


Regards,
Clemens


Re: [PATCH v1] hpet: Make cmd parameter of hpet_ioctl_common() unsigned

2017-03-14 Thread Clemens Ladisch
Matthias Kaehlcke wrote:
> The value passed by the two callers of the function is unsigned anyway.

Indeed; and those are just simple wrappers.

> Making the parameter unsigned fixes the following warning when building
> with clang:
>
> drivers/char/hpet.c:588:7: error: overflow converting case value to switch 
> condition type (2149083139 to 18446744071563667459) [-Werror,-Wswitch]
> case HPET_INFO:
>  ^
> include/uapi/linux/hpet.h:18:19: note: expanded from macro 'HPET_INFO'
> ^
> include/uapi/asm-generic/ioctl.h:77:28: note: expanded from macro '_IOR'
> ^
> include/uapi/asm-generic/ioctl.h:66:2: note: expanded from macro '_IOC'
> (((dir)  << _IOC_DIRSHIFT) | \
>
> Signed-off-by: Matthias Kaehlcke <m...@chromium.org>

Acked-by: Clemens Ladisch <clem...@ladisch.de>

> ---
>  drivers/char/hpet.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/char/hpet.c b/drivers/char/hpet.c
> index 20b32bb8c2af..0d633b76c29e 100644
> --- a/drivers/char/hpet.c
> +++ b/drivers/char/hpet.c
> @@ -574,7 +574,7 @@ static inline unsigned long hpet_time_div(struct hpets 
> *hpets,
>  }
>
>  static int
> -hpet_ioctl_common(struct hpet_dev *devp, int cmd, unsigned long arg,
> +hpet_ioctl_common(struct hpet_dev *devp, unsigned int cmd, unsigned long arg,
> struct hpet_info *info)
>  {
>   struct hpet_timer __iomem *timer;


Re: [PATCH v1] hpet: Make cmd parameter of hpet_ioctl_common() unsigned

2017-03-14 Thread Clemens Ladisch
Matthias Kaehlcke wrote:
> The value passed by the two callers of the function is unsigned anyway.

Indeed; and those are just simple wrappers.

> Making the parameter unsigned fixes the following warning when building
> with clang:
>
> drivers/char/hpet.c:588:7: error: overflow converting case value to switch 
> condition type (2149083139 to 18446744071563667459) [-Werror,-Wswitch]
> case HPET_INFO:
>  ^
> include/uapi/linux/hpet.h:18:19: note: expanded from macro 'HPET_INFO'
> ^
> include/uapi/asm-generic/ioctl.h:77:28: note: expanded from macro '_IOR'
> ^
> include/uapi/asm-generic/ioctl.h:66:2: note: expanded from macro '_IOC'
> (((dir)  << _IOC_DIRSHIFT) | \
>
> Signed-off-by: Matthias Kaehlcke 

Acked-by: Clemens Ladisch 

> ---
>  drivers/char/hpet.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/char/hpet.c b/drivers/char/hpet.c
> index 20b32bb8c2af..0d633b76c29e 100644
> --- a/drivers/char/hpet.c
> +++ b/drivers/char/hpet.c
> @@ -574,7 +574,7 @@ static inline unsigned long hpet_time_div(struct hpets 
> *hpets,
>  }
>
>  static int
> -hpet_ioctl_common(struct hpet_dev *devp, int cmd, unsigned long arg,
> +hpet_ioctl_common(struct hpet_dev *devp, unsigned int cmd, unsigned long arg,
> struct hpet_info *info)
>  {
>   struct hpet_timer __iomem *timer;


Re: [alsa-devel] [RFC] [ALSA][CONTROL] Added 2 ioctls for reading/writing values of multiple controls in one go (at once)

2017-02-02 Thread Clemens Ladisch
Satendra Singh Thakur wrote:
> -Added an ioctl to read values of multiple elements at once
> -Added an ioctl to write values of multiple elements at once
> -In the absence of above ioctls user needs to call N ioctls to
>  read/write value of N elements which requires N context switches

And why are these N context switches a problem?

> -Above mentioned ioctl will be useful for alsa utils such as amixer
>  which reads all controls of given sound card

Is there a noticeable difference?


Regards,
Clemens


Re: [alsa-devel] [RFC] [ALSA][CONTROL] Added 2 ioctls for reading/writing values of multiple controls in one go (at once)

2017-02-02 Thread Clemens Ladisch
Satendra Singh Thakur wrote:
> -Added an ioctl to read values of multiple elements at once
> -Added an ioctl to write values of multiple elements at once
> -In the absence of above ioctls user needs to call N ioctls to
>  read/write value of N elements which requires N context switches

And why are these N context switches a problem?

> -Above mentioned ioctl will be useful for alsa utils such as amixer
>  which reads all controls of given sound card

Is there a noticeable difference?


Regards,
Clemens


Re: [alsa-devel] [PATCH 6/7] dmasound_core: Move two assignments for the variable "ret" in state_open()

2017-01-24 Thread Clemens Ladisch
SF Markus Elfring wrote:
> A local variable was set to an error code in two cases before a concrete
> error situation was detected.

And why would that be a problem?

http://yarchive.net/comp/linux/error_jumps.html

> - ret = -EBUSY;
> - if (state.busy)
> + if (state.busy) {
> + ret = -EBUSY;
>   goto out;
> + }


Regards,
Clemens


Re: [alsa-devel] [PATCH 6/7] dmasound_core: Move two assignments for the variable "ret" in state_open()

2017-01-24 Thread Clemens Ladisch
SF Markus Elfring wrote:
> A local variable was set to an error code in two cases before a concrete
> error situation was detected.

And why would that be a problem?

http://yarchive.net/comp/linux/error_jumps.html

> - ret = -EBUSY;
> - if (state.busy)
> + if (state.busy) {
> + ret = -EBUSY;
>   goto out;
> + }


Regards,
Clemens


Re: [alsa-devel] [PATCH v2 1/2] ALSA: usb-audio: more tolerant packetsize

2016-12-06 Thread Clemens Ladisch
Jiada Wang wrote:
> since commit 57e6dae1087b ("ALSA: usb-audio: do not trust too-big
> wMaxPacketSize values"), the expected packetsize is always limited
> to nominal + 25%. It was discovered, that some devices

Android audio accessory

> have a much
> higher jitter in used packetsizes than 25% which would result in BABBLE
> condition and dropping of packets.
> A better solution is so assume the jitter to be the nominal packetsize:
> -one nearly empty packet followed by a almost 150% sized one.
>
> V2: changed to assume max frequency is +50 of nominal packetsize.
>
> Signed-off-by: Andreas Pape <ap...@de.adit-jv.com>
> Signed-off-by: Jiada Wang <jiada_w...@mentor.com>

Acked-by: Clemens Ladisch <clem...@ladisch.de>

> --- a/sound/usb/endpoint.c
> +++ b/sound/usb/endpoint.c
> @@ -632,8 +632,8 @@ static int data_ep_set_params(struct snd_usb_endpoint *ep,
>   ep->stride = frame_bits >> 3;
>   ep->silence_value = pcm_format == SNDRV_PCM_FORMAT_U8 ? 0x80 : 0;
>
> - /* assume max. frequency is 25% higher than nominal */
> - ep->freqmax = ep->freqn + (ep->freqn >> 2);
> + /* assume max. frequency is 50% higher than nominal */
> + ep->freqmax = ep->freqn + (ep->freqn >> 1);
>   /* Round up freqmax to nearest integer in order to calculate maximum
>* packet size, which must represent a whole number of frames.
>* This is accomplished by adding 0x0. before converting the


Re: [alsa-devel] [PATCH v2 1/2] ALSA: usb-audio: more tolerant packetsize

2016-12-06 Thread Clemens Ladisch
Jiada Wang wrote:
> since commit 57e6dae1087b ("ALSA: usb-audio: do not trust too-big
> wMaxPacketSize values"), the expected packetsize is always limited
> to nominal + 25%. It was discovered, that some devices

Android audio accessory

> have a much
> higher jitter in used packetsizes than 25% which would result in BABBLE
> condition and dropping of packets.
> A better solution is so assume the jitter to be the nominal packetsize:
> -one nearly empty packet followed by a almost 150% sized one.
>
> V2: changed to assume max frequency is +50 of nominal packetsize.
>
> Signed-off-by: Andreas Pape 
> Signed-off-by: Jiada Wang 

Acked-by: Clemens Ladisch 

> --- a/sound/usb/endpoint.c
> +++ b/sound/usb/endpoint.c
> @@ -632,8 +632,8 @@ static int data_ep_set_params(struct snd_usb_endpoint *ep,
>   ep->stride = frame_bits >> 3;
>   ep->silence_value = pcm_format == SNDRV_PCM_FORMAT_U8 ? 0x80 : 0;
>
> - /* assume max. frequency is 25% higher than nominal */
> - ep->freqmax = ep->freqn + (ep->freqn >> 2);
> + /* assume max. frequency is 50% higher than nominal */
> + ep->freqmax = ep->freqn + (ep->freqn >> 1);
>   /* Round up freqmax to nearest integer in order to calculate maximum
>* packet size, which must represent a whole number of frames.
>* This is accomplished by adding 0x0. before converting the


Re: [alsa-devel] [PATCH 1/3 v1] ALSA: usb-audio: more tolerant packetsize

2016-12-01 Thread Clemens Ladisch
Jiada Wang wrote:
> On 11/30/2016 11:41 PM, Clemens Ladisch wrote:
>> Jiada Wang wrote:
>>> since commit 57e6dae1087bbaa6b33d3dd8a8e90b63888939a3 the expected 
>>> packetsize is always limited to
>>> nominal + 25%. It was discovered, that some devices
>>
>> Which devices?
>
> It was a LG nexus

So it was the Android audio accessory mode.

>>> have a much higher jitter in used packetsizes than 25%
>>
>> How high?
>
> the nominal packet size was somewhere around 176bytes
> +25% would result in max expected packets to be ~220bytes
> We observed some packets exceeding this size (256byte)

256 bytes per USB frame would correspond to 64 kHz, instead of the
nominal 44.1 kHz.

The audio accessory sample format is fixed, and that mode is no longer
developed, so increasing the limit to +50% would be sufficient to work
around this problem.

I don't know if this is a bug in Google's generic AOA code, or if LG did
some changes; I have not heard any other report so far ...


Regards,
Clemens


Re: [alsa-devel] [PATCH 1/3 v1] ALSA: usb-audio: more tolerant packetsize

2016-12-01 Thread Clemens Ladisch
Jiada Wang wrote:
> On 11/30/2016 11:41 PM, Clemens Ladisch wrote:
>> Jiada Wang wrote:
>>> since commit 57e6dae1087bbaa6b33d3dd8a8e90b63888939a3 the expected 
>>> packetsize is always limited to
>>> nominal + 25%. It was discovered, that some devices
>>
>> Which devices?
>
> It was a LG nexus

So it was the Android audio accessory mode.

>>> have a much higher jitter in used packetsizes than 25%
>>
>> How high?
>
> the nominal packet size was somewhere around 176bytes
> +25% would result in max expected packets to be ~220bytes
> We observed some packets exceeding this size (256byte)

256 bytes per USB frame would correspond to 64 kHz, instead of the
nominal 44.1 kHz.

The audio accessory sample format is fixed, and that mode is no longer
developed, so increasing the limit to +50% would be sufficient to work
around this problem.

I don't know if this is a bug in Google's generic AOA code, or if LG did
some changes; I have not heard any other report so far ...


Regards,
Clemens


Re: [alsa-devel] [PATCH 1/3 v1] ALSA: usb-audio: more tolerant packetsize

2016-12-01 Thread Clemens Ladisch
Takashi Iwai wrote:
> Clemens Ladisch wrote:
>> Takashi Iwai wrote:
>>> [...]
>>> In the commit mentioned above, we changed the logic to take +25%
>>> frequency as the basis, and it my *reduce* if ep->maxpacksize is lower
>>> than that.
>>>
>>> OTOH, if ep->maxpacksize is sane, we can rely on it rather than the
>>> implicit +25% frequency.  That said, maybe we can check
>>> ep->maxpacksize whether it fits within the expected range, then adapt
>>> it, or take +25% freq as fallback?
>>
>> You are describing how the current code behaves.  The +25% limit _is_
>> what the code takes as the expected range.
>
> Well, the question is what is the "sane" range.  +25% doesn't fit for
> some devices.

The USB audio specification _requires_ that there is as little jitter
as possible.

It's no surprise that some device violates the specification.  But
we don't know what the actual error is; whether we could adjust the
packet size for this particular device only, or increase the limit
for all devices, or use a completely different workaround.

> If maxpacksize fits without +100% as this patch suggests, can we rely
> on it instead?

The packet size affect the following computations, like the number of
packets per URB.  I don't know how bad the effects would be.


Regards,
Clemens


Re: [alsa-devel] [PATCH 1/3 v1] ALSA: usb-audio: more tolerant packetsize

2016-12-01 Thread Clemens Ladisch
Takashi Iwai wrote:
> Clemens Ladisch wrote:
>> Takashi Iwai wrote:
>>> [...]
>>> In the commit mentioned above, we changed the logic to take +25%
>>> frequency as the basis, and it my *reduce* if ep->maxpacksize is lower
>>> than that.
>>>
>>> OTOH, if ep->maxpacksize is sane, we can rely on it rather than the
>>> implicit +25% frequency.  That said, maybe we can check
>>> ep->maxpacksize whether it fits within the expected range, then adapt
>>> it, or take +25% freq as fallback?
>>
>> You are describing how the current code behaves.  The +25% limit _is_
>> what the code takes as the expected range.
>
> Well, the question is what is the "sane" range.  +25% doesn't fit for
> some devices.

The USB audio specification _requires_ that there is as little jitter
as possible.

It's no surprise that some device violates the specification.  But
we don't know what the actual error is; whether we could adjust the
packet size for this particular device only, or increase the limit
for all devices, or use a completely different workaround.

> If maxpacksize fits without +100% as this patch suggests, can we rely
> on it instead?

The packet size affect the following computations, like the number of
packets per URB.  I don't know how bad the effects would be.


Regards,
Clemens


Re: [alsa-devel] [PATCH 1/3 v1] ALSA: usb-audio: more tolerant packetsize

2016-12-01 Thread Clemens Ladisch
Takashi Iwai wrote:
> Clemens Ladisch wrote:
>> Jiada Wang wrote:
>>> since commit 57e6dae1087bbaa6b33d3dd8a8e90b63888939a3 the expected 
>>> packetsize is always limited to
>>> nominal + 25%. It was discovered, that some devices
>>
>> Which devices?
>>
>>> have a much higher jitter in used packetsizes than 25%
>>
>> How high?  (Please note that the USB specification restricts the jitter
>> to at most one frame in consecutive packets.)
>>
>>> which would result in BABBLE condition and dropping of packets.
>>> A better solution is so assume the jitter to be the nominal packetsize
>>
>> This solution is better for this one particular device, but how does it
>> affect normal devices, or the Scarlett 2i4 on EHCI affected?
>
> Actually, which value does this affected device in ep->maxpacksize?
> In the commit mentioned above, we changed the logic to take +25%
> frequency as the basis, and it my *reduce* if ep->maxpacksize is lower
> than that.
>
> OTOH, if ep->maxpacksize is sane, we can rely on it rather than the
> implicit +25% frequency.  That said, maybe we can check
> ep->maxpacksize whether it fits within the expected range, then adapt
> it, or take +25% freq as fallback?

You are describing how the current code behaves.  The +25% limit _is_
what the code takes as the expected range.


I'm wondering if that unknown device just declares a wrong interval value.


Regards,
Clemens


Re: [alsa-devel] [PATCH 1/3 v1] ALSA: usb-audio: more tolerant packetsize

2016-12-01 Thread Clemens Ladisch
Takashi Iwai wrote:
> Clemens Ladisch wrote:
>> Jiada Wang wrote:
>>> since commit 57e6dae1087bbaa6b33d3dd8a8e90b63888939a3 the expected 
>>> packetsize is always limited to
>>> nominal + 25%. It was discovered, that some devices
>>
>> Which devices?
>>
>>> have a much higher jitter in used packetsizes than 25%
>>
>> How high?  (Please note that the USB specification restricts the jitter
>> to at most one frame in consecutive packets.)
>>
>>> which would result in BABBLE condition and dropping of packets.
>>> A better solution is so assume the jitter to be the nominal packetsize
>>
>> This solution is better for this one particular device, but how does it
>> affect normal devices, or the Scarlett 2i4 on EHCI affected?
>
> Actually, which value does this affected device in ep->maxpacksize?
> In the commit mentioned above, we changed the logic to take +25%
> frequency as the basis, and it my *reduce* if ep->maxpacksize is lower
> than that.
>
> OTOH, if ep->maxpacksize is sane, we can rely on it rather than the
> implicit +25% frequency.  That said, maybe we can check
> ep->maxpacksize whether it fits within the expected range, then adapt
> it, or take +25% freq as fallback?

You are describing how the current code behaves.  The +25% limit _is_
what the code takes as the expected range.


I'm wondering if that unknown device just declares a wrong interval value.


Regards,
Clemens


Re: [alsa-devel] [PATCH 1/3 v1] ALSA: usb-audio: more tolerant packetsize

2016-11-30 Thread Clemens Ladisch
Jiada Wang wrote:
> since commit 57e6dae1087bbaa6b33d3dd8a8e90b63888939a3 the expected packetsize 
> is always limited to
> nominal + 25%. It was discovered, that some devices

Which devices?

> have a much higher jitter in used packetsizes than 25%

How high?  (Please note that the USB specification restricts the jitter
to at most one frame in consecutive packets.)

> which would result in BABBLE condition and dropping of packets.
> A better solution is so assume the jitter to be the nominal packetsize

This solution is better for this one particular device, but how does it
affect normal devices, or the Scarlett 2i4 on EHCI affected?


Regards,
Clemens


Re: [alsa-devel] [PATCH 1/3 v1] ALSA: usb-audio: more tolerant packetsize

2016-11-30 Thread Clemens Ladisch
Jiada Wang wrote:
> since commit 57e6dae1087bbaa6b33d3dd8a8e90b63888939a3 the expected packetsize 
> is always limited to
> nominal + 25%. It was discovered, that some devices

Which devices?

> have a much higher jitter in used packetsizes than 25%

How high?  (Please note that the USB specification restricts the jitter
to at most one frame in consecutive packets.)

> which would result in BABBLE condition and dropping of packets.
> A better solution is so assume the jitter to be the nominal packetsize

This solution is better for this one particular device, but how does it
affect normal devices, or the Scarlett 2i4 on EHCI affected?


Regards,
Clemens


Re: Multiple problems with the Linux kernel on an AMD desktop

2016-11-25 Thread Clemens Ladisch
Rogério Brito wrote:
> [  130.007219] evbug: Event. Dev: input6, Type: 0, Code: 0, Value: 0

The evbug module is intended for debugging; it dumps all input events
into syslog.  If you do not want these messages, do not load this module.
(If it is loaded automatically, you have an actual bug.)


Regards,
Clemens


Re: Multiple problems with the Linux kernel on an AMD desktop

2016-11-25 Thread Clemens Ladisch
Rogério Brito wrote:
> [  130.007219] evbug: Event. Dev: input6, Type: 0, Code: 0, Value: 0

The evbug module is intended for debugging; it dumps all input events
into syslog.  If you do not want these messages, do not load this module.
(If it is loaded automatically, you have an actual bug.)


Regards,
Clemens


Re: Multiple problems with the Linux kernel on an AMD desktop

2016-11-25 Thread Clemens Ladisch
Rogério Brito wrote:
> * I have never been able to boot this computer of mine without the option
>   irqpoll---otherwise, I get the nobody cared message.

The "nobody cared" message indicates that there were too many interrupts
that no driver felt responsible for, so the kernel has disabled that
interrupt vector.  The irqpoll option is a workaround to get the devices
on that interrupt vector to work, but it's not perfect.

It's possible that most of your problems are caused by the irqpoll option.

What IRQ is the problematic one (see the "nobody cared" message)?  What
devices are connected to it (see /proc/interrupts)?  Does the problem go
away when you prevent the corresponding driver(s) from loading?


Regards,
Clemens


Re: Multiple problems with the Linux kernel on an AMD desktop

2016-11-25 Thread Clemens Ladisch
Rogério Brito wrote:
> * I have never been able to boot this computer of mine without the option
>   irqpoll---otherwise, I get the nobody cared message.

The "nobody cared" message indicates that there were too many interrupts
that no driver felt responsible for, so the kernel has disabled that
interrupt vector.  The irqpoll option is a workaround to get the devices
on that interrupt vector to work, but it's not perfect.

It's possible that most of your problems are caused by the irqpoll option.

What IRQ is the problematic one (see the "nobody cared" message)?  What
devices are connected to it (see /proc/interrupts)?  Does the problem go
away when you prevent the corresponding driver(s) from loading?


Regards,
Clemens


Re: [PATCH v2 2/4] samples: move timers example code from Documentation

2016-09-21 Thread Clemens Ladisch
Shuah Khan wrote:
> Move timers examples to samples and remove it from Documentation
> Makefile. Create a new Makefile to build timers. It can be built
> from top level directory or from timers directory:
>
> Run make -C samples/timers or cd samples/timers; make
>
> Acked-by: Jonathan Corbet <cor...@lwn.net>
> Signed-off-by: Shuah Khan <shua...@osg.samsung.com>

Acked-by: Clemens Ladisch <clem...@ladisch.de>


Re: [PATCH v2 2/4] samples: move timers example code from Documentation

2016-09-21 Thread Clemens Ladisch
Shuah Khan wrote:
> Move timers examples to samples and remove it from Documentation
> Makefile. Create a new Makefile to build timers. It can be built
> from top level directory or from timers directory:
>
> Run make -C samples/timers or cd samples/timers; make
>
> Acked-by: Jonathan Corbet 
> Signed-off-by: Shuah Khan 

Acked-by: Clemens Ladisch 


Re: [alsa-devel] [PATCH] ALSA: usb: constify snd_pcm_ops structures

2016-09-08 Thread Clemens Ladisch
Julia Lawall wrote:
> Check for snd_pcm_ops structures that are only stored in the ops field of a
> snd_soc_platform_driver structure or passed as the third argument to
> snd_pcm_set_ops.  The corresponding field or parameter is declared const,
> so snd_pcm_ops structures that have this property can be declared as const
> also.
>
> The semantic patch that makes this change is as follows:
> (http://coccinelle.lip6.fr/)
>
> // 
> @r disable optional_qualifier@
> identifier i;
> position p;
> @@
> static struct snd_pcm_ops i@p = { ... };
>
> @ok1@
> identifier r.i;
> struct snd_soc_platform_driver e;
> position p;
> @@
> e.ops = @p;
>
> @ok2@
> identifier r.i;
> expression e1, e2;
> position p;
> @@
> snd_pcm_set_ops(e1, e2, @p)
>
> @bad@
> position p != {r.p,ok1.p,ok2.p};
> identifier r.i;
> struct snd_pcm_ops e;
> @@
> e@i@p
>
> @depends on !bad disable optional_qualifier@
> identifier r.i;
> @@
> static
> +const
>  struct snd_pcm_ops i = { ... };
> // 
>
> Signed-off-by: Julia Lawall <julia.law...@lip6.fr>

Acked-by: Clemens Ladisch <clem...@ladisch.de>

> ---
>  sound/usb/6fire/pcm.c   |2 +-
>  sound/usb/caiaq/audio.c |2 +-
>  sound/usb/hiface/pcm.c  |2 +-
>  sound/usb/misc/ua101.c  |4 ++--
>  sound/usb/usx2y/usx2yhwdeppcm.c |2 +-
>  5 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/sound/usb/6fire/pcm.c b/sound/usb/6fire/pcm.c
> index 36f4115..224a6a5 100644
> --- a/sound/usb/6fire/pcm.c
> +++ b/sound/usb/6fire/pcm.c
> @@ -555,7 +555,7 @@ static snd_pcm_uframes_t usb6fire_pcm_pointer(
>   return ret;
>  }
>
> -static struct snd_pcm_ops pcm_ops = {
> +static const struct snd_pcm_ops pcm_ops = {
>   .open = usb6fire_pcm_open,
>   .close = usb6fire_pcm_close,
>   .ioctl = snd_pcm_lib_ioctl,
> diff --git a/sound/usb/hiface/pcm.c b/sound/usb/hiface/pcm.c
> index 2c44139..23e66ed 100644
> --- a/sound/usb/hiface/pcm.c
> +++ b/sound/usb/hiface/pcm.c
> @@ -511,7 +511,7 @@ static snd_pcm_uframes_t hiface_pcm_pointer(struct 
> snd_pcm_substream *alsa_sub)
>   return bytes_to_frames(alsa_sub->runtime, dma_offset);
>  }
>
> -static struct snd_pcm_ops pcm_ops = {
> +static const struct snd_pcm_ops pcm_ops = {
>   .open = hiface_pcm_open,
>   .close = hiface_pcm_close,
>   .ioctl = snd_pcm_lib_ioctl,
> diff --git a/sound/usb/usx2y/usx2yhwdeppcm.c b/sound/usb/usx2y/usx2yhwdeppcm.c
> index 90766a9..1f28cb2a 100644
> --- a/sound/usb/usx2y/usx2yhwdeppcm.c
> +++ b/sound/usb/usx2y/usx2yhwdeppcm.c
> @@ -587,7 +587,7 @@ static int snd_usX2Y_usbpcm_close(struct 
> snd_pcm_substream *substream)
>  }
>
>
> -static struct snd_pcm_ops snd_usX2Y_usbpcm_ops =
> +static const struct snd_pcm_ops snd_usX2Y_usbpcm_ops =
>  {
>   .open = snd_usX2Y_usbpcm_open,
>   .close =snd_usX2Y_usbpcm_close,
> diff --git a/sound/usb/misc/ua101.c b/sound/usb/misc/ua101.c
> index c19a5dd..71a0e9e 100644
> --- a/sound/usb/misc/ua101.c
> +++ b/sound/usb/misc/ua101.c
> @@ -890,7 +890,7 @@ static snd_pcm_uframes_t playback_pcm_pointer(struct 
> snd_pcm_substream *subs)
>   return ua101_pcm_pointer(ua, >playback);
>  }
>
> -static struct snd_pcm_ops capture_pcm_ops = {
> +static const struct snd_pcm_ops capture_pcm_ops = {
>   .open = capture_pcm_open,
>   .close = capture_pcm_close,
>   .ioctl = snd_pcm_lib_ioctl,
> @@ -903,7 +903,7 @@ static struct snd_pcm_ops capture_pcm_ops = {
>   .mmap = snd_pcm_lib_mmap_vmalloc,
>  };
>
> -static struct snd_pcm_ops playback_pcm_ops = {
> +static const struct snd_pcm_ops playback_pcm_ops = {
>   .open = playback_pcm_open,
>   .close = playback_pcm_close,
>   .ioctl = snd_pcm_lib_ioctl,
> diff --git a/sound/usb/caiaq/audio.c b/sound/usb/caiaq/audio.c
> index 8f66ba7..af5ad84 100644
> --- a/sound/usb/caiaq/audio.c
> +++ b/sound/usb/caiaq/audio.c
> @@ -338,7 +338,7 @@ unlock:
>  }
>
>  /* operators for both playback and capture */
> -static struct snd_pcm_ops snd_usb_caiaq_ops = {
> +static const struct snd_pcm_ops snd_usb_caiaq_ops = {
>   .open = snd_usb_caiaq_substream_open,
>   .close =snd_usb_caiaq_substream_close,
>   .ioctl =snd_pcm_lib_ioctl,


Re: [alsa-devel] [PATCH] ALSA: usb: constify snd_pcm_ops structures

2016-09-08 Thread Clemens Ladisch
Julia Lawall wrote:
> Check for snd_pcm_ops structures that are only stored in the ops field of a
> snd_soc_platform_driver structure or passed as the third argument to
> snd_pcm_set_ops.  The corresponding field or parameter is declared const,
> so snd_pcm_ops structures that have this property can be declared as const
> also.
>
> The semantic patch that makes this change is as follows:
> (http://coccinelle.lip6.fr/)
>
> // 
> @r disable optional_qualifier@
> identifier i;
> position p;
> @@
> static struct snd_pcm_ops i@p = { ... };
>
> @ok1@
> identifier r.i;
> struct snd_soc_platform_driver e;
> position p;
> @@
> e.ops = @p;
>
> @ok2@
> identifier r.i;
> expression e1, e2;
> position p;
> @@
> snd_pcm_set_ops(e1, e2, @p)
>
> @bad@
> position p != {r.p,ok1.p,ok2.p};
> identifier r.i;
> struct snd_pcm_ops e;
> @@
> e@i@p
>
> @depends on !bad disable optional_qualifier@
> identifier r.i;
> @@
> static
> +const
>  struct snd_pcm_ops i = { ... };
> // 
>
> Signed-off-by: Julia Lawall 

Acked-by: Clemens Ladisch 

> ---
>  sound/usb/6fire/pcm.c   |2 +-
>  sound/usb/caiaq/audio.c |2 +-
>  sound/usb/hiface/pcm.c  |2 +-
>  sound/usb/misc/ua101.c  |4 ++--
>  sound/usb/usx2y/usx2yhwdeppcm.c |2 +-
>  5 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/sound/usb/6fire/pcm.c b/sound/usb/6fire/pcm.c
> index 36f4115..224a6a5 100644
> --- a/sound/usb/6fire/pcm.c
> +++ b/sound/usb/6fire/pcm.c
> @@ -555,7 +555,7 @@ static snd_pcm_uframes_t usb6fire_pcm_pointer(
>   return ret;
>  }
>
> -static struct snd_pcm_ops pcm_ops = {
> +static const struct snd_pcm_ops pcm_ops = {
>   .open = usb6fire_pcm_open,
>   .close = usb6fire_pcm_close,
>   .ioctl = snd_pcm_lib_ioctl,
> diff --git a/sound/usb/hiface/pcm.c b/sound/usb/hiface/pcm.c
> index 2c44139..23e66ed 100644
> --- a/sound/usb/hiface/pcm.c
> +++ b/sound/usb/hiface/pcm.c
> @@ -511,7 +511,7 @@ static snd_pcm_uframes_t hiface_pcm_pointer(struct 
> snd_pcm_substream *alsa_sub)
>   return bytes_to_frames(alsa_sub->runtime, dma_offset);
>  }
>
> -static struct snd_pcm_ops pcm_ops = {
> +static const struct snd_pcm_ops pcm_ops = {
>   .open = hiface_pcm_open,
>   .close = hiface_pcm_close,
>   .ioctl = snd_pcm_lib_ioctl,
> diff --git a/sound/usb/usx2y/usx2yhwdeppcm.c b/sound/usb/usx2y/usx2yhwdeppcm.c
> index 90766a9..1f28cb2a 100644
> --- a/sound/usb/usx2y/usx2yhwdeppcm.c
> +++ b/sound/usb/usx2y/usx2yhwdeppcm.c
> @@ -587,7 +587,7 @@ static int snd_usX2Y_usbpcm_close(struct 
> snd_pcm_substream *substream)
>  }
>
>
> -static struct snd_pcm_ops snd_usX2Y_usbpcm_ops =
> +static const struct snd_pcm_ops snd_usX2Y_usbpcm_ops =
>  {
>   .open = snd_usX2Y_usbpcm_open,
>   .close =snd_usX2Y_usbpcm_close,
> diff --git a/sound/usb/misc/ua101.c b/sound/usb/misc/ua101.c
> index c19a5dd..71a0e9e 100644
> --- a/sound/usb/misc/ua101.c
> +++ b/sound/usb/misc/ua101.c
> @@ -890,7 +890,7 @@ static snd_pcm_uframes_t playback_pcm_pointer(struct 
> snd_pcm_substream *subs)
>   return ua101_pcm_pointer(ua, >playback);
>  }
>
> -static struct snd_pcm_ops capture_pcm_ops = {
> +static const struct snd_pcm_ops capture_pcm_ops = {
>   .open = capture_pcm_open,
>   .close = capture_pcm_close,
>   .ioctl = snd_pcm_lib_ioctl,
> @@ -903,7 +903,7 @@ static struct snd_pcm_ops capture_pcm_ops = {
>   .mmap = snd_pcm_lib_mmap_vmalloc,
>  };
>
> -static struct snd_pcm_ops playback_pcm_ops = {
> +static const struct snd_pcm_ops playback_pcm_ops = {
>   .open = playback_pcm_open,
>   .close = playback_pcm_close,
>   .ioctl = snd_pcm_lib_ioctl,
> diff --git a/sound/usb/caiaq/audio.c b/sound/usb/caiaq/audio.c
> index 8f66ba7..af5ad84 100644
> --- a/sound/usb/caiaq/audio.c
> +++ b/sound/usb/caiaq/audio.c
> @@ -338,7 +338,7 @@ unlock:
>  }
>
>  /* operators for both playback and capture */
> -static struct snd_pcm_ops snd_usb_caiaq_ops = {
> +static const struct snd_pcm_ops snd_usb_caiaq_ops = {
>   .open = snd_usb_caiaq_substream_open,
>   .close =snd_usb_caiaq_substream_close,
>   .ioctl =snd_pcm_lib_ioctl,


Re: [alsa-devel] [PATCH 0/6] constify snd_pcm_ops structures

2016-09-02 Thread Clemens Ladisch
Julia Lawall wrote:
> Constify snd_pcm_ops structures.

For 3/5/6:
Acked-by: Clemens Ladisch <clem...@ladisch.de>


Re: [alsa-devel] [PATCH 0/6] constify snd_pcm_ops structures

2016-09-02 Thread Clemens Ladisch
Julia Lawall wrote:
> Constify snd_pcm_ops structures.

For 3/5/6:
Acked-by: Clemens Ladisch 


Re: [PATCH v2 0/3] USB Audio Gadget refactoring

2016-08-16 Thread Clemens Ladisch
Peter Chen wrote:
> On Tue, Aug 16, 2016 at 11:32:55AM +0200, Clemens Ladisch wrote:
>> Windows does not have UAC2 support.
>
> Thanks, before windows7 or all windows versions have no UAC2 support?

So far, no version has it.


Regards,
Clemens


Re: [PATCH v2 0/3] USB Audio Gadget refactoring

2016-08-16 Thread Clemens Ladisch
Peter Chen wrote:
> On Tue, Aug 16, 2016 at 11:32:55AM +0200, Clemens Ladisch wrote:
>> Windows does not have UAC2 support.
>
> Thanks, before windows7 or all windows versions have no UAC2 support?

So far, no version has it.


Regards,
Clemens


Re: [PATCH v2 0/3] USB Audio Gadget refactoring

2016-08-16 Thread Clemens Ladisch
Peter Chen wrote:
> I find UAC2 (UAC1 is ok)  support is not well with the latest mainline
> kernel w/o your patch set. The windows7 can't install the driver
> successfully

Windows does not have UAC2 support.

> and the playback shows underrun (using local codec)
> using Linux host.

> # arecord -f dat -t wav -D hw:1,0 | aplay -D hw:0,0 &

The clocks of the two devices are not synchronized.

In the ALSA API, a PCM device is assumed to have its own clock, so it is
not possible to synchronize the USB gadget to the actual sound device
without some separate mechanism (like the old uac1 gadget probably has).


Regards,
Clemens


Re: [PATCH v2 0/3] USB Audio Gadget refactoring

2016-08-16 Thread Clemens Ladisch
Peter Chen wrote:
> I find UAC2 (UAC1 is ok)  support is not well with the latest mainline
> kernel w/o your patch set. The windows7 can't install the driver
> successfully

Windows does not have UAC2 support.

> and the playback shows underrun (using local codec)
> using Linux host.

> # arecord -f dat -t wav -D hw:1,0 | aplay -D hw:0,0 &

The clocks of the two devices are not synchronized.

In the ALSA API, a PCM device is assumed to have its own clock, so it is
not possible to synchronize the USB gadget to the actual sound device
without some separate mechanism (like the old uac1 gadget probably has).


Regards,
Clemens


Re: [RFC PATCH 0/5] USB Audio Gadget refactoring

2016-07-26 Thread Clemens Ladisch
Ruslan Bilovol wrote:
> On Fri, Jul 15, 2016 at 10:43 AM, Clemens Ladisch <clem...@ladisch.de> wrote:
>>>> On Tue, May 24, 2016 at 2:50 AM, Ruslan Bilovol
>>>> <ruslan.bilo...@gmail.com> wrote:
>>>>> it may break current usecase for some people
>>
>> And what are the benefits that justify breaking the kernel API?
>
> Main limitation with current f_uac1 design is - it can be used only on systems
> with real ALSA card present and can have only exact number of
> channels / sampling rate as sink card has. [...]
> A real cases when it's required to have UAC1 gadget represented as virtual
> sound card on gadget side: [...]

Thanks.

> Of course disadvantage of new approach for UAC1 gadget is you need to
> use some userspace application for routing audio from virtual to real
> sound card, like in case of UAC2 gadget. But thanks to existing
> applications like alsaloop it's not difficult nowadays.

I don't know what the maintainer will say, but you would increase the
chances of this change being accepted when you show a concrete example
of how to change the userspace configuration from the old to the new
driver.  (And add it to the documentation.)


Regards,
Clemens


Re: [RFC PATCH 0/5] USB Audio Gadget refactoring

2016-07-26 Thread Clemens Ladisch
Ruslan Bilovol wrote:
> On Fri, Jul 15, 2016 at 10:43 AM, Clemens Ladisch  wrote:
>>>> On Tue, May 24, 2016 at 2:50 AM, Ruslan Bilovol
>>>>  wrote:
>>>>> it may break current usecase for some people
>>
>> And what are the benefits that justify breaking the kernel API?
>
> Main limitation with current f_uac1 design is - it can be used only on systems
> with real ALSA card present and can have only exact number of
> channels / sampling rate as sink card has. [...]
> A real cases when it's required to have UAC1 gadget represented as virtual
> sound card on gadget side: [...]

Thanks.

> Of course disadvantage of new approach for UAC1 gadget is you need to
> use some userspace application for routing audio from virtual to real
> sound card, like in case of UAC2 gadget. But thanks to existing
> applications like alsaloop it's not difficult nowadays.

I don't know what the maintainer will say, but you would increase the
chances of this change being accepted when you show a concrete example
of how to change the userspace configuration from the old to the new
driver.  (And add it to the documentation.)


Regards,
Clemens


Re: [RFC PATCH 0/5] USB Audio Gadget refactoring

2016-07-15 Thread Clemens Ladisch
>> On Tue, May 24, 2016 at 2:50 AM, Ruslan Bilovol
>>  wrote:
>>> it may break current usecase for some people

And what are the benefits that justify breaking the kernel API?


Regards,
Clemens


Re: [RFC PATCH 0/5] USB Audio Gadget refactoring

2016-07-15 Thread Clemens Ladisch
>> On Tue, May 24, 2016 at 2:50 AM, Ruslan Bilovol
>>  wrote:
>>> it may break current usecase for some people

And what are the benefits that justify breaking the kernel API?


Regards,
Clemens


Re: [PATCH] usb: core: Do not use sizeof on pointer type

2016-04-22 Thread Clemens Ladisch
Vaishali Thakkar wrote:
> When sizeof is applied to a pointer typed expression, it gives
> the size of the pointer.

And why would that be wrong in this case?

> +++ b/drivers/usb/core/hcd.c
> @@ -1386,7 +1386,7 @@ static int hcd_alloc_coherent(struct usb_bus *bus,
>   return -EFAULT;
>   }
>
> - vaddr = hcd_buffer_alloc(bus, size + sizeof(vaddr),
> + vaddr = hcd_buffer_alloc(bus, size + sizeof(*vaddr),
>mem_flags, dma_handle);
>   if (!vaddr)
>   return -ENOMEM;
>

Please note the following comment:

/*
 * Store the virtual address of the buffer at the end
 * of the allocated dma buffer. [...]


Regards,
Clemens


Re: [PATCH] usb: core: Do not use sizeof on pointer type

2016-04-22 Thread Clemens Ladisch
Vaishali Thakkar wrote:
> When sizeof is applied to a pointer typed expression, it gives
> the size of the pointer.

And why would that be wrong in this case?

> +++ b/drivers/usb/core/hcd.c
> @@ -1386,7 +1386,7 @@ static int hcd_alloc_coherent(struct usb_bus *bus,
>   return -EFAULT;
>   }
>
> - vaddr = hcd_buffer_alloc(bus, size + sizeof(vaddr),
> + vaddr = hcd_buffer_alloc(bus, size + sizeof(*vaddr),
>mem_flags, dma_handle);
>   if (!vaddr)
>   return -ENOMEM;
>

Please note the following comment:

/*
 * Store the virtual address of the buffer at the end
 * of the allocated dma buffer. [...]


Regards,
Clemens


Re: [alsa-devel] linux-4.6-rc4/sound/pci/ens1370.c:1551: possible bad expression ?

2016-04-21 Thread Clemens Ladisch
David Binderman wrote:
> [linux-4.6-rc4/sound/pci/ens1370.c:1551]: (style) Expression '(X & 0xf)>= 
> 0x4' is always false.

What tool generated this message?

> Source code is
>
> if ((ensoniq->ctrl & ES_1371_GPIO_OUTM)>= 4)
> val = 1;

This message is wrong; it is certainly possible for this to be true.

However, there is a different bug: 4 must be ES_1371_GPIO_OUT(4).


Regards,
Clemens


Re: [alsa-devel] linux-4.6-rc4/sound/pci/ens1370.c:1551: possible bad expression ?

2016-04-21 Thread Clemens Ladisch
David Binderman wrote:
> [linux-4.6-rc4/sound/pci/ens1370.c:1551]: (style) Expression '(X & 0xf)>= 
> 0x4' is always false.

What tool generated this message?

> Source code is
>
> if ((ensoniq->ctrl & ES_1371_GPIO_OUTM)>= 4)
> val = 1;

This message is wrong; it is certainly possible for this to be true.

However, there is a different bug: 4 must be ES_1371_GPIO_OUT(4).


Regards,
Clemens


Re: [PATCH v2 06/14] USB: ch341: reinitialize chip on reconfiguration

2016-04-10 Thread Clemens Ladisch
Grigori Goronzy wrote:
> Changing the LCR register after initialization does not seem to be
> reliable on all chips (particularly not on CH341A).  Restructure
> initialization and configuration to always reinit the chip on
> configuration changes instead and pass the LCR register value directly
> to the initialization command.

> +++ b/drivers/usb/serial/ch341.c
> @@ -155,9 +157,7 @@ static int ch341_set_baudrate(struct usb_device *dev,
>   a = (factor & 0xff00) | divisor;
>   b = factor & 0xff;
>
> - r = ch341_control_out(dev, 0x9a, 0x1312, a);
> - if (!r)
> - r = ch341_control_out(dev, 0x9a, 0x0f2c, b);
> + r = ch341_control_out(dev, CH341_SERIAL_INIT, 0x9c | (ctrl << 8) , a | 
> 0x80);

The variable b is no longer used, so it is no longer necessary to
compute the lower eight bits of the factor.


Regards,
Clemens


Re: [PATCH v2 06/14] USB: ch341: reinitialize chip on reconfiguration

2016-04-10 Thread Clemens Ladisch
Grigori Goronzy wrote:
> Changing the LCR register after initialization does not seem to be
> reliable on all chips (particularly not on CH341A).  Restructure
> initialization and configuration to always reinit the chip on
> configuration changes instead and pass the LCR register value directly
> to the initialization command.

> +++ b/drivers/usb/serial/ch341.c
> @@ -155,9 +157,7 @@ static int ch341_set_baudrate(struct usb_device *dev,
>   a = (factor & 0xff00) | divisor;
>   b = factor & 0xff;
>
> - r = ch341_control_out(dev, 0x9a, 0x1312, a);
> - if (!r)
> - r = ch341_control_out(dev, 0x9a, 0x0f2c, b);
> + r = ch341_control_out(dev, CH341_SERIAL_INIT, 0x9c | (ctrl << 8) , a | 
> 0x80);

The variable b is no longer used, so it is no longer necessary to
compute the lower eight bits of the factor.


Regards,
Clemens


Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize

2016-03-15 Thread Clemens Ladisch
Felipe Ferreri Tonello wrote:
> On 11/03/16 23:07, Michal Nazarewicz wrote:
>> I’m also wondering whether it would be beneficial to get rid of buflen
>> all together and use wMaxPacketSie for in endpoints as well?  Is that
>> feasible?
>
> Yes, we could just remove the buflen parameter.
>
> The only scenario where I can see buflen been "useful" is if the user
> wants to have a smaller buffer size for the OUT endpoint. Should we
> support this case or not?

Splitting data into multiple packets would not make sense from
a performance perspective.  The only possible reason would be to work
around a (theoretical) bug in some host software that does not handle
larger buffers, but there aren't that many host implementations, and I
am not aware of any with such a bug.


Regards,
Clemens


Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize

2016-03-15 Thread Clemens Ladisch
Felipe Ferreri Tonello wrote:
> On 11/03/16 23:07, Michal Nazarewicz wrote:
>> I’m also wondering whether it would be beneficial to get rid of buflen
>> all together and use wMaxPacketSie for in endpoints as well?  Is that
>> feasible?
>
> Yes, we could just remove the buflen parameter.
>
> The only scenario where I can see buflen been "useful" is if the user
> wants to have a smaller buffer size for the OUT endpoint. Should we
> support this case or not?

Splitting data into multiple packets would not make sense from
a performance perspective.  The only possible reason would be to work
around a (theoretical) bug in some host software that does not handle
larger buffers, but there aren't that many host implementations, and I
am not aware of any with such a bug.


Regards,
Clemens


Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize

2016-03-11 Thread Clemens Ladisch
Steve Calfee wrote:
> On Wed, Mar 9, 2016 at 11:39 AM, Felipe F. Tonello  
> wrote:
>> [...]
>> This patch fixes this problem by setting the minimum usb_request's buffer 
>> size
>> for the OUT endpoint as its wMaxPacketSize.
>>
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -366,7 +366,9 @@ static int f_midi_set_alt(struct usb_function *f, 
>> unsigned intf, unsigned alt)
>> struct usb_request *req =
>> -   midi_alloc_ep_req(midi->out_ep, midi->buflen);
>> +   midi_alloc_ep_req(midi->out_ep,
>> +   max_t(unsigned, midi->buflen,
>> +   bulk_out_desc.wMaxPacketSize));
>
> Won't you get a buffer overrun if midi->buflen is less than wMaxPacketSize?

No.  f_midi_handle_out_data() uses only the request buffer.


Regards,
Clemens


Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize

2016-03-11 Thread Clemens Ladisch
Steve Calfee wrote:
> On Wed, Mar 9, 2016 at 11:39 AM, Felipe F. Tonello  
> wrote:
>> [...]
>> This patch fixes this problem by setting the minimum usb_request's buffer 
>> size
>> for the OUT endpoint as its wMaxPacketSize.
>>
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -366,7 +366,9 @@ static int f_midi_set_alt(struct usb_function *f, 
>> unsigned intf, unsigned alt)
>> struct usb_request *req =
>> -   midi_alloc_ep_req(midi->out_ep, midi->buflen);
>> +   midi_alloc_ep_req(midi->out_ep,
>> +   max_t(unsigned, midi->buflen,
>> +   bulk_out_desc.wMaxPacketSize));
>
> Won't you get a buffer overrun if midi->buflen is less than wMaxPacketSize?

No.  f_midi_handle_out_data() uses only the request buffer.


Regards,
Clemens


Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine

2016-03-04 Thread Clemens Ladisch
Felipe Ferreri Tonello wrote:
> On March 4, 2016 8:07:40 AM GMT+00:00, Clemens Ladisch <clem...@ladisch.de> 
> wrote:
>> Felipe Ferreri Tonello wrote:
>>> On 03/03/16 11:38, Clemens Ladisch wrote:
>>>> But in what way was the old state machine not "proper"?
>>>
>>> Because it didn't reflect all the correct and possible MIDI states
>>
>> The whole point of the one-byte real-time messages is that they do not
>> affect the parsing of the surrounding MIDI stream.  So not making them
>> part of the state machine is the proper way of handling them.  (Also
>> see the flowchart in appendix A of the spec.)
>
> I really don't get your point. So why do we have a state machine at all?

To parse all the other messages.


Regards,
Clemens


Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine

2016-03-04 Thread Clemens Ladisch
Felipe Ferreri Tonello wrote:
> On March 4, 2016 8:07:40 AM GMT+00:00, Clemens Ladisch  
> wrote:
>> Felipe Ferreri Tonello wrote:
>>> On 03/03/16 11:38, Clemens Ladisch wrote:
>>>> But in what way was the old state machine not "proper"?
>>>
>>> Because it didn't reflect all the correct and possible MIDI states
>>
>> The whole point of the one-byte real-time messages is that they do not
>> affect the parsing of the surrounding MIDI stream.  So not making them
>> part of the state machine is the proper way of handling them.  (Also
>> see the flowchart in appendix A of the spec.)
>
> I really don't get your point. So why do we have a state machine at all?

To parse all the other messages.


Regards,
Clemens


Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine

2016-03-04 Thread Clemens Ladisch
Felipe Ferreri Tonello wrote:
> On 03/03/16 11:38, Clemens Ladisch wrote:
>> But in what way was the old state machine not "proper"?
>
> Because it didn't reflect all the correct and possible MIDI states

The whole point of the one-byte real-time messages is that they do not
affect the parsing of the surrounding MIDI stream.  So not making them
part of the state machine is the proper way of handling them.  (Also
see the flowchart in appendix A of the spec.)

> This patch doesn't change any functionality. But the important thing
> here is that it improves the driver maintainability [...]

Then I won't get in the way of this driver's maintainer.


Regards,
Clemens


Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine

2016-03-04 Thread Clemens Ladisch
Felipe Ferreri Tonello wrote:
> On 03/03/16 11:38, Clemens Ladisch wrote:
>> But in what way was the old state machine not "proper"?
>
> Because it didn't reflect all the correct and possible MIDI states

The whole point of the one-byte real-time messages is that they do not
affect the parsing of the surrounding MIDI stream.  So not making them
part of the state machine is the proper way of handling them.  (Also
see the flowchart in appendix A of the spec.)

> This patch doesn't change any functionality. But the important thing
> here is that it improves the driver maintainability [...]

Then I won't get in the way of this driver's maintainer.


Regards,
Clemens


Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine

2016-03-03 Thread Clemens Ladisch
Felipe Ferreri Tonello wrote:
> On 02/03/16 21:09, Clemens Ladisch wrote:
>> Felipe F. Tonello wrote:
>>> This refactor results in a cleaner state machine code
>>
>> It increases the number of states, and now juggles two state variables.
>> I cannot agree to it being cleaner.
>
> Yes, it increases the number of states. That was done in order to
> actually implement a proper finite state machine with one state at a
> time and a transition state.

I know, "clean" is subjective.  But in what way was the old state
machine not "proper"?

And how is handling two states (port->state and next_state) cleaner?
As far as I can tell, the requirement for a separate variable comes not
from any inherent complexity of the state machine itself, but only
because the transmit_packet function was inlined.


Regards,
Clemens


Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine

2016-03-03 Thread Clemens Ladisch
Felipe Ferreri Tonello wrote:
> On 02/03/16 21:09, Clemens Ladisch wrote:
>> Felipe F. Tonello wrote:
>>> This refactor results in a cleaner state machine code
>>
>> It increases the number of states, and now juggles two state variables.
>> I cannot agree to it being cleaner.
>
> Yes, it increases the number of states. That was done in order to
> actually implement a proper finite state machine with one state at a
> time and a transition state.

I know, "clean" is subjective.  But in what way was the old state
machine not "proper"?

And how is handling two states (port->state and next_state) cleaner?
As far as I can tell, the requirement for a separate variable comes not
from any inherent complexity of the state machine itself, but only
because the transmit_packet function was inlined.


Regards,
Clemens


Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine

2016-03-02 Thread Clemens Ladisch
Felipe F. Tonello wrote:
> This refactor results in a cleaner state machine code

It increases the number of states, and now juggles two state variables.
I cannot agree to it being cleaner.

> and as a result fixed a bug when packaging a USB-MIDI packet right after
> a non-conformant MIDI byte stream.

I have been unable to determine where exactly the new code behaves
differently.  Can you show an example?


Regards,
Clemens


Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine

2016-03-02 Thread Clemens Ladisch
Felipe F. Tonello wrote:
> This refactor results in a cleaner state machine code

It increases the number of states, and now juggles two state variables.
I cannot agree to it being cleaner.

> and as a result fixed a bug when packaging a USB-MIDI packet right after
> a non-conformant MIDI byte stream.

I have been unable to determine where exactly the new code behaves
differently.  Can you show an example?


Regards,
Clemens


Re: [PATCH v7 0/5] usb: usbtmc: Add support for missing functions in USBTMC-USB488 spec

2016-01-28 Thread Clemens Ladisch
Dave Penkler wrote:
> Implement support for the USB488 defined READ_STATUS_BYTE ioctl (1/5)
> and SRQ notifications with fasync (2/5) and poll/select (3/5) in order
> to be able to synchronize with variable duration instrument
> operations.
>
> Add convenience ioctl to return all device capabilities (4/5)
>
> Add ioctls for other USB488 requests: REN_CONTROL, GOTO_LOCAL and
> LOCAL_LOCKOUT. (5/5)
> [...]
>  Testing:
> All functions tested ok on an USBTMC-USB488 compliant oscilloscope

How?  Did you extend the usbtmc_ioctl tool?


Regards,
Clemens


Re: [PATCH v7 0/5] usb: usbtmc: Add support for missing functions in USBTMC-USB488 spec

2016-01-28 Thread Clemens Ladisch
Dave Penkler wrote:
> Implement support for the USB488 defined READ_STATUS_BYTE ioctl (1/5)
> and SRQ notifications with fasync (2/5) and poll/select (3/5) in order
> to be able to synchronize with variable duration instrument
> operations.
>
> Add convenience ioctl to return all device capabilities (4/5)
>
> Add ioctls for other USB488 requests: REN_CONTROL, GOTO_LOCAL and
> LOCAL_LOCKOUT. (5/5)
> [...]
>  Testing:
> All functions tested ok on an USBTMC-USB488 compliant oscilloscope

How?  Did you extend the usbtmc_ioctl tool?


Regards,
Clemens


Re: [PATCH 1/2] usb: gadget: f_midi: refactor state machine

2015-12-23 Thread Clemens Ladisch
Felipe Ferreri Tonello wrote:
>> Running status is feature.
>
>What do you mean by that?

That this behavior is intended, and required.

> I don't qualify writing a *wrong* MIDI-USB
>packet because of a previous MIDI message as a feature.

The MIDI Specification qualifies Running Status as a feature.


Regards,
Clemens

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH 1/2] usb: gadget: f_midi: refactor state machine

2015-12-23 Thread Clemens Ladisch
Felipe Ferreri Tonello wrote:
>> Running status is feature.
>
>What do you mean by that?

That this behavior is intended, and required.

> I don't qualify writing a *wrong* MIDI-USB
>packet because of a previous MIDI message as a feature.

The MIDI Specification qualifies Running Status as a feature.


Regards,
Clemens

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH 1/2] usb: gadget: f_midi: refactor state machine

2015-12-22 Thread Clemens Ladisch
Felipe F. Tonello wrote:
> This refactor includes the following:
>  * Cleaner state machine code;

It does not correctly handle system real time messages inserted between
the status and data bytes of other messages.

>  * Reset state if MIDI message parsed is non-conformant;

Why?  In a byte stream like "C1 C3 44", where the data byte of the first
message was lost, the reset would also drop the second message.

>  * Fixed bug when a conformant MIDI message was followed by a non-conformant
>causing the MIDI-USB message to use old temporary data (port->data[0..1]),
>thus packing a wrong MIDI-USB request.

Running status is feature.


Regards,
Clemens
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH 1/2] usb: gadget: f_midi: refactor state machine

2015-12-22 Thread Clemens Ladisch
Felipe F. Tonello wrote:
> This refactor includes the following:
>  * Cleaner state machine code;

It does not correctly handle system real time messages inserted between
the status and data bytes of other messages.

>  * Reset state if MIDI message parsed is non-conformant;

Why?  In a byte stream like "C1 C3 44", where the data byte of the first
message was lost, the reset would also drop the second message.

>  * Fixed bug when a conformant MIDI message was followed by a non-conformant
>causing the MIDI-USB message to use old temporary data (port->data[0..1]),
>thus packing a wrong MIDI-USB request.

Running status is feature.


Regards,
Clemens
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] ALSA: usb-audio: constify usb_protocol_ops structures

2015-12-11 Thread Clemens Ladisch
Julia Lawall wrote:
> The usb_protocol_ops structures are never modified, so declare them as
> const.
>
> Done with the help of Coccinelle.
>
> Signed-off-by: Julia Lawall 

Acked-by: Clemens Ladisch 

> ---
>  sound/usb/midi.c |   25 +
>  1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/sound/usb/midi.c b/sound/usb/midi.c
> index ee212e7..cc39f63 100644
> --- a/sound/usb/midi.c
> +++ b/sound/usb/midi.c
> @@ -112,7 +112,7 @@ struct snd_usb_midi {
>   struct usb_interface *iface;
>   const struct snd_usb_audio_quirk *quirk;
>   struct snd_rawmidi *rmidi;
> - struct usb_protocol_ops *usb_protocol_ops;
> + const struct usb_protocol_ops *usb_protocol_ops;
>   struct list_head list;
>   struct timer_list error_timer;
>   spinlock_t disc_lock;
> @@ -671,31 +671,32 @@ static void snd_usbmidi_standard_output(struct 
> snd_usb_midi_out_endpoint *ep,
>   }
>  }
>
> -static struct usb_protocol_ops snd_usbmidi_standard_ops = {
> +static const struct usb_protocol_ops snd_usbmidi_standard_ops = {
>   .input = snd_usbmidi_standard_input,
>   .output = snd_usbmidi_standard_output,
>   .output_packet = snd_usbmidi_output_standard_packet,
>  };
>
> -static struct usb_protocol_ops snd_usbmidi_midiman_ops = {
> +static const struct usb_protocol_ops snd_usbmidi_midiman_ops = {
>   .input = snd_usbmidi_midiman_input,
>   .output = snd_usbmidi_standard_output,
>   .output_packet = snd_usbmidi_output_midiman_packet,
>  };
>
> -static struct usb_protocol_ops snd_usbmidi_maudio_broken_running_status_ops 
> = {
> +static const
> +struct usb_protocol_ops snd_usbmidi_maudio_broken_running_status_ops = {
>   .input = snd_usbmidi_maudio_broken_running_status_input,
>   .output = snd_usbmidi_standard_output,
>   .output_packet = snd_usbmidi_output_standard_packet,
>  };
>
> -static struct usb_protocol_ops snd_usbmidi_cme_ops = {
> +static const struct usb_protocol_ops snd_usbmidi_cme_ops = {
>   .input = snd_usbmidi_cme_input,
>   .output = snd_usbmidi_standard_output,
>   .output_packet = snd_usbmidi_output_standard_packet,
>  };
>
> -static struct usb_protocol_ops snd_usbmidi_ch345_broken_sysex_ops = {
> +static const struct usb_protocol_ops snd_usbmidi_ch345_broken_sysex_ops = {
>   .input = ch345_broken_sysex_input,
>   .output = snd_usbmidi_standard_output,
>   .output_packet = snd_usbmidi_output_standard_packet,
> @@ -795,7 +796,7 @@ static void snd_usbmidi_akai_output(struct 
> snd_usb_midi_out_endpoint *ep,
>   }
>  }
>
> -static struct usb_protocol_ops snd_usbmidi_akai_ops = {
> +static const struct usb_protocol_ops snd_usbmidi_akai_ops = {
>   .input = snd_usbmidi_akai_input,
>   .output = snd_usbmidi_akai_output,
>  };
> @@ -835,7 +836,7 @@ static void snd_usbmidi_novation_output(struct 
> snd_usb_midi_out_endpoint *ep,
>   urb->transfer_buffer_length = 2 + count;
>  }
>
> -static struct usb_protocol_ops snd_usbmidi_novation_ops = {
> +static const struct usb_protocol_ops snd_usbmidi_novation_ops = {
>   .input = snd_usbmidi_novation_input,
>   .output = snd_usbmidi_novation_output,
>  };
> @@ -867,7 +868,7 @@ static void snd_usbmidi_raw_output(struct 
> snd_usb_midi_out_endpoint *ep,
>   urb->transfer_buffer_length = count;
>  }
>
> -static struct usb_protocol_ops snd_usbmidi_raw_ops = {
> +static const struct usb_protocol_ops snd_usbmidi_raw_ops = {
>   .input = snd_usbmidi_raw_input,
>   .output = snd_usbmidi_raw_output,
>  };
> @@ -883,7 +884,7 @@ static void snd_usbmidi_ftdi_input(struct 
> snd_usb_midi_in_endpoint *ep,
>   snd_usbmidi_input_data(ep, 0, buffer + 2, buffer_length - 2);
>  }
>
> -static struct usb_protocol_ops snd_usbmidi_ftdi_ops = {
> +static const struct usb_protocol_ops snd_usbmidi_ftdi_ops = {
>   .input = snd_usbmidi_ftdi_input,
>   .output = snd_usbmidi_raw_output,
>  };
> @@ -927,7 +928,7 @@ static void snd_usbmidi_us122l_output(struct 
> snd_usb_midi_out_endpoint *ep,
>   urb->transfer_buffer_length = ep->max_transfer;
>  }
>
> -static struct usb_protocol_ops snd_usbmidi_122l_ops = {
> +static const struct usb_protocol_ops snd_usbmidi_122l_ops = {
>   .input = snd_usbmidi_us122l_input,
>   .output = snd_usbmidi_us122l_output,
>  };
> @@ -1060,7 +1061,7 @@ static void snd_usbmidi_emagic_output(struct 
> snd_usb_midi_out_endpoint *ep,
>   urb->transfer_buffer_length = ep->max_transfer - buf_free;
>  }
>
> -static struct usb_protocol_ops snd_usbmidi_emagic_ops = {
> +static const struct usb_protocol_ops snd_usbmidi_emagic_

Re: [PATCH] ALSA: usb-audio: constify usb_protocol_ops structures

2015-12-11 Thread Clemens Ladisch
Julia Lawall wrote:
> The usb_protocol_ops structures are never modified, so declare them as
> const.
>
> Done with the help of Coccinelle.
>
> Signed-off-by: Julia Lawall <julia.law...@lip6.fr>

Acked-by: Clemens Ladisch <clem...@ladisch.de>

> ---
>  sound/usb/midi.c |   25 +
>  1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/sound/usb/midi.c b/sound/usb/midi.c
> index ee212e7..cc39f63 100644
> --- a/sound/usb/midi.c
> +++ b/sound/usb/midi.c
> @@ -112,7 +112,7 @@ struct snd_usb_midi {
>   struct usb_interface *iface;
>   const struct snd_usb_audio_quirk *quirk;
>   struct snd_rawmidi *rmidi;
> - struct usb_protocol_ops *usb_protocol_ops;
> + const struct usb_protocol_ops *usb_protocol_ops;
>   struct list_head list;
>   struct timer_list error_timer;
>   spinlock_t disc_lock;
> @@ -671,31 +671,32 @@ static void snd_usbmidi_standard_output(struct 
> snd_usb_midi_out_endpoint *ep,
>   }
>  }
>
> -static struct usb_protocol_ops snd_usbmidi_standard_ops = {
> +static const struct usb_protocol_ops snd_usbmidi_standard_ops = {
>   .input = snd_usbmidi_standard_input,
>   .output = snd_usbmidi_standard_output,
>   .output_packet = snd_usbmidi_output_standard_packet,
>  };
>
> -static struct usb_protocol_ops snd_usbmidi_midiman_ops = {
> +static const struct usb_protocol_ops snd_usbmidi_midiman_ops = {
>   .input = snd_usbmidi_midiman_input,
>   .output = snd_usbmidi_standard_output,
>   .output_packet = snd_usbmidi_output_midiman_packet,
>  };
>
> -static struct usb_protocol_ops snd_usbmidi_maudio_broken_running_status_ops 
> = {
> +static const
> +struct usb_protocol_ops snd_usbmidi_maudio_broken_running_status_ops = {
>   .input = snd_usbmidi_maudio_broken_running_status_input,
>   .output = snd_usbmidi_standard_output,
>   .output_packet = snd_usbmidi_output_standard_packet,
>  };
>
> -static struct usb_protocol_ops snd_usbmidi_cme_ops = {
> +static const struct usb_protocol_ops snd_usbmidi_cme_ops = {
>   .input = snd_usbmidi_cme_input,
>   .output = snd_usbmidi_standard_output,
>   .output_packet = snd_usbmidi_output_standard_packet,
>  };
>
> -static struct usb_protocol_ops snd_usbmidi_ch345_broken_sysex_ops = {
> +static const struct usb_protocol_ops snd_usbmidi_ch345_broken_sysex_ops = {
>   .input = ch345_broken_sysex_input,
>   .output = snd_usbmidi_standard_output,
>   .output_packet = snd_usbmidi_output_standard_packet,
> @@ -795,7 +796,7 @@ static void snd_usbmidi_akai_output(struct 
> snd_usb_midi_out_endpoint *ep,
>   }
>  }
>
> -static struct usb_protocol_ops snd_usbmidi_akai_ops = {
> +static const struct usb_protocol_ops snd_usbmidi_akai_ops = {
>   .input = snd_usbmidi_akai_input,
>   .output = snd_usbmidi_akai_output,
>  };
> @@ -835,7 +836,7 @@ static void snd_usbmidi_novation_output(struct 
> snd_usb_midi_out_endpoint *ep,
>   urb->transfer_buffer_length = 2 + count;
>  }
>
> -static struct usb_protocol_ops snd_usbmidi_novation_ops = {
> +static const struct usb_protocol_ops snd_usbmidi_novation_ops = {
>   .input = snd_usbmidi_novation_input,
>   .output = snd_usbmidi_novation_output,
>  };
> @@ -867,7 +868,7 @@ static void snd_usbmidi_raw_output(struct 
> snd_usb_midi_out_endpoint *ep,
>   urb->transfer_buffer_length = count;
>  }
>
> -static struct usb_protocol_ops snd_usbmidi_raw_ops = {
> +static const struct usb_protocol_ops snd_usbmidi_raw_ops = {
>   .input = snd_usbmidi_raw_input,
>   .output = snd_usbmidi_raw_output,
>  };
> @@ -883,7 +884,7 @@ static void snd_usbmidi_ftdi_input(struct 
> snd_usb_midi_in_endpoint *ep,
>   snd_usbmidi_input_data(ep, 0, buffer + 2, buffer_length - 2);
>  }
>
> -static struct usb_protocol_ops snd_usbmidi_ftdi_ops = {
> +static const struct usb_protocol_ops snd_usbmidi_ftdi_ops = {
>   .input = snd_usbmidi_ftdi_input,
>   .output = snd_usbmidi_raw_output,
>  };
> @@ -927,7 +928,7 @@ static void snd_usbmidi_us122l_output(struct 
> snd_usb_midi_out_endpoint *ep,
>   urb->transfer_buffer_length = ep->max_transfer;
>  }
>
> -static struct usb_protocol_ops snd_usbmidi_122l_ops = {
> +static const struct usb_protocol_ops snd_usbmidi_122l_ops = {
>   .input = snd_usbmidi_us122l_input,
>   .output = snd_usbmidi_us122l_output,
>  };
> @@ -1060,7 +1061,7 @@ static void snd_usbmidi_emagic_output(struct 
> snd_usb_midi_out_endpoint *ep,
>   urb->transfer_buffer_length = ep->max_transfer - buf_free;
>  }
>
> -static struct usb_protocol_ops snd_usbmidi_emagic_ops = {
&

Re: Ques: [kernel/time/*] Is there any disadvantage in using usleep_range for more than 20ms delay ?

2015-12-08 Thread Clemens Ladisch
Aniroop Mathur wrote:
> As in the kernel documentation, it is mentioned to use msleep for
> 10ms+ delay, I am confused whether there would be any disadvantage in
> using usleep_range for higher delays values because normally drivers
> have variety of delays used (2, 10, 20, 40, 100, 500 ms).
>
> So, could you please help to confirm that if we use usleep_range for
> inserting delays greater than 20 ms, would it be harmful or beneficial
> or does not make any difference at all ?

As the documentation told you, usleep_range() is likely to require
a separate interrupt, while msleep() is likely to round to some other,
already-scheduled interrupt.  The former is possibly harmful regarding
CPU and power usage; you have to balance it against your need for
accuracy.

(And usleep_range() has a 32-bit nanosecond limit on 32-bit
architectures.)


Regards,
Clemens
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: Ques: [kernel/time/*] Is there any disadvantage in using usleep_range for more than 20ms delay ?

2015-12-08 Thread Clemens Ladisch
Aniroop Mathur wrote:
> As in the kernel documentation, it is mentioned to use msleep for
> 10ms+ delay, I am confused whether there would be any disadvantage in
> using usleep_range for higher delays values because normally drivers
> have variety of delays used (2, 10, 20, 40, 100, 500 ms).
>
> So, could you please help to confirm that if we use usleep_range for
> inserting delays greater than 20 ms, would it be harmful or beneficial
> or does not make any difference at all ?

As the documentation told you, usleep_range() is likely to require
a separate interrupt, while msleep() is likely to round to some other,
already-scheduled interrupt.  The former is possibly harmful regarding
CPU and power usage; you have to balance it against your need for
accuracy.

(And usleep_range() has a 32-bit nanosecond limit on 32-bit
architectures.)


Regards,
Clemens
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] Documentation: email-clients.txt

2015-12-07 Thread Clemens Ladisch
Sanidhya Solanki wrote:
>  Claws Mail (GUI)
>
> -Works. Some people use this successfully for patches.
> +Tested and Works as of December 2015. Some people use this successfully
> +for patches.

"Some people" still doesn't include you:

> -Thunderbird is an Outlook clone that likes to mangle text, but there
> are ways -to coerce it into behaving.
> +Thunderbird is an Outlook clone that likes to mangle text, but there
> are +ways to coerce it into behaving. In December 2015, the internal
> editor +options do not appear to work.

How exactly?  Thunderbird appears to work for me, and doesn't break long lines 
or sends plain text as "flowed".

And version numbers might be more useful than dates.


Regards,
Clemens
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] Documentation: email-clients.txt

2015-12-07 Thread Clemens Ladisch
Sanidhya Solanki wrote:
>  Claws Mail (GUI)
>
> -Works. Some people use this successfully for patches.
> +Tested and Works as of December 2015. Some people use this successfully
> +for patches.

"Some people" still doesn't include you:

> -Thunderbird is an Outlook clone that likes to mangle text, but there
> are ways -to coerce it into behaving.
> +Thunderbird is an Outlook clone that likes to mangle text, but there
> are +ways to coerce it into behaving. In December 2015, the internal
> editor +options do not appear to work.

How exactly?  Thunderbird appears to work for me, and doesn't break long lines 
or sends plain text as "flowed".

And version numbers might be more useful than dates.


Regards,
Clemens
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: A new, fast and "unbreakable" encryption algorithm

2015-12-03 Thread Clemens Ladisch
Ismail Kizir wrote:
> What means "did not look random"?

A plaintext consisting of repeated bytes (zero, or other values)
eventually makes your algorithm go into a loop, which results in
repeated bytes.

> On the pictures, there is also an example of "full 0"(it appears red,
> but it is full 0 bmp) example.
> And it "looks" perfectly random.

No, red is _not_ perfectly random.  When I see a red picture, I have
evidence that the plaintext was zeroes.


Regards,
Clemens
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: A new, fast and "unbreakable" encryption algorithm

2015-12-03 Thread Clemens Ladisch
Ismail Kizir wrote:
> What means "did not look random"?

A plaintext consisting of repeated bytes (zero, or other values)
eventually makes your algorithm go into a loop, which results in
repeated bytes.

> On the pictures, there is also an example of "full 0"(it appears red,
> but it is full 0 bmp) example.
> And it "looks" perfectly random.

No, red is _not_ perfectly random.  When I see a red picture, I have
evidence that the plaintext was zeroes.


Regards,
Clemens
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


  1   2   3   4   >