Solved Hercules RMX2

Solved Hercules RMX2

[Daniel Schürmann]

Hallo Alsa Developers,

I have fixed Support for Hercules RMX2.

You can find the Bug and the patch here:
https://bugs.launchpad.net/mixxx/+bug/1096687

The issue was caused by the first time setting the rate.
I have changed the usb_msg_timeout to 1500 ms to make it work.
The measures time is a ~1240 ms.
All following rate settings are finished in not time < 4 ms

Sound is now working fine but there are still IO errors just after
connecting the device

errno 5 - input/output error

Here the extended Log:
Apr 14 23:50:37 KellerPc kernel: [19213.746929] snd_usb_autoresume() -19
ID:6f8b113 chip->shutdown 0 chip->probing 1.
Apr 14 23:50:37 KellerPc kernel: [19213.746934] -5 1 cannot get ctl value:
req = 0x83, wValue = 0x201, wIndex = 0x0, type = 4
Apr 14 23:50:37 KellerPc kernel: [19213.747053] snd_usb_autoresume() -19
ID:6f8b113 chip->shutdown 0 chip->probing 1.
Apr 14 23:50:37 KellerPc kernel: [19213.747058] -5 1 cannot get ctl value:
req = 0x83, wValue = 0x200, wIndex = 0x0, type = 4
Apr 14 23:50:37 KellerPc kernel: [19213.747430] snd_usb_autoresume() -19
ID:6f8b113 chip->shutdown 0 chip->probing 1.
Apr 14 23:50:37 KellerPc kernel: [19213.747435] -5 1 cannot get ctl value:
req = 0x83, wValue = 0x201, wIndex = 0x0, type = 4
Apr 14 23:50:37 KellerPc kernel: [19213.747554] snd_usb_autoresume() -19
ID:6f8b113 chip->shutdown 0 chip->probing 1.
Apr 14 23:50:37 KellerPc kernel: [19213.747559] -5 1 cannot get ctl value:
req = 0x83, wValue = 0x200, wIndex = 0x0, type = 4
Apr 14 23:50:37 KellerPc kernel: [19213.752599] usbcore: registered new
interface driver snd-usb-audio
Apr 14 23:50:37 KellerPc kernel: [19213.775204] snd_usb_ctl_msg()
dev:ed5eb000 pipe:80000d80 request:02, requesttype:a1, value:0201,
index:0a00, data_size:8.
Apr 14 23:50:37 KellerPc kernel: [19213.776264] snd_usb_ctl_msg()
dev:ed5eb000 pipe:80000d80 request:02, requesttype:a1, value:0201,
index:0a00, data_size:8.
Apr 14 23:50:37 KellerPc kernel: [19213.776436] snd_usb_ctl_msg()
dev:ed5eb000 pipe:80000d80 request:02, requesttype:a1, value:0201,
index:0a00, data_size:8.

The current solution is fine for me, but maybe there is an other place for
tweaking the startup timing. Any ideas?

Is it possible to include the fix in trunk?

Thank you.

Kind regards,

Daniel Schürmann

Re: Solved Hercules RMX2

[Gabriel M. Beddingfield]

Hi Daniel,

On 04/15/2013 03:04 PM, Daniel Schürmann wrote:
> Hallo Alsa Developers,
>
> I have fixed Support for Hercules RMX2.
>
> You can find the Bug and the patch here:
> https://bugs.launchpad.net/mixxx/+bug/1096687
[snip]
> Is it possible to include the fix in trunk?

Since the RMX2's /driver/ isn't in the Linux kernel -- the answer is 
probably no.  Notice that the DEB package has "DKMS" in the name -- 
indicating that this is an out-of-tree driver.

So, you would need to start by submitting a patch to add the RMX driver 
to the kernel.

-gabriel

Re: Solved Hercules RMX2

[Daniel Schürmann]

Hi Gabriel,

Thank you for your quick response.
The patch against the alsa-kernel is also attached at
https://bugs.launchpad.net/mixxx/+bug/1096687 and here:

diff --git a/sound/usb/helper.c b/sound/usb/helper.c
index c1db28f..e044804 100644
--- a/sound/usb/helper.c
+++ b/sound/usb/helper.c
@@ -23,6 +23,9 @@
 #include "helper.h"
 #include "quirks.h"

+/* Hercules RMX2 needs 1240 ms for setting the sample rate the first time */
+#define USB_MSG_TIMEOUT 1500
+
 /*
  * combine bytes and get an integer value
  */
@@ -93,7 +96,7 @@ int snd_usb_ctl_msg(struct usb_device *dev, unsigned
int pipe, __u8 request,
 			return -ENOMEM;
 	}
 	err = usb_control_msg(dev, pipe, request, requesttype,
-			      value, index, buf, size, 1000);
+			      value, index, buf, size, USB_MSG_TIMEOUT);
 	if (size > 0) {
 		memcpy(data, buf, size);
 		kfree(buf);



Kind regards,

Daniel





2013/4/16 Gabriel M. Beddingfield <gabrbedd@gmail.com>

>
> Hi Daniel,
>
>
> On 04/15/2013 03:04 PM, Daniel Schürmann wrote:
>
>> Hallo Alsa Developers,
>>
>> I have fixed Support for Hercules RMX2.
>>
>> You can find the Bug and the patch here:
>> https://bugs.launchpad.net/**mixxx/+bug/1096687<https://bugs.launchpad.net/mixxx/+bug/1096687>
>>
> [snip]
>
>  Is it possible to include the fix in trunk?
>>
>
> Since the RMX2's /driver/ isn't in the Linux kernel -- the answer is
> probably no.  Notice that the DEB package has "DKMS" in the name --
> indicating that this is an out-of-tree driver.
>
> So, you would need to start by submitting a patch to add the RMX driver to
> the kernel.
>
> -gabriel
>
>
>

Re: Solved Hercules RMX2

[Gabriel M. Beddingfield]

+Daniel Mack <daniel@caiaq.de> (I think he's doing a lot of USB audio 
maint. these days)

On 04/16/2013 08:12 AM, Daniel Schürmann wrote:
> Hi Gabriel,
>
> Thank you for your quick response.
> The patch against the alsa-kernel is also attached at
> https://bugs.launchpad.net/mixxx/+bug/1096687 and here:

OK, I see now.  FYI, most maintainers prefer that you submit patches 
according to the guidelines in Documentation/SubmittingPatches.  (I'm 
not a maintainer, BTW...)

>
> diff --git a/sound/usb/helper.c b/sound/usb/helper.c
> index c1db28f..e044804 100644
> --- a/sound/usb/helper.c
> +++ b/sound/usb/helper.c
> @@ -23,6 +23,9 @@
>   #include "helper.h"
>   #include "quirks.h"
>
> +/* Hercules RMX2 needs 1240 ms for setting the sample rate the first time */
> +#define USB_MSG_TIMEOUT 1500
> +
>   /*
>    * combine bytes and get an integer value
>    */
> @@ -93,7 +96,7 @@ int snd_usb_ctl_msg(struct usb_device *dev, unsigned int pipe, __u8 request,
>   			return -ENOMEM;
>   	}
>   	err = usb_control_msg(dev, pipe, request, requesttype,
> -			      value, index, buf, size, 1000);
> +			      value, index, buf, size, USB_MSG_TIMEOUT);
>   	if (size > 0) {
>   		memcpy(data, buf, size);
>   		kfree(buf);

This changes the value for every USB audio device... not just the RMX2. 
  Daniel, is there a better way to do this?

-gabriel

Re: Solved Hercules RMX2

[Daniel Mack]

On 16.04.2013 17:35, Gabriel M. Beddingfield wrote:
> On 04/16/2013 08:12 AM, Daniel Schürmann wrote:
>> Hi Gabriel,
>>
>> Thank you for your quick response.
>> The patch against the alsa-kernel is also attached at
>> https://bugs.launchpad.net/mixxx/+bug/1096687 and here:
> 
> OK, I see now.  FYI, most maintainers prefer that you submit patches 
> according to the guidelines in Documentation/SubmittingPatches.  (I'm 
> not a maintainer, BTW...)
> 
>>
>> diff --git a/sound/usb/helper.c b/sound/usb/helper.c
>> index c1db28f..e044804 100644
>> --- a/sound/usb/helper.c
>> +++ b/sound/usb/helper.c
>> @@ -23,6 +23,9 @@
>>   #include "helper.h"
>>   #include "quirks.h"
>>
>> +/* Hercules RMX2 needs 1240 ms for setting the sample rate the first time */
>> +#define USB_MSG_TIMEOUT 1500
>> +
>>   /*
>>    * combine bytes and get an integer value
>>    */
>> @@ -93,7 +96,7 @@ int snd_usb_ctl_msg(struct usb_device *dev, unsigned int pipe, __u8 request,
>>   			return -ENOMEM;
>>   	}
>>   	err = usb_control_msg(dev, pipe, request, requesttype,
>> -			      value, index, buf, size, 1000);
>> +			      value, index, buf, size, USB_MSG_TIMEOUT);
>>   	if (size > 0) {
>>   		memcpy(data, buf, size);
>>   		kfree(buf);
> 
> This changes the value for every USB audio device... not just the RMX2. 
>   Daniel, is there a better way to do this?

Yes, you can store an integer in struct snd_usb_audio (call it
usb_msg_timeout or s.th.), initialize it to 1000 from
snd_usb_audio_probe() and add a quirk to override that value in
snd_usb_apply_boot_quirk().


Thanks,
Daniel

Re: Solved Hercules RMX2

[Daniel Schürmann]

Hi Daniel,

Thank you for the suggestion. If there is no other solution than increasing
the timeout, IMHO the code is simpler if we just increase the timeout for
all devices. This way we will automatically fix all other devices based on
the same hardware.

The long timeout is only quired by the first write command and 1 s is
already quite long. Do you have another idea how to fix this?

Do you have a Idea what the remaining error messages are about? I cannot
see a misbehavior anyway.

Just read:
https://www.kernel.org/doc/Documentation/SubmittingPatches
Is the patch fine to send out?
Against which Kernel should I patch?
Where should I send it?

Thank you for your help,

Daniel





2013/4/16 Daniel Mack <zonque@gmail.com>

> On 16.04.2013 17:35, Gabriel M. Beddingfield wrote:
> > On 04/16/2013 08:12 AM, Daniel Schürmann wrote:
> >> Hi Gabriel,
> >>
> >> Thank you for your quick response.
> >> The patch against the alsa-kernel is also attached at
> >> https://bugs.launchpad.net/mixxx/+bug/1096687 and here:
> >
> > OK, I see now.  FYI, most maintainers prefer that you submit patches
> > according to the guidelines in Documentation/SubmittingPatches.  (I'm
> > not a maintainer, BTW...)
> >
> >>
> >> diff --git a/sound/usb/helper.c b/sound/usb/helper.c
> >> index c1db28f..e044804 100644
> >> --- a/sound/usb/helper.c
> >> +++ b/sound/usb/helper.c
> >> @@ -23,6 +23,9 @@
> >>   #include "helper.h"
> >>   #include "quirks.h"
> >>
> >> +/* Hercules RMX2 needs 1240 ms for setting the sample rate the first
> time */
> >> +#define USB_MSG_TIMEOUT 1500
> >> +
> >>   /*
> >>    * combine bytes and get an integer value
> >>    */
> >> @@ -93,7 +96,7 @@ int snd_usb_ctl_msg(struct usb_device *dev, unsigned
> int pipe, __u8 request,
> >>                      return -ENOMEM;
> >>      }
> >>      err = usb_control_msg(dev, pipe, request, requesttype,
> >> -                          value, index, buf, size, 1000);
> >> +                          value, index, buf, size, USB_MSG_TIMEOUT);
> >>      if (size > 0) {
> >>              memcpy(data, buf, size);
> >>              kfree(buf);
> >
> > This changes the value for every USB audio device... not just the RMX2.
> >   Daniel, is there a better way to do this?
>
> Yes, you can store an integer in struct snd_usb_audio (call it
> usb_msg_timeout or s.th.), initialize it to 1000 from
> snd_usb_audio_probe() and add a quirk to override that value in
> snd_usb_apply_boot_quirk().
>
>
> Thanks,
> Daniel
>
>

Re: Solved Hercules RMX2

[Daniel Mack]

Hi Daniel,

On 16.04.2013 21:14, Daniel Schürmann wrote:
> Thank you for the suggestion. If there is no other solution than
> increasing the timeout, IMHO the code is simpler if we just increase the
> timeout for all devices. This way we will automatically fix all other
> devices based on the same hardware.

How many are there? Can you come up with a list of VID/PID? I never
heard of problems of that kind really.

OTOH, I can't think of any obvious reason how a longer timeout should
cause any problem for other devices, but as a general concern, we like
to keep quirks as specific to the device as possible.

> The long timeout is only quired by the first write command and 1 s is
> already quite long. Do you have another idea how to fix this?

Fix the firmware :) I can't think of anything that take a full second
inside such a device.

> Do you have a Idea what the remaining error messages are about? I cannot
> see a misbehavior anyway.

Also firmware problems. You can ignore them for now.

> Just read:
> https://www.kernel.org/doc/Documentation/SubmittingPatches
> Is the patch fine to send out?

Which one? Also, before sending it, run scripts/checkpatch.pl on it,
which will point out obvious style issues.

> Against which Kernel should I patch?

https://git.kernel.org/cgit/linux/kernel/git/broonie/sound.git/log/?h=for-next

> Where should I send it?

To the alsa-devel ML, and take Takashi and Clemens in Cc: (both copied
in this mail).



Thanks,
Daniel


> 
> 
> 2013/4/16 Daniel Mack <zonque@gmail.com <mailto:zonque@gmail.com>>
> 
>     On 16.04.2013 17:35, Gabriel M. Beddingfield wrote:
>     > On 04/16/2013 08:12 AM, Daniel Schürmann wrote:
>     >> Hi Gabriel,
>     >>
>     >> Thank you for your quick response.
>     >> The patch against the alsa-kernel is also attached at
>     >> https://bugs.launchpad.net/mixxx/+bug/1096687 and here:
>     >
>     > OK, I see now.  FYI, most maintainers prefer that you submit patches
>     > according to the guidelines in Documentation/SubmittingPatches.  (I'm
>     > not a maintainer, BTW...)
>     >
>     >>
>     >> diff --git a/sound/usb/helper.c b/sound/usb/helper.c
>     >> index c1db28f..e044804 100644
>     >> --- a/sound/usb/helper.c
>     >> +++ b/sound/usb/helper.c
>     >> @@ -23,6 +23,9 @@
>     >>   #include "helper.h"
>     >>   #include "quirks.h"
>     >>
>     >> +/* Hercules RMX2 needs 1240 ms for setting the sample rate the
>     first time */
>     >> +#define USB_MSG_TIMEOUT 1500
>     >> +
>     >>   /*
>     >>    * combine bytes and get an integer value
>     >>    */
>     >> @@ -93,7 +96,7 @@ int snd_usb_ctl_msg(struct usb_device *dev,
>     unsigned int pipe, __u8 request,
>     >>                      return -ENOMEM;
>     >>      }
>     >>      err = usb_control_msg(dev, pipe, request, requesttype,
>     >> -                          value, index, buf, size, 1000);
>     >> +                          value, index, buf, size, USB_MSG_TIMEOUT);
>     >>      if (size > 0) {
>     >>              memcpy(data, buf, size);
>     >>              kfree(buf);
>     >
>     > This changes the value for every USB audio device... not just the
>     RMX2.
>     >   Daniel, is there a better way to do this?
> 
>     Yes, you can store an integer in struct snd_usb_audio (call it
>     usb_msg_timeout or s.th.), initialize it to 1000 from
>     snd_usb_audio_probe() and add a quirk to override that value in
>     snd_usb_apply_boot_quirk().
> 
> 
>     Thanks,
>     Daniel
> 
> 

Re: Solved Hercules RMX2

[Daniel Mack]

On 16.04.2013 21:23, Daniel Mack wrote:
> On 16.04.2013 21:14, Daniel Schürmann wrote:

>> Against which Kernel should I patch?
> 
> https://git.kernel.org/cgit/linux/kernel/git/broonie/sound.git/log/?h=for-next

Eh, sorry. This one:

https://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/ for-next

Fwd: Solved Hercules RMX2

[Daniel Schürmann]

Hi Daniel,

thank your advice!

I am not a Hercules developer, I am developer of the Mixxx DJ Software and
I am just trying to make
the thing work!

I have just read within the USB 2.0 speck to find out if there are specific
timeouts to deal with.

I have found:
§ 9.2.6.4 Standard Device Requests

If I understand it correct, there is a 5-second limit for storing data into
the device.
I think changing the sample rate is such a operation.

So what about changing the Limit to 5000 ms for all "Set requests"
bmRequest = 21.
See Audio20 § 5.2.2
All other timeouts might be reduced to 500 ms but I would leave them
untouched at 1000 ms , just in case.

I would prefer this general solution over a device specific one.
What do you think?

Kind regards,

Daniel




2013/4/16 Daniel Mack <zonque@gmail.com>

> On 16.04.2013 21:23, Daniel Mack wrote:
> > On 16.04.2013 21:14, Daniel Schürmann wrote:
>
> >> Against which Kernel should I patch?
> >
> >
> https://git.kernel.org/cgit/linux/kernel/git/broonie/sound.git/log/?h=for-next
>
> Eh, sorry. This one:
>
> https://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/ for-next
>
>

Re: Fwd: Solved Hercules RMX2

[Daniel Mack]

Hi Daniel,

On 17.04.2013 08:32, Daniel Schürmann wrote:
> thank your advice!
> 
> I am not a Hercules developer, I am developer of the Mixxx DJ Software and
> I am just trying to make
> the thing work!
> 
> I have just read within the USB 2.0 speck to find out if there are specific
> timeouts to deal with.
> 
> I have found:
> § 9.2.6.4 Standard Device Requests
> 
> If I understand it correct, there is a 5-second limit for storing data into
> the device.
> I think changing the sample rate is such a operation.
> 
> So what about changing the Limit to 5000 ms for all "Set requests"
> bmRequest = 21.
> See Audio20 § 5.2.2
> All other timeouts might be reduced to 500 ms but I would leave them
> untouched at 1000 ms , just in case.

Ok, can you cook a patch which sets the limit parameter
usb_control_msg() to 5000 for SET request but leaves the former default
value of 1000 in other cases? The patch should be all local to
snd_usb_ctl_msg() then.


Thanks,
Daniel

[PATCH] ALSA: snd-usb-audio: set the timeout for usb control set messages to 5000 ms

[Daniel Schürmann]

This is a patch against
https://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/
581cbef46ae60073252208fc1f26dd1044f6e215

Set the timeout for USB control set messages according to the USB 2 spec
§9.2.6.4 to 5000 ms.
To avoid new issues, the get timeout is unchanged at 1000 ms, though it
is 500 ms in the spec.
This patch is required to run the Hercules RMX2 which needs a timeout >
1240 ms

Signed-off-by: Daniel Schürmann <daschuer@mixxx.org>
---
diff --git a/sound/usb/helper.c b/sound/usb/helper.c
index c1db28f..b4f3876 100644
--- a/sound/usb/helper.c
+++ b/sound/usb/helper.c
@@ -21,7 +21,11 @@
 
 #include "usbaudio.h"
 #include "helper.h"
-#include "quirks.h"
+
+/* Value from 9.2.6.4 USB 2 spec */
+#define USB_MSG_SET_TIMEOUT 5000
+/* Value from spec is 500 but we pick 1000 for legacy reasons */
+#define USB_MSG_GET_TIMEOUT 1000
 
 /*
  * combine bytes and get an integer value
@@ -86,14 +90,24 @@ int snd_usb_ctl_msg(struct usb_device *dev, unsigned
int pipe, __u8 request,
 {
     int err;
     void *buf = NULL;
+    int timeout;
 
     if (size > 0) {
         buf = kmemdup(data, size, GFP_KERNEL);
         if (!buf)
             return -ENOMEM;
     }
+
+    if (requesttype & USB_DIR_IN) {
+        /* Get Request */
+        timeout = USB_MSG_GET_TIMEOUT;
+    } else {
+        /* Set Request */
+        timeout = USB_MSG_SET_TIMEOUT;
+    }
     err = usb_control_msg(dev, pipe, request, requesttype,
-                  value, index, buf, size, 1000);
+                  value, index, buf, size, timeout);
+
     if (size > 0) {
         memcpy(data, buf, size);
         kfree(buf);

Re: [PATCH] ALSA: snd-usb-audio: set the timeout for usb control set messages to 5000 ms

[Daniel Mack]

Hi Daniel,

On 19.04.2013 23:39, Daniel Schürmann wrote:
> This is a patch against
> https://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/
> 581cbef46ae60073252208fc1f26dd1044f6e215

Yeah, we're getting there :)

> Set the timeout for USB control set messages according to the USB 2 spec
> §9.2.6.4 to 5000 ms.
> To avoid new issues, the get timeout is unchanged at 1000 ms, though it
> is 500 ms in the spec.
> This patch is required to run the Hercules RMX2 which needs a timeout >
> 1240 ms
> 
> Signed-off-by: Daniel Schürmann <daschuer@mixxx.org>
> ---
> diff --git a/sound/usb/helper.c b/sound/usb/helper.c
> index c1db28f..b4f3876 100644
> --- a/sound/usb/helper.c
> +++ b/sound/usb/helper.c
> @@ -21,7 +21,11 @@
>  
>  #include "usbaudio.h"
>  #include "helper.h"
> -#include "quirks.h"

That is a stray change which shouldn't be part of this patch.

> +
> +/* Value from 9.2.6.4 USB 2 spec */
> +#define USB_MSG_SET_TIMEOUT 5000
> +/* Value from spec is 500 but we pick 1000 for legacy reasons */
> +#define USB_MSG_GET_TIMEOUT 1000
>  
>  /*
>   * combine bytes and get an integer value
> @@ -86,14 +90,24 @@ int snd_usb_ctl_msg(struct usb_device *dev, unsigned
> int pipe, __u8 request,
>  {
>      int err;
>      void *buf = NULL;
> +    int timeout;

Can be unsigned, but that's a minor.

>  
>      if (size > 0) {
>          buf = kmemdup(data, size, GFP_KERNEL);
>          if (!buf)
>              return -ENOMEM;
>      }
> +
> +    if (requesttype & USB_DIR_IN) {
> +        /* Get Request */
> +        timeout = USB_MSG_GET_TIMEOUT;
> +    } else {
> +        /* Set Request */
> +        timeout = USB_MSG_SET_TIMEOUT;
> +    }

You can omit the comments, they don't add any value for someone who's
reading the code. And you don't need the curly braces.

Other than that, looks good to me.


Can you send an updated version?


Thanks,
Daniel


>      err = usb_control_msg(dev, pipe, request, requesttype,
> -                  value, index, buf, size, 1000);
> +                  value, index, buf, size, timeout);
> +
>      if (size > 0) {
>          memcpy(data, buf, size);
>          kfree(buf);
>