Re: [PATCH v3 06/22] dmaengine: ste_dma40: Handle runtime PM resume errors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] dmaengine: ste_dma40: skip channel teardown on PM resume failure
- [High] dmaengine: ste_dma40: leave descriptor queued on PM resume failure
--

commit ed522d6297daebccbf846812d9e9d09ad29b8000
Author: Linus Walleij <linusw@kernel.org>

    dmaengine: ste_dma40: Handle runtime PM resume errors
    
    This commit replaces pm_runtime_get_sync() with pm_runtime_resume_and_get()
    in several ste_dma40 channel operations. It adds error checking to abort
    hardware access if the PM resume fails.

> diff --git a/drivers/dma/ste_dma40.c b/drivers/dma/ste_dma40.c
> index 1baee6f4d919a..c6f633353f3ea 100644
> --- a/drivers/dma/ste_dma40.c
> +++ b/drivers/dma/ste_dma40.c

[ ... ]

> @@ -1521,8 +1527,13 @@ static struct d40_desc *d40_queue_start(struct d40_chan *d40c)
>  
>  	if (d40d != NULL) {
>  		if (!d40c->busy) {
> +			err = pm_runtime_resume_and_get(d40c->base->dev);
> +			if (err < 0) {
> +				chan_err(d40c, "Failed to resume DMA: %d\n",
> +					 err);
> +				return ERR_PTR(err);
> +			}

[Severity: High]
Does returning an error here without removing the pending descriptor from the
queue or scheduling a completion tasklet cause issues? If a caller such as
dma_async_issue_pending() triggers this, it appears the DMA client will hang
indefinitely waiting for a transfer completion callback that never arrives.

>  			d40c->busy = true;
> -			pm_runtime_get_sync(d40c->base->dev);
>  		}
>  
>  		/* Remove from queue */

[ ... ]

> @@ -2042,9 +2053,6 @@ static int d40_free_dma(struct d40_chan *d40c)
>  	struct d40_phy_res *phy = d40c->phy_chan;
>  	bool is_src;
>  
> -	/* Terminate all queued and active transfers */
> -	d40_term_all(d40c);
> -
>  	if (phy == NULL) {
>  		chan_err(d40c, "phy == null\n");
>  		return -EINVAL;
> @@ -2066,7 +2074,13 @@ static int d40_free_dma(struct d40_chan *d40c)
>  		return -EINVAL;
>  	}
>  
> -	pm_runtime_get_sync(d40c->base->dev);
> +	res = pm_runtime_resume_and_get(d40c->base->dev);
> +	if (res < 0)
> +		return res;
> +
> +	/* Terminate all queued and active transfers */
> +	d40_term_all(d40c);
> +

[Severity: High]
Can this early return on PM resume failure cause hardware channels and
descriptor memory to leak? By returning before calling d40_term_all() and
d40_alloc_mask_free(), the physical channel state seems permanently locked to
allocated and descriptors are not released.

>  	res = d40_channel_execute_command(d40c, D40_DMA_STOP);
>  	if (res) {
>  		chan_err(d40c, "stop failed\n");

[ ... ]

> @@ -2658,7 +2675,10 @@ static int d40_terminate_all(struct dma_chan *chan)
>  
>  	spin_lock_irqsave(&d40c->lock, flags);
>  
> -	pm_runtime_get_sync(d40c->base->dev);
> +	ret = pm_runtime_resume_and_get(d40c->base->dev);
> +	if (ret < 0)
> +		goto unlock;
> +

[Severity: High]
Does jumping to the unlock label here skip d40_term_all()? If
pm_runtime_resume_and_get() fails, it looks like pending and active
descriptors are left in the queues and permanently leaked instead of being
cleaned up.

>  	ret = d40_channel_execute_command(d40c, D40_DMA_STOP);
>  	if (ret)
>  		chan_err(d40c, "Failed to stop channel\n");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260918-dma40-fixes-v3-0-8dd8450669e8@kernel.org?part=6