[PATCH 2/2] USB: max3421: Fix scheduling while atomic in max3421_remove()

[PATCH 2/2] USB: max3421: Fix scheduling while atomic in max3421_remove()

[Matt Lee]

A bug in `max3421_remove()` caused a "scheduling while atomic" crash when
`kthread_stop()` was called while holding a spinlock.

This patch ensures that `kthread_stop()` is called outside the spinlock,
fixing the crash and improving system stability when unloading the driver.

Signed-off-by: Matt Lee <matt@oscium.com>
---
 drivers/usb/host/max3421-hcd.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/usb/host/max3421-hcd.c b/drivers/usb/host/max3421-hcd.c
index abcdef0..1234567 100644
--- a/drivers/usb/host/max3421-hcd.c
+++ b/drivers/usb/host/max3421-hcd.c
@@ -1936,11 +1945,10 @@ max3421_remove(struct spi_device *spi)
        usb_remove_hcd(hcd);

        spin_lock_irqsave(&max3421_hcd->lock, flags);
+       spin_unlock_irqrestore(&max3421_hcd->lock, flags);

        kthread_stop(max3421_hcd->spi_thread);

-       spin_unlock_irqrestore(&max3421_hcd->lock, flags);
-
        free_irq(spi->irq, hcd);

        usb_put_hcd(hcd);
-- 
2.34.1

Re: [PATCH 2/2] USB: max3421: Fix scheduling while atomic in max3421_remove()

[Greg KH]

On Mon, Feb 10, 2025 at 07:48:29AM -0600, Matt Lee wrote:
> A bug in `max3421_remove()` caused a "scheduling while atomic" crash when
> `kthread_stop()` was called while holding a spinlock.
> 
> This patch ensures that `kthread_stop()` is called outside the spinlock,
> fixing the crash and improving system stability when unloading the driver.
> 
> Signed-off-by: Matt Lee <matt@oscium.com>
> ---
>  drivers/usb/host/max3421-hcd.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/host/max3421-hcd.c b/drivers/usb/host/max3421-hcd.c
> index abcdef0..1234567 100644
> --- a/drivers/usb/host/max3421-hcd.c
> +++ b/drivers/usb/host/max3421-hcd.c
> @@ -1936,11 +1945,10 @@ max3421_remove(struct spi_device *spi)
>         usb_remove_hcd(hcd);
> 
>         spin_lock_irqsave(&max3421_hcd->lock, flags);
> +       spin_unlock_irqrestore(&max3421_hcd->lock, flags);
> 
>         kthread_stop(max3421_hcd->spi_thread);
> 
> -       spin_unlock_irqrestore(&max3421_hcd->lock, flags);
> -

So now the lock does nothing?  Are you sure this change is correct?

thanks,

greg k-h

Re: [PATCH 2/2] USB: max3421: Fix scheduling while atomic in max3421_remove()

[Matt Lee]

The submitted patch fixes a crash we are experiencing on every shutdown.
Perhaps the better solution is to remove the spin_lock_irqsave/restore?

I tested removing the spin_lock completely and the crash issue was eliminated.
Happy to update the patch if this is the preferred method.

Kind regards,

Matt


On Mon, Feb 10, 2025 at 8:12 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Feb 10, 2025 at 07:48:29AM -0600, Matt Lee wrote:
> > A bug in `max3421_remove()` caused a "scheduling while atomic" crash when
> > `kthread_stop()` was called while holding a spinlock.
> >
> > This patch ensures that `kthread_stop()` is called outside the spinlock,
> > fixing the crash and improving system stability when unloading the driver.
> >
> > Signed-off-by: Matt Lee <matt@oscium.com>
> > ---
> >  drivers/usb/host/max3421-hcd.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/drivers/usb/host/max3421-hcd.c b/drivers/usb/host/max3421-hcd.c
> > index abcdef0..1234567 100644
> > --- a/drivers/usb/host/max3421-hcd.c
> > +++ b/drivers/usb/host/max3421-hcd.c
> > @@ -1936,11 +1945,10 @@ max3421_remove(struct spi_device *spi)
> >         usb_remove_hcd(hcd);
> >
> >         spin_lock_irqsave(&max3421_hcd->lock, flags);
> > +       spin_unlock_irqrestore(&max3421_hcd->lock, flags);
> >
> >         kthread_stop(max3421_hcd->spi_thread);
> >
> > -       spin_unlock_irqrestore(&max3421_hcd->lock, flags);
> > -
>
> So now the lock does nothing?  Are you sure this change is correct?
>
> thanks,
>
> greg k-h

Re: [PATCH 2/2] USB: max3421: Fix scheduling while atomic in max3421_remove()

[Greg KH]

On Mon, Feb 10, 2025 at 09:42:46AM -0600, Matt Lee wrote:
> The submitted patch fixes a crash we are experiencing on every shutdown.
> Perhaps the better solution is to remove the spin_lock_irqsave/restore?

Please don't top post.

Having a lock/unlock pair without anything in it can be used for
something (it's a gate primitive), but I don't think that it's being
used for that here.

> I tested removing the spin_lock completely and the crash issue was eliminated.
> Happy to update the patch if this is the preferred method.

Ideally fix the root issue in that this lock is needed to protect the
data here, so removing it doesn't actually fix the real problem, only
trade the crash for a race condition, right?

thanks,

greg k-h