Re: [PATCH] firewire-ohci: work around oversized DMA reads on JMicron controllers
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
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
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
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
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
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
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
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
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
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
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
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
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
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()
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()
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
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
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
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
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
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
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
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
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?
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?
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
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
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
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
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.
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.
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
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
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
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
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)
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)
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()
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()
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
>> 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
>> 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
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
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 ?
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 ?
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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 ?
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 ?
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
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
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
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
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/