Re: [PATCH v12 1/4] iommu/arm-smmu: Add pm_runtime/sleep ops

["Rafael J. Wysocki" <rafael-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

On Mon, Jul 23, 2018 at 7:59 AM, Marek Szyprowski
<m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
> Hi Rafael,
>
> On 2018-07-11 22:36, Rafael J. Wysocki wrote:
>> On Wed, Jul 11, 2018 at 3:40 PM, Marek Szyprowski
>> <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:

[cut]

>>> Frankly, if there are no other reasons I suggest to wire system
>>> suspend/resume to pm_runtime_force_* helpers:
>>> SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
>>>                           pm_runtime_force_resume).
>> Not a good idea at all IMO.
>>
>> Use PM driver flags rather I'd say.
>
> Frankly, till now I wasn't aware of the DPM_FLAG_* in struct dev_pm_info
> 'driver_flags'.

They are a relatively recent addition.

> I've briefly checked them but I don't see the equivalent
> of using SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> pm_runtime_force_resume): keep device suspend if it was runtime suspended
> AND really call pm_runtime_suspend if it was not runtime suspended on
> system suspend.

DPM_FLAG_SMART_SUSPEND is expected to cause that to happen.  If you
want the device to remain in suspend after the system-wide resume from
sleep (if possible), you can set DPM_FLAG_LEAVE_SUSPENDED for it too.

Currently a caveat is that genpd still doesn't support the flags.  I
have patches for that, but I haven't got to posting them yet.  If you
are interested, I can push this work forward relatively quickly, so
please let me know.

>>> This way you will have everything related to suspending and resuming in
>>> one place and you would not need to bother about all possible cases (like
>>> suspending from runtime pm active and suspending from runtime pm suspended
>>> cases as well as restoring proper device state on resume). This is
>>> especially important in recent kernel releases, where devices are
>>> system-suspended regardless their runtime pm states (in older kernels
>>> devices were first runtime resumed for system suspend, what made code
>>> simpler, but wasn't best from power consumption perspective).
>>>
>>> If you go this way, You only need to ensure that runtime resume will also
>>> restore proper device state besides enabling all the clocks. This will
>>> also prepare your driver to properly operate inside power domain, where it
>>> is possible for device to loose its internal state after runtime suspend
>>> when respective power domain has been turned off.
>> I'm not sure if you are aware of the pm_runtime_force_* limitations, though.
>
> What are those limitations?

First off, they don't work with middle-layer code implementing its own
PM callbacks that actually operate on devices (like the PCI bus type
or the generic ACPI PM domain).  This means that drivers using them
may not work on systems with ACPI, for example.

Second, pm_runtime_force_resume() will always try to leave the device
in suspend during system-wide resume from sleep which may not be
desirable.

Finally, they expect the runtime PM status to be updated by
system-wide PM callbacks of devices below the one using them (eg. its
children and their children etc) which generally is not required and
may not take place unless the drivers of those devices use
pm_runtime_force_* themselves.

So careful there.