[PATCH 6/9] PM / ACPI: Provide option to disable direct_complete for ACPI devices

[Ulf Hansson]

On 22 June 2017 at 16:38, Rafael J. Wysocki <rjw at rjwysocki.net> wrote:
> On Thursday, June 22, 2017 11:35:35 AM Ulf Hansson wrote:
>> On 21 June 2017 at 23:42, Rafael J. Wysocki <rafael at kernel.org> wrote:
>> > On Wed, Jun 21, 2017 at 9:21 PM, Ulf Hansson <ulf.hansson at 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.
>> >>
>> >> 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.
>> >>
>> >> Typically a driver should call acpi_dev_disable_direct_comlete() during
>> >> ->probe() and acpi_dev_enable_direct_complete() in ->remove().
>> >>
>> >> Signed-off-by: Ulf Hansson <ulf.hansson at linaro.org>
>> >> ---
>> >>  drivers/acpi/device_pm.c | 37 ++++++++++++++++++++++++++++++++++++-
>> >>  include/acpi/acpi_bus.h  |  1 +
>> >>  include/linux/acpi.h     |  4 ++++
>> >>  3 files changed, 41 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
>> >> index ee51e75..2393a1a 100644
>> >> --- a/drivers/acpi/device_pm.c
>> >> +++ b/drivers/acpi/device_pm.c
>> >> @@ -879,6 +879,41 @@ EXPORT_SYMBOL_GPL(acpi_subsys_runtime_resume);
>> >>
>> >>  #ifdef CONFIG_PM_SLEEP
>> >>  /**
>> >> + * acpi_dev_disable_direct_complete - Disable the direct_complete path for ACPI.
>> >> + * @dev: Device to disable the path for.
>> >> + *
>> >> + * Per default the ACPI PM domain tries to use the direct_complete path for its
>> >> + * devices during system sleep. This function allows a user, typically a driver
>> >> + * during probe, to disable the direct_complete path from being used by ACPI.
>> >> + */
>> >> +void acpi_dev_disable_direct_complete(struct device *dev)
>> >> +{
>> >> +       struct acpi_device *adev = ACPI_COMPANION(dev);
>> >> +
>> >> +       if (adev)
>> >> +               adev->no_direct_complete = true;
>> >
>> > We have an analogous flag in PCI now and it is called needs_resume, so
>> > it would be good to be consistent with that.
>>
>> I was trying to come up with a nice name, however I couldn't find
>> anything better than no_direct_complete.
>>
>> However as the next patch somewhat, extends the flag to also be use
>> for the runtime PM centric path, I should perhaps choose something
>> more related to that?
>>
>> I think make "needs_resume" is going to e bit confusing, especially
>> while extending the usage for the flag it in the next patch, no?
>
> OK, fair enough, but I still think the name isn't really nice. :-)

How about use_rpm_centric_sleep or use_rpm_ops_for_sleep?

:-)

>
> In fact, I'm not sure if the new flag is necessary at all.
>
> If the driver is expected to use pm_runtime_force_suspend|resume() along with
> it every time, why not to make the ACPI PM domain code check if the driver's
> callback pointers point to pm_runtime_force_suspend|resume() and work
> accordingly?

I get the idea, however it would limit the driver from doing other
specific things from its sleep callbacks.

It may for example deal with wakeups during system suspend, and when
that is done, it can call pm_runtime_force_suspend() from that
callback.

Potentially that could be done from the runtime PM callback itself,
(via checking pm_runtime_enabled)...

Kind regards
Uffe