sashiko review (Re: [PATCH v5 14/17] dmaengine: sh: rz-dmac: Add suspend to RAM support)

[Claudiu Beznea]

>commit 536ec6e9c07fa47423de7d20ebc829e45cbcedfb
>Author: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
>Subject: dmaengine: sh: rz-dmac: Add suspend to RAM support
>This commit adds suspend to RAM support for the Renesas RZ/G3S SoC by
>saving and restoring the DMA IP registers. It adds specific suspend and
>resume handling for cyclic channels, and waits for non-cyclic channels
>to finish before suspending.
>> diff --git a/drivers/dma/sh/rz-dmac.c b/drivers/dma/sh/rz-dmac.c
>> index df91657fd5e3d..b0a8590f4a9da 100644
>> --- a/drivers/dma/sh/rz-dmac.c
>> +++ b/drivers/dma/sh/rz-dmac.c
>[ ... ]
>> @@ -1002,20 +1008,57 @@ static int rz_dmac_device_pause(struct dma_chan *chan)
>>      return rz_dmac_device_pause_set(channel, BIT(RZ_DMAC_CHAN_STATUS_PAUSED));
>>  }
>>
>[ ... ]
>>  static int rz_dmac_device_resume_set(struct rz_dmac_chan *channel,
>>                                   unsigned long clear_bitmask)
>>  {
>> -    int ret = 0;
>>      u32 val;
>> +    int ret;
>>
>>      lockdep_assert_held(&channel->vc.lock);
>>
>[ ... ]
>> +    rz_dmac_ch_writel(channel, CHCTRL_CLRSUS | CHCTRL_SETEN, CHCTRL, 1);
>>
>> -    rz_dmac_ch_writel(channel, CHCTRL_CLRSUS, CHCTRL, 1);
>>      ret = read_poll_timeout_atomic(rz_dmac_ch_readl, val,
>> -                                   !(val & CHSTAT_SUS), 1, 1024, false,
>> -                                   channel, CHSTAT, 1);
>> +                                   ((val & (CHSTAT_SUS | CHSTAT_EN)) == 
CHSTAT_EN),
>> +                                   1, 1024, false, channel, CHSTAT, 1);
>Can this condition time out spuriously for short non-cyclic DMA transfers? If
>a transfer finishes successfully before the first poll read, the hardware
>would clear CHSTAT_EN to 0. The polling condition would then evaluate to
>false, causing the macro to incorrectly return -ETIMEDOUT.

Yes, in theory that can happen.

>>
>>      channel->status &= ~clear_bitmask;
>>
>[ ... ]
>> @@ -1406,6 +1459,151 @@ static void rz_dmac_remove(struct platform_device *pdev)
>>      pm_runtime_disable(&pdev->dev);
>>  }
>>
>> +static int rz_dmac_suspend_prepare(struct device *dev)
>> +{
>> +    struct rz_dmac *dmac = dev_get_drvdata(dev);
>> +    int ret;
>> +
>> +    PM_RUNTIME_ACQUIRE_IF_ENABLED(dmac->dev, pm);
>> +    ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
>> +    if (ret)
>> +            return ret;
>> +
>> +    for (unsigned int i = 0; i < dmac->n_channels; i++) {
>> +            struct rz_dmac_chan *channel = &dmac->channels[i];
>> +
>> +            guard(spinlock_irqsave)(&channel->vc.lock);
>> +
>> +            /* Wait for transfer completion, except in cyclic case. */
>> +            if (channel->status & BIT(RZ_DMAC_CHAN_STATUS_CYCLIC))
>> +                    continue;
>> +
>> +            if (rz_dmac_chan_is_enabled(channel))
>> +                    return -EAGAIN;
>Could returning -EAGAIN here cause a CPU livelock? In dpm_prepare(), an
>-EAGAIN error is cleared and retried continuously in a while
>(!list_empty(&dpm_list)) loop. Since the device is not advanced on error,
>the PM core might loop infinitely without any backoff until the transfer
>completes.
>Also, is there a race condition with non-cyclic channels after this callback
>returns 0? Since the DMA controller's prepare callback runs before consumer
>peripherals, an active consumer could immediately submit new non-cyclic
>transfers before the actual suspend phase begins.

The expectation was to wait until all the transfers complete. But, indeed, there 
might be some issues with this approach. E.g., the serial driver simulates 
cyclic transfers for rx by enqueuing individual SG transfers, and, after each SG 
is completed a new SG is enqueued. So, at any time, the channel is enabled. That 
would lead to the suspend being blocked at this point. As I don't see a way to 
go out of this, for the moment I will drop the prepare callback until the serial 
RX DMA will be adjusted (maybe, to use the cyclic support).


>> +    }
>> +
>> +    return 0;
>> +}
>> +
>[ ... ]
>> +static int rz_dmac_suspend(struct device *dev)
>> +{
>> +    struct rz_dmac *dmac = dev_get_drvdata(dev);
>> +    int ret = 0;
>> +
>> +    for (unsigned int i = 0; i < dmac->n_channels; i++) {
>> +            struct rz_dmac_chan *channel = &dmac->channels[i];
>> +
>> +            guard(spinlock_irqsave)(&channel->vc.lock);
>This isn't a bug, but does this function mix scope-based guard() cleanup
>with goto-based error handling? The cleanup guidelines suggest that
>functions should either convert all resources to scope-based cleanup or
>convert none of them to avoid confusing ownership semantics.
>> +
>> +            if (!(channel->status & BIT(RZ_DMAC_CHAN_STATUS_CYCLIC)))
>> +                    continue;
>> +
>> +            ret = rz_dmac_device_pause_internal(channel);
>> +            if (ret) {
>> +                    dev_err(dev, "Failed to suspend channel %s\n",
>> +                            dma_chan_name(&channel->vc.chan));
>> +                    break;
>> +            }
>> +
>> +            channel->pm_state.nxla = rz_dmac_ch_readl(channel, NXLA, 1);
>Does saving only the NXLA register result in data loss for cyclic channels?
>When a cyclic channel is paused, it stops at its current descriptor. Without
>saving and restoring the Current Link Address and Current Transfer Byte
>count, resuming the channel might force it to skip the remaining bytes of the
>interrupted descriptor and start fetching the next one.

This is something expected, hardware have no capability to overcome this issue.

>> +    }
>> +
>> +    if (ret)
>> +            goto suspend_recover;
>> +
>> +    ret = reset_control_assert(dmac->rstc);
>Since non-cyclic channels are unconditionally skipped in the loop above and
>the hardware reset is asserted here, could newly queued non-cyclic requests
>from consumers be violently interrupted mid-transfer?

The expectation is that non-cyclic transfers have already been paused by the 
time this code executes.

>> +    if (ret)
>> +            goto suspend_recover;
>> +
>> +    ret = pm_runtime_put_sync(dev);
>> +    if (ret < 0)
>> +            goto reset_deassert;
>> +
>> +    return 0;
>> +
>> +reset_deassert:
>> +    reset_control_deassert(dmac->rstc);
>> +suspend_recover:
>> +    rz_dmac_suspend_recover(dmac);
>> +    return ret;
>> +}
>> +
>[ ... ]
>> +static const struct dev_pm_ops rz_dmac_pm_ops = {
>> +    .prepare = rz_dmac_suspend_prepare,
>This isn't a bug, but should rz_dmac_suspend_prepare be wrapped in
>pm_sleep_ptr() here? The power management subsystem guidelines recommend
>using pm_sleep_ptr() for individual callbacks within the ops structure to
>prevent dead code and unused variable warnings when CONFIG_PM_SLEEP is
>disabled.

I will drop this function here in the next version as I discovered some issues 
with it and the serial RX DMA.

>> +    SYSTEM_SLEEP_PM_OPS(rz_dmac_suspend, rz_dmac_resume)
>> +};
>> +
>>  static const struct rz_dmac_info rz_dmac_v2h_info = {
>>      .icu_register_dma_req = rzv2h_icu_register_dma_req,
>>      .default_dma_req_no = RZV2H_ICU_DMAC_REQ_NO_DEFAULT,
>> @@ -1432,6 +1630,7 @@ static struct platform_driver rz_dmac_driver = {
>>      .driver         = {
>>              .name   = "rz-dmac",
>>              .of_match_table = of_rz_dmac_match,
>> +            .pm     = pm_sleep_ptr(&rz_dmac_pm_ops),
>This isn't a bug, but should pm_ptr() be used for the dev_pm_ops structure
>pointer instead of pm_sleep_ptr()? The power management guidelines recommend
>using pm_ptr() for the structure pointer, while reserving pm_sleep_ptr() for
>the individual sleep callbacks inside the structure.

OK, will switch to pm_ptr()

>>      },
>>      .probe          = rz_dmac_probe,
>>      .remove         = rz_dmac_remove,