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)
