Date:   Fri, 16 Feb 2018 18:04:50 +0100
From:   Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To:     Mauro Carvalho Chehab <mchehab@...pensource.com>
Cc:     Frederic Weisbecker <frederic@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Alan Stern <stern@...land.harvard.edu>,
        linux-usb@...r.kernel.org
Subject: [RFC PATCH] usb: hcd: complete URBs in threaded-IRQ context instead
 of tasklet

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@...utronix.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

Powered by blists - more mailing lists