Re: [PATCH RESEND] serial: imx: Fix potential deadlock on sport->port.lock

Re: [PATCH RESEND] serial: imx: Fix potential deadlock on sport->port.lock

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1726 bytes --]

[Cc += Vinod Koul, dmaengine@vger.kernel.org]

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 <dg573847474@gmail.com>

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/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH RESEND] serial: imx: Fix potential deadlock on sport->port.lock

[Vinod Koul]

On 28-09-23, 08:07, Uwe Kleine-König wrote:
> [Cc += Vinod Koul, dmaengine@vger.kernel.org]
> 
> 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 <dg573847474@gmail.com>
> 
> 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

Re: [PATCH RESEND] serial: imx: Fix potential deadlock on sport->port.lock

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2037 bytes --]

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, dmaengine@vger.kernel.org]
> > 
> > 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 <u.kleine-koenig@pengutronix.de>

(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/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]