Received-SPF: pass (google.com: domain of linux-kernel+bounces-73440-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Tue, 20 Feb 2024 09:25:40 -0800
From: Davidlohr Bueso <dave@stgolabs.net>
To: Tejun Heo <tj@kernel.org>
Cc: torvalds@linux-foundation.org, mpatocka@redhat.com, 
	linux-kernel@vger.kernel.org, dm-devel@lists.linux.dev, msnitzer@redhat.com, 
	ignat@cloudflare.com, damien.lemoal@wdc.com, bob.liu@oracle.com, houtao1@huawei.com, 
	peterz@infradead.org, mingo@kernel.org, netdev@vger.kernel.org, allen.lkml@gmail.com, 
	kernel-team@meta.com, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, 
	Alan Stern <stern@rowland.harvard.edu>, linux-usb@vger.kernel.org, mchehab@kernel.org
Subject: Re: [PATCH 5/8] usb: core: hcd: Convert from tasklet to BH workqueue
Message-ID: <bckroyio6l2nt54refuord4pm6mqylt3adx6z2bg6iczxkbnyk@bb5447rqahj5>
Mail-Followup-To: Tejun Heo <tj@kernel.org>, torvalds@linux-foundation.org, 
	mpatocka@redhat.com, linux-kernel@vger.kernel.org, dm-devel@lists.linux.dev, 
	msnitzer@redhat.com, ignat@cloudflare.com, damien.lemoal@wdc.com, bob.liu@oracle.com, 
	houtao1@huawei.com, peterz@infradead.org, mingo@kernel.org, netdev@vger.kernel.org, 
	allen.lkml@gmail.com, kernel-team@meta.com, 
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Alan Stern <stern@rowland.harvard.edu>, 
	linux-usb@vger.kernel.org, mchehab@kernel.org
References: <20240130091300.2968534-1-tj@kernel.org>
 <20240130091300.2968534-6-tj@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <20240130091300.2968534-6-tj@kernel.org>
User-Agent: NeoMutt/20231221

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@kernel.org>
>Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>Cc: Alan Stern <stern@rowland.harvard.edu>
>Cc: linux-usb@vger.kernel.org

Acked-by: Davidlohr Bueso <dave@stgolabs.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
>