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

[Daniel Mack <zonque@gmail.com>]

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