[v2,5/9] PM / ACPI: Provide option to disable direct_complete for ACPI devices

On Thursday, August 24, 2017 2:20:00 AM CEST Rafael J. Wysocki wrote:
> On Thursday, August 24, 2017 2:13:55 AM CEST Rafael J. Wysocki wrote:
> > On Thursday, August 24, 2017 1:39:44 AM CEST Rafael J. Wysocki wrote:
> > > On Wed, Aug 23, 2017 at 4:42 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > In some cases a driver for an ACPI device needs to be able to prevent the
> > > > ACPI PM domain from using the direct_complete path during system sleep.
> > > >
> > > > One typical case is when the driver for the device needs its device to stay
> > > > runtime enabled, during the __device_suspend phase. This isn't the case
> > > > when the direct_complete path is being executed by the PM core, as it then
> > > > disables runtime PM for the device in __device_suspend(). Any following
> > > > attempts to runtime resume the device after that point, just fails.
> > > 
> > > OK, that is a problem.
> > > 
> > > > A workaround to this problem is to let the driver runtime resume its device
> > > > from its ->prepare() callback, as that would prevent the direct_complete
> > > > path from being executed. However, that may often be a waste, especially if
> > > > it turned out that no one really needed the device.
> > > >
> > > > For this reason, invent acpi_dev_disable|enable_direct_complete(), to allow
> > > > drivers to inform the ACPI PM domain to change its default behaviour during
> > > > system sleep, and thus control whether it may use the direct_complete path
> > > > or not.
> > > 
> > > But I'm not sure this is the right place to address it as it very well
> > > may affect a PCI device too.
> > > 
> > > Also, this is about the device and not about its ACPI companion
> > > object, so putting the flag in there is somewhat unclean, so to speak.
> > > 
> > > It looks like we need a flag in dev_pm_info saying something along the
> > > lines of "my system suspend callback can deal with runtime PM" that
> > > will cause the direct_complete update to occur in
> > > __device_suspend_late() instead of __device_suspend().
> > 
> > IOW, something like the patch below.
> > 
> > It actually should work with the ACPI PM domain code as is except that it
> > will cause the device to be runtime resumed every time during suspend.
> > 
> > But acpi_subsys_suspend() can be changed to avoid resuming the device if
> > power.force_suspend is set.
> 
> Or better yet, if power.direct_complete is not set, because that means the
> device needs to be resumed anyway.
> 
> And if power.direct_complete is set at that point, power.force_suspend has to
> be set too.

Overall, like below, and of course drivers that ser power.force_suspend need to
be able to deal with devices that have been runtime resumed during
__device_suspend().

---
 drivers/acpi/device_pm.c  |    8 ++++++++
 drivers/base/power/main.c |   11 +++++++++--
 include/linux/pm.h        |    1 +
 3 files changed, 18 insertions(+), 2 deletions(-)

[...]

>> >
>> > It actually should work with the ACPI PM domain code as is except that it
>> > will cause the device to be runtime resumed every time during suspend.
>> >
>> > But acpi_subsys_suspend() can be changed to avoid resuming the device if
>> > power.force_suspend is set.
>>
>> Or better yet, if power.direct_complete is not set, because that means the
>> device needs to be resumed anyway.
>>
>> And if power.direct_complete is set at that point, power.force_suspend has to
>> be set too.

I believe I understand your goal here. You want to consider if it is
possible to extend the behavior of the direct_complete path, instead
of enabling the runtime PM centric path for the ACPI PM domain, to
accomplish the same optimizations.

I think the answer to that, is that it simply can't. See more
information about why further down below.

>
> Overall, like below, and of course drivers that ser power.force_suspend need to
> be able to deal with devices that have been runtime resumed during
> __device_suspend().

The a concern I see with this approach, is that is going to be too
complex to understand.

We have already seen how the i2c-designware-driver was screwed up when
it was trying to take advantage of the optimizations provided by the
current direct_complete path. I think we should be careful when
considering making it even more complex.

In the runtime PM centric path, driver authors can easily deploy
system sleep support. In some cases it may even consists of only two
lines of code. As can be shown in patch9 for the i2c driver.

Just to be clear, that doesn't mean that I think the direct_complete
path isn't useful. Especially it is a good match for the ACPI PM
domain, as it can easily affect all its devices, without having to
care much about the behavior of the drivers.

>
> ---
>  drivers/acpi/device_pm.c  |    8 ++++++++
>  drivers/base/power/main.c |   11 +++++++++--
>  include/linux/pm.h        |    1 +
>  3 files changed, 18 insertions(+), 2 deletions(-)
>
> Index: linux-pm/drivers/base/power/main.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/main.c
> +++ linux-pm/drivers/base/power/main.c
> @@ -1271,9 +1271,16 @@ static int __device_suspend_late(struct
>                 goto Complete;
>         }
>
> -       if (dev->power.syscore || dev->power.direct_complete)
> +       if (dev->power.syscore)
>                 goto Complete;
>
> +       if (dev->power.direct_complete) {
> +               if (pm_runtime_status_suspended(dev))
> +                       goto Complete;
> +
> +               dev->power.direct_complete = false;
> +       }
> +
>         if (dev->pm_domain) {
>                 info = "late power domain ";
>                 callback = pm_late_early_op(&dev->pm_domain->ops, state);
> @@ -1482,7 +1489,7 @@ static int __device_suspend(struct devic
>         if (dev->power.syscore)
>                 goto Complete;
>
> -       if (dev->power.direct_complete) {
> +       if (dev->power.direct_complete && !dev->power.force_suspend) {
>                 if (pm_runtime_status_suspended(dev)) {
>                         pm_runtime_disable(dev);
>                         if (pm_runtime_status_suspended(dev))
> Index: linux-pm/include/linux/pm.h
> ===================================================================
> --- linux-pm.orig/include/linux/pm.h
> +++ linux-pm/include/linux/pm.h
> @@ -554,6 +554,7 @@ struct dev_pm_info {
>         pm_message_t            power_state;
>         unsigned int            can_wakeup:1;
>         unsigned int            async_suspend:1;
> +       unsigned int            force_suspend:1;
>         bool                    in_dpm_list:1;  /* Owned by the PM core */
>         bool                    is_prepared:1;  /* Owned by the PM core */
>         bool                    is_suspended:1; /* Ditto */
> Index: linux-pm/drivers/acpi/device_pm.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/device_pm.c
> +++ linux-pm/drivers/acpi/device_pm.c
> @@ -1025,9 +1025,17 @@ EXPORT_SYMBOL_GPL(acpi_subsys_prepare);
>   *
>   * Follow PCI and resume devices suspended at run time before running their
>   * system suspend callbacks.
> + *
> + * If the power.direct_complete flag is set for the device, though, skip all
> + * that, because the device doesn't need to be resumed then and if it is
> + * resumed via runtime PM, the core will notice that and will carry out the
> + * late suspend for it.
>   */
>  int acpi_subsys_suspend(struct device *dev)
>  {
> +       if (dev->power.direct_complete)
> +               return 0;
> +
>         pm_runtime_resume(dev);
>         return pm_generic_suspend(dev);
>  }
>

The above change would offer an improvement, however there are more
benefits from the runtime PM centric path that it doesn't cover. The
below is pasted from the changelog for patch9 (I will make sure to
fold in this in the changelog for $subject patch next version).

In case the device is/gets runtime resumed before the device_suspend()
phase is entered, the PM core doesn't run the direct_complete path for
the device during system sleep. During system resume, this lead to
that the device will always be brought back to full power when the
i2c-dw-plat driver's ->resume() callback is invoked. This may not be
necessary, thus increasing the resume time for the device and wasting
power.

In the runtime PM centric path, the pm_runtime_force_resume() helper
takes better care, as it resumes the device only in case it's really
needed. Normally this means it can be postponed to be dealt with via
regular calls to runtime PM (pm_runtime_get*()) instead. In other
words, the device remains in its low power state until someone request
a new i2c transfer, whenever that may be.

As far as I can think of, the direct_complete path can't be adjusted
to support this. Or can it?

Kind regards
Uffe