Re: [PATCH v6 07/23] dmaengine: ste_dma40: Handle runtime PM resume errors

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

On Thu, Sep 24, 2026 at 10:35:19AM +0200, Linus Walleij wrote:
> Several channel operations use pm_runtime_get_sync() and access DMA40
> registers without checking whether runtime resume succeeded. If resume
> fails, the registers may be inaccessible. pm_runtime_get_sync() also
> increments the usage counter on failure, making error unwinding easy to
> unbalance.
>
> Use pm_runtime_resume_and_get() and avoid register access when resume
> fails. Acquire the runtime PM reference before allocating a channel so
> failure needs no channel-allocation rollback.
>
> If a queued transfer cannot be started because resume failed, retire all
> issued descriptors through the normal tasklet path. Since
> dma_async_issue_pending() cannot return an error, leaving them queued would
> make clients wait indefinitely for callbacks.
>
> Termination and channel release must also clean up software state when the
> controller cannot resume. d40_term_all() only releases descriptor state and
> does not access DMA40 registers, so it remains unconditional.
>
> Balance each transient runtime PM reference in its successful acquisition
> block, without bookkeeping flags. Release the outstanding busy reference
> and channel allocation state when freeing a channel. Skip only the hardware
> stop that requires register access.
>
> Fixes: 7fb3e75e1833 ("dmaengine/ste_dma40: support pm in dma40")
> Assisted-by: LLM
> Signed-off-by: Linus Walleij <linusw@kernel.org>
> ---

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

>  drivers/dma/ste_dma40.c | 74 ++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 52 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/dma/ste_dma40.c b/drivers/dma/ste_dma40.c
> index 690e41ca40f0..712719f0c4cf 100644
> --- a/drivers/dma/ste_dma40.c
> +++ b/drivers/dma/ste_dma40.c
> @@ -1579,11 +1579,14 @@ static int d40_pause(struct dma_chan *chan)
>  		return 0;
>
>  	spin_lock_irqsave(&d40c->lock, flags);
> -	pm_runtime_get_sync(d40c->base->dev);
> +	res = pm_runtime_resume_and_get(d40c->base->dev);
> +	if (res < 0)
> +		goto unlock;
>
>  	res = d40_channel_execute_command(d40c, D40_DMA_SUSPEND_REQ);
>
>  	pm_runtime_put_autosuspend(d40c->base->dev);
> + unlock:
>  	spin_unlock_irqrestore(&d40c->lock, flags);
>  	return res;
>  }
> @@ -1603,13 +1606,16 @@ static int d40_resume(struct dma_chan *chan)
>  		return 0;
>
>  	spin_lock_irqsave(&d40c->lock, flags);
> -	pm_runtime_get_sync(d40c->base->dev);
> +	res = pm_runtime_resume_and_get(d40c->base->dev);
> +	if (res < 0)
> +		goto unlock;
>
>  	/* If bytes left to transfer or linked tx resume job */
>  	if (d40_residue(d40c) || d40_tx_is_linked(d40c))
>  		res = d40_channel_execute_command(d40c, D40_DMA_RUN);
>
>  	pm_runtime_put_autosuspend(d40c->base->dev);
> + unlock:
>  	spin_unlock_irqrestore(&d40c->lock, flags);
>  	return res;
>  }
> @@ -1646,8 +1652,20 @@ 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);
> +				do {
> +					d40_desc_remove(d40d);
> +					d40_desc_done(d40c, d40d);
> +					d40c->pending_tx++;
> +					d40d = d40_first_queued(d40c);
> +				} while (d40d);
> +				tasklet_schedule(&d40c->tasklet);
> +				return ERR_PTR(err);
> +			}
>  			d40c->busy = true;
> -			pm_runtime_get_sync(d40c->base->dev);
>  		}
>
>  		/* Remove from queue */
> @@ -2164,9 +2182,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;
> @@ -2188,11 +2203,18 @@ static int d40_free_dma(struct d40_chan *d40c)
>  		return -EINVAL;
>  	}
>
> -	pm_runtime_get_sync(d40c->base->dev);
> -	res = d40_channel_execute_command(d40c, D40_DMA_STOP);
> -	if (res) {
> -		chan_err(d40c, "stop failed\n");
> -		goto mark_last_busy;
> +	/* Release descriptor state; this does not access DMA40 registers. */
> +	d40_term_all(d40c);
> +
> +	res = pm_runtime_resume_and_get(d40c->base->dev);
> +	if (res >= 0) {
> +		res = d40_channel_execute_command(d40c, D40_DMA_STOP);
> +		if (res)
> +			chan_err(d40c, "stop failed\n");
> +
> +		pm_runtime_put_autosuspend(d40c->base->dev);
> +		if (res)
> +			return res;
>  	}
>
>  	d40_alloc_mask_free(phy, is_src, chan_is_logical(d40c) ? event : 0);
> @@ -2208,8 +2230,6 @@ static int d40_free_dma(struct d40_chan *d40c)
>  	d40c->busy = false;
>  	d40c->phy_chan = NULL;
>  	d40c->configured = false;
> - mark_last_busy:
> -	pm_runtime_put_autosuspend(d40c->base->dev);
>  	return res;
>  }
>
> @@ -2584,10 +2604,14 @@ static int d40_alloc_chan_resources(struct dma_chan *chan)
>  		err = d40_config_memcpy(d40c);
>  		if (err) {
>  			chan_err(d40c, "Failed to configure memcpy channel\n");
> -			goto mark_last_busy;
> +			goto unlock;
>  		}
>  	}
>
> +	err = pm_runtime_resume_and_get(d40c->base->dev);
> +	if (err < 0)
> +		goto unlock;
> +
>  	err = d40_allocate_channel(d40c, &is_free_phy);
>  	if (err) {
>  		chan_err(d40c, "Failed to allocate channel\n");
> @@ -2595,8 +2619,6 @@ static int d40_alloc_chan_resources(struct dma_chan *chan)
>  		goto mark_last_busy;
>  	}
>
> -	pm_runtime_get_sync(d40c->base->dev);
> -
>  	d40_set_prio_realtime(d40c);
>
>  	if (chan_is_logical(d40c)) {
> @@ -2628,6 +2650,7 @@ static int d40_alloc_chan_resources(struct dma_chan *chan)
>  		d40_config_write(d40c);
>   mark_last_busy:
>  	pm_runtime_put_autosuspend(d40c->base->dev);
> + unlock:
>  	spin_unlock_irqrestore(&d40c->lock, flags);
>  	return err;
>  }
> @@ -2788,19 +2811,26 @@ static int d40_terminate_all(struct dma_chan *chan)
>
>  	spin_lock_irqsave(&d40c->lock, flags);
>
> -	pm_runtime_get_sync(d40c->base->dev);
> -	ret = d40_channel_execute_command(d40c, D40_DMA_STOP);
> -	if (ret)
> -		chan_err(d40c, "Failed to stop channel\n");
> +	ret = pm_runtime_resume_and_get(d40c->base->dev);
> +	if (ret >= 0) {
> +		ret = d40_channel_execute_command(d40c, D40_DMA_STOP);
> +		if (ret)
> +			chan_err(d40c, "Failed to stop channel\n");
> +
> +		pm_runtime_put_autosuspend(d40c->base->dev);
> +	}
>
> +	/*
> +	 * Always release software state, even when the controller cannot
> +	 * resume. d40_term_all() does not access DMA40 registers.
> +	 */
>  	d40_term_all(d40c);
> -	pm_runtime_put_autosuspend(d40c->base->dev);
>  	if (d40c->busy)
>  		pm_runtime_put_autosuspend(d40c->base->dev);
>  	d40c->busy = false;
>
>  	spin_unlock_irqrestore(&d40c->lock, flags);
> -	return 0;
> +	return ret;
>  }
>
>  static int
>
> --
> 2.55.0
>