Re: [PATCH 5/8] usb: core: hcd: Convert from tasklet to BH workqueue

[Davidlohr Bueso]

On Mon, 29 Jan 2024, Tejun Heo wrote:

> The only generic interface to execute asynchronously in the BH context is
> tasklet; however, it's marked deprecated and has some design flaws. To
> replace tasklets, BH workqueue support was recently added. A BH workqueue
> behaves similarly to regular workqueues except that the queued work items
> are executed in the BH context.
>
> This patch converts usb hcd from tasklet to BH workqueue.

In the past this tasklet removal was held up by Mauro's device not properly
streaming - hopefully this no longer the case. Cc'ing.

https://lore.kernel.org/all/20180216170450.yl5owfphuvltstnt@xxxxxxxxxxxxx/

> Signed-off-by: Tejun Heo <tj@xxxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> Cc: linux-usb@xxxxxxxxxxxxxxx

Acked-by: Davidlohr Bueso <dave@xxxxxxxxxxxx>

> ---
> drivers/usb/core/hcd.c  | 23 ++++++++++++-----------
> include/linux/usb/hcd.h |  2 +-
> 2 files changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 12b6dfeaf658..edf74458474a 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -1664,9 +1664,10 @@ 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_giveback_urb_bh(struct work_struct *work)
> {
> -	struct giveback_urb_bh *bh = from_tasklet(bh, t, bh);
> +	struct giveback_urb_bh *bh =
> +		container_of(work, struct giveback_urb_bh, bh);
> 	struct list_head local_list;
>
> 	spin_lock_irq(&bh->lock);
> @@ -1691,9 +1692,9 @@ static void usb_giveback_urb_bh(struct tasklet_struct *t)
> 	spin_lock_irq(&bh->lock);
> 	if (!list_empty(&bh->head)) {
> 		if (bh->high_prio)
> -			tasklet_hi_schedule(&bh->bh);
> +			queue_work(system_bh_highpri_wq, &bh->bh);
> 		else
> -			tasklet_schedule(&bh->bh);
> +			queue_work(system_bh_wq, &bh->bh);
> 	}
> 	bh->running = false;
> 	spin_unlock_irq(&bh->lock);
> @@ -1706,7 +1707,7 @@ static void usb_giveback_urb_bh(struct tasklet_struct *t)
>  * @status: completion status code for the URB.
>  *
>  * Context: atomic. The completion callback is invoked in caller's context.
> - * For HCDs with HCD_BH flag set, the completion callback is invoked in tasklet
> + * For HCDs with HCD_BH flag set, the completion callback is invoked in BH
>  * context (except for URBs submitted to the root hub which always complete in
>  * caller's context).
>  *
> @@ -1725,7 +1726,7 @@ void usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb, int status)
> 	struct giveback_urb_bh *bh;
> 	bool running;
>
> -	/* pass status to tasklet via unlinked */
> +	/* pass status to BH via unlinked */
> 	if (likely(!urb->unlinked))
> 		urb->unlinked = status;
>
> @@ -1747,9 +1748,9 @@ void usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb, int status)
> 	if (running)
> 		;
> 	else if (bh->high_prio)
> -		tasklet_hi_schedule(&bh->bh);
> +		queue_work(system_bh_highpri_wq, &bh->bh);
> 	else
> -		tasklet_schedule(&bh->bh);
> +		queue_work(system_bh_wq, &bh->bh);
> }
> EXPORT_SYMBOL_GPL(usb_hcd_giveback_urb);
>
> @@ -2540,7 +2541,7 @@ static void init_giveback_urb_bh(struct giveback_urb_bh *bh)
>
> 	spin_lock_init(&bh->lock);
> 	INIT_LIST_HEAD(&bh->head);
> -	tasklet_setup(&bh->bh, usb_giveback_urb_bh);
> +	INIT_WORK(&bh->bh, usb_giveback_urb_bh);
> }
>
> struct usb_hcd *__usb_create_hcd(const struct hc_driver *driver,
> @@ -2926,7 +2927,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 */
> +	/* initialize BHs */
> 	init_giveback_urb_bh(&hcd->high_prio_bh);
> 	hcd->high_prio_bh.high_prio = true;
> 	init_giveback_urb_bh(&hcd->low_prio_bh);
> @@ -3036,7 +3037,7 @@ void usb_remove_hcd(struct usb_hcd *hcd)
> 	mutex_unlock(&usb_bus_idr_lock);
>
> 	/*
> -	 * tasklet_kill() isn't needed here because:
> +	 * flush_work() isn't needed here because:
> 	 * - driver's disconnect() called from usb_disconnect() should
> 	 *   make sure its URBs are completed during the disconnect()
> 	 *   callback
> diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
> index 00724b4f6e12..f698aac71de3 100644
> --- a/include/linux/usb/hcd.h
> +++ b/include/linux/usb/hcd.h
> @@ -55,7 +55,7 @@ struct giveback_urb_bh {
> 	bool high_prio;
> 	spinlock_t lock;
> 	struct list_head  head;
> -	struct tasklet_struct bh;
> +	struct work_struct bh;
> 	struct usb_host_endpoint *completing_ep;
> };
>
> --
> 2.43.0