Re: [PATCH 1/2] dmaengine: dw: Prevent tx-status calling DMA-desc callback

From: Serge Semin
Date: Fri Sep 13 2024 - 05:25:50 EST

Next message: Andy Shevchenko: "Re: [PATCH v1 1/1] tee: amdtee: Use %pUl printk() format specifier to print GUIDs"
Previous message: Mao Jinlong: "[PATCH v4 1/2] dt-bindings: arm: coresight: Update the pattern of ete node name"
In reply to: Greg Kroah-Hartman: "Re: [PATCH 1/2] dmaengine: dw: Prevent tx-status calling DMA-desc callback"
Next in thread: Serge Semin: "[PATCH 2/2] dmaengine: dw: Fix XFER bit set, but channel not idle error"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxx>
> >
> > ---
> >
> > 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

Next message: Andy Shevchenko: "Re: [PATCH v1 1/1] tee: amdtee: Use %pUl printk() format specifier to print GUIDs"
Previous message: Mao Jinlong: "[PATCH v4 1/2] dt-bindings: arm: coresight: Update the pattern of ete node name"
In reply to: Greg Kroah-Hartman: "Re: [PATCH 1/2] dmaengine: dw: Prevent tx-status calling DMA-desc callback"
Next in thread: Serge Semin: "[PATCH 2/2] dmaengine: dw: Fix XFER bit set, but channel not idle error"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]