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

[Clemens Ladisch <clemens@ladisch.de>]

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?


Regards,
Clemens