| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20201205071132.noumprob5kgkrvyy@linux-p48b.lan>
Date: Fri, 4 Dec 2020 23:11:32 -0800
From: Davidlohr Bueso <dave@...olabs.net>
To: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>,
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: 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)
Powered by blists - more mailing lists