As &sport->port.lock is acquired under irq context along the following
call chain from imx_uart_rtsint(), other acquisition of the same lock
inside process context or softirq context should disable irq avoid double
lock.
<deadlock #1>
imx_uart_dma_rx_callback()
--> spin_lock(&sport->port.lock)
<interrupt>
--> imx_uart_rtsint()
--> spin_lock(&sport->port.lock)
This flaw was found by an experimental static analysis tool I am
developing for irq-related deadlock.
To prevent the potential deadlock, the patch uses spin_lock_irqsave()
on the &sport->port.lock inside imx_uart_dma_rx_callback() to prevent
the possible deadlock scenario.
Signed-off-by: Chengfeng Ye <[email protected]>
---
drivers/tty/serial/imx.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 13cb78340709..7bb3aa19d51c 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1165,13 +1165,14 @@ static void imx_uart_dma_rx_callback(void *data)
unsigned int w_bytes = 0;
unsigned int r_bytes;
unsigned int bd_size;
+ unsigned long flags;
status = dmaengine_tx_status(chan, sport->rx_cookie, &state);
if (status == DMA_ERROR) {
- spin_lock(&sport->port.lock);
+ spin_lock_irqsave(&sport->port.lock, flags);
imx_uart_clear_rx_errors(sport);
- spin_unlock(&sport->port.lock);
+ spin_unlock_irqrestore(&sport->port.lock, flags);
return;
}
@@ -1200,9 +1201,9 @@ static void imx_uart_dma_rx_callback(void *data)
r_bytes = rx_ring->head - rx_ring->tail;
/* If we received something, check for 0xff flood */
- spin_lock(&sport->port.lock);
+ spin_lock_irqsave(&sport->port.lock, flags);
imx_uart_check_flood(sport, imx_uart_readl(sport, USR2));
- spin_unlock(&sport->port.lock);
+ spin_unlock_irqrestore(&sport->port.lock, flags);
if (!(sport->port.ignore_status_mask & URXD_DUMMY_READ)) {
--
2.17.1
[Cc += Vinod Koul, [email protected]]
Hello,
On Wed, Sep 27, 2023 at 06:19:39PM +0000, Chengfeng Ye wrote:
> As &sport->port.lock is acquired under irq context along the following
> call chain from imx_uart_rtsint(), other acquisition of the same lock
> inside process context or softirq context should disable irq avoid double
> lock.
>
> <deadlock #1>
>
> imx_uart_dma_rx_callback()
> --> spin_lock(&sport->port.lock)
> <interrupt>
> --> imx_uart_rtsint()
> --> spin_lock(&sport->port.lock)
>
> This flaw was found by an experimental static analysis tool I am
> developing for irq-related deadlock.
Ah, I understood before that you really experienced that deadlock (or a
lockdep splat). I didn't test anything, but I think the
imx_uart_dma_rx_callback() is called indirectly by
sdma_update_channel_loop() which is called in irq context. I don't know
if this is the case for all dma drivers?!
@Vinod: Maybe you can chime in here: Is a dma callback always called in
irq context?
If yes, this patch isn't needed. Otherwise it might be a good idea to
not use the special knowledge and switch to spin_lock_irqsave() as
suggested.
> To prevent the potential deadlock, the patch uses spin_lock_irqsave()
> on the &sport->port.lock inside imx_uart_dma_rx_callback() to prevent
> the possible deadlock scenario.
>
> Signed-off-by: Chengfeng Ye <[email protected]>
If we agree this patch is a good idea, we can add:
Fixes: 496a4471b7c3 ("serial: imx: work-around for hardware RX flood")
Thanks
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On 28-09-23, 08:07, Uwe Kleine-K?nig wrote:
> [Cc += Vinod Koul, [email protected]]
>
> Hello,
>
> On Wed, Sep 27, 2023 at 06:19:39PM +0000, Chengfeng Ye wrote:
> > As &sport->port.lock is acquired under irq context along the following
> > call chain from imx_uart_rtsint(), other acquisition of the same lock
> > inside process context or softirq context should disable irq avoid double
> > lock.
> >
> > <deadlock #1>
> >
> > imx_uart_dma_rx_callback()
> > --> spin_lock(&sport->port.lock)
> > <interrupt>
> > --> imx_uart_rtsint()
> > --> spin_lock(&sport->port.lock)
> >
> > This flaw was found by an experimental static analysis tool I am
> > developing for irq-related deadlock.
>
> Ah, I understood before that you really experienced that deadlock (or a
> lockdep splat). I didn't test anything, but I think the
> imx_uart_dma_rx_callback() is called indirectly by
> sdma_update_channel_loop() which is called in irq context. I don't know
> if this is the case for all dma drivers?!
>
> @Vinod: Maybe you can chime in here: Is a dma callback always called in
> irq context?
Not in callback but a tasklet context. The DMA irq handler is supposed
to use a tasklet for invoking the callback
> If yes, this patch isn't needed. Otherwise it might be a good idea to
> not use the special knowledge and switch to spin_lock_irqsave() as
> suggested.
>
> > To prevent the potential deadlock, the patch uses spin_lock_irqsave()
> > on the &sport->port.lock inside imx_uart_dma_rx_callback() to prevent
> > the possible deadlock scenario.
> >
> > Signed-off-by: Chengfeng Ye <[email protected]>
>
> If we agree this patch is a good idea, we can add:
>
> Fixes: 496a4471b7c3 ("serial: imx: work-around for hardware RX flood")
>
> Thanks
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-K?nig |
> Industrial Linux Solutions | https://www.pengutronix.de/ |
--
~Vinod
Hello Vinod,
thanks for your quick answer!
On Thu, Sep 28, 2023 at 01:08:15PM +0530, Vinod Koul wrote:
> On 28-09-23, 08:07, Uwe Kleine-K?nig wrote:
> > [Cc += Vinod Koul, [email protected]]
> >
> > Hello,
> >
> > On Wed, Sep 27, 2023 at 06:19:39PM +0000, Chengfeng Ye wrote:
> > > As &sport->port.lock is acquired under irq context along the following
> > > call chain from imx_uart_rtsint(), other acquisition of the same lock
> > > inside process context or softirq context should disable irq avoid double
> > > lock.
> > >
> > > <deadlock #1>
> > >
> > > imx_uart_dma_rx_callback()
> > > --> spin_lock(&sport->port.lock)
> > > <interrupt>
> > > --> imx_uart_rtsint()
> > > --> spin_lock(&sport->port.lock)
> > >
> > > This flaw was found by an experimental static analysis tool I am
> > > developing for irq-related deadlock.
> >
> > Ah, I understood before that you really experienced that deadlock (or a
> > lockdep splat). I didn't test anything, but I think the
> > imx_uart_dma_rx_callback() is called indirectly by
> > sdma_update_channel_loop() which is called in irq context. I don't know
> > if this is the case for all dma drivers?!
> >
> > @Vinod: Maybe you can chime in here: Is a dma callback always called in
> > irq context?
>
> Not in callback but a tasklet context. The DMA irq handler is supposed
> to use a tasklet for invoking the callback
So drivers/dma/imx-sdma.c is bogous as it calls
-> sdma_int_handler()
-> sdma_update_channel_loop()
-> dmaengine_desc_get_callback_invoke()
resulting in imx_uart_dma_rx_callback() (and others) being called in irq
context, right?
In that case:
Acked-by: Uwe Kleine-K?nig <[email protected]>
(for the imx-UART patch that stops assuming imx_uart_dma_rx_callback()
is called with irqs off).
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |