Re: [PATCH 4/9] USB: Convert from tasklet to BH workqueue
> >
> > This patch converts drivers/infiniband/* from tasklet to BH workqueue.
> >
> > Based on the work done by Tejun Heo
> > Branch: https://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git for-6.10
> >
> > Signed-off-by: Allen Pais
> > ---
>
> > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> > index c0e005670d67..88d8e1c366cd 100644
> > --- a/drivers/usb/core/hcd.c
> > +++ b/drivers/usb/core/hcd.c
>
> > @@ -1662,10 +1663,9 @@ static void __usb_hcd_giveback_urb(struct urb *urb)
> > usb_put_urb(urb);
> > }
> >
> > -static void usb_giveback_urb_bh(struct work_struct *work)
> > +static void usb_giveback_urb_bh(struct work_struct *t)
> > {
> > - struct giveback_urb_bh *bh =
> > - container_of(work, struct giveback_urb_bh, bh);
> > + struct giveback_urb_bh *bh = from_work(bh, t, bh);
> > struct list_head local_list;
> >
> > spin_lock_irq(>lock);
>
> Is there any reason for this apparently pointless change of a local
> variable's name?
No, it was done just to keep things consistent across the kernel.
I can revert it back to *work if you'd prefer.
Thanks.
>
> Alan Stern
>
--
- Allen
Re: [PATCH 4/9] USB: Convert from tasklet to BH workqueue
On Wed, Mar 27, 2024 at 04:03:09PM +, Allen Pais 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 drivers/infiniband/* from tasklet to BH workqueue.
>
> Based on the work done by Tejun Heo
> Branch: https://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git for-6.10
>
> Signed-off-by: Allen Pais
> ---
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index c0e005670d67..88d8e1c366cd 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -1662,10 +1663,9 @@ static void __usb_hcd_giveback_urb(struct urb *urb)
> usb_put_urb(urb);
> }
>
> -static void usb_giveback_urb_bh(struct work_struct *work)
> +static void usb_giveback_urb_bh(struct work_struct *t)
> {
> - struct giveback_urb_bh *bh =
> - container_of(work, struct giveback_urb_bh, bh);
> + struct giveback_urb_bh *bh = from_work(bh, t, bh);
> struct list_head local_list;
>
> spin_lock_irq(>lock);
Is there any reason for this apparently pointless change of a local
variable's name?
Alan Stern
Re: [PATCH 4/9] USB: Convert from tasklet to BH workqueue
> > 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 drivers/infiniband/* from tasklet to BH workqueue.
>
> No it does not, I think your changelog is wrong :(
Whoops, sorry about that. I messed up the commit messages. I will fix it in v2.
>
> >
> > Based on the work done by Tejun Heo
> > Branch: https://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git for-6.10
> >
> > Signed-off-by: Allen Pais
> > ---
> > drivers/usb/atm/usbatm.c| 55 +++--
> > drivers/usb/atm/usbatm.h| 3 +-
> > drivers/usb/core/hcd.c | 22 ++--
> > drivers/usb/gadget/udc/fsl_qe_udc.c | 21 +--
> > drivers/usb/gadget/udc/fsl_qe_udc.h | 4 +--
> > drivers/usb/host/ehci-sched.c | 2 +-
> > drivers/usb/host/fhci-hcd.c | 3 +-
> > drivers/usb/host/fhci-sched.c | 10 +++---
> > drivers/usb/host/fhci.h | 5 +--
> > drivers/usb/host/xhci-dbgcap.h | 3 +-
> > drivers/usb/host/xhci-dbgtty.c | 15
> > include/linux/usb/cdc_ncm.h | 2 +-
> > include/linux/usb/usbnet.h | 2 +-
> > 13 files changed, 76 insertions(+), 71 deletions(-)
> >
> > diff --git a/drivers/usb/atm/usbatm.c b/drivers/usb/atm/usbatm.c
> > index 2da6615fbb6f..74849f24e52e 100644
> > --- a/drivers/usb/atm/usbatm.c
> > +++ b/drivers/usb/atm/usbatm.c
> > @@ -17,7 +17,7 @@
> > * - Removed the limit on the number of devices
> > * - Module now autoloads on device plugin
> > * - Merged relevant parts of sarlib
> > - * - Replaced the kernel thread with a tasklet
> > + * - Replaced the kernel thread with a work
>
> a "work"?
will fix the comments.
>
> > * - New packet transmission code
> > * - Changed proc file contents
> > * - Fixed all known SMP races
> > @@ -68,6 +68,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #ifdef VERBOSE_DEBUG
> > static int usbatm_print_packet(struct usbatm_data *instance, const
> > unsigned char *data, int len);
> > @@ -249,7 +250,7 @@ static void usbatm_complete(struct urb *urb)
> > /* vdbg("%s: urb 0x%p, status %d, actual_length %d",
> >__func__, urb, status, urb->actual_length); */
> >
> > - /* Can be invoked from task context, protect against interrupts */
> > + /* Can be invoked from work context, protect against interrupts */
>
> "workqueue"? This too seems wrong.
>
> Same for other comment changes in this patch.
Thanks for the quick review, I will fix the comments and send out v2.
- Alle
> thanks,
>
> greg k-h
>
Re: [PATCH 4/9] USB: Convert from tasklet to BH workqueue
On Wed, Mar 27, 2024 at 04:03:09PM +, Allen Pais 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 drivers/infiniband/* from tasklet to BH workqueue.
No it does not, I think your changelog is wrong :(
>
> Based on the work done by Tejun Heo
> Branch: https://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git for-6.10
>
> Signed-off-by: Allen Pais
> ---
> drivers/usb/atm/usbatm.c| 55 +++--
> drivers/usb/atm/usbatm.h| 3 +-
> drivers/usb/core/hcd.c | 22 ++--
> drivers/usb/gadget/udc/fsl_qe_udc.c | 21 +--
> drivers/usb/gadget/udc/fsl_qe_udc.h | 4 +--
> drivers/usb/host/ehci-sched.c | 2 +-
> drivers/usb/host/fhci-hcd.c | 3 +-
> drivers/usb/host/fhci-sched.c | 10 +++---
> drivers/usb/host/fhci.h | 5 +--
> drivers/usb/host/xhci-dbgcap.h | 3 +-
> drivers/usb/host/xhci-dbgtty.c | 15
> include/linux/usb/cdc_ncm.h | 2 +-
> include/linux/usb/usbnet.h | 2 +-
> 13 files changed, 76 insertions(+), 71 deletions(-)
>
> diff --git a/drivers/usb/atm/usbatm.c b/drivers/usb/atm/usbatm.c
> index 2da6615fbb6f..74849f24e52e 100644
> --- a/drivers/usb/atm/usbatm.c
> +++ b/drivers/usb/atm/usbatm.c
> @@ -17,7 +17,7 @@
> * - Removed the limit on the number of devices
> * - Module now autoloads on device plugin
> * - Merged relevant parts of sarlib
> - * - Replaced the kernel thread with a tasklet
> + * - Replaced the kernel thread with a work
a "work"?
> * - New packet transmission code
> * - Changed proc file contents
> * - Fixed all known SMP races
> @@ -68,6 +68,7 @@
> #include
> #include
> #include
> +#include
>
> #ifdef VERBOSE_DEBUG
> static int usbatm_print_packet(struct usbatm_data *instance, const unsigned
> char *data, int len);
> @@ -249,7 +250,7 @@ static void usbatm_complete(struct urb *urb)
> /* vdbg("%s: urb 0x%p, status %d, actual_length %d",
>__func__, urb, status, urb->actual_length); */
>
> - /* Can be invoked from task context, protect against interrupts */
> + /* Can be invoked from work context, protect against interrupts */
"workqueue"? This too seems wrong.
Same for other comment changes in this patch.
thanks,
greg k-h
Re: [PATCH 4/9] USB: Convert from tasklet to BH workqueue
Hi Allen, the usbatm bits look very reasonable to me. Unfortunately I don't have the hardware to test any more. Still, for what it's worth: Signed-off-by: Duncan Sands
