Re: [PATCH v3 6/8] PM / ACPI: Enable the runtime PM centric approach for system sleep

[Ulf Hansson <ulf.hansson@linaro.org>]

On 29 August 2017 at 17:27, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Tuesday, August 29, 2017 4:56:48 PM CEST Ulf Hansson wrote:
>> This change enables the ACPI PM domain to cope with drivers that deploys
>> the runtime PM centric path for system sleep.
>
> [cut]
>
>> @@ -1052,11 +1066,20 @@ EXPORT_SYMBOL_GPL(acpi_subsys_complete);
>>   * @dev: Device to handle.
>>   *
>>   * Follow PCI and resume devices suspended at run time before running their
>> - * system suspend callbacks.
>> + * system suspend callbacks. However, try to avoid it in case the runtime PM
>> + * centric path is used for the device and then trust the driver to do the
>> + * right thing.
>>   */
>>  int acpi_subsys_suspend(struct device *dev)
>>  {
>> -     pm_runtime_resume(dev);
>> +     struct acpi_device *adev = ACPI_COMPANION(dev);
>> +
>> +     if (!adev)
>> +             return 0;
>> +
>> +     if (!dev_pm_is_rpm_sleep(dev) || acpi_dev_needs_resume(dev, adev))
>> +             pm_runtime_resume(dev);
>> +
>>       return pm_generic_suspend(dev);
>>  }
>>  EXPORT_SYMBOL_GPL(acpi_subsys_suspend);
>
> Well, I tried to avoid calling acpi_dev_needs_resume() for multiple times
> and that's why I added the update_state thing.
>
> Moreover, the is_rpm_sleep flag here has to mean not only that
> direct_complete should not be used with the device, but also that its driver
> is fine with not resuming it.

Let me try to explain this better. I realize the changelog is
misleading around this particular section! Huh, apologize for that!

First, patch1 makes the PM core treat the is_rpm_sleep flag as the
direct_complete isn't allowed for the device.

For that reason, when the is_rpm_sleep is set, there is no point
calling acpi_dev_needs_resume() from acpi_subsys_prepare(), but
instead that can be deferred to acpi_subsys_suspend() - because it
doesn't matter if acpi_subsys_prepare() returns 0 or 1, in either case
the acpi_subsys_suspend() will be called. That's really what goes on
here.

The end result is the same. If the acpi_dev_needs_resume() thinks that
the device needs to be runtime resumed, pm_runtime_resume() is called
for the device in acpi_subsys_suspend().

So, this has nothing to do with whether the driver "is fine with not
resuming it" thing.

>
> IMO it is not a good idea to use one flag for these two different things at the
> same time at all.

Yeah, I guess my upper comment addresses your immediate concern here?

However, there is one other thing the is_rpm_flag means. That is that
the driver has informed the ACPI PM domain, to trust the driver to
deal with system sleep, via re-using the runtime PM callbacks.
So the flag does still have two meanings, but that we can change - of course.

>
> Thanks,
> Rafael
>

Kind regards
Uffe