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

[Rafael J. Wysocki]

On Thu, Jun 22, 2017 at 11:29 PM, Ulf Hansson <ulf.hansson at linaro.org> wrote:
> 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?
>
> :-)

no_direct_complete is better than that IMO.

>>
>> 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.

I see.

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

Anyway, it would be good to have a way to verify that the driver is
doing the right thing or we'll have bugs in that area and people
wondering what's up.

Thanks,
Rafael