At Thu, 08 Nov 2012 01:42:59 +0100, Daniel Mack wrote: > > On 07.11.2012 20:19, Takashi Iwai wrote: > > At Wed, 7 Nov 2012 12:34:43 -0500 (EST), > > Alan Stern wrote: > >> > >> On Mon, 5 Nov 2012, Christof Meerwald wrote: > >> > >>> BTW, I have been able to reproduce the problem on a completely > >>> different machine (also running Ubuntu 12.10, but different hardware). > >>> The important thing appears to be that the USB audio device is > >>> connected via a USB 2.0 hub (and then using the test code posted in > >>> http://pastebin.com/aHGe1S1X specifying the audio device as > >>> "plughw:Set" (or whatever it's called) seems to trigger the freeze). > >> > >> Christof: Thank you for that reference, it was a big help. After > >> crashing my system many times I have tracked the problem, at least in > >> part. The patch below should prevent your system from freezing. > >> > >> > >> Takashi: It turns out the the problem is triggered when the audio > >> subsystem calls snd_usb_endpoint_stop() with wait == 0 and then calls > >> snd_usb_endpoint_start(). Since the driver doesn't wait for the > >> outstanding URBs to finish, it tries to submit them again while they > >> are still active. > >> > >> Normally the USB core would realize this and fail the submission, but a > >> bug in ehci-hcd prevented this from happening. (That bug is what the > >> patch below fixes.) The URB gets added to the active list twice, > >> resulting in list corruption and an oops in interrupt context, which > >> freezes the system. > >> > >> The user program that triggers the problem basically looks like this: > >> > >> snd_pcm_prepare(rec_pcm); > >> snd_pcm_start(rec_pcm); > >> snd_pcm_drop(rec_pcm); > >> > >> snd_pcm_prepare(rec_pcm); > >> snd_pcm_start(rec_pcm); > >> > >> The snd_pcm_drop call unlinks the URBs but does not wait for them to > >> finish. Then the second snd_pcm_start call submits the URBs before > >> they have finished. > > > Thanks for investigating on this and to everyone who so quickyl tested > the provided patch. Seems like we got the right idea where the problem > really is. > > However, the proposed patch seems wrong to me (see below). > > >> What is the right solution for this problem? > > > > How about the patch below? (It's for 3.6, and won't be applied cleanly > > to 3.7, but easy to adapt.) > > > > > > Takashi > > > > --- > > diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c > > index d9de667..38830e2 100644 > > --- a/sound/usb/endpoint.c > > +++ b/sound/usb/endpoint.c > > @@ -35,6 +35,7 @@ > > > > #define EP_FLAG_ACTIVATED 0 > > #define EP_FLAG_RUNNING 1 > > +#define EP_FLAG_STOPPING 2 > > > > /* > > * snd_usb_endpoint is a model that abstracts everything related to an > > @@ -502,10 +503,19 @@ static int wait_clear_urbs(struct snd_usb_endpoint *ep) > > if (alive) > > snd_printk(KERN_ERR "timeout: still %d active urbs on EP #%x\n", > > alive, ep->ep_num); > > + clear_bit(EP_FLAG_STOPPING, &ep->flags); > > > > return 0; > > } > > > > +/* wait until urbs are really dropped */ > > +void snd_usb_endpoint_sync_stop(struct snd_usb_endpoint *ep) > > +{ > > + if (test_bit(EP_FLAG_STOPPING, &ep->flags)) > > + wait_clear_urbs(ep); > > +} > > + > > + > > /* > > * unlink active urbs. > > */ > > @@ -913,6 +923,8 @@ void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep, > > > > if (wait) > > wait_clear_urbs(ep); > > + else > > + set_bit(EP_FLAG_STOPPING, &ep->flags); > > } > > } > > > > diff --git a/sound/usb/endpoint.h b/sound/usb/endpoint.h > > index cbbbdf2..c1540a4 100644 > > --- a/sound/usb/endpoint.h > > +++ b/sound/usb/endpoint.h > > @@ -16,6 +16,7 @@ int snd_usb_endpoint_set_params(struct snd_usb_endpoint *ep, > > int snd_usb_endpoint_start(struct snd_usb_endpoint *ep, int can_sleep); > > void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep, > > int force, int can_sleep, int wait); > > +void snd_usb_endpoint_sync_stop(struct snd_usb_endpoint *ep); > > int snd_usb_endpoint_activate(struct snd_usb_endpoint *ep); > > int snd_usb_endpoint_deactivate(struct snd_usb_endpoint *ep); > > void snd_usb_endpoint_free(struct list_head *head); > > diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c > > index f782ce1..aee3ab0 100644 > > --- a/sound/usb/pcm.c > > +++ b/sound/usb/pcm.c > > @@ -546,6 +546,11 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream) > > if (snd_BUG_ON(!subs->data_endpoint)) > > return -EIO; > > > > + if (subs->sync_endpoint) > > + snd_usb_endpoint_sync_stop(subs->sync_endpoint); > > + if (subs->data_endpoint) > > + snd_usb_endpoint_sync_stop(subs->data_endpoint); > > We can't simply stop both endpoints in the prepare callback. The new function doesn't stop the stream by itself but it just syncs if the stream is being stopped beforehand. So, it's safe to call it there. Maybe the name was confusing. It should have been like snd_usb_endpoint_sync_pending_stop() or such. Takashi > The essence > of the new reference-counting system is that we can use endpoints from > multiple contexts, and the logic inside endpoint.c will care about when > to start up and take down the urbs. The idea here is that endoints can > be run for many purposes, and the new implementation that was added > allows capture endpoints to run purely as timing reference for playback. > > This bug needs to be fixed in the ehci controller, or we need some other > solution in the snd-usb-audio driver. I'll do some test once I'm back > from ELC. > > > Daniel > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
