Re: [PATCH v4 03/12] dmaengine: switchtec-dma: always clear DMA base registers on chan_stop()

[Frank Li]

On Tue, Jul 28, 2026 at 11:15:14AM -0600, Logan Gunthorpe wrote:
> switchtec_dma_chan_stop() returned early if halt_channel() timed out,
> skipping the writes that clear sq_base/cq_base on the channel, and
> gave its caller no way to tell the halt hadn't been confirmed.
> switchtec_dma_free_chan_resources() unconditionally frees the
> descriptor rings right after calling this function, so if the
> hardware failed to halt, it could keep writing into memory that had
> already been freed.
>
> Attempt the register clear regardless of whether the halt was successful
> and have switchtec_dma_chan_stop() return the halt result so callers can
> tell when it wasn't confirmed. switchtec_dma_free_chan_resources() now
> skips freeing the descriptor rings (leaking them instead) in case the
> hardware continues to write into that memory.
>
> But all this is hardening that is pretty unlikely to be hit in the real
> world.
>
> Reported-by: Sashiko <sashiko-bot@xxxxxxxxxx>
> Link: https://lore.kernel.org/dmaengine/20260721162531.BA01A1F01560@xxxxxxxxxxxxxxx
> Signed-off-by: Logan Gunthorpe <logang@xxxxxxxxxxxx>
> ---

This common problem when timeout happen. Before we have good method to
handle it, I suggest leave it as it for now.

I don't want to introduce new problem by fix an unlikely happen problem.

Frank

>  drivers/dma/switchtec_dma.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> index f77da31aeb65..cbbc9ad08247 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c
> @@ -543,26 +543,33 @@ switchtec_dma_abort_desc(struct switchtec_dma_chan *swdma_chan, int force)
>  	spin_unlock_bh(&swdma_chan->complete_lock);
>  }
>
> -static void switchtec_dma_chan_stop(struct switchtec_dma_chan *swdma_chan)
> +static int switchtec_dma_chan_stop(struct switchtec_dma_chan *swdma_chan)
>  {
> +	struct pci_dev *pdev;
>  	int rc;
>
>  	rc = halt_channel(swdma_chan);
> -	if (rc)
> -		return;
>
>  	rcu_read_lock();
> -	if (!rcu_dereference(swdma_chan->swdma_dev->pdev)) {
> +	pdev = rcu_dereference(swdma_chan->swdma_dev->pdev);
> +	if (!pdev) {
>  		rcu_read_unlock();
> -		return;
> +		return rc;
>  	}
>
> +	if (rc)
> +		pci_err(pdev,
> +			"Channel %d halt timed out, clearing DMA base registers anyway\n",
> +			swdma_chan->index);
> +
>  	writel(0, &swdma_chan->mmio_chan_fw->sq_base_lo);
>  	writel(0, &swdma_chan->mmio_chan_fw->sq_base_hi);
>  	writel(0, &swdma_chan->mmio_chan_fw->cq_base_lo);
>  	writel(0, &swdma_chan->mmio_chan_fw->cq_base_hi);
>
>  	rcu_read_unlock();
> +
> +	return rc;
>  }
>
>  static int switchtec_dma_terminate_all(struct dma_chan *chan)
> @@ -1050,6 +1057,7 @@ static void switchtec_dma_free_chan_resources(struct dma_chan *chan)
>  {
>  	struct switchtec_dma_chan *swdma_chan =
>  		container_of(chan, struct switchtec_dma_chan, dma_chan);
> +	int rc;
>
>  	spin_lock_bh(&swdma_chan->submit_lock);
>  	swdma_chan->ring_active = false;
> @@ -1059,9 +1067,10 @@ static void switchtec_dma_free_chan_resources(struct dma_chan *chan)
>  	swdma_chan->comp_ring_active = false;
>  	spin_unlock_bh(&swdma_chan->complete_lock);
>
> -	switchtec_dma_chan_stop(swdma_chan);
> +	rc = switchtec_dma_chan_stop(swdma_chan);
>  	switchtec_dma_abort_desc(swdma_chan, 0);
> -	switchtec_dma_free_desc(swdma_chan);
> +	if (!rc)
> +		switchtec_dma_free_desc(swdma_chan);
>
>  	disable_channel(swdma_chan);
>  }
> --
> 2.47.3
>