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