Message-ID: <yiizpt6ow6rhtsqwpvkgvuf6gsnpmps6lra2uuqj7hsslsgpdr@gjvqs3tjznnm>
Date: Fri, 13 Sep 2024 12:25:11 +0300
From: Serge Semin <fancer.lancer@...il.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Viresh Kumar <vireshk@...nel.org>, 
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, Andy Shevchenko <andy@...nel.org>, Vinod Koul <vkoul@...nel.org>, 
	Maciej Sosnowski <maciej.sosnowski@...el.com>, Haavard Skinnemoen <haavard.skinnemoen@...el.com>, 
	Dan Williams <dan.j.williams@...el.com>, Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>, 
	Jiri Slaby <jirislaby@...nel.org>, dmaengine@...r.kernel.org, linux-serial@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] dmaengine: dw: Prevent tx-status calling DMA-desc
 callback

Hi Greg

On Thu, Sep 12, 2024 at 07:27:22AM +0200, Greg Kroah-Hartman wrote:
> On Wed, Sep 11, 2024 at 09:46:09PM +0300, Serge Semin wrote:
> > The dmaengine_tx_status() method implemented in the DW DMAC driver is
> > responsible for not just DMA-transfer status getting, but may also cause
> > the transfer finalization with the Tx-descriptors callback invocation.
> > This makes the simple DMA-transfer status getting being much more complex
> > than it seems with a wider room for possible bugs.
> > 
> > In particular a deadlock has been discovered in the DW 8250 UART device
> > driver interacting with the DW DMA controller channels. Here is the
> > call-trace causing the deadlock:
> > 
> > serial8250_handle_irq()
> >   uart_port_lock_irqsave(port); ----------------------+
> >   handle_rx_dma()                                     |
> >     serial8250_rx_dma_flush()                         |
> >       __dma_rx_complete()                             |
> >         dmaengine_tx_status()                         |
> >           dwc_scan_descriptors()                      |
> >             dwc_complete_all()                        |
> >               dwc_descriptor_complete()               |
> >                 dmaengine_desc_callback_invoke()      |
> >                   cb->callback(cb->callback_param);   |
> >                   ||                                  |
> >                   dma_rx_complete();                  |
> >                     uart_port_lock_irqsave(port); ----+ <- Deadlock!
> > 
> > So in case if the DMA-engine finished working at some point before the
> > serial8250_rx_dma_flush() invocation and the respective tasklet hasn't
> > been executed yet to finalize the DMA transfer, then calling the
> > dmaengine_tx_status() will cause the DMA-descriptors status update and the
> > Tx-descriptor callback invocation.
> > 
> > Generalizing the case up: if the dmaengine_tx_status() method callee and
> > the Tx-descriptor callback refer to the related critical section, then
> > calling dmaengine_tx_status() from the Tx-descriptor callback will
> > inevitably cause a deadlock around the guarding lock as it happens in the
> > Serial 8250 DMA implementation above. (Note the deadlock doesn't happen
> > very often, but can be eventually discovered if the being received data
> > size is greater than the Rx DMA-buffer size defined in the 8250_dma.c
> > driver. In my case reducing the Rx DMA-buffer size increased the deadlock
> > probability.)
> > 
> > Alas there is no obvious way to prevent the deadlock by fixing the
> > 8250-port drivers because the UART-port lock must be held for the entire
> > port IRQ handling procedure. Thus the best way to fix the discovered
> > problem (and prevent similar ones in the drivers using the DW DMAC device
> > channels) is to simplify the DMA-transfer status getter by removing the
> > Tx-descriptors state update from there and making the function to serve
> > just one purpose - calculate the DMA-transfer residue and return the
> > transfer status. The DMA-transfer status update will be performed in the
> > bottom-half procedure only.
> > 
> > Fixes: 3bfb1d20b547 ("dmaengine: Driver for the Synopsys DesignWare DMA controller")
> > Signed-off-by: Serge Semin <fancer.lancer@...il.com>
> > 
> > ---
> > 
> > Changelog RFC:
> > - Instead of just dropping the dwc_scan_descriptors() method invocation
> >   calculate the residue in the Tx-status getter.
> > ---
> >  drivers/dma/dw/core.c | 90 ++++++++++++++++++++++++-------------------
> >  1 file changed, 50 insertions(+), 40 deletions(-)
> > 
> > diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
> > index dd75f97a33b3..af1871646eb9 100644
> > --- a/drivers/dma/dw/core.c
> > +++ b/drivers/dma/dw/core.c
> > @@ -39,6 +39,8 @@
> >  	BIT(DMA_SLAVE_BUSWIDTH_2_BYTES)		| \
> >  	BIT(DMA_SLAVE_BUSWIDTH_4_BYTES)
> >  
> > +static u32 dwc_get_hard_llp_desc_residue(struct dw_dma_chan *dwc, struct dw_desc *desc);
> > +
> >  /*----------------------------------------------------------------------*/
> >  
> >  static struct device *chan2dev(struct dma_chan *chan)
> > @@ -297,14 +299,12 @@ static inline u32 dwc_get_sent(struct dw_dma_chan *dwc)
> >  
> >  static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
> >  {
> > -	dma_addr_t llp;
> >  	struct dw_desc *desc, *_desc;
> >  	struct dw_desc *child;
> >  	u32 status_xfer;
> >  	unsigned long flags;
> >  
> >  	spin_lock_irqsave(&dwc->lock, flags);
> > -	llp = channel_readl(dwc, LLP);
> >  	status_xfer = dma_readl(dw, RAW.XFER);
> >  
> >  	if (status_xfer & dwc->mask) {
> > @@ -358,41 +358,16 @@ static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
> >  		return;
> >  	}
> >  
> > -	dev_vdbg(chan2dev(&dwc->chan), "%s: llp=%pad\n", __func__, &llp);
> > +	dev_vdbg(chan2dev(&dwc->chan), "%s: hard LLP mode\n", __func__);
> >  
> >  	list_for_each_entry_safe(desc, _desc, &dwc->active_list, desc_node) {
> > -		/* Initial residue value */
> > -		desc->residue = desc->total_len;
> > -
> > -		/* Check first descriptors addr */
> > -		if (desc->txd.phys == DWC_LLP_LOC(llp)) {
> > -			spin_unlock_irqrestore(&dwc->lock, flags);
> > -			return;
> > -		}
> > -
> > -		/* Check first descriptors llp */
> > -		if (lli_read(desc, llp) == llp) {
> > -			/* This one is currently in progress */
> > -			desc->residue -= dwc_get_sent(dwc);
> > +		desc->residue = dwc_get_hard_llp_desc_residue(dwc, desc);
> > +		if (desc->residue) {
> >  			spin_unlock_irqrestore(&dwc->lock, flags);
> >  			return;
> >  		}
> >  
> > -		desc->residue -= desc->len;
> > -		list_for_each_entry(child, &desc->tx_list, desc_node) {
> > -			if (lli_read(child, llp) == llp) {
> > -				/* Currently in progress */
> > -				desc->residue -= dwc_get_sent(dwc);
> > -				spin_unlock_irqrestore(&dwc->lock, flags);
> > -				return;
> > -			}
> > -			desc->residue -= child->len;
> > -		}
> > -
> > -		/*
> > -		 * No descriptors so far seem to be in progress, i.e.
> > -		 * this one must be done.
> > -		 */
> > +		/* No data left to be send. Finalize the transfer then */
> >  		spin_unlock_irqrestore(&dwc->lock, flags);
> >  		dwc_descriptor_complete(dwc, desc, true);
> >  		spin_lock_irqsave(&dwc->lock, flags);
> > @@ -976,6 +951,45 @@ static struct dw_desc *dwc_find_desc(struct dw_dma_chan *dwc, dma_cookie_t c)
> >  	return NULL;
> >  }
> >  
> > +static u32 dwc_get_soft_llp_desc_residue(struct dw_dma_chan *dwc, struct dw_desc *desc)
> > +{
> > +	u32 residue = desc->residue;
> > +
> > +	if (residue)
> > +		residue -= dwc_get_sent(dwc);
> > +
> > +	return residue;
> > +}
> > +
> > +static u32 dwc_get_hard_llp_desc_residue(struct dw_dma_chan *dwc, struct dw_desc *desc)
> > +{
> > +	u32 residue = desc->total_len;
> > +	struct dw_desc *child;
> > +	dma_addr_t llp;
> > +
> > +	llp = channel_readl(dwc, LLP);
> > +
> > +	/* Check first descriptor for been pending to be fetched by DMAC */
> > +	if (desc->txd.phys == DWC_LLP_LOC(llp))
> > +		return residue;
> > +
> > +	/* Check first descriptor LLP to see if it's currently in-progress */
> > +	if (lli_read(desc, llp) == llp)
> > +		return residue - dwc_get_sent(dwc);
> > +
> > +	/* Check subordinate LLPs to find the currently in-progress desc */
> > +	residue -= desc->len;
> > +	list_for_each_entry(child, &desc->tx_list, desc_node) {
> > +		if (lli_read(child, llp) == llp)
> > +			return residue - dwc_get_sent(dwc);
> > +
> > +		residue -= child->len;
> > +	}
> > +
> > +	/* Shall return zero if no in-progress desc found */
> > +	return residue;
> > +}
> > +
> >  static u32 dwc_get_residue_and_status(struct dw_dma_chan *dwc, dma_cookie_t cookie,
> >  				      enum dma_status *status)
> >  {
> > @@ -988,9 +1002,11 @@ static u32 dwc_get_residue_and_status(struct dw_dma_chan *dwc, dma_cookie_t cook
> >  	desc = dwc_find_desc(dwc, cookie);
> >  	if (desc) {
> >  		if (desc == dwc_first_active(dwc)) {
> > -			residue = desc->residue;
> > -			if (test_bit(DW_DMA_IS_SOFT_LLP, &dwc->flags) && residue)
> > -				residue -= dwc_get_sent(dwc);
> > +			if (test_bit(DW_DMA_IS_SOFT_LLP, &dwc->flags))
> > +				residue = dwc_get_soft_llp_desc_residue(dwc, desc);
> > +			else
> > +				residue = dwc_get_hard_llp_desc_residue(dwc, desc);
> > +
> >  			if (test_bit(DW_DMA_IS_PAUSED, &dwc->flags))
> >  				*status = DMA_PAUSED;
> >  		} else {
> > @@ -1012,12 +1028,6 @@ dwc_tx_status(struct dma_chan *chan,
> >  	struct dw_dma_chan	*dwc = to_dw_dma_chan(chan);
> >  	enum dma_status		ret;
> >  
> > -	ret = dma_cookie_status(chan, cookie, txstate);
> > -	if (ret == DMA_COMPLETE)
> > -		return ret;
> > -
> > -	dwc_scan_descriptors(to_dw_dma(chan->device), dwc);
> > -
> >  	ret = dma_cookie_status(chan, cookie, txstate);
> >  	if (ret == DMA_COMPLETE)
> >  		return ret;
> > -- 
> > 2.43.0
> > 
> > 
> 
> Hi,
> 
> This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
> a patch that has triggered this response.  He used to manually respond
> to these common problems, but in order to save his sanity (he kept
> writing the same thing over and over, yet to different people), I was
> created.  Hopefully you will not take offence and will fix the problem
> in your patch and resubmit it so that it can be accepted into the Linux
> kernel tree.
> 
> You are receiving this message because of the following common error(s)
> as indicated below:
> 
> - You have marked a patch with a "Fixes:" tag for a commit that is in an
>   older released kernel, yet you do not have a cc: stable line in the
>   signed-off-by area at all, which means that the patch will not be
>   applied to any older kernel releases.  To properly fix this, please
>   follow the documented rules in the
>   Documentation/process/stable-kernel-rules.rst file for how to resolve
>   this.
> 
> If you wish to discuss this problem further, or you have questions about
> how to resolve this issue, please feel free to respond to this email and
> Greg will reply once he has dug out from the pending patches received
> from other developers.

Got it. I'll wait for the maintainers to react and discuss the
problems the series fixes. Then, if required, I'll re-submit the patch
set with the stable list Cc'ed.

-Serge(y)

> 
> thanks,
> 
> greg k-h's patch email bot

Powered by blists - more mailing lists