Re: [PATCH V2 1/8] dmaengine: zynqmp_dma: Fix PM rollback on sw_desc_pool alloc failure

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] The patch fixes a runtime PM reference leak when `sw_desc_pool` allocation fails, but misses an identical PM reference leak and a memory leak when `desc_pool_v` allocation fails.
- [High] The probe function leaks the PM runtime usage counter on error paths if PM runtime is enabled.
- [High] Dropping the spinlock inside `list_for_each_entry_safe` in `zynqmp_dma_chan_desc_cleanup` exposes the list to concurrent corruption.
--

--- Patch [1]: [PATCH V2 1/8] dmaengine: zynqmp_dma: Fix PM rollback on sw_desc_pool alloc failure ---
commit b97e6284964e1fb9d4c743aeb8e1818fb367a1d3
Author: Golla Nagendra <[email protected]>

dmaengine: zynqmp_dma: Fix PM rollback on sw_desc_pool alloc failure

If sw_desc_pool allocation fails after pm_runtime_resume_and_get()
succeeds in zynqmp_dma_alloc_chan_resources(), the error path returns
without dropping the runtime PM reference. Route that failure path
through err_pm so the runtime PM reference is balanced before
returning the error.

> diff --git a/drivers/dma/xilinx/zynqmp_dma.c b/drivers/dma/xilinx/zynqmp_dma.c
> index f6a812e49ddca..9f1e69700dd02 100644
> --- a/drivers/dma/xilinx/zynqmp_dma.c
> +++ b/drivers/dma/xilinx/zynqmp_dma.c
> @@ -483,8 +483,10 @@ static int zynqmp_dma_alloc_chan_resources(struct dma_chan *dchan)
>  		return ret;
>  
>  	chan->sw_desc_pool = kzalloc_objs(*desc, ZYNQMP_DMA_NUM_DESCS);
> -	if (!chan->sw_desc_pool)
> -		return -ENOMEM;
> +	if (!chan->sw_desc_pool) {
> +		ret = -ENOMEM;
> +		goto err_pm;
> +	}
>  
>  	chan->idle = true;
>  	chan->desc_free_cnt = ZYNQMP_DMA_NUM_DESCS;

[ ... ]

> @@ -516,6 +518,10 @@ static int zynqmp_dma_alloc_chan_resources(struct dma_chan *dchan)
>  	}
>  
>  	return ZYNQMP_DMA_NUM_DESCS;
> +
> +err_pm:
> +	pm_runtime_put_autosuspend(chan->dev);
> +	return ret;
>  }

[Severity: High]
This is a pre-existing issue, but I noticed another leak in this same
function. If dma_alloc_coherent() fails:

drivers/dma/xilinx/zynqmp_dma.c:zynqmp_dma_alloc_chan_resources() {
    ...
    chan->desc_pool_v = dma_alloc_coherent(...);
    if (!chan->desc_pool_v)
        return -ENOMEM;
    ...
}

Does this path also leak the PM reference and chan->sw_desc_pool since
it returns directly instead of jumping to an error label?

[Severity: High]
This is a pre-existing issue, but the probe function seems to have a similar
PM leak on error paths.

drivers/dma/xilinx/zynqmp_dma.c:zynqmp_dma_probe() {
    ...
    ret = pm_runtime_resume_and_get(zdev->dev);
    ...
err_disable_pm:
    if (!pm_runtime_enabled(zdev->dev))
        zynqmp_dma_runtime_suspend(zdev->dev);
    pm_runtime_disable(zdev->dev);
    return ret;
}

If pm_runtime_resume_and_get() succeeds but a subsequent step fails, we jump
to err_disable_pm. Is the PM runtime usage counter leaked here since there
is no pm_runtime_put_noidle() called before disabling PM?

[Severity: High]
This is a pre-existing issue, but there might be a race condition in
zynqmp_dma_chan_desc_cleanup().

drivers/dma/xilinx/zynqmp_dma.c:zynqmp_dma_chan_desc_cleanup() {
    ...
    list_for_each_entry_safe(desc, next, &chan->done_list, node) {
        ...
        if (dmaengine_desc_callback_valid(&cb)) {
            spin_unlock_irqrestore(&chan->lock, irqflags);
            dmaengine_desc_callback_invoke(&cb, NULL);
            spin_lock_irqsave(&chan->lock, irqflags);
        }
    ...
}

Since the spinlock is temporarily dropped inside the list_for_each_entry_safe
loop, can concurrent modifications to the list invalidate the next pointer
and cause memory corruption when the lock is reacquired?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1