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

[Claudiu Beznea]

>commit 537bc713380d43d1dfa0d61269accb53a834436d
>Author: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
>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.