Re: [PATCH v5 13/17] dmaengine: sh: rz-dmac: Add runtime PM support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Early returns on runtime PM failure bypass critical software cleanup, causing resource leaks and Use-After-Free
- [High] `rz_dmac_tx_status()` returns a negative integer instead of a valid `enum dma_status`
- [Medium] Silent DMA queue stall in `rz_dmac_xfer_desc()` on PM failure
- [Medium] `rz_dmac_tx_status()` incurs unnecessary PM locking overhead for software-only status queries
--

commit 537bc713380d43d1dfa0d61269accb53a834436d
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

dmaengine: sh: rz-dmac: Add runtime PM support

This commit protects driver exposed APIs with runtime PM suspend/resume
calls before accessing HW registers. The changes avoid accessing HW
registers after a failed system suspend leaves the runtime PM state
improperly reinitialized, preparing the driver for suspend-to-RAM support.

> diff --git a/drivers/dma/sh/rz-dmac.c b/drivers/dma/sh/rz-dmac.c
> index d6ad070be705c..df91657fd5e3d 100644
> --- a/drivers/dma/sh/rz-dmac.c
> +++ b/drivers/dma/sh/rz-dmac.c
> @@ -488,7 +488,15 @@ static void rz_dmac_prepare_descs_for_cyclic(struct rz_dmac_chan *channel)
>  
>  static void rz_dmac_xfer_desc(struct rz_dmac_chan *chan)
>  {
> +	struct dma_chan *ch = &chan->vc.chan;
> +	struct rz_dmac *dmac = to_rz_dmac(ch->device);
>  	struct virt_dma_desc *vd;
> +	int ret;
> +
> +	PM_RUNTIME_ACQUIRE_IF_ENABLED(dmac->dev, pm);
> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +	if (ret)
> +		return;

Does this return early without pulling the descriptor from the queue? If PM
acquisition fails, it seems the descriptor is left permanently stranded on the
virtual channel's issued list, and the channel queue could silently stall.

[ ... ]

> @@ -549,6 +557,12 @@ 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)
> +		return;

If the runtime PM acquisition fails, does returning early bypass all subsequent
software state cleanup like vchan_free_chan_resources() and freeing allocated
DMA descriptors? Even if the hardware cannot be accessed, shouldn't the
software-allocated resources still be freed to avoid memory leaks?

[ ... ]

> @@ -697,8 +711,15 @@ 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)
> +		return ret;

Similarly, does returning early here skip clearing the virt-dma lists? Failing
to do so could leave orphaned descriptors active, which might lead to
use-after-free issues when the client subsequently frees its DMA buffers.

[ ... ]

> @@ -909,8 +935,15 @@ static enum dma_status rz_dmac_tx_status(struct dma_chan *chan,
>  					 struct dma_tx_state *txstate)
>  {
>  	struct rz_dmac_chan *channel = to_rz_dmac_chan(chan);
> +	struct rz_dmac *dmac = to_rz_dmac(chan->device);
>  	enum dma_status status;
>  	u32 residue;
> +	int ret;
> +
> +	PM_RUNTIME_ACQUIRE_IF_ENABLED(dmac->dev, pm);
> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +	if (ret)
> +		return ret;

Is it valid to return a negative integer error code directly as an
enum dma_status? Client drivers might check the status against specific
enumeration constants like DMA_ERROR, and could fail to recognize this
raw error code.

Also, does this add unnecessary PM locking overhead on the hot path for
software-only status queries? Since dma_cookie_status() might resolve the
status entirely in software, and hardware residue is only queried if the
status is DMA_IN_PROGRESS and txstate is not NULL, could the PM acquisition
be deferred until after verifying that reading the hardware residue is
actually necessary?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512121219.216159-1-claudiu.beznea.uj@bp.renesas.com?part=13