Re: [PATCH] dmaengine: imx-sdma: fix deadlock in interrupt handler

[Sascha Hauer]

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@xxxxxxxxxxxxx>
> > Link: https://lore.kernel.org/all/AM0PR08MB308979EC3A8A53AE6E2D3408802CA@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
> > ---
> >  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<------------------------------------