Re: [PATCH 1/2] ALSA: snd-usb: tighten EP_FLAG_RUNNING checks

[Daniel Mack <zonque@gmail.com>]

On 12.07.2012 21:19, Joseph Salisbury wrote:
> On 07/12/2012 11:27 AM, Daniel Mack wrote:
>> On 12.07.2012 17:20, Clemens Ladisch wrote:
>>> Daniel Mack wrote:
>>>> On 12.07.2012 16:29, Clemens Ladisch wrote:
>>>>> Daniel Mack wrote:
>>>>>> In endpoint.c, bail out earlier in case the stream is stopped.
>>>>>> ...
>>>>>> @@ -350,7 +350,8 @@ static void snd_complete_urb(struct urb *urb)
>>>>>>  		     urb->status == -ENODEV ||		/* device removed */
>>>>>>  		     urb->status == -ECONNRESET ||	/* unlinked */
>>>>>>  		     urb->status == -ESHUTDOWN ||	/* device disabled */
>>>>>> -		     ep->chip->shutdown))		/* device disconnected */
>>>>>> +		     ep->chip->shutdown) ||		/* device disconnected */
>>>>>> +		     !test_bit(EP_FLAG_RUNNING, &ep->flags))
>>>>>>  		goto exit_clear;
>>>>>
>>>>> Is this really needed?
>>>>> The URBs will be unlinked at the same time.
>>>>
>>>> This just brings the code in sync with what we had before. If URBs are
>>>> just unlinked but not killed, they will return with data payload, and in
>>>> case of implicit feedback streams, the retire code could issue new
>>>> output packets.
>>>
>>> And is that bad?
>>>
>>> The EP_FLAG_RUNNING bit is cleared immediately before the URBs are
>>> unlinked, and some URB could have been completed regularly immediately
>>> before that.  So if there is any case where the additional test_bit()
>>> call prevents submitting a new playback packet, there is another similar
>>> case where you've just got such a playback packet anyway.
>>>
>>> In other words: if that test_bit() call were necessary to protect
>>> against something bad, it wouldn't be sufficient.
>>>
>>> The driver ensures that (implicit feedback) playback URBs are unlinked
>>> only after the corresponding capture URBs have been completely killed,
>>> doesn't it?
>>
>> You're right. Especially because sending out packets in implicit
>> feedback mode happens from queue_pending_output_urbs() which has an
>> extra chip.
>>
>> So we can drop the whole patch then. Thanks for the review!
>>
>>
>> Daniel
> 
> Hi Daniel,
> 
> I tested without your first patch.  I'm not sure why, but dropping the
> first patch(ALSA: snd-usb: tighten EP_FLAG_RUNNING checks) causes a long
> delay, about 30 seconds, during bootup of an ubuntu system.
> 
> During the delay, I am able to enter my password, but the system seems
> to be doing something in the background, slowing the login process.
> 
> I confirmed that adding the patch back causes the delay to go away.

That really sounds odd and totally unrelated. Is there any message in
dmesg that looks suspicious? Otherwise, I would ask you to test again.
Could be it was caused by something that runs on every n'th boot or so.


Daniel