| Message ID | ea44fb76-0a9d-4009-8eba-021f0928cc77@0882a8b5-c6c3-11e9-b005-00805fc181fe.uuid.home.arpa (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | can: mcp251x: fix deadlock if an interrupt occurs during mcp251x_open | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Series ignored based on subject |
On 8/17/24 17:45, Simon Arlott wrote: > The mcp251x_hw_wake() function is called with the mpc_lock mutex held and > disables the interrupt handler so that no interrupts can be processed while > waking the device. If an interrupt has already occurred then waiting for > the interrupt handler to complete will deadlock because it will be trying > to acquire the same mutex. > > CPU0 CPU1 > ---- ---- > mcp251x_open() > mutex_lock(&priv->mcp_lock) > request_threaded_irq() > <interrupt> > mcp251x_can_ist() > mutex_lock(&priv->mcp_lock) > mcp251x_hw_wake() > disable_irq() <-- deadlock > > Use disable_irq_nosync() instead because the interrupt handler does > everything while holding the mutex so it doesn't matter if it's still > running. > > Signed-off-by: Simon Arlott <simon@octiron.net> You have to provide a Fixes: tag for bugfixes [PATCH net] otherwise the change looks fine for me, Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > --- > drivers/net/can/spi/mcp251x.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/can/spi/mcp251x.c b/drivers/net/can/spi/mcp251x.c > index 3b8736ff0345..ec5c64006a16 100644 > --- a/drivers/net/can/spi/mcp251x.c > +++ b/drivers/net/can/spi/mcp251x.c > @@ -752,7 +752,7 @@ static int mcp251x_hw_wake(struct spi_device *spi) > int ret; > > /* Force wakeup interrupt to wake device, but don't execute IST */ > - disable_irq(spi->irq); > + disable_irq_nosync(spi->irq); > mcp251x_write_2regs(spi, CANINTE, CANINTE_WAKIE, CANINTF_WAKIF); > > /* Wait for oscillator startup timer after wake up */
diff --git a/drivers/net/can/spi/mcp251x.c b/drivers/net/can/spi/mcp251x.c index 3b8736ff0345..ec5c64006a16 100644 --- a/drivers/net/can/spi/mcp251x.c +++ b/drivers/net/can/spi/mcp251x.c @@ -752,7 +752,7 @@ static int mcp251x_hw_wake(struct spi_device *spi) int ret; /* Force wakeup interrupt to wake device, but don't execute IST */ - disable_irq(spi->irq); + disable_irq_nosync(spi->irq); mcp251x_write_2regs(spi, CANINTE, CANINTE_WAKIE, CANINTF_WAKIF); /* Wait for oscillator startup timer after wake up */
The mcp251x_hw_wake() function is called with the mpc_lock mutex held and disables the interrupt handler so that no interrupts can be processed while waking the device. If an interrupt has already occurred then waiting for the interrupt handler to complete will deadlock because it will be trying to acquire the same mutex. CPU0 CPU1 ---- ---- mcp251x_open() mutex_lock(&priv->mcp_lock) request_threaded_irq() <interrupt> mcp251x_can_ist() mutex_lock(&priv->mcp_lock) mcp251x_hw_wake() disable_irq() <-- deadlock Use disable_irq_nosync() instead because the interrupt handler does everything while holding the mutex so it doesn't matter if it's still running. Signed-off-by: Simon Arlott <simon@octiron.net> --- drivers/net/can/spi/mcp251x.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)