| Message ID | 20230927181939.60554-1-dg573847474@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND] serial: imx: Fix potential deadlock on sport->port.lock | expand |
[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
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/ |
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
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)) {
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 <dg573847474@gmail.com> --- drivers/tty/serial/imx.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)