lkml.org 
[lkml]   [2018]   [Jun]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] usb: don't offload isochronous urb completions to ksoftirq
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

\
 
 \ /
  Last update: 2018-06-13 16:14    [W:0.038 / U:16.984 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site