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

[Claudiu Beznea <claudiu.beznea@kernel.org>]

 >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.

The rz_dmac_xfer_desc() is called dma_async_issue_pending() or after each 
descriptor is completed. The documentation of dma_async_issue_pending states:

/**
  * dma_async_issue_pending - flush pending transactions to HW
  * @chan: target DMA channel
  *
  * This allows drivers to push copies to HW in batches,
  * reducing MMIO writes where possible.
  */

Thus, I'm not sure we should free the descriptors on this path. I think that's 
the scope of ->device_terminate_all() and ->device_free_chan_resources()

 >[ ... ]
 >> @@ -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?

I'll adjust it like this.

 >[ ... ]
 >> @@ -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.

I'll adjust it.

 >[ ... ]
 >> @@ -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.

Indeed, I'll adjust it.

 >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?

This will be adjusted as well.