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

[Davidlohr Bueso <dave@xxxxxxxxxxxx>]

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)