Re: [PATCH 5/5] dmaengine: xilinx_dma: Recover a withheld AXI DMA channel

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] dmaengine: xilinx_dma: chan->quiesced not cleared on failed reset
- [Medium] dmaengine: xilinx_dma: race in xilinx_dma_tx_submit() across channel recovery
--

commit 0230d2d57b7d259e756e6f7875674a232d1f812a
Author: Jeff C. Jensen <kernel@nullptr.com>

dmaengine: xilinx_dma: Recover a withheld AXI DMA channel

This commit adds two-step recovery for a withheld single-channel AXI DMA
controller by resetting the engine in xilinx_dma_terminate_all() and
reclaiming the quarantined descriptors in xilinx_dma_synchronize().

> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
> index 0316c6e7881f9..c72c0fc985c5c 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c

[ ... ]

> @@ -2917,12 +2920,109 @@ xilinx_mcdma_prep_slave_sg(struct dma_chan *dchan, struct scatterlist *sgl,
>  	return NULL;
>  }
>  

[ ... ]

> +static int xilinx_dma_reset_withheld_channel(struct xilinx_dma_chan *chan)
> +{
> +	unsigned long flags;
> +	int err;
> +
> +	if (!xilinx_dma_reset_is_isolated(chan)) {
> +		dev_err(chan->dev,
> +			"channel %p stays withheld: a reset would disturb the other channel\n",
> +			chan);
> +		return -EBUSY;
> +	}
> +
> +	err = chan->stop_transfer(chan);
> +	if (err)
> +		dev_dbg(chan->dev, "channel %p did not halt before its reset\n", chan);
> +
> +	err = xilinx_dma_chan_reset(chan);
> +	if (err) {
> +		dev_err(chan->dev, "channel %p stays withheld: reset failed again\n", chan);
> +		return err;
> +	}
> +
> +	spin_lock_irqsave(&chan->lock, flags);
> +	chan->quiesced = true;
> +	spin_unlock_irqrestore(&chan->lock, flags);
> +
> +	return 0;
> +}

[Severity: High]
What happens if terminate is called twice on a withheld channel before
dmaengine_synchronize() runs, and the second reset times out?

Specifically, in drivers/dma/xilinx/xilinx_dma.c:

1. A single-channel AXI DMA controller fails a reset and is withheld via
   xilinx_dma_quarantine_channel() with chan->quarantined = true and
   chan->err = true.

2. The client calls dmaengine_terminate_async(), which runs
   xilinx_dma_terminate_all() -> xilinx_dma_reset_withheld_channel().
   When xilinx_dma_chan_reset() succeeds, xilinx_dma_reset() clears
   chan->err = false and sets chan->idle = true, and
   xilinx_dma_reset_withheld_channel() sets chan->quiesced = true.
   Because dmaengine_synchronize() has not run yet, chan->quarantined
   remains true.

3. Before dmaengine_synchronize() runs, terminate is called again (for
   example, via dmaengine_terminate_sync() or a second
   dmaengine_terminate_async() during teardown).
   xilinx_dma_terminate_all() sees xilinx_dma_chan_quarantined(chan) is
   still true and invokes xilinx_dma_reset_withheld_channel() a second
   time.

4. xilinx_dma_reset_withheld_channel() unconditionally starts another
   hardware soft reset via xilinx_dma_chan_reset(chan) (writing
   XILINX_DMA_DMACR_RESET to XILINX_DMA_REG_DMACR) without clearing
   chan->quiesced. If this second reset times out,
   xilinx_dma_reset_withheld_channel() returns -ETIMEDOUT, but leaves
   chan->quiesced = true and chan->err = false from the earlier reset
   while XILINX_DMA_DMACR_RESET is still incomplete in hardware and
   XILINX_DMA_DMAXR_ALL_IRQ_MASK has not been re-enabled.

Similarly, xilinx_dma_quarantine_channel() does not clear
chan->quiesced = false when withholding a channel:

drivers/dma/xilinx/xilinx_dma.c:xilinx_dma_quarantine_channel() {
	spin_lock_irqsave(&chan->lock, flags);
	chan->quarantined = true;
	chan->err = true;
	/* a completion pass mid-callback stops at its current descriptor */
	chan->terminating = true;
	...
}

When dmaengine_synchronize() subsequently runs (either directly or
unconditionally via dma_release_channel() -> dma_chan_put()),
xilinx_dma_synchronize() -> xilinx_dma_recover_channel() sees both
chan->quarantined and chan->quiesced set to true. Wouldn't that cause
xilinx_dma_recover_channel() to free chan->quarantine_list, release the
segment reservations in chan->seg_used, and allow new transfers or
xilinx_dma_free_chan_resources() to free chan->seg_v while the second
hardware reset has not completed?

> +
> +/**
> + * xilinx_dma_recover_channel - Return a withheld channel to service
> + * @chan: Driver specific DMA channel
> + *
> + * Must run after the completion tasklet has been killed, so that no completion
> + * pass holds a withheld descriptor. Frees the withheld descriptors, which
> + * restores their segment links and releases their reservations, and returns
> + * the channel to idle with no error, so it takes work again. Does nothing
> + * unless the engine was reset after the channel was withheld.
> + */
> +static void xilinx_dma_recover_channel(struct xilinx_dma_chan *chan)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&chan->lock, flags);
> +	if (!chan->quarantined || !chan->quiesced) {
> +		spin_unlock_irqrestore(&chan->lock, flags);
> +		return;
> +	}
> +	xilinx_dma_free_desc_list(chan, &chan->quarantine_list);
> +	chan->quarantined = false;
> +	chan->quiesced = false;
> +	chan->cyclic = false;
> +	chan->err = false;
> +	chan->idle = true;
> +	chan->desc_pendingcount = 0;
> +	chan->desc_submitcount = 0;
> +	spin_unlock_irqrestore(&chan->lock, flags);
> +
> +	dev_info(chan->dev, "channel %p recovered\n", chan);
> +}

[Severity: Medium]
Can clearing chan->quarantined = false in xilinx_dma_recover_channel()
race with an in-flight xilinx_dma_tx_submit() that dropped chan->lock to
run xilinx_dma_chan_reset()?

In drivers/dma/xilinx/xilinx_dma.c, xilinx_dma_tx_submit() drops
chan->lock around xilinx_dma_chan_reset() and
xilinx_dma_quarantine_controller(), then reacquires chan->lock to check
chan->quarantined:

drivers/dma/xilinx/xilinx_dma.c:xilinx_dma_tx_submit() {
	...
	if (chan->err) {
		spin_unlock_irqrestore(&chan->lock, flags);
		err = xilinx_dma_chan_reset(chan);
		if (err < 0) {
			if (chan->xdev->dma_config->dmatype != XDMA_TYPE_AXIDMA)
				return err;
			xilinx_dma_quarantine_controller(chan);
			cookie = err;
		}
		spin_lock_irqsave(&chan->lock, flags);
		if (chan->quarantined)
			goto withhold;
	}

	cookie = dma_cookie_assign(tx);

	/* Put this transaction onto the tail of the pending queue */
	append_desc_queue(chan, desc);
	...
}

If Thread A's xilinx_dma_chan_reset() fails in xilinx_dma_tx_submit() and
calls xilinx_dma_quarantine_controller(), Thread A's descriptor desc has
not yet been added to chan->quarantine_list while chan->lock is still
dropped.

If Thread B concurrently runs dmaengine_terminate_sync() ->
xilinx_dma_terminate_all() -> xilinx_dma_reset_withheld_channel() and
xilinx_dma_synchronize() -> xilinx_dma_recover_channel() before Thread A
reacquires chan->lock, xilinx_dma_recover_channel() frees
chan->quarantine_list and sets chan->quarantined = false.

When Thread A then reacquires chan->lock in xilinx_dma_tx_submit(), it
sees chan->quarantined is false, skips the withhold path, overwrites
cookie = err with dma_cookie_assign(tx), and appends desc to
chan->pending_list after dmaengine_terminate_sync() has already completed.
If another descriptor was prepared after recovery, its segments were
reserved at chan->seg_next (after desc's segments), so appending desc
behind it queues AXI DMA ring segments out of ring order; and if desc is
cyclic, the recovery reset also cleared XILINX_DMA_CR_CYCLIC_BD_EN_MASK in
XILINX_DMA_REG_DMACR that xilinx_dma_prep_dma_cyclic() wrote before
submission.

Should xilinx_dma_tx_submit() check whether its own reset attempt failed
rather than relying on chan->quarantined remaining true across the lock
drop?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-xilinx-dma-work-v1-0-20faea2e7a80@nullptr.com?part=5