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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 20 Feb 2024 09:25:40 -0800
From: Davidlohr Bueso <dave@...olabs.net>
To: Tejun Heo <tj@...nel.org>
Cc: torvalds@...ux-foundation.org, mpatocka@...hat.com, 
	linux-kernel@...r.kernel.org, dm-devel@...ts.linux.dev, msnitzer@...hat.com, 
	ignat@...udflare.com, damien.lemoal@....com, bob.liu@...cle.com, houtao1@...wei.com, 
	peterz@...radead.org, mingo@...nel.org, netdev@...r.kernel.org, allen.lkml@...il.com, 
	kernel-team@...a.com, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	Alan Stern <stern@...land.harvard.edu>, linux-usb@...r.kernel.org, mchehab@...nel.org
Subject: Re: [PATCH 5/8] usb: core: hcd: Convert from tasklet to BH workqueue

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@breakpoint.cc/

>
>Signed-off-by: Tejun Heo <tj@...nel.org>
>Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
>Cc: Alan Stern <stern@...land.harvard.edu>
>Cc: linux-usb@...r.kernel.org

Acked-by: Davidlohr Bueso <dave@...olabs.net>

>---
> 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
>