Re: [PATCH] usb: don't offload isochronous urb completions to ksoftirq

[Mikulas Patocka]

On Wed, 13 Jun 2018, Alan Stern wrote:

> On Wed, 13 Jun 2018, Mikulas Patocka wrote:
> 
> [Skipping lots of material...]
> 
> > BTW I found this piece of code in sound/usb/usx2y/usbusx2yaudio.c:
> >                         urb->transfer_buffer_length = subs->maxpacksize * nr_of_packs();
> >                         if ((err = usb_submit_urb(urb, GFP_ATOMIC)) < 0) {
> >                                 snd_printk (KERN_ERR "cannot submit datapipe for urb %d, err = %d\n", i, err);
> >                                 err = -EPIPE;
> >                                 goto cleanup;
> >                         } else
> >                                 if (i == 0)
> >                                         usX2Y->wait_iso_frame = urb->start_frame;
> >                         urb->transfer_flags = 0;
> > It seems like a bug to modify transfer_flags after the urb is submitted.
> 
> Yes, it is definitely a bug.
> 
> > I have a single-core machine with usb2 soundcard. When I increase mplayer
> > priority (to real-time or high non-realtime priority), the sound is
> > stuttering. The reason for the stuttering is that mplayer at high priority
> > preempts the softirq thread, preventing URBs from being completed. It was
> > caused by the patch 428aac8a81058 that offloads URB completion to softirq.
> > --- linux-4.17.orig/drivers/usb/host/ehci-q.c	2018-06-12 22:35:21.000000000 +0200
> > +++ linux-4.17/drivers/usb/host/ehci-q.c	2018-06-12 22:44:09.000000000 +0200
> > @@ -238,6 +238,8 @@ static int qtd_copy_status (
> >  
> >  static void
> >  ehci_urb_done(struct ehci_hcd *ehci, struct urb *urb, int status)
> > +__releases(ehci->lock)
> > +__acquires(ehci->lock)
> >  {
> >  	if (usb_pipetype(urb->pipe) == PIPE_INTERRUPT) {
> >  		/* ... update hc-wide periodic stats */
> > @@ -264,7 +266,17 @@ ehci_urb_done(struct ehci_hcd *ehci, str
> >  #endif
> >  
> >  	usb_hcd_unlink_urb_from_ep(ehci_to_hcd(ehci), urb);
> > -	usb_hcd_giveback_urb(ehci_to_hcd(ehci), urb, status);
> > +	if (urb->transfer_flags & URB_FAST_COMPLETION) {
> > +		/*
> > +		 * USB audio experiences skipping of we offload completion
> > +		 * to ksoftirq.
> > +		 */
> 
> This comment seems unnecessary.
> 
> > +		spin_unlock(&ehci->lock);
> > +		usb_hcd_giveback_urb(ehci_to_hcd(ehci), urb, status);
> > +		spin_lock(&ehci->lock);
> > +	} else {
> > +		usb_hcd_giveback_urb(ehci_to_hcd(ehci), urb, status);
> > +	}
> >  }
> 
> I'm not at all sure about this.  Have you audited all of ehci-hcd to 
> make sure the driver doesn't assume that ehci->lock remains held while 
> an URB is given back?  It's been so long since I worked on this area 
> that I don't remember the answer offhand.
> 
> Alan Stern

I compared the current ehci code the code in the kernel 3.11 (that was the 
last kernel that didn't offload callbacks) and it is very similar. So, we 
can assume that if it was ok in the kernel 3.11, it is ok now.

itd_complete and sitd_complete are the same except for small formatting 
changes.

itd_submit and sitd_submit newly call ehci_urb_done, but it drops the 
spinlock after the call, therefore it tolerates that ehci_urb_done drops 
the spinlock.

qh_completions is almost the same in the kernel 3.11 and upstream, so if 
it tolerated dropped spinlock and resubmission in 3.11, it should tolerate 
it now.


BTW - if you think that dropping the spinlock could cause trouble - should 
we add the urbs to temporary list and call the callbacks just after the 
spinlock is dropped? But that would just add a lot of junk code to the 
ehci driver.

Another possibility is to hack the softirq subsystem so that tasklet_hi is 
never offloaded - but I don't know if it makes sense to make this change 
jsut because of ehci.

Mikulas