Re: [PATCH v6 14/18] dmaengine: sh: rz-dmac: Add runtime PM support

[Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>]

On Tue, May 26, 2026 at 11:47:06AM +0300, Claudiu Beznea wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Protect the driver exposed APIs with runtime PM suspend/resume calls
> before accessing HW registers. As the current driver leaves runtime PM
> enabled in probe, the purpose of the changes in this patch is to avoid
> accessing HW registers after a failed system suspend leaves the runtime
> PM state of the device improperly reinitialized.
> 
> In that case, the driver remains bound to the device, the APIs are still
> exposed, and any access to HW registers without runtime resuming the
> device may lead to synchronous aborts.
> 
> To avoid leaking resources in case of runtime PM failures, save the error
> code returned by PM_RUNTIME_ACQUIRE_ERR() in rz_dmac_terminate_all() and
> return it only at the end of the function to allow the cleanup code to
> run. A similar approach is used in rz_dmac_free_chan_resources().
> 
> Because some exposed APIs (e.g. ->device_terminate_all()) may be called
> from atomic context according to the documentation, mark the DMA device as
> pm_runtime_irq_safe().
> 
> This patch prepares the driver for suspend-to-RAM support.
>

Tested-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>

> Tested-by: John Madieu <john.madieu.xa@bp.renesas.com>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
> 
> Changes in v6:
> - updated patch description
> - collected tags
> - in rz_dmac_free_chan_resources() and rz_dmac_terminate_all() don't touch
>   the HW registers if runtime resume failed but allow freeing resources
>   as suggested by sashiko; along with it added debug messages in case the
>   RPM resume failed
> - dropped the runtime resume from rz_dmac_xfer_desc() and move it instead
>   in rz_dmac_issue_pending() only to avoid calling rpm resume code in
>   interrupt path as, if we are in the interrupt path the device is sanely
>   in runtime resume state
> - moved the RPM resume code in from rz_dmac_tx_status to
>   rz_dmac_chan_get_residue(), as close as possible to the HW registers read
>   to avoid RPM resume in case the residue could be returned w/o interracting
>   with the HW
> - updated patch description
> 
> Changes in v5:
> - none, this patch is new
> 
>  drivers/dma/sh/rz-dmac.c | 60 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 57 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/dma/sh/rz-dmac.c b/drivers/dma/sh/rz-dmac.c
> index 93394b9934c8..bd4ca8e939f1 100644
> --- a/drivers/dma/sh/rz-dmac.c
> +++ b/drivers/dma/sh/rz-dmac.c
> @@ -549,12 +549,22 @@ static void rz_dmac_free_chan_resources(struct dma_chan *chan)
>  	struct rz_dmac *dmac = to_rz_dmac(chan->device);
>  	struct rz_dmac_desc *desc, *_desc;
>  	unsigned long flags;
> +	int ret;
> +
> +	PM_RUNTIME_ACQUIRE_IF_ENABLED(dmac->dev, pm);
> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +	if (ret) {
> +		dev_err(dmac->dev, "RPM resume failed for channel %s, ret=%d\n!",
> +			dma_chan_name(chan), ret);
> +	}
>  
>  	spin_lock_irqsave(&channel->vc.lock, flags);
>  
>  	rz_lmdesc_setup(channel, channel->lmdesc.base);
>  
> -	rz_dmac_disable_hw(channel);
> +	/*  Skip touching HW if RPM resume failed. Let the cleanup do its jobs. */
> +	if (!ret)
> +		rz_dmac_disable_hw(channel);
>  
>  	if (channel->mid_rid >= 0) {
>  		clear_bit(channel->mid_rid, dmac->modules);
> @@ -697,11 +707,22 @@ rz_dmac_prep_dma_cyclic(struct dma_chan *chan, dma_addr_t buf_addr,
>  static int rz_dmac_terminate_all(struct dma_chan *chan)
>  {
>  	struct rz_dmac_chan *channel = to_rz_dmac_chan(chan);
> +	struct rz_dmac *dmac = to_rz_dmac(chan->device);
>  	unsigned long flags;
>  	LIST_HEAD(head);
> +	int ret;
> +
> +	PM_RUNTIME_ACQUIRE_IF_ENABLED(dmac->dev, pm);
> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +	if (ret) {
> +		dev_err(dmac->dev, "RPM resume failed for channel %s, ret=%d\n!",
> +			dma_chan_name(chan), ret);
> +	}
>  
>  	spin_lock_irqsave(&channel->vc.lock, flags);
> -	rz_dmac_disable_hw(channel);
> +	/* Don't return if RPM failed. Let the cleanup do its jobs. */
> +	if (!ret)
> +		rz_dmac_disable_hw(channel);
>  	rz_lmdesc_setup(channel, channel->lmdesc.base);
>  
>  	if (channel->desc) {
> @@ -716,13 +737,20 @@ static int rz_dmac_terminate_all(struct dma_chan *chan)
>  	spin_unlock_irqrestore(&channel->vc.lock, flags);
>  	vchan_dma_desc_free_list(&channel->vc, &head);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static void rz_dmac_issue_pending(struct dma_chan *chan)
>  {
>  	struct rz_dmac_chan *channel = to_rz_dmac_chan(chan);
> +	struct rz_dmac *dmac = to_rz_dmac(chan->device);
>  	unsigned long flags;
> +	int ret;
> +
> +	PM_RUNTIME_ACQUIRE_IF_ENABLED(dmac->dev, pm);
> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +	if (ret)
> +		return;
>  
>  	spin_lock_irqsave(&channel->vc.lock, flags);
>  
> @@ -807,6 +835,11 @@ static void rz_dmac_device_synchronize(struct dma_chan *chan)
>  
>  	vchan_synchronize(&channel->vc);
>  
> +	PM_RUNTIME_ACQUIRE_IF_ENABLED(dmac->dev, pm);
> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +	if (ret)
> +		return;
> +
>  	ret = read_poll_timeout(rz_dmac_ch_readl, chstat, !(chstat & CHSTAT_EN),
>  				100, 100000, false, channel, CHSTAT, 1);
>  	if (ret < 0)
> @@ -866,6 +899,7 @@ static int rz_dmac_chan_get_residue(struct device *dev, struct rz_dmac_chan *cha
>  	struct rz_dmac_desc *desc = NULL;
>  	struct virt_dma_desc *vd;
>  	u32 crla, crtb, i;
> +	int ret;
>  
>  	vd = vchan_find_desc(&channel->vc, cookie);
>  	if (vd) {
> @@ -884,6 +918,11 @@ static int rz_dmac_chan_get_residue(struct device *dev, struct rz_dmac_chan *cha
>  		return 0;
>  	}
>  
> +	PM_RUNTIME_ACQUIRE_IF_ENABLED(dev, pm);
> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +	if (ret)
> +		return ret;
> +
>  	/*
>  	 * We need to read two registers. Make sure the hardware does not move
>  	 * to next lmdesc while reading the current lmdesc. Trying it 3 times
> @@ -965,6 +1004,13 @@ static int rz_dmac_device_pause_set(struct rz_dmac_chan *channel,
>  static int rz_dmac_device_pause(struct dma_chan *chan)
>  {
>  	struct rz_dmac_chan *channel = to_rz_dmac_chan(chan);
> +	struct rz_dmac *dmac = to_rz_dmac(chan->device);
> +	int ret;
> +
> +	PM_RUNTIME_ACQUIRE_IF_ENABLED(dmac->dev, pm);
> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +	if (ret)
> +		return ret;
>  
>  	guard(spinlock_irqsave)(&channel->vc.lock);
>  
> @@ -994,6 +1040,13 @@ static int rz_dmac_device_resume_set(struct rz_dmac_chan *channel,
>  static int rz_dmac_device_resume(struct dma_chan *chan)
>  {
>  	struct rz_dmac_chan *channel = to_rz_dmac_chan(chan);
> +	struct rz_dmac *dmac = to_rz_dmac(chan->device);
> +	int ret;
> +
> +	PM_RUNTIME_ACQUIRE_IF_ENABLED(dmac->dev, pm);
> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +	if (ret)
> +		return ret;
>  
>  	guard(spinlock_irqsave)(&channel->vc.lock);
>  
> @@ -1274,6 +1327,7 @@ static int rz_dmac_probe(struct platform_device *pdev)
>  		return dev_err_probe(&pdev->dev, PTR_ERR(dmac->rstc),
>  				     "failed to get resets\n");
>  
> +	pm_runtime_irq_safe(&pdev->dev);
>  	pm_runtime_enable(&pdev->dev);
>  	ret = pm_runtime_resume_and_get(&pdev->dev);
>  	if (ret < 0) {
> -- 
> 2.43.0
>