Messages in this thread | | | Date | Fri, 4 Dec 2020 23:11:32 -0800 | From | Davidlohr Bueso <> | Subject | Re: [RFC PATCH] usb: hcd: complete URBs in threaded-IRQ context instead of tasklet |
| |
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)
| |