[RFC PATCH] ALSA: usb - fix race in creation of M-Audio Fast track pro driver

[RFC PATCH] ALSA: usb - fix race in creation of M-Audio Fast track pro driver

[David Henningsson]

A patch in the 3.2 kernel caused regression with hotplugging the
M-Audio Fast track pro, or sound after suspend. I don't have the
device so I haven't done a full analysis, but it seems userspace
(both udev and pulseaudio) got confused when a card was created,
immediately destroyed, and then created again.

However, at least one person in the bug report (martin djfun)
reports that this patch resolves the issue for him. It also leaves
a message in the log:
"snd-usb-audio: probe of 1-1.1:1.1 failed with error -5" which is
a bit misleading. It is better than non-working audio, but maybe
there's a more elegant solution?

BugLink: https://bugs.launchpad.net/bugs/1095315
Signed-off-by: David Henningsson <david.henningsson@canonical.com>
---
 sound/usb/quirks.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

...oh, and if you end up taking this patch as it is, you should probably
add cc to stable (3.2+).

diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c
index acc12f0..e71fc8b 100644
--- a/sound/usb/quirks.c
+++ b/sound/usb/quirks.c
@@ -387,11 +387,13 @@ static int snd_usb_fasttrackpro_boot_quirk(struct usb_device *dev)
 		 * rules
 		 */
 		err = usb_driver_set_configuration(dev, 2);
-		if (err < 0) {
+		if (err < 0)
 			snd_printdd("error usb_driver_set_configuration: %d\n",
 				    err);
-			return -ENODEV;
-		}
+		/* Always return an error, so that we stop creating a device
+		   that will just be destroyed and recreated with a new
+		   configuration */
+		return -ENODEV;
 	} else
 		snd_printk(KERN_INFO "usb-audio: Fast Track Pro config OK\n");
 
-- 
1.7.9.5

Re: [RFC PATCH] ALSA: usb - fix race in creation of M-Audio Fast track pro driver

[Clemens Ladisch]

David Henningsson wrote:
> this patch resolves the issue for him. It also leaves a message
> in the log:
> "snd-usb-audio: probe of 1-1.1:1.1 failed with error -5"

I'd guess this is for an interface not claimed by the driver but
without an explicit QUIRK_IGNORE_INTERFACE entry.


Regards,
Clemens

Re: [RFC PATCH] ALSA: usb - fix race in creation of M-Audio Fast track pro driver

[David Henningsson]

On 01/04/2013 09:19 PM, Clemens Ladisch wrote:
> David Henningsson wrote:
>> this patch resolves the issue for him. It also leaves a message
>> in the log:
>> "snd-usb-audio: probe of 1-1.1:1.1 failed with error -5"
>
> I'd guess this is for an interface not claimed by the driver but
> without an explicit QUIRK_IGNORE_INTERFACE entry.

My patch causes this error because the quirk fails with an error, which 
is later on turned into a -EIO. Any quirk_ignore_interface entry will 
therefore be ignored. Using the quirk_ignore_interface entry *instead 
of* my patch seems like an interesting option, but...

Looking at the code again, it seems like we have a more generic race 
problem, unless I'm missing something:

  - the same ALSA sound card can contain more than one USB interface.
  - snd_card_register is called whenever a USB interface finishes probing.
  - snd_card_register fires off uevents, causing stuff to happen such as 
volume restore (alsactl) and PulseAudio starts probing the device
  - so when userspace starts accessing the device, the device is not 
fully functional, because not all USB interfaces have yet been probed
  - as a result, volumes might not get restored (if all mixer controls 
do not belong to the first interface), and PulseAudio thinks the device 
is not capable of all the stuff it actually can do (if all pcm streams 
do not belong to the first interface).

I guess this is probably not a problem normally because the kernel is 
faster than userspace, but it is revealed here because switching 
configurations takes longer time than probing another interface.


-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic

Re: [RFC PATCH] ALSA: usb - fix race in creation of M-Audio Fast track pro driver

[David Henningsson]

On 01/04/2013 05:02 PM, David Henningsson wrote:
> A patch in the 3.2 kernel caused regression with hotplugging the
> M-Audio Fast track pro, or sound after suspend. I don't have the
> device so I haven't done a full analysis, but it seems userspace
> (both udev and pulseaudio) got confused when a card was created,
> immediately destroyed, and then created again.

Since there were no counter proposal patches and the below code actually 
fixes a problem for users, would you mind committing it?



>
> However, at least one person in the bug report (martin djfun)
> reports that this patch resolves the issue for him. It also leaves
> a message in the log:
> "snd-usb-audio: probe of 1-1.1:1.1 failed with error -5" which is
> a bit misleading. It is better than non-working audio, but maybe
> there's a more elegant solution?
>
> BugLink: https://bugs.launchpad.net/bugs/1095315
> Signed-off-by: David Henningsson <david.henningsson@canonical.com>
> ---
>   sound/usb/quirks.c |    8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
>
> ...oh, and if you end up taking this patch as it is, you should probably
> add cc to stable (3.2+).
>
> diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c
> index acc12f0..e71fc8b 100644
> --- a/sound/usb/quirks.c
> +++ b/sound/usb/quirks.c
> @@ -387,11 +387,13 @@ static int snd_usb_fasttrackpro_boot_quirk(struct usb_device *dev)
>   		 * rules
>   		 */
>   		err = usb_driver_set_configuration(dev, 2);
> -		if (err < 0) {
> +		if (err < 0)
>   			snd_printdd("error usb_driver_set_configuration: %d\n",
>   				    err);
> -			return -ENODEV;
> -		}
> +		/* Always return an error, so that we stop creating a device
> +		   that will just be destroyed and recreated with a new
> +		   configuration */
> +		return -ENODEV;
>   	} else
>   		snd_printk(KERN_INFO "usb-audio: Fast Track Pro config OK\n");
>
>



-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic

Re: [RFC PATCH] ALSA: usb - fix race in creation of M-Audio Fast track pro driver

[Takashi Iwai]

At Mon, 14 Jan 2013 08:21:56 +0100,
David Henningsson wrote:
> 
> 
> On 01/04/2013 05:02 PM, David Henningsson wrote:
> > A patch in the 3.2 kernel caused regression with hotplugging the
> > M-Audio Fast track pro, or sound after suspend. I don't have the
> > device so I haven't done a full analysis, but it seems userspace
> > (both udev and pulseaudio) got confused when a card was created,
> > immediately destroyed, and then created again.
> 
> Since there were no counter proposal patches and the below code actually 
> fixes a problem for users, would you mind committing it?

OK, applied now.


Takashi

> 
> 
> 
> >
> > However, at least one person in the bug report (martin djfun)
> > reports that this patch resolves the issue for him. It also leaves
> > a message in the log:
> > "snd-usb-audio: probe of 1-1.1:1.1 failed with error -5" which is
> > a bit misleading. It is better than non-working audio, but maybe
> > there's a more elegant solution?
> >
> > BugLink: https://bugs.launchpad.net/bugs/1095315
> > Signed-off-by: David Henningsson <david.henningsson@canonical.com>
> > ---
> >   sound/usb/quirks.c |    8 +++++---
> >   1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > ...oh, and if you end up taking this patch as it is, you should probably
> > add cc to stable (3.2+).
> >
> > diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c
> > index acc12f0..e71fc8b 100644
> > --- a/sound/usb/quirks.c
> > +++ b/sound/usb/quirks.c
> > @@ -387,11 +387,13 @@ static int snd_usb_fasttrackpro_boot_quirk(struct usb_device *dev)
> >   		 * rules
> >   		 */
> >   		err = usb_driver_set_configuration(dev, 2);
> > -		if (err < 0) {
> > +		if (err < 0)
> >   			snd_printdd("error usb_driver_set_configuration: %d\n",
> >   				    err);
> > -			return -ENODEV;
> > -		}
> > +		/* Always return an error, so that we stop creating a device
> > +		   that will just be destroyed and recreated with a new
> > +		   configuration */
> > +		return -ENODEV;
> >   	} else
> >   		snd_printk(KERN_INFO "usb-audio: Fast Track Pro config OK\n");
> >
> >
> 
> 
> 
> -- 
> David Henningsson, Canonical Ltd.
> https://launchpad.net/~diwic
>