dev_warn internally acquires the lock that is already held when
sdma_update_channel_loop is called. Therefore it is acquired twice and
this is detected as a deadlock. Temporarily release the lock while
logging to avoid this.

Signed-off-by: Tim van der Staaij <tim.vanderstaaij@zigngroup.com>
Link: https://lore.kernel.org/all/AM0PR08MB308979EC3A8A53AE6E2D3408802CA@AM0PR08MB3089.eurprd08.prod.outlook.com/
---
 drivers/dma/imx-sdma.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index 51012bd39900..3a7cd783a567 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -904,7 +904,10 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac)
 	 * owned buffer is available (i.e. BD_DONE was set too late).
 	 */
 	if (sdmac->desc && !is_sdma_channel_enabled(sdmac->sdma, sdmac->channel)) {
+		spin_unlock(&sdmac->vc.lock);
 		dev_warn(sdmac->sdma->dev, "restart cyclic channel %d\n", sdmac->channel);
+		spin_lock(&sdmac->vc.lock);
+
 		sdma_enable_channel(sdmac->sdma, sdmac->channel);
 	}
 }
-- 
2.41.0

Hello,

On Thu, Sep 21, 2023 at 09:57:11AM +0000, Tim van der Staaij | Zign wrote:
> dev_warn internally acquires the lock that is already held when
> sdma_update_channel_loop is called. Therefore it is acquired twice and
> this is detected as a deadlock. Temporarily release the lock while
> logging to avoid this.
> 
> Signed-off-by: Tim van der Staaij <tim.vanderstaaij@zigngroup.com>
> Link: https://lore.kernel.org/all/AM0PR08MB308979EC3A8A53AE6E2D3408802CA@AM0PR08MB3089.eurprd08.prod.outlook.com/
> ---
>  drivers/dma/imx-sdma.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index 51012bd39900..3a7cd783a567 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -904,7 +904,10 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac)
>  	 * owned buffer is available (i.e. BD_DONE was set too late).
>  	 */
>  	if (sdmac->desc && !is_sdma_channel_enabled(sdmac->sdma, sdmac->channel)) {
> +		spin_unlock(&sdmac->vc.lock);
>  		dev_warn(sdmac->sdma->dev, "restart cyclic channel %d\n", sdmac->channel);
> +		spin_lock(&sdmac->vc.lock);
> +

I don't know if Sascha's patch helps, but this patch looks definitively
wrong. If this was the right approach (and I doubt it is) this
would definitively lack an explaining code comment.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

On Mon, Sep 25, 2023 at 11:20:04AM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Thu, Sep 21, 2023 at 09:57:11AM +0000, Tim van der Staaij | Zign wrote:
> > dev_warn internally acquires the lock that is already held when
> > sdma_update_channel_loop is called. Therefore it is acquired twice and
> > this is detected as a deadlock. Temporarily release the lock while
> > logging to avoid this.
> > 
> > Signed-off-by: Tim van der Staaij <tim.vanderstaaij@zigngroup.com>
> > Link: https://lore.kernel.org/all/AM0PR08MB308979EC3A8A53AE6E2D3408802CA@AM0PR08MB3089.eurprd08.prod.outlook.com/
> > ---
> >  drivers/dma/imx-sdma.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> > index 51012bd39900..3a7cd783a567 100644
> > --- a/drivers/dma/imx-sdma.c
> > +++ b/drivers/dma/imx-sdma.c
> > @@ -904,7 +904,10 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac)
> >  	 * owned buffer is available (i.e. BD_DONE was set too late).
> >  	 */
> >  	if (sdmac->desc && !is_sdma_channel_enabled(sdmac->sdma, sdmac->channel)) {
> > +		spin_unlock(&sdmac->vc.lock);
> >  		dev_warn(sdmac->sdma->dev, "restart cyclic channel %d\n", sdmac->channel);
> > +		spin_lock(&sdmac->vc.lock);
> > +
> 
> I don't know if Sascha's patch helps

I do ;)

I inserted a dev_info() into the imx-sdma driver and got said circular
locking warning. With my patch applied it's gone. Nevertheless I would
wait for Tims feedback and resend it with some more people on Cc. I
never used lockdep_set_subclass() and I am not sure if it's legal to
put it into the UART startup function like I did.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Hi Tim,

On Thu, Sep 21, 2023 at 09:57:11AM +0000, Tim van der Staaij | Zign wrote:
> dev_warn internally acquires the lock that is already held when
> sdma_update_channel_loop is called. Therefore it is acquired twice and
> this is detected as a deadlock. Temporarily release the lock while
> logging to avoid this.
> 
> Signed-off-by: Tim van der Staaij <tim.vanderstaaij@zigngroup.com>
> Link: https://lore.kernel.org/all/AM0PR08MB308979EC3A8A53AE6E2D3408802CA@AM0PR08MB3089.eurprd08.prod.outlook.com/
> ---
>  drivers/dma/imx-sdma.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index 51012bd39900..3a7cd783a567 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -904,7 +904,10 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac)
>  	 * owned buffer is available (i.e. BD_DONE was set too late).
>  	 */
>  	if (sdmac->desc && !is_sdma_channel_enabled(sdmac->sdma, sdmac->channel)) {
> +		spin_unlock(&sdmac->vc.lock);
>  		dev_warn(sdmac->sdma->dev, "restart cyclic channel %d\n", sdmac->channel);
> +		spin_lock(&sdmac->vc.lock);

This is strange. Why and how does dev_warn() call back into the SDMA
driver?

We shouldn't merge this without having a clue what exactly goes wrong
here. Please provide the corresponding lockdep output.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

On Fri, Sep 22, 2023 at 11:50:32AM +0200, Sascha Hauer wrote:
> Hi Tim,
> 
> On Thu, Sep 21, 2023 at 09:57:11AM +0000, Tim van der Staaij | Zign wrote:
> > dev_warn internally acquires the lock that is already held when
> > sdma_update_channel_loop is called. Therefore it is acquired twice and
> > this is detected as a deadlock. Temporarily release the lock while
> > logging to avoid this.
> > 
> > Signed-off-by: Tim van der Staaij <tim.vanderstaaij@zigngroup.com>
> > Link: https://lore.kernel.org/all/AM0PR08MB308979EC3A8A53AE6E2D3408802CA@AM0PR08MB3089.eurprd08.prod.outlook.com/
> > ---
> >  drivers/dma/imx-sdma.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> > index 51012bd39900..3a7cd783a567 100644
> > --- a/drivers/dma/imx-sdma.c
> > +++ b/drivers/dma/imx-sdma.c
> > @@ -904,7 +904,10 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac)
> >  	 * owned buffer is available (i.e. BD_DONE was set too late).
> >  	 */
> >  	if (sdmac->desc && !is_sdma_channel_enabled(sdmac->sdma, sdmac->channel)) {
> > +		spin_unlock(&sdmac->vc.lock);
> >  		dev_warn(sdmac->sdma->dev, "restart cyclic channel %d\n", sdmac->channel);
> > +		spin_lock(&sdmac->vc.lock);
> 
> This is strange. Why and how does dev_warn() call back into the SDMA
> driver?
> 
> We shouldn't merge this without having a clue what exactly goes wrong
> here. Please provide the corresponding lockdep output.

I have overlooked that you actually did provide the lockdep output in a
link.

I think this is a false positive. The i.MX UART driver makes sure that
the console UART never uses DMA, so it shouldn't happen that the DMA
driver issuing console messages calls back into the DMA driver.

Could you give the following patch a test?

Sascha


-------------------------------8<------------------------------------

From 5ac9902683710c300a64a731bcda6b3b089b2706 Mon Sep 17 00:00:00 2001
From: Sascha Hauer <s.hauer@pengutronix.de>
Date: Mon, 25 Sep 2023 10:39:44 +0200
Subject: [PATCH] serial: imx: Put DMA enabled UART in separate lock subclass

The i.MX UART driver never uses DMA on UARTs providing the console.
Put the UART port lock in a separate subclass to avoid lockdep
complaining about possible deadlocks when the DMA driver issues
console messages under its own spinlock held.

Reported-by: Tim van der Staaij <Tim.vanderstaaij@zigngroup.com>
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/tty/serial/imx.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 7341d060f85cb..c30113cf5db85 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1458,8 +1458,10 @@ static int imx_uart_startup(struct uart_port *port)
 	imx_uart_writel(sport, ucr4 & ~UCR4_DREN, UCR4);
 
 	/* Can we enable the DMA support? */
-	if (!uart_console(port) && imx_uart_dma_init(sport) == 0)
+	if (!uart_console(port) && imx_uart_dma_init(sport) == 0) {
+		lockdep_set_subclass(&port->lock, 1);
 		dma_is_inited = 1;
+	}
 
 	spin_lock_irqsave(&sport->port.lock, flags);
 
-- 
2.39.2

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Hi Sascha,

> I think this is a false positive. The i.MX UART driver makes sure that
the console UART never uses DMA, so it shouldn't happen that the DMA
driver issuing console messages calls back into the DMA driver.
>
> Could you give the following patch a test?

Thank you for looking into this. I thought I had an idea of what was going on
but it seems that was based on some wrong assumptions (I'm mostly a Linux user
and not familiar with kernel code yet).

I tested with your patch and it does indeed fix the issue on my machine. Note
that I have been testing this in a similar way as you did. The log message
which triggers this issue in practice is a rare occurrence on my system
because of its condition, so I added a dev_warn_once() to
sdma_update_channel_loop() just outside of the conditional, which fires as
soon as some data is received through DMA. This consistently reproduces the
lockdep warning without your patch, so I'm confident that the patch works.

> I inserted a dev_info() into the imx-sdma driver and got said circular
locking warning. With my patch applied it's gone. Nevertheless I would
wait for Tims feedback and resend it with some more people on Cc. I
never used lockdep_set_subclass() and I am not sure if it's legal to
put it into the UART startup function like I did.

Sounds good, could you submit the patch and keep me in cc?

Thanks,
Tim

Hi Tim,

On Thu, Sep 21, 2023 at 6:57 AM Tim van der Staaij | Zign
<Tim.vanderstaaij@zigngroup.com> wrote:
>
> dev_warn internally acquires the lock that is already held when
> sdma_update_channel_loop is called. Therefore it is acquired twice and
> this is detected as a deadlock. Temporarily release the lock while
> logging to avoid this.
>
> Signed-off-by: Tim van der Staaij <tim.vanderstaaij@zigngroup.com>
> Link: https://lore.kernel.org/all/AM0PR08MB308979EC3A8A53AE6E2D3408802CA@AM0PR08MB3089.eurprd08.prod.outlook.com/

checkpatch gives a warning on this patch:

WARNING: From:/Signed-off-by: email name mismatch: 'From: "Tim van der
Staaij | Zign" <Tim.vanderstaaij@zigngroup.com>' != 'Signed-off-by:
Tim van der Staaij <tim.vanderstaaij@zigngroup.com>'

Should it contain a Fixes tag so that it can be backported to older
stable-kernels?

Reviewed-by: Fabio Estevam <festevam@gmail.com>