Re: [PATCH v3 02/11] dmaengine: switchtec-dma: fix resource leak in alloc_chan_resources

[Frank Li <Frank.li@oss.nxp.com>]

On Mon, Jul 27, 2026 at 12:15:17PM -0600, Logan Gunthorpe wrote:
> switchtec_dma_alloc_chan_resources() returns directly on any later
> failure, without ever freeing the descriptor rings and coherent DMA
> memory it just allocated. The dmaengine core does not call
> device_free_chan_resources() when device_alloc_chan_resources() fails,
> so the driver has to unwind its own partial state.
>
> The device-removed check also runs after ring_active and
> comp_ring_active have already been set true, so a failure there left
> the channel marked active despite alloc_chan_resources() reporting
> failure.
>
> Add an error-unwind path that disables the channel and frees the
> descriptor rings on every failure after allocation. ring_active and
> comp_ring_active are cleared under the same locks
> switchtec_dma_free_chan_resources() already uses, since the completion
> tasklet checks comp_ring_active under complete_lock before touching
> the completion ring, and a stale IRQ can still be in flight when this
> unwind path runs.
>
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Link: https://lore.kernel.org/dmaengine/20260707165555.350951F000E9@smtp.kernel.org
> Fixes: 30eba9df76ad ("dmaengine: switchtec-dma: Implement hardware initialization and cleanup")
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/dma/switchtec_dma.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> index a4a7d66d042d..f77da31aeb65 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c
> @@ -988,15 +988,15 @@ static int switchtec_dma_alloc_chan_resources(struct dma_chan *chan)
>
>  	rc = enable_channel(swdma_chan);
>  	if (rc)
> -		return rc;
> +		goto err_free_desc;
>
>  	rc = reset_channel(swdma_chan);
>  	if (rc)
> -		return rc;
> +		goto err_disable_channel;
>
>  	rc = unhalt_channel(swdma_chan);
>  	if (rc)
> -		return rc;
> +		goto err_disable_channel;
>
>  	swdma_chan->ring_active = true;
>  	swdma_chan->comp_ring_active = true;
> @@ -1007,7 +1007,8 @@ static int switchtec_dma_alloc_chan_resources(struct dma_chan *chan)
>  	rcu_read_lock();
>  	if (!rcu_dereference(swdma_dev->pdev)) {
>  		rcu_read_unlock();
> -		return -ENODEV;
> +		rc = -ENODEV;
> +		goto err_ring_inactive;
>  	}
>
>  	perf_cfg = readl(&swdma_chan->mmio_chan_fw->perf_cfg);
> @@ -1029,6 +1030,20 @@ static int switchtec_dma_alloc_chan_resources(struct dma_chan *chan)
>  		FIELD_GET(PERF_MRRS_MASK, perf_cfg));
>
>  	return SWITCHTEC_DMA_SQ_SIZE;
> +
> +err_ring_inactive:
> +	spin_lock_bh(&swdma_chan->submit_lock);
> +	swdma_chan->ring_active = false;
> +	spin_unlock_bh(&swdma_chan->submit_lock);
> +
> +	spin_lock_bh(&swdma_chan->complete_lock);
> +	swdma_chan->comp_ring_active = false;
> +	spin_unlock_bh(&swdma_chan->complete_lock);
> +err_disable_channel:
> +	disable_channel(swdma_chan);
> +err_free_desc:
> +	switchtec_dma_free_desc(swdma_chan);
> +	return rc;
>  }
>
>  static void switchtec_dma_free_chan_resources(struct dma_chan *chan)
> --
> 2.47.3
>