PM regression with commit 5de85b9d57ab PM runtime re-init in v4.5-rc1

[Ulf Hansson]

On 3 February 2016 at 00:41, Tony Lindgren <tony at atomide.com> wrote:
> * Ulf Hansson <ulf.hansson at linaro.org> [160202 12:48]:
>> On 2 February 2016 at 17:35, Tony Lindgren <tony at atomide.com> wrote:
>> > That's a valid error though, let's not remove it. The reason why we
>> > call runtime_resume() twice is because runtime_suspend callback never
>> > gets called like I explain above.
>>
>> This isn't an error, it's just a hickup in the synchronization of the
>> runtime PM status.
>
> I'd rather not get the hardware state out of sync with PM runtime..
>
>> Very similar what happens at the first probe, when the driver core has
>> initialized the runtime PM status to RPM_SUSPENDED at the device
>> registration.
>
> Well we actually pretty much have devices in that state to start
> with.

That's not true, not even for omap.

There are definitely devices which HW state is reflected by RPM_ACTIVE
at boot. It's the responsible of the subsystem/driver (including PM
domains) to make sure the runtime PM status is updated accordingly, to
reflect the real HW state.

For omap hwmod, at device registration in omap_device_build_from_dt()
it may actually invoke pm_runtime_set_active() if the device is
enabled. To my understanding, that's done to synchronize the real HW
state with the runtime PM status, right?

>
>> To me, it's the responsible of the PM domain to *help* with the
>> synchronization, not prevent it as it currently does.
>
> The problem is that the hardware state gets out of sync with
> PM runtime. And that's going to be a pain to debug later on.

I don't see the problem, but of course you know omap for better than I do.

So if you are concerned about this, perhaps adding a dev_dbg print
when the omap hwmod's ->runtime_suspend() callback returns zero could
be a way forward?

>
>> > --- a/drivers/mmc/host/omap_hsmmc.c
>> > +++ b/drivers/mmc/host/omap_hsmmc.c
>> > @@ -2232,6 +2232,7 @@ err_irq:
>> >                 dma_release_channel(host->tx_chan);
>> >         if (host->rx_chan)
>> >                 dma_release_channel(host->rx_chan);
>> > +       pm_runtime_dont_use_autosuspend(host->dev);
>>
>> It's good know this works, although do you intend to fix this sequence
>> for all omap drivers/devices that's part of the hwmod PM domain?
>>
>> I haven't checked the number of drivers this would affect, but I
>> imagine there could be quite many with similar behaviour and thus may
>> suffer from the same issue.
>
> Yeah not sure what the right fix is. But I'd rather patch the
> few drivers using autosuspend if we come to the conclusion
> that there is no bug in PM runtime.
>
>> Could you please test my version 2 of the patch I attached earlier. I
>> still believe it's the best way to solve the regression, if it works
>> of course. :-)
>
> And I don't like it because of the reasons above :) But yeah
> gave it a quick try and that too works as expected.

Okay, thanks for testing!

Kind regards
Uffe