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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
