[RFC PATCH] usb: hcd: complete URBs in threaded-IRQ context instead of tasklet

[RFC PATCH] usb: hcd: complete URBs in threaded-IRQ context instead of tasklet

[Sebastian Andrzej Siewior]

I've been going over Frederic's softirq patches and it seems that there
were two problems. One was network related, the other was Mauro's USB
dvb-[stc] device which was not able to stream properly over time.

Here is an attempt to let the URB complete in the threaded handler
instead of going to the tasklet. In case the system is SMP then the
patch [0] would be required in order to have the IRQ and its thread on
the same CPU.

Mauro, would you mind giving it a shot?

[0] genirq: Let irq thread follow the effective hard irq affinity
    https://git.kernel.org/tip/cbf8699996a6e7f2f674b3a2a4cef9f666ff613e

---

The urb->complete callback was initially invoked in IRQ context after
the HCD dropped its lock because the callback could re-queue the URB
again. Later this completion was deferred to the tasklet allowing the
HCD hold the lock. Also the BH handler can be interrupted by the IRQ
handler adding more "completed" requests to its queue.
While this batching is good in general, the softirq defers its doing for
short period of time if it is running constantly without a break. This
breaks some use cases where constant USB throughput is required.
As an alternative approach to tasklet handling, I defer the URB
completion to the HCD's threaded handler. There are two lists for
"high-prio" proccessing and lower priority (to mimic current behaviour).
The URBs in the high-priority list are always preffered over the URBs
in the low-priority list.
The URBs from the root-hub never create an interrupt so I currently
process them in a workqueue (I'm not sure if an URB-enqueue in the
completion handler would break something).

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/usb/core/hcd.c  | 131 ++++++++++++++++++++++++++++++++----------------
 include/linux/usb/hcd.h |  14 +++---
 2 files changed, 95 insertions(+), 50 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index fc32391a34d5..8d6dd4d3cbe7 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1775,33 +1775,75 @@ static void __usb_hcd_giveback_urb(struct urb *urb)
 	usb_put_urb(urb);
 }
 
-static void usb_giveback_urb_bh(unsigned long param)
+static void usb_hcd_rh_gb_urb(struct work_struct *work)
 {
-	struct giveback_urb_bh *bh = (struct giveback_urb_bh *)param;
-	struct list_head local_list;
+	struct giveback_urb	*bh = container_of(work, struct giveback_urb,
+						   rh_compl);
+	struct list_head	urb_list;
 
 	spin_lock_irq(&bh->lock);
-	bh->running = true;
- restart:
-	list_replace_init(&bh->head, &local_list);
+	list_replace_init(&bh->rh_head, &urb_list);
 	spin_unlock_irq(&bh->lock);
 
-	while (!list_empty(&local_list)) {
+	while (!list_empty(&urb_list)) {
 		struct urb *urb;
 
-		urb = list_entry(local_list.next, struct urb, urb_list);
+		urb = list_first_entry(&urb_list, struct urb, urb_list);
 		list_del_init(&urb->urb_list);
-		bh->completing_ep = urb->ep;
 		__usb_hcd_giveback_urb(urb);
-		bh->completing_ep = NULL;
+	}
+}
+
+
+#define URB_PRIO_HIGH	0
+#define URB_PRIO_LOW	1
+
+static irqreturn_t usb_hcd_gb_urb(int irq, void *__hcd)
+{
+	struct usb_hcd		*hcd = __hcd;
+	struct giveback_urb	*bh = &hcd->gb_urb;
+	struct list_head	urb_list[2];
+	int			i;
+
+	INIT_LIST_HEAD(&urb_list[URB_PRIO_HIGH]);
+	INIT_LIST_HEAD(&urb_list[URB_PRIO_LOW]);
+
+	spin_lock_irq(&bh->lock);
+ restart:
+	list_splice_tail_init(&bh->prio_hi_head, &urb_list[URB_PRIO_HIGH]);
+	list_splice_tail_init(&bh->prio_lo_head, &urb_list[URB_PRIO_LOW]);
+	spin_unlock_irq(&bh->lock);
+
+	for (i = 0; i < ARRAY_SIZE(urb_list); i++) {
+		while (!list_empty(&urb_list[i])) {
+			struct urb *urb;
+
+			urb = list_first_entry(&urb_list[i],
+					       struct urb, urb_list);
+			list_del_init(&urb->urb_list);
+			if (i == URB_PRIO_HIGH)
+				bh->completing_ep = urb->ep;
+
+			__usb_hcd_giveback_urb(urb);
+
+			if (i == URB_PRIO_HIGH)
+				bh->completing_ep = NULL;
+
+			if (i == URB_PRIO_LOW &&
+			    !list_empty_careful(&urb_list[URB_PRIO_HIGH])) {
+				spin_lock_irq(&bh->lock);
+				goto restart;
+			}
+		}
 	}
 
 	/* check if there are new URBs to giveback */
 	spin_lock_irq(&bh->lock);
-	if (!list_empty(&bh->head))
+	if (!list_empty(&bh->prio_hi_head) ||
+	    !list_empty(&bh->prio_lo_head))
 		goto restart;
-	bh->running = false;
 	spin_unlock_irq(&bh->lock);
+	return IRQ_HANDLED;
 }
 
 /**
@@ -1823,37 +1865,32 @@ static void usb_giveback_urb_bh(unsigned long param)
  */
 void usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb, int status)
 {
-	struct giveback_urb_bh *bh;
-	bool running, high_prio_bh;
+	struct giveback_urb	*bh = &hcd->gb_urb;
+	struct list_head	*lh;
 
 	/* pass status to tasklet via unlinked */
 	if (likely(!urb->unlinked))
 		urb->unlinked = status;
 
-	if (!hcd_giveback_urb_in_bh(hcd) && !is_root_hub(urb->dev)) {
+	if (is_root_hub(urb->dev)) {
+		spin_lock(&bh->lock);
+		list_add_tail(&urb->urb_list, &bh->rh_head);
+		spin_unlock(&bh->lock);
+		queue_work(system_highpri_wq, &bh->rh_compl);
+		return;
+	}
+	if (!hcd_giveback_urb_in_bh(hcd)) {
 		__usb_hcd_giveback_urb(urb);
 		return;
 	}
 
-	if (usb_pipeisoc(urb->pipe) || usb_pipeint(urb->pipe)) {
-		bh = &hcd->high_prio_bh;
-		high_prio_bh = true;
-	} else {
-		bh = &hcd->low_prio_bh;
-		high_prio_bh = false;
-	}
-
-	spin_lock(&bh->lock);
-	list_add_tail(&urb->urb_list, &bh->head);
-	running = bh->running;
-	spin_unlock(&bh->lock);
-
-	if (running)
-		;
-	else if (high_prio_bh)
-		tasklet_hi_schedule(&bh->bh);
+	if (usb_pipeisoc(urb->pipe) || usb_pipeint(urb->pipe))
+		lh = &bh->prio_hi_head;
 	else
-		tasklet_schedule(&bh->bh);
+		lh = &bh->prio_lo_head;
+	spin_lock(&bh->lock);
+	list_add_tail(&urb->urb_list, lh);
+	spin_unlock(&bh->lock);
 }
 EXPORT_SYMBOL_GPL(usb_hcd_giveback_urb);
 
@@ -2436,9 +2473,17 @@ irqreturn_t usb_hcd_irq (int irq, void *__hcd)
 		rc = IRQ_NONE;
 	else if (hcd->driver->irq(hcd) == IRQ_NONE)
 		rc = IRQ_NONE;
-	else
-		rc = IRQ_HANDLED;
+	else {
+		struct giveback_urb	*bh = &hcd->gb_urb;
 
+		spin_lock(&bh->lock);
+		if (!list_empty(&bh->prio_hi_head) ||
+		    !list_empty(&bh->prio_lo_head))
+			rc = IRQ_WAKE_THREAD;
+		else
+			rc = IRQ_HANDLED;
+		spin_unlock(&bh->lock);
+	}
 	return rc;
 }
 EXPORT_SYMBOL_GPL(usb_hcd_irq);
@@ -2492,12 +2537,13 @@ EXPORT_SYMBOL_GPL (usb_hc_died);
 
 /*-------------------------------------------------------------------------*/
 
-static void init_giveback_urb_bh(struct giveback_urb_bh *bh)
+static void init_giveback_urb(struct giveback_urb *bh)
 {
-
 	spin_lock_init(&bh->lock);
-	INIT_LIST_HEAD(&bh->head);
-	tasklet_init(&bh->bh, usb_giveback_urb_bh, (unsigned long)bh);
+	INIT_LIST_HEAD(&bh->prio_lo_head);
+	INIT_LIST_HEAD(&bh->prio_hi_head);
+	INIT_LIST_HEAD(&bh->rh_head);
+	INIT_WORK(&bh->rh_compl, usb_hcd_rh_gb_urb);
 }
 
 struct usb_hcd *__usb_create_hcd(const struct hc_driver *driver,
@@ -2672,7 +2718,8 @@ static int usb_hcd_request_irqs(struct usb_hcd *hcd,
 
 		snprintf(hcd->irq_descr, sizeof(hcd->irq_descr), "%s:usb%d",
 				hcd->driver->description, hcd->self.busnum);
-		retval = request_irq(irqnum, &usb_hcd_irq, irqflags,
+		retval = request_threaded_irq(irqnum, usb_hcd_irq,
+					      usb_hcd_gb_urb, irqflags,
 				hcd->irq_descr, hcd);
 		if (retval != 0) {
 			dev_err(hcd->self.controller,
@@ -2863,9 +2910,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
 			&& device_can_wakeup(&hcd->self.root_hub->dev))
 		dev_dbg(hcd->self.controller, "supports USB remote wakeup\n");
 
-	/* initialize tasklets */
-	init_giveback_urb_bh(&hcd->high_prio_bh);
-	init_giveback_urb_bh(&hcd->low_prio_bh);
+	init_giveback_urb(&hcd->gb_urb);
 
 	/* enable irqs just before we start the controller,
 	 * if the BIOS provides legacy PCI irqs.
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 176900528822..12573515acb6 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -64,11 +64,12 @@
 
 /*-------------------------------------------------------------------------*/
 
-struct giveback_urb_bh {
-	bool running;
+struct giveback_urb {
 	spinlock_t lock;
-	struct list_head  head;
-	struct tasklet_struct bh;
+	struct list_head	prio_lo_head;
+	struct list_head	prio_hi_head;
+	struct list_head	rh_head;
+	struct work_struct	rh_compl;
 	struct usb_host_endpoint *completing_ep;
 };
 
@@ -169,8 +170,7 @@ struct usb_hcd {
 	resource_size_t		rsrc_len;	/* memory/io resource length */
 	unsigned		power_budget;	/* in mA, 0 = no limit */
 
-	struct giveback_urb_bh  high_prio_bh;
-	struct giveback_urb_bh  low_prio_bh;
+	struct giveback_urb	gb_urb;
 
 	/* bandwidth_mutex should be taken before adding or removing
 	 * any new bus bandwidth constraints:
@@ -410,7 +410,7 @@ static inline int hcd_giveback_urb_in_bh(struct usb_hcd *hcd)
 static inline bool hcd_periodic_completion_in_progress(struct usb_hcd *hcd,
 		struct usb_host_endpoint *ep)
 {
-	return hcd->high_prio_bh.completing_ep == ep;
+	return hcd->gb_urb.completing_ep == ep;
 }
 
 extern int usb_hcd_link_urb_to_ep(struct usb_hcd *hcd, struct urb *urb);
-- 
2.16.1

Re: [RFC PATCH] usb: hcd: complete URBs in threaded-IRQ context instead of tasklet

[Alan Stern]

On Fri, 16 Feb 2018, Sebastian Andrzej Siewior wrote:

> I've been going over Frederic's softirq patches and it seems that there
> were two problems. One was network related, the other was Mauro's USB
> dvb-[stc] device which was not able to stream properly over time.
> 
> Here is an attempt to let the URB complete in the threaded handler
> instead of going to the tasklet. In case the system is SMP then the
> patch [0] would be required in order to have the IRQ and its thread on
> the same CPU.
> 
> Mauro, would you mind giving it a shot?
> 
> [0] genirq: Let irq thread follow the effective hard irq affinity
>     https://git.kernel.org/tip/cbf8699996a6e7f2f674b3a2a4cef9f666ff613e
> 
> ---
> 
> The urb->complete callback was initially invoked in IRQ context after
> the HCD dropped its lock because the callback could re-queue the URB
> again. Later this completion was deferred to the tasklet allowing the
> HCD hold the lock. Also the BH handler can be interrupted by the IRQ
> handler adding more "completed" requests to its queue.
> While this batching is good in general, the softirq defers its doing for
> short period of time if it is running constantly without a break. This
> breaks some use cases where constant USB throughput is required.
> As an alternative approach to tasklet handling, I defer the URB
> completion to the HCD's threaded handler. There are two lists for
> "high-prio" proccessing and lower priority (to mimic current behaviour).
> The URBs in the high-priority list are always preffered over the URBs
> in the low-priority list.

We originally used tasklets because we didn't want to incur the delays 
associated with running in a process context.  It seems odd to be 
reversing that decision now.

> The URBs from the root-hub never create an interrupt so I currently
> process them in a workqueue (I'm not sure if an URB-enqueue in the
> completion handler would break something).

It worked okay before we changed over to using tasklets.

Alan Stern

Re: [RFC PATCH] usb: hcd: complete URBs in threaded-IRQ context instead of tasklet

[Sebastian Andrzej Siewior]

On 2018-02-16 13:29:01 [-0500], Alan Stern wrote:
> We originally used tasklets because we didn't want to incur the delays 
> associated with running in a process context.  It seems odd to be 
> reversing that decision now.

The theaded interrupt runs SCHED_FIFO priority 50 by default. The only
thing that can interrupt it are interrupts, a softirq (not ksoftirqd)
and other tasks with a higher priority than 50.
There should be no downside performance wise.

> > The URBs from the root-hub never create an interrupt so I currently
> > process them in a workqueue (I'm not sure if an URB-enqueue in the
> > completion handler would break something).
> 
> It worked okay before we changed over to using tasklets.

Ah okay. I've seen that HCDs were no longer dropping their internal lock
and I wasn't sure if such a change was also applied in RH-code.

> Alan Stern

Sebastian

Re: [RFC PATCH] usb: hcd: complete URBs in threaded-IRQ context instead of tasklet

[Alan Stern]

On Fri, 16 Feb 2018, Sebastian Andrzej Siewior wrote:

> On 2018-02-16 13:29:01 [-0500], Alan Stern wrote:
> > We originally used tasklets because we didn't want to incur the delays 
> > associated with running in a process context.  It seems odd to be 
> > reversing that decision now.
> 
> The theaded interrupt runs SCHED_FIFO priority 50 by default. The only
> thing that can interrupt it are interrupts, a softirq (not ksoftirqd)
> and other tasks with a higher priority than 50.
> There should be no downside performance wise.

Maybe.  It would be nice to see some real measurements.

> > > The URBs from the root-hub never create an interrupt so I currently
> > > process them in a workqueue (I'm not sure if an URB-enqueue in the
> > > completion handler would break something).
> > 
> > It worked okay before we changed over to using tasklets.
> 
> Ah okay. I've seen that HCDs were no longer dropping their internal lock
> and I wasn't sure if such a change was also applied in RH-code.

Oh... my memory was faulty.  After going back and looking at the old
source, I see that before we switched to tasklets the code _did_ drop
the hcd_root_hub_lock when giving back root-hub URBs.  Now it doesn't.

(It's also worth noticing that the current code gives back root-hub 
URBs in a tasklet even for HCDs that don't support using tasklets for 
non-root-hub URBs.)

In any case, latency doesn't matter much for root-hub URBs.

Alan Stern

Re: [RFC PATCH] usb: hcd: complete URBs in threaded-IRQ context instead of tasklet

[Sebastian Andrzej Siewior]

On 2018-02-16 16:46:41 [-0500], Alan Stern wrote:
> > The theaded interrupt runs SCHED_FIFO priority 50 by default. The only
> > thing that can interrupt it are interrupts, a softirq (not ksoftirqd)
> > and other tasks with a higher priority than 50.
> > There should be no downside performance wise.
> 
> Maybe.  It would be nice to see some real measurements.

I had an usb3 flash stick behind the EHCI controller which was passed
through from the host to a kvm guest. The performance numbers in the
guest were equal (some noise was there) with and without the patch.
The numbers with the patch were worse if lockdep was enabled which isn't
much of a surprise.
If you have anything specific requirements for a measurement then please
let me know and I see what I can do.

> Alan Stern

Sebastian

Re: [RFC PATCH] usb: hcd: complete URBs in threaded-IRQ context instead of tasklet

[Alan Stern]

On Thu, 22 Feb 2018, Sebastian Andrzej Siewior wrote:

> On 2018-02-16 16:46:41 [-0500], Alan Stern wrote:
> > > The theaded interrupt runs SCHED_FIFO priority 50 by default. The only
> > > thing that can interrupt it are interrupts, a softirq (not ksoftirqd)
> > > and other tasks with a higher priority than 50.
> > > There should be no downside performance wise.
> > 
> > Maybe.  It would be nice to see some real measurements.
> 
> I had an usb3 flash stick behind the EHCI controller which was passed
> through from the host to a kvm guest. The performance numbers in the
> guest were equal (some noise was there) with and without the patch.
> The numbers with the patch were worse if lockdep was enabled which isn't
> much of a surprise.
> If you have anything specific requirements for a measurement then please
> let me know and I see what I can do.

No, I didn't have anything more specific in mind.

In principle then, using threaded-interrupt bottom halves instead of
tasklets should be fine.  I don't object to making such a change.

However, using a work queue for root-hub URBs is pretty ugly.  It would
be better to reinstate the code that dropped hcd_root_hub_lock around
root-hub givebacks, which was removed by commit 94dfd7edfd5c ("USB:
HCD: support giveback of URB in tasklet context"); then it would be 
safe to give back those URBs in the bottom half.

Alan Stern

Re: [RFC PATCH] usb: hcd: complete URBs in threaded-IRQ context instead of tasklet

[Mauro Carvalho Chehab]

Hi Sebastian,

Em Fri, 16 Feb 2018 18:04:50 +0100
Sebastian Andrzej Siewior <bigeasy@linutronix.de> escreveu:

> I've been going over Frederic's softirq patches and it seems that there
> were two problems. One was network related, the other was Mauro's USB
> dvb-[stc] device which was not able to stream properly over time.
> 
> Here is an attempt to let the URB complete in the threaded handler
> instead of going to the tasklet. In case the system is SMP then the
> patch [0] would be required in order to have the IRQ and its thread on
> the same CPU.
> 
> Mauro, would you mind giving it a shot?

Sorry for taking some time to test it, has been busy those days...

Anyway, I tested it today. Didn't work. It keep losing data.

Regards,

> 
> [0] genirq: Let irq thread follow the effective hard irq affinity
>     https://git.kernel.org/tip/cbf8699996a6e7f2f674b3a2a4cef9f666ff613e
> 
> ---
> 
> The urb->complete callback was initially invoked in IRQ context after
> the HCD dropped its lock because the callback could re-queue the URB
> again. Later this completion was deferred to the tasklet allowing the
> HCD hold the lock. Also the BH handler can be interrupted by the IRQ
> handler adding more "completed" requests to its queue.
> While this batching is good in general, the softirq defers its doing for
> short period of time if it is running constantly without a break. This
> breaks some use cases where constant USB throughput is required.
> As an alternative approach to tasklet handling, I defer the URB
> completion to the HCD's threaded handler. There are two lists for
> "high-prio" proccessing and lower priority (to mimic current behaviour).
> The URBs in the high-priority list are always preffered over the URBs
> in the low-priority list.
> The URBs from the root-hub never create an interrupt so I currently
> process them in a workqueue (I'm not sure if an URB-enqueue in the
> completion handler would break something).
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/usb/core/hcd.c  | 131 ++++++++++++++++++++++++++++++++----------------
>  include/linux/usb/hcd.h |  14 +++---
>  2 files changed, 95 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index fc32391a34d5..8d6dd4d3cbe7 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -1775,33 +1775,75 @@ static void __usb_hcd_giveback_urb(struct urb *urb)
>  	usb_put_urb(urb);
>  }
>  
> -static void usb_giveback_urb_bh(unsigned long param)
> +static void usb_hcd_rh_gb_urb(struct work_struct *work)
>  {
> -	struct giveback_urb_bh *bh = (struct giveback_urb_bh *)param;
> -	struct list_head local_list;
> +	struct giveback_urb	*bh = container_of(work, struct giveback_urb,
> +						   rh_compl);
> +	struct list_head	urb_list;
>  
>  	spin_lock_irq(&bh->lock);
> -	bh->running = true;
> - restart:
> -	list_replace_init(&bh->head, &local_list);
> +	list_replace_init(&bh->rh_head, &urb_list);
>  	spin_unlock_irq(&bh->lock);
>  
> -	while (!list_empty(&local_list)) {
> +	while (!list_empty(&urb_list)) {
>  		struct urb *urb;
>  
> -		urb = list_entry(local_list.next, struct urb, urb_list);
> +		urb = list_first_entry(&urb_list, struct urb, urb_list);
>  		list_del_init(&urb->urb_list);
> -		bh->completing_ep = urb->ep;
>  		__usb_hcd_giveback_urb(urb);
> -		bh->completing_ep = NULL;
> +	}
> +}
> +
> +
> +#define URB_PRIO_HIGH	0
> +#define URB_PRIO_LOW	1
> +
> +static irqreturn_t usb_hcd_gb_urb(int irq, void *__hcd)
> +{
> +	struct usb_hcd		*hcd = __hcd;
> +	struct giveback_urb	*bh = &hcd->gb_urb;
> +	struct list_head	urb_list[2];
> +	int			i;
> +
> +	INIT_LIST_HEAD(&urb_list[URB_PRIO_HIGH]);
> +	INIT_LIST_HEAD(&urb_list[URB_PRIO_LOW]);
> +
> +	spin_lock_irq(&bh->lock);
> + restart:
> +	list_splice_tail_init(&bh->prio_hi_head, &urb_list[URB_PRIO_HIGH]);
> +	list_splice_tail_init(&bh->prio_lo_head, &urb_list[URB_PRIO_LOW]);
> +	spin_unlock_irq(&bh->lock);
> +
> +	for (i = 0; i < ARRAY_SIZE(urb_list); i++) {
> +		while (!list_empty(&urb_list[i])) {
> +			struct urb *urb;
> +
> +			urb = list_first_entry(&urb_list[i],
> +					       struct urb, urb_list);
> +			list_del_init(&urb->urb_list);
> +			if (i == URB_PRIO_HIGH)
> +				bh->completing_ep = urb->ep;
> +
> +			__usb_hcd_giveback_urb(urb);
> +
> +			if (i == URB_PRIO_HIGH)
> +				bh->completing_ep = NULL;
> +
> +			if (i == URB_PRIO_LOW &&
> +			    !list_empty_careful(&urb_list[URB_PRIO_HIGH])) {
> +				spin_lock_irq(&bh->lock);
> +				goto restart;
> +			}
> +		}
>  	}
>  
>  	/* check if there are new URBs to giveback */
>  	spin_lock_irq(&bh->lock);
> -	if (!list_empty(&bh->head))
> +	if (!list_empty(&bh->prio_hi_head) ||
> +	    !list_empty(&bh->prio_lo_head))
>  		goto restart;
> -	bh->running = false;
>  	spin_unlock_irq(&bh->lock);
> +	return IRQ_HANDLED;
>  }
>  
>  /**
> @@ -1823,37 +1865,32 @@ static void usb_giveback_urb_bh(unsigned long param)
>   */
>  void usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb, int status)
>  {
> -	struct giveback_urb_bh *bh;
> -	bool running, high_prio_bh;
> +	struct giveback_urb	*bh = &hcd->gb_urb;
> +	struct list_head	*lh;
>  
>  	/* pass status to tasklet via unlinked */
>  	if (likely(!urb->unlinked))
>  		urb->unlinked = status;
>  
> -	if (!hcd_giveback_urb_in_bh(hcd) && !is_root_hub(urb->dev)) {
> +	if (is_root_hub(urb->dev)) {
> +		spin_lock(&bh->lock);
> +		list_add_tail(&urb->urb_list, &bh->rh_head);
> +		spin_unlock(&bh->lock);
> +		queue_work(system_highpri_wq, &bh->rh_compl);
> +		return;
> +	}
> +	if (!hcd_giveback_urb_in_bh(hcd)) {
>  		__usb_hcd_giveback_urb(urb);
>  		return;
>  	}
>  
> -	if (usb_pipeisoc(urb->pipe) || usb_pipeint(urb->pipe)) {
> -		bh = &hcd->high_prio_bh;
> -		high_prio_bh = true;
> -	} else {
> -		bh = &hcd->low_prio_bh;
> -		high_prio_bh = false;
> -	}
> -
> -	spin_lock(&bh->lock);
> -	list_add_tail(&urb->urb_list, &bh->head);
> -	running = bh->running;
> -	spin_unlock(&bh->lock);
> -
> -	if (running)
> -		;
> -	else if (high_prio_bh)
> -		tasklet_hi_schedule(&bh->bh);
> +	if (usb_pipeisoc(urb->pipe) || usb_pipeint(urb->pipe))
> +		lh = &bh->prio_hi_head;
>  	else
> -		tasklet_schedule(&bh->bh);
> +		lh = &bh->prio_lo_head;
> +	spin_lock(&bh->lock);
> +	list_add_tail(&urb->urb_list, lh);
> +	spin_unlock(&bh->lock);
>  }
>  EXPORT_SYMBOL_GPL(usb_hcd_giveback_urb);
>  
> @@ -2436,9 +2473,17 @@ irqreturn_t usb_hcd_irq (int irq, void *__hcd)
>  		rc = IRQ_NONE;
>  	else if (hcd->driver->irq(hcd) == IRQ_NONE)
>  		rc = IRQ_NONE;
> -	else
> -		rc = IRQ_HANDLED;
> +	else {
> +		struct giveback_urb	*bh = &hcd->gb_urb;
>  
> +		spin_lock(&bh->lock);
> +		if (!list_empty(&bh->prio_hi_head) ||
> +		    !list_empty(&bh->prio_lo_head))
> +			rc = IRQ_WAKE_THREAD;
> +		else
> +			rc = IRQ_HANDLED;
> +		spin_unlock(&bh->lock);
> +	}
>  	return rc;
>  }
>  EXPORT_SYMBOL_GPL(usb_hcd_irq);
> @@ -2492,12 +2537,13 @@ EXPORT_SYMBOL_GPL (usb_hc_died);
>  
>  /*-------------------------------------------------------------------------*/
>  
> -static void init_giveback_urb_bh(struct giveback_urb_bh *bh)
> +static void init_giveback_urb(struct giveback_urb *bh)
>  {
> -
>  	spin_lock_init(&bh->lock);
> -	INIT_LIST_HEAD(&bh->head);
> -	tasklet_init(&bh->bh, usb_giveback_urb_bh, (unsigned long)bh);
> +	INIT_LIST_HEAD(&bh->prio_lo_head);
> +	INIT_LIST_HEAD(&bh->prio_hi_head);
> +	INIT_LIST_HEAD(&bh->rh_head);
> +	INIT_WORK(&bh->rh_compl, usb_hcd_rh_gb_urb);
>  }
>  
>  struct usb_hcd *__usb_create_hcd(const struct hc_driver *driver,
> @@ -2672,7 +2718,8 @@ static int usb_hcd_request_irqs(struct usb_hcd *hcd,
>  
>  		snprintf(hcd->irq_descr, sizeof(hcd->irq_descr), "%s:usb%d",
>  				hcd->driver->description, hcd->self.busnum);
> -		retval = request_irq(irqnum, &usb_hcd_irq, irqflags,
> +		retval = request_threaded_irq(irqnum, usb_hcd_irq,
> +					      usb_hcd_gb_urb, irqflags,
>  				hcd->irq_descr, hcd);
>  		if (retval != 0) {
>  			dev_err(hcd->self.controller,
> @@ -2863,9 +2910,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
>  			&& device_can_wakeup(&hcd->self.root_hub->dev))
>  		dev_dbg(hcd->self.controller, "supports USB remote wakeup\n");
>  
> -	/* initialize tasklets */
> -	init_giveback_urb_bh(&hcd->high_prio_bh);
> -	init_giveback_urb_bh(&hcd->low_prio_bh);
> +	init_giveback_urb(&hcd->gb_urb);
>  
>  	/* enable irqs just before we start the controller,
>  	 * if the BIOS provides legacy PCI irqs.
> diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
> index 176900528822..12573515acb6 100644
> --- a/include/linux/usb/hcd.h
> +++ b/include/linux/usb/hcd.h
> @@ -64,11 +64,12 @@
>  
>  /*-------------------------------------------------------------------------*/
>  
> -struct giveback_urb_bh {
> -	bool running;
> +struct giveback_urb {
>  	spinlock_t lock;
> -	struct list_head  head;
> -	struct tasklet_struct bh;
> +	struct list_head	prio_lo_head;
> +	struct list_head	prio_hi_head;
> +	struct list_head	rh_head;
> +	struct work_struct	rh_compl;
>  	struct usb_host_endpoint *completing_ep;
>  };
>  
> @@ -169,8 +170,7 @@ struct usb_hcd {
>  	resource_size_t		rsrc_len;	/* memory/io resource length */
>  	unsigned		power_budget;	/* in mA, 0 = no limit */
>  
> -	struct giveback_urb_bh  high_prio_bh;
> -	struct giveback_urb_bh  low_prio_bh;
> +	struct giveback_urb	gb_urb;
>  
>  	/* bandwidth_mutex should be taken before adding or removing
>  	 * any new bus bandwidth constraints:
> @@ -410,7 +410,7 @@ static inline int hcd_giveback_urb_in_bh(struct usb_hcd *hcd)
>  static inline bool hcd_periodic_completion_in_progress(struct usb_hcd *hcd,
>  		struct usb_host_endpoint *ep)
>  {
> -	return hcd->high_prio_bh.completing_ep == ep;
> +	return hcd->gb_urb.completing_ep == ep;
>  }
>  
>  extern int usb_hcd_link_urb_to_ep(struct usb_hcd *hcd, struct urb *urb);



Thanks,
Mauro

Re: [RFC PATCH] usb: hcd: complete URBs in threaded-IRQ context instead of tasklet

[Sebastian Andrzej Siewior]

On 2018-02-27 14:39:34 [-0300], Mauro Carvalho Chehab wrote:
> Hi Sebastian,
Hi Mauro,

> Sorry for taking some time to test it, has been busy those days...
:)

> Anyway, I tested it today. Didn't work. It keep losing data.

Okay, this was unexpected. What I learned from the thread is that you
use the dwc2 controller and once upgrade to a kernel which completes the
URBs in BH context then you starting losing data from your DVB-s USB
device. And it was assumed that this is because BH/ksoftirq is getting
"paused" if it is running for too long. If that is the case then a
revert of "let us complete the URB in BH context" should get it working
again. Is that so?

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -4287,7 +4287,9 @@ void dwc2_host_complete(struct dwc2_hsotg *hsotg, struct dwc2_qtd *qtd,
 	kfree(qtd->urb);
 	qtd->urb = NULL;
 
+	spin_unlock(&hsotg->lock);
 	usb_hcd_giveback_urb(dwc2_hsotg_to_hcd(hsotg), urb, status);
+	spin_lock(&hsotg->lock);
 }
 
 /*
@@ -4968,7 +4970,7 @@ static struct hc_driver dwc2_hc_driver = {
 	.hcd_priv_size = sizeof(struct wrapper_priv_data),
 
 	.irq = _dwc2_hcd_irq,
-	.flags = HCD_MEMORY | HCD_USB2 | HCD_BH,
+	.flags = HCD_MEMORY | HCD_USB2,
 
 	.start = _dwc2_hcd_start,
 	.stop = _dwc2_hcd_stop,
-- 
2.16.2


> Regards,

Sebastian

Re: [RFC PATCH] usb: hcd: complete URBs in threaded-IRQ context instead of tasklet

[Sebastian Andrzej Siewior]

On 2018-03-08 10:57:39 [+0100], To Mauro Carvalho Chehab wrote:
> On 2018-02-27 14:39:34 [-0300], Mauro Carvalho Chehab wrote:
> > Hi Sebastian,
> Hi Mauro,
> 
> > Sorry for taking some time to test it, has been busy those days...
> :)
> 
> > Anyway, I tested it today. Didn't work. It keep losing data.
> 
> Okay, this was unexpected. What I learned from the thread is that you
> use the dwc2 controller and once upgrade to a kernel which completes the
> URBs in BH context then you starting losing data from your DVB-s USB
> device. And it was assumed that this is because BH/ksoftirq is getting
> "paused" if it is running for too long. If that is the case then a
> revert of "let us complete the URB in BH context" should get it working
> again. Is that so?

ping

Sebastian

Re: [RFC PATCH] usb: hcd: complete URBs in threaded-IRQ context instead of tasklet

[Davidlohr Bueso]

On Mon, 16 Apr 2018, Sebastian Andrzej Siewior wrote:

>On 2018-03-08 10:57:39 [+0100], To Mauro Carvalho Chehab wrote:
>> On 2018-02-27 14:39:34 [-0300], Mauro Carvalho Chehab wrote:
>> > Hi Sebastian,
>> Hi Mauro,
>>
>> > Sorry for taking some time to test it, has been busy those days...
>> :)
>>
>> > Anyway, I tested it today. Didn't work. It keep losing data.
>>
>> Okay, this was unexpected. What I learned from the thread is that you
>> use the dwc2 controller and once upgrade to a kernel which completes the
>> URBs in BH context then you starting losing data from your DVB-s USB
>> device. And it was assumed that this is because BH/ksoftirq is getting
>> "paused" if it is running for too long. If that is the case then a
>> revert of "let us complete the URB in BH context" should get it working
>> again. Is that so?
>
>ping

I ran into this while looking at getting rid of tasklets in drivers/usb.

Mauro, were you ever able to try reverting 8add17cf8e4 like Sebastian suggested?
If not would you mind trying the below, please? Considering this thread is from
over two years ago, it's a rebase of Sebastian's patch to complete urbs in process
context + the dwc2 changes not to use defer urb into bh.

Thanks,
Davidlohr

----8<---------------------------------------------------------------------------
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 60886a7464c3..4952a8fc1719 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1665,33 +1665,76 @@ static void __usb_hcd_giveback_urb(struct urb *urb)
	usb_put_urb(urb);
 }

-static void usb_giveback_urb_bh(struct tasklet_struct *t)
+static void usb_hcd_rh_gb_urb(struct work_struct *work)
 {
-	struct giveback_urb_bh *bh = from_tasklet(bh, t, bh);
-	struct list_head local_list;
+	struct giveback_urb *bh;
+	struct list_head urb_list;
+
+	bh = container_of(work, struct giveback_urb, rh_compl);

	spin_lock_irq(&bh->lock);
-	bh->running = true;
- restart:
-	list_replace_init(&bh->head, &local_list);
+	list_replace_init(&bh->rh_head, &urb_list);
	spin_unlock_irq(&bh->lock);

-	while (!list_empty(&local_list)) {
+	while (!list_empty(&urb_list)) {
		struct urb *urb;

-		urb = list_entry(local_list.next, struct urb, urb_list);
+		urb = list_first_entry(&urb_list, struct urb, urb_list);
		list_del_init(&urb->urb_list);
-		bh->completing_ep = urb->ep;
		__usb_hcd_giveback_urb(urb);
-		bh->completing_ep = NULL;
+	}
+}
+
+#define URB_PRIO_HIGH	0
+#define URB_PRIO_LOW	1
+
+static irqreturn_t usb_hcd_gb_urb(int irq, void *__hcd)
+{
+	struct usb_hcd *hcd = __hcd;
+	struct giveback_urb *bh = &hcd->gb_urb;
+	struct list_head urb_list[2];
+	int i;
+
+	INIT_LIST_HEAD(&urb_list[URB_PRIO_HIGH]);
+	INIT_LIST_HEAD(&urb_list[URB_PRIO_LOW]);
+
+	spin_lock_irq(&bh->lock);
+ restart:
+	list_splice_tail_init(&bh->prio_hi_head, &urb_list[URB_PRIO_HIGH]);
+	list_splice_tail_init(&bh->prio_lo_head, &urb_list[URB_PRIO_LOW]);
+	spin_unlock_irq(&bh->lock);
+
+	for (i = 0; i < ARRAY_SIZE(urb_list); i++) {
+		while (!list_empty(&urb_list[i])) {
+			struct urb *urb;
+
+			urb = list_first_entry(&urb_list[i],
+					       struct urb, urb_list);
+			list_del_init(&urb->urb_list);
+			if (i == URB_PRIO_HIGH)
+				bh->completing_ep = urb->ep;
+
+			__usb_hcd_giveback_urb(urb);
+
+			if (i == URB_PRIO_HIGH)
+				bh->completing_ep = NULL;
+
+			if (i == URB_PRIO_LOW &&
+			    !list_empty_careful(&urb_list[URB_PRIO_HIGH])) {
+				spin_lock_irq(&bh->lock);
+				goto restart;
+			}
+		}
	}

	/* check if there are new URBs to giveback */
	spin_lock_irq(&bh->lock);
-	if (!list_empty(&bh->head))
+	if (!list_empty(&bh->prio_hi_head) ||
+	    !list_empty(&bh->prio_lo_head))
		goto restart;
-	bh->running = false;
	spin_unlock_irq(&bh->lock);
+
+	return IRQ_HANDLED;
 }

 /**
@@ -1717,37 +1760,34 @@ static void usb_giveback_urb_bh(struct tasklet_struct *t)
  */
 void usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb, int status)
 {
-	struct giveback_urb_bh *bh;
-	bool running, high_prio_bh;
+	struct giveback_urb	*bh = &hcd->gb_urb;
+	struct list_head	*lh;

	/* pass status to tasklet via unlinked */
	if (likely(!urb->unlinked))
		urb->unlinked = status;

-	if (!hcd_giveback_urb_in_bh(hcd) && !is_root_hub(urb->dev)) {
-		__usb_hcd_giveback_urb(urb);
+	if (is_root_hub(urb->dev)) {
+		spin_lock(&bh->lock);
+		list_add_tail(&urb->urb_list, &bh->rh_head);
+		spin_unlock(&bh->lock);
+		queue_work(system_highpri_wq, &bh->rh_compl);
		return;
	}

-	if (usb_pipeisoc(urb->pipe) || usb_pipeint(urb->pipe)) {
-		bh = &hcd->high_prio_bh;
-		high_prio_bh = true;
-	} else {
-		bh = &hcd->low_prio_bh;
-		high_prio_bh = false;
+	if (!hcd_giveback_urb_in_bh(hcd)) {
+		__usb_hcd_giveback_urb(urb);
+		return;
	}

+	if (usb_pipeisoc(urb->pipe) || usb_pipeint(urb->pipe))
+		lh = &bh->prio_hi_head;
+	else
+		lh = &bh->prio_lo_head;
+
	spin_lock(&bh->lock);
-	list_add_tail(&urb->urb_list, &bh->head);
-	running = bh->running;
+	list_add_tail(&urb->urb_list, lh);
	spin_unlock(&bh->lock);
-
-	if (running)
-		;
-	else if (high_prio_bh)
-		tasklet_hi_schedule(&bh->bh);
-	else
-		tasklet_schedule(&bh->bh);
 }
 EXPORT_SYMBOL_GPL(usb_hcd_giveback_urb);

@@ -2334,8 +2374,17 @@ irqreturn_t usb_hcd_irq (int irq, void *__hcd)
		rc = IRQ_NONE;
	else if (hcd->driver->irq(hcd) == IRQ_NONE)
		rc = IRQ_NONE;
-	else
-		rc = IRQ_HANDLED;
+	else {
+		struct giveback_urb	*bh = &hcd->gb_urb;
+
+		spin_lock(&bh->lock);
+		if (!list_empty(&bh->prio_hi_head) ||
+		    !list_empty(&bh->prio_lo_head))
+			rc = IRQ_WAKE_THREAD;
+		else
+			rc = IRQ_HANDLED;
+		spin_unlock(&bh->lock);
+	}

	return rc;
 }
@@ -2410,12 +2459,12 @@ EXPORT_SYMBOL_GPL (usb_hc_died);

 /*-------------------------------------------------------------------------*/

-static void init_giveback_urb_bh(struct giveback_urb_bh *bh)
+static void init_giveback_urb(struct giveback_urb *bh)
 {
-
-	spin_lock_init(&bh->lock);
-	INIT_LIST_HEAD(&bh->head);
-	tasklet_setup(&bh->bh, usb_giveback_urb_bh);
+	INIT_LIST_HEAD(&bh->prio_lo_head);
+	INIT_LIST_HEAD(&bh->prio_hi_head);
+	INIT_LIST_HEAD(&bh->rh_head);
+	INIT_WORK(&bh->rh_compl, usb_hcd_rh_gb_urb);
 }

 struct usb_hcd *__usb_create_hcd(const struct hc_driver *driver,
@@ -2593,8 +2642,9 @@ static int usb_hcd_request_irqs(struct usb_hcd *hcd,

		snprintf(hcd->irq_descr, sizeof(hcd->irq_descr), "%s:usb%d",
				hcd->driver->description, hcd->self.busnum);
-		retval = request_irq(irqnum, &usb_hcd_irq, irqflags,
-				hcd->irq_descr, hcd);
+		retval = request_threaded_irq(irqnum, &usb_hcd_irq,
+					      usb_hcd_gb_urb, irqflags,
+					      hcd->irq_descr, hcd);
		if (retval != 0) {
			dev_err(hcd->self.controller,
					"request interrupt %d failed\n",
@@ -2783,9 +2833,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
			&& device_can_wakeup(&hcd->self.root_hub->dev))
		dev_dbg(hcd->self.controller, "supports USB remote wakeup\n");

-	/* initialize tasklets */
-	init_giveback_urb_bh(&hcd->high_prio_bh);
-	init_giveback_urb_bh(&hcd->low_prio_bh);
+	init_giveback_urb(&hcd->gb_urb);

	/* enable irqs just before we start the controller,
	 * if the BIOS provides legacy PCI irqs.
diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index e9ac215b9663..fa6a0e7eb899 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -4162,7 +4162,9 @@ void dwc2_host_complete(struct dwc2_hsotg *hsotg, struct dwc2_qtd *qtd,
	kfree(qtd->urb);
	qtd->urb = NULL;

+	spin_unlock(&hsotg->lock);
	usb_hcd_giveback_urb(dwc2_hsotg_to_hcd(hsotg), urb, status);
+	spin_lock(&hsotg->lock);
 }

 /*
@@ -4902,7 +4904,7 @@ static struct hc_driver dwc2_hc_driver = {
	.hcd_priv_size = sizeof(struct wrapper_priv_data),

	.irq = _dwc2_hcd_irq,
-	.flags = HCD_MEMORY | HCD_USB2 | HCD_BH,
+	.flags = HCD_MEMORY | HCD_USB2,

	.start = _dwc2_hcd_start,
	.stop = _dwc2_hcd_stop,
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 96281cd50ff6..15a55aaa0e9c 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -64,11 +64,12 @@

 /*-------------------------------------------------------------------------*/

-struct giveback_urb_bh {
-	bool running;
+struct giveback_urb {
	spinlock_t lock;
-	struct list_head  head;
-	struct tasklet_struct bh;
+	struct list_head	prio_lo_head;
+	struct list_head	prio_hi_head;
+	struct list_head	rh_head;
+	struct work_struct	rh_compl;
	struct usb_host_endpoint *completing_ep;
 };

@@ -179,8 +180,7 @@ struct usb_hcd {
	resource_size_t		rsrc_len;	/* memory/io resource length */
	unsigned		power_budget;	/* in mA, 0 = no limit */

-	struct giveback_urb_bh  high_prio_bh;
-	struct giveback_urb_bh  low_prio_bh;
+	struct giveback_urb     gb_urb;

	/* bandwidth_mutex should be taken before adding or removing
	 * any new bus bandwidth constraints:
@@ -420,7 +420,7 @@ static inline int hcd_giveback_urb_in_bh(struct usb_hcd *hcd)
 static inline bool hcd_periodic_completion_in_progress(struct usb_hcd *hcd,
		struct usb_host_endpoint *ep)
 {
-	return hcd->high_prio_bh.completing_ep == ep;
+	return hcd->gb_urb.completing_ep == ep;
 }

 static inline bool hcd_uses_dma(struct usb_hcd *hcd)

Re: [RFC PATCH] usb: hcd: complete URBs in threaded-IRQ context instead of tasklet

[Davidlohr Bueso]

On Thu, 03 Dec 2020, Bueso wrote:

>On Mon, 16 Apr 2018, Sebastian Andrzej Siewior wrote:
>
>>On 2018-03-08 10:57:39 [+0100], To Mauro Carvalho Chehab wrote:
>>>On 2018-02-27 14:39:34 [-0300], Mauro Carvalho Chehab wrote:
>>>> Hi Sebastian,
>>>Hi Mauro,
>>>
>>>> Sorry for taking some time to test it, has been busy those days...
>>>:)
>>>
>>>> Anyway, I tested it today. Didn't work. It keep losing data.
>>>
>>>Okay, this was unexpected. What I learned from the thread is that you
>>>use the dwc2 controller and once upgrade to a kernel which completes the
>>>URBs in BH context then you starting losing data from your DVB-s USB
>>>device. And it was assumed that this is because BH/ksoftirq is getting
>>>"paused" if it is running for too long. If that is the case then a
>>>revert of "let us complete the URB in BH context" should get it working
>>>again. Is that so?
>>
>>ping
>
>I ran into this while looking at getting rid of tasklets in drivers/usb.
>
>Mauro, were you ever able to try reverting 8add17cf8e4 like Sebastian suggested?
>If not would you mind trying the below, please? Considering this thread is from
>over two years ago, it's a rebase of Sebastian's patch to complete urbs in process
>context + the dwc2 changes not to use defer urb into bh.

Hmm Mauro's email bounced, updating with a valid address.

>
>Thanks,
>Davidlohr
>
>----8<---------------------------------------------------------------------------
>diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
>index 60886a7464c3..4952a8fc1719 100644
>--- a/drivers/usb/core/hcd.c
>+++ b/drivers/usb/core/hcd.c
>@@ -1665,33 +1665,76 @@ static void __usb_hcd_giveback_urb(struct urb *urb)
>	usb_put_urb(urb);
>}
>
>-static void usb_giveback_urb_bh(struct tasklet_struct *t)
>+static void usb_hcd_rh_gb_urb(struct work_struct *work)
>{
>-	struct giveback_urb_bh *bh = from_tasklet(bh, t, bh);
>-	struct list_head local_list;
>+	struct giveback_urb *bh;
>+	struct list_head urb_list;
>+
>+	bh = container_of(work, struct giveback_urb, rh_compl);
>
>	spin_lock_irq(&bh->lock);
>-	bh->running = true;
>- restart:
>-	list_replace_init(&bh->head, &local_list);
>+	list_replace_init(&bh->rh_head, &urb_list);
>	spin_unlock_irq(&bh->lock);
>
>-	while (!list_empty(&local_list)) {
>+	while (!list_empty(&urb_list)) {
>		struct urb *urb;
>
>-		urb = list_entry(local_list.next, struct urb, urb_list);
>+		urb = list_first_entry(&urb_list, struct urb, urb_list);
>		list_del_init(&urb->urb_list);
>-		bh->completing_ep = urb->ep;
>		__usb_hcd_giveback_urb(urb);
>-		bh->completing_ep = NULL;
>+	}
>+}
>+
>+#define URB_PRIO_HIGH	0
>+#define URB_PRIO_LOW	1
>+
>+static irqreturn_t usb_hcd_gb_urb(int irq, void *__hcd)
>+{
>+	struct usb_hcd *hcd = __hcd;
>+	struct giveback_urb *bh = &hcd->gb_urb;
>+	struct list_head urb_list[2];
>+	int i;
>+
>+	INIT_LIST_HEAD(&urb_list[URB_PRIO_HIGH]);
>+	INIT_LIST_HEAD(&urb_list[URB_PRIO_LOW]);
>+
>+	spin_lock_irq(&bh->lock);
>+ restart:
>+	list_splice_tail_init(&bh->prio_hi_head, &urb_list[URB_PRIO_HIGH]);
>+	list_splice_tail_init(&bh->prio_lo_head, &urb_list[URB_PRIO_LOW]);
>+	spin_unlock_irq(&bh->lock);
>+
>+	for (i = 0; i < ARRAY_SIZE(urb_list); i++) {
>+		while (!list_empty(&urb_list[i])) {
>+			struct urb *urb;
>+
>+			urb = list_first_entry(&urb_list[i],
>+					       struct urb, urb_list);
>+			list_del_init(&urb->urb_list);
>+			if (i == URB_PRIO_HIGH)
>+				bh->completing_ep = urb->ep;
>+
>+			__usb_hcd_giveback_urb(urb);
>+
>+			if (i == URB_PRIO_HIGH)
>+				bh->completing_ep = NULL;
>+
>+			if (i == URB_PRIO_LOW &&
>+			    !list_empty_careful(&urb_list[URB_PRIO_HIGH])) {
>+				spin_lock_irq(&bh->lock);
>+				goto restart;
>+			}
>+		}
>	}
>
>	/* check if there are new URBs to giveback */
>	spin_lock_irq(&bh->lock);
>-	if (!list_empty(&bh->head))
>+	if (!list_empty(&bh->prio_hi_head) ||
>+	    !list_empty(&bh->prio_lo_head))
>		goto restart;
>-	bh->running = false;
>	spin_unlock_irq(&bh->lock);
>+
>+	return IRQ_HANDLED;
>}
>
>/**
>@@ -1717,37 +1760,34 @@ static void usb_giveback_urb_bh(struct tasklet_struct *t)
> */
>void usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb, int status)
>{
>-	struct giveback_urb_bh *bh;
>-	bool running, high_prio_bh;
>+	struct giveback_urb	*bh = &hcd->gb_urb;
>+	struct list_head	*lh;
>
>	/* pass status to tasklet via unlinked */
>	if (likely(!urb->unlinked))
>		urb->unlinked = status;
>
>-	if (!hcd_giveback_urb_in_bh(hcd) && !is_root_hub(urb->dev)) {
>-		__usb_hcd_giveback_urb(urb);
>+	if (is_root_hub(urb->dev)) {
>+		spin_lock(&bh->lock);
>+		list_add_tail(&urb->urb_list, &bh->rh_head);
>+		spin_unlock(&bh->lock);
>+		queue_work(system_highpri_wq, &bh->rh_compl);
>		return;
>	}
>
>-	if (usb_pipeisoc(urb->pipe) || usb_pipeint(urb->pipe)) {
>-		bh = &hcd->high_prio_bh;
>-		high_prio_bh = true;
>-	} else {
>-		bh = &hcd->low_prio_bh;
>-		high_prio_bh = false;
>+	if (!hcd_giveback_urb_in_bh(hcd)) {
>+		__usb_hcd_giveback_urb(urb);
>+		return;
>	}
>
>+	if (usb_pipeisoc(urb->pipe) || usb_pipeint(urb->pipe))
>+		lh = &bh->prio_hi_head;
>+	else
>+		lh = &bh->prio_lo_head;
>+
>	spin_lock(&bh->lock);
>-	list_add_tail(&urb->urb_list, &bh->head);
>-	running = bh->running;
>+	list_add_tail(&urb->urb_list, lh);
>	spin_unlock(&bh->lock);
>-
>-	if (running)
>-		;
>-	else if (high_prio_bh)
>-		tasklet_hi_schedule(&bh->bh);
>-	else
>-		tasklet_schedule(&bh->bh);
>}
>EXPORT_SYMBOL_GPL(usb_hcd_giveback_urb);
>
>@@ -2334,8 +2374,17 @@ irqreturn_t usb_hcd_irq (int irq, void *__hcd)
>		rc = IRQ_NONE;
>	else if (hcd->driver->irq(hcd) == IRQ_NONE)
>		rc = IRQ_NONE;
>-	else
>-		rc = IRQ_HANDLED;
>+	else {
>+		struct giveback_urb	*bh = &hcd->gb_urb;
>+
>+		spin_lock(&bh->lock);
>+		if (!list_empty(&bh->prio_hi_head) ||
>+		    !list_empty(&bh->prio_lo_head))
>+			rc = IRQ_WAKE_THREAD;
>+		else
>+			rc = IRQ_HANDLED;
>+		spin_unlock(&bh->lock);
>+	}
>
>	return rc;
>}
>@@ -2410,12 +2459,12 @@ EXPORT_SYMBOL_GPL (usb_hc_died);
>
>/*-------------------------------------------------------------------------*/
>
>-static void init_giveback_urb_bh(struct giveback_urb_bh *bh)
>+static void init_giveback_urb(struct giveback_urb *bh)
>{
>-
>-	spin_lock_init(&bh->lock);
>-	INIT_LIST_HEAD(&bh->head);
>-	tasklet_setup(&bh->bh, usb_giveback_urb_bh);
>+	INIT_LIST_HEAD(&bh->prio_lo_head);
>+	INIT_LIST_HEAD(&bh->prio_hi_head);
>+	INIT_LIST_HEAD(&bh->rh_head);
>+	INIT_WORK(&bh->rh_compl, usb_hcd_rh_gb_urb);
>}
>
>struct usb_hcd *__usb_create_hcd(const struct hc_driver *driver,
>@@ -2593,8 +2642,9 @@ static int usb_hcd_request_irqs(struct usb_hcd *hcd,
>
>		snprintf(hcd->irq_descr, sizeof(hcd->irq_descr), "%s:usb%d",
>				hcd->driver->description, hcd->self.busnum);
>-		retval = request_irq(irqnum, &usb_hcd_irq, irqflags,
>-				hcd->irq_descr, hcd);
>+		retval = request_threaded_irq(irqnum, &usb_hcd_irq,
>+					      usb_hcd_gb_urb, irqflags,
>+					      hcd->irq_descr, hcd);
>		if (retval != 0) {
>			dev_err(hcd->self.controller,
>					"request interrupt %d failed\n",
>@@ -2783,9 +2833,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
>			&& device_can_wakeup(&hcd->self.root_hub->dev))
>		dev_dbg(hcd->self.controller, "supports USB remote wakeup\n");
>
>-	/* initialize tasklets */
>-	init_giveback_urb_bh(&hcd->high_prio_bh);
>-	init_giveback_urb_bh(&hcd->low_prio_bh);
>+	init_giveback_urb(&hcd->gb_urb);
>
>	/* enable irqs just before we start the controller,
>	 * if the BIOS provides legacy PCI irqs.
>diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>index e9ac215b9663..fa6a0e7eb899 100644
>--- a/drivers/usb/dwc2/hcd.c
>+++ b/drivers/usb/dwc2/hcd.c
>@@ -4162,7 +4162,9 @@ void dwc2_host_complete(struct dwc2_hsotg *hsotg, struct dwc2_qtd *qtd,
>	kfree(qtd->urb);
>	qtd->urb = NULL;
>
>+	spin_unlock(&hsotg->lock);
>	usb_hcd_giveback_urb(dwc2_hsotg_to_hcd(hsotg), urb, status);
>+	spin_lock(&hsotg->lock);
>}
>
>/*
>@@ -4902,7 +4904,7 @@ static struct hc_driver dwc2_hc_driver = {
>	.hcd_priv_size = sizeof(struct wrapper_priv_data),
>
>	.irq = _dwc2_hcd_irq,
>-	.flags = HCD_MEMORY | HCD_USB2 | HCD_BH,
>+	.flags = HCD_MEMORY | HCD_USB2,
>
>	.start = _dwc2_hcd_start,
>	.stop = _dwc2_hcd_stop,
>diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
>index 96281cd50ff6..15a55aaa0e9c 100644
>--- a/include/linux/usb/hcd.h
>+++ b/include/linux/usb/hcd.h
>@@ -64,11 +64,12 @@
>
>/*-------------------------------------------------------------------------*/
>
>-struct giveback_urb_bh {
>-	bool running;
>+struct giveback_urb {
>	spinlock_t lock;
>-	struct list_head  head;
>-	struct tasklet_struct bh;
>+	struct list_head	prio_lo_head;
>+	struct list_head	prio_hi_head;
>+	struct list_head	rh_head;
>+	struct work_struct	rh_compl;
>	struct usb_host_endpoint *completing_ep;
>};
>
>@@ -179,8 +180,7 @@ struct usb_hcd {
>	resource_size_t		rsrc_len;	/* memory/io resource length */
>	unsigned		power_budget;	/* in mA, 0 = no limit */
>
>-	struct giveback_urb_bh  high_prio_bh;
>-	struct giveback_urb_bh  low_prio_bh;
>+	struct giveback_urb     gb_urb;
>
>	/* bandwidth_mutex should be taken before adding or removing
>	 * any new bus bandwidth constraints:
>@@ -420,7 +420,7 @@ static inline int hcd_giveback_urb_in_bh(struct usb_hcd *hcd)
>static inline bool hcd_periodic_completion_in_progress(struct usb_hcd *hcd,
>		struct usb_host_endpoint *ep)
>{
>-	return hcd->high_prio_bh.completing_ep == ep;
>+	return hcd->gb_urb.completing_ep == ep;
>}
>
>static inline bool hcd_uses_dma(struct usb_hcd *hcd)