Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
Date:   Thu, 28 Sep 2023 13:08:15 +0530
From:   Vinod Koul <vkoul@kernel.org>
To:     Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= 
        <u.kleine-koenig@pengutronix.de>
Cc:     Chengfeng Ye <dg573847474@gmail.com>, gregkh@linuxfoundation.org,
        jirislaby@kernel.org, shawnguo@kernel.org, s.hauer@pengutronix.de,
        kernel@pengutronix.de, sorganov@gmail.com, festevam@gmail.com,
        ilpo.jarvinen@linux.intel.com,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-serial@vger.kernel.org, dmaengine@vger.kernel.org
Subject: Re: [PATCH RESEND] serial: imx: Fix potential deadlock on
 sport->port.lock
Message-ID: <ZRUtZ/M3Ml6ltc2m@matsya>
References: <20230927181939.60554-1-dg573847474@gmail.com>
 <20230928060749.qzs6qgcesstclqqk@pengutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20230928060749.qzs6qgcesstclqqk@pengutronix.de>
Precedence: bulk

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