Re: USB Audio problems

[Takashi Iwai <tiwai@suse.de>]

At Thu, 20 Nov 2003 00:50:03 -0500,
Karim Yaghmour wrote:
> 
> 
> Well, it seems that I'm going to have to answer my own self ... :)
> 
> The following is what I've been able to find using additional tracing
> info. Also there's a fix for usbaudio.c.
> 
> Basically, it's as I said before: the usbaudio driver uses URBs
> without even checking if they're in use or not. Surprisingly, it
> does this with one channel, but not the other. Here's what I get
> in the traces when I separated the events by channel:
> Channel A (the hex data is the value of *subs):
> -----------------------------------------------------------------------------------
(snip)
> To make a long story short, the most important issue here is to check whether
> the URB is active before using it. If it is, then it must be unlinked first.
> 
> Here's my modified start_urbs() function from usbaudio.c:
> static int start_urbs(snd_usb_substream_t *subs, snd_pcm_runtime_t *runtime)
> {
>      unsigned int i;
>      int err;
> 
>      for (i = 0; i < subs->nurbs; i++) {
>          snd_assert(subs->dataurb[i].urb, return -EINVAL);
>          /* Deactivate URB prior to using it. K.Y. */
>          if (test_and_clear_bit(i, &subs->active_mask)) {
>              set_bit(i, &subs->unlink_mask);
>              usb_unlink_urb(subs->dataurb[i].urb);
>          }
>          if (subs->ops.prepare(subs, runtime, subs->dataurb[i].urb) < 0) {
>              snd_printk(KERN_ERR "cannot prepare datapipe for urb %d\n", i);
>              goto __error;
>          }
>      }
>      if (subs->syncpipe) {
>          for (i = 0; i < SYNC_URBS; i++) {
>              snd_assert(subs->syncurb[i].urb, return -EINVAL);
>              if (subs->ops.prepare_sync(subs, runtime, subs->syncurb[i].urb) < 0) {
>                  snd_printk(KERN_ERR "cannot prepare syncpipe for urb %d\n", i);
>                  goto __error;
>              }
>          }
>      }
> 
>      subs->running = 1;
>      for (i = 0; i < subs->nurbs; i++) {
>          trace_std_formatted_event(event_start_urb, i,  (unsigned int) subs);
>          if ((err = usb_submit_urb(subs->dataurb[i].urb, GFP_KERNEL)) < 0) {
>              trace_std_formatted_event(event_abuse_urb, i,  (unsigned int) subs);
>              snd_printk(KERN_ERR "cannot submit datapipe for urb %d, err = %d\n", i, err);
>              goto __error;
>          }
>          set_bit(i, &subs->active_mask);
>          clear_bit(i, &subs->unlink_mask);
>      }
>      if (subs->syncpipe) {
>          for (i = 0; i < SYNC_URBS; i++) {
>              if ((err = usb_submit_urb(subs->syncurb[i].urb, GFP_KERNEL)) < 0) {
>                  snd_printk(KERN_ERR "cannot submit syncpipe for urb %d, err = %d\n", i, err);
>                  goto __error;
>              }
>              set_bit(i + 16, &subs->active_mask);
>          }
>      }
>      return 0;
> 
>   __error:
>      // snd_pcm_stop(subs->pcm_substream, SNDRV_PCM_STATE_XRUN);
>      deactivate_urbs(subs, 0);
>      return -EPIPE;
> }
> 
> Using this code, I haven't had any more errors at all.
> 
> In order to use this, just remove all the trace_* functions. This new
> function just checks if a URB is already in use prior to using it and
> clears it. Also, it clears the unlink_mask after the call to
> usb_submit_urb in order for it to be "clearable" by deactivate_urbs().
> Without this last clear_bit(), URBs can only be cleared once by
> deactivate_urbs() and it takes a new init_substream_urbs() to set the
> entire mask back to 0. I don't see how this makes any sense.

it'd be better to clean unlink_mask in the complete callback for the
case you use async unlink mode (see below).
and, the check of active_mask should be done in prepare callback, not
in the trigger callback.  the trigger callback must be as short as
possible.  we can put deactivate_urbs() in prepre callback so that the
urbs become clean before starting streams.
(but still we have a problem of async unlink because prepare callback
 is also in the spinlocked context.)

the current code is surely buggy, because it issues sync unlink
inside the spinlocked context.  it's problematic on 2.6 kernel or SMP
system, and may result in kernel oops.  i added async_unlink module
option to change the behavior in the new version.

but it's still disabled as default, because unlinking multiple urbs
asynchronouly on 2.4 kernel causes kernel panic.  sigh.
unfortunately, there is no perfect solution to satisfy all versions
yet.  perhaps we need to redesign the linked-pcm streams to make it
possible to call prepare callback without spinlocks.


Takashi


-------------------------------------------------------
This SF.net email is sponsored by: SF.net Giveback Program.
Does SourceForge.net help you be more productive?  Does it
help you create better code?  SHARE THE LOVE, and help us help
YOU!  Click Here: http://sourceforge.net/donate/