Date:	Thu, 08 Nov 2012 08:31:35 +0100
From:	Daniel Mack <zonque@...il.com>
To:	Takashi Iwai <tiwai@...e.de>
CC:	Alan Stern <stern@...land.harvard.edu>,
	Christof Meerwald <cmeerw@...erw.org>,
	"Artem S. Tashkinov" <t.artem@...os.com>,
	Kernel development list <linux-kernel@...r.kernel.org>,
	USB list <linux-usb@...r.kernel.org>
Subject: Re: A reliable kernel panic (3.6.2) and system crash when visiting
 a particular website

On 08.11.2012 07:43, Takashi Iwai wrote:
> 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.

Ah, right. I was errornously looking closer to Alan's patch but then
replied to yours. Alright then - thanks for explaining :)


Daniel

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

Powered by blists - more mailing lists