Re: [PATCH v2 3/3] ACPI / PCI / PM: Rework acpi_pci_propagate_wakeup()

[Rafael J. Wysocki]

On Mon, Jul 31, 2017 at 11:59 PM, Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> On Fri, Jul 21, 2017 at 11:30:24PM +0200, Rafael J. Wysocki wrote:
>> From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>>
>> The acpi_pci_propagate_wakeup() routine is there to handle cases in
>> which PCI bridges (or PCIe ports) are expected to signal wakeup
>> for devices below them, but currently it doesn't do that correctly.
>>
>> The problem is that acpi_pci_propagate_wakeup() uses
>> acpi_pm_set_device_wakeup() for bridges and if that routine is
>> called for multiple times to disable wakeup for the same device,
>> it will disable it on the first invocation and the next calls
>> will have no effect (it works analogously when called to enable
>> wakeup, but that is not a problem).
>>
>> Now, say acpi_pci_propagate_wakeup() has been called for two
>> different devices under the same bridge and it has called
>> acpi_pm_set_device_wakeup() for that bridge each time.  The
>> bridge is now enabled to generate wakeup signals.  Next,
>> suppose that one of the devices below it resumes and
>> acpi_pci_propagate_wakeup() is called to disable wakeup for that
>> device.  It will then call acpi_pm_set_device_wakeup() for the bridge
>> and that will effectively disable remote wakeup for all devices under
>> it even though some of them may still be suspended and remote wakeup
>> may be expected to work for them.
>>
>> To address this (arguably theoretical) issue, allow
>> wakeup.enable_count under struct acpi_device to grow beyond 1 in
>> certain situations.  In particular, allow that to happen in
>> acpi_pci_propagate_wakeup() when wakeup is enabled or disabled
>> for PCI bridges, so that wakeup is actually disabled for the
>> bridge when all devices under it resume and not when just one
>> of them does that.
>>
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>
> Acked-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>

Thanks!

> But see questions below, stemming from my ignorance of ACPI PM.  I
> don't want to start a discussion and delay this.  If my questions
> don't suggest any useful changes, please ignore them.
>
>> ---
>>
>> -> v2: Rearrange checks in acpi_device_wakeup_enable() to reduce indentation
>>           level and possibly save some unnecessary checks for max_count == 1.
>>
>> ---
>>  drivers/acpi/device_pm.c |   46 +++++++++++++++++++++++++++++-----------------
>>  drivers/pci/pci-acpi.c   |    4 ++--
>>  include/acpi/acpi_bus.h  |   14 ++++++++++++--
>>  3 files changed, 43 insertions(+), 21 deletions(-)
>>
>> Index: linux-pm/drivers/acpi/device_pm.c
>> ===================================================================
>> --- linux-pm.orig/drivers/acpi/device_pm.c
>> +++ linux-pm/drivers/acpi/device_pm.c
>> @@ -682,19 +682,8 @@ static void acpi_pm_notify_work_func(str
>>
>>  static DEFINE_MUTEX(acpi_wakeup_lock);
>>
>> -/**
>> - * acpi_device_wakeup_enable - Enable wakeup functionality for device.
>> - * @adev: ACPI device to enable wakeup functionality for.
>> - * @target_state: State the system is transitioning into.
>> - *
>> - * Enable the GPE associated with @adev so that it can generate wakeup signals
>> - * for the device in response to external (remote) events and enable wakeup
>> - * power for it.
>> - *
>> - * Callers must ensure that @adev is a valid ACPI device node before executing
>> - * this function.
>> - */
>> -static int acpi_device_wakeup_enable(struct acpi_device *adev, u32 target_state)
>> +static int __acpi_device_wakeup_enable(struct acpi_device *adev,
>> +                                    u32 target_state, int max_count)
>
> It looks like @max_count is always either 1 or INT_MAX, so it's
> effectively a boolean.  Is there a way to interpret @max_count in
> terms of the PCI topology?  It's obviously related to
> wakeup->enable_count; does wakeup->enable_count basically mean "the
> number of subordinate devices that are enabled to generate wakeups"?

Yes, it does, for bridges.

> __acpi_pm_set_device_wakeup() is exported; maybe only to make the
> inlined callers in acpi_bus.h work?  It might be worth un-inlining
> them to avoid having to export __acpi_pm_set_device_wakeup().  I'm
> just thinking that exporting it makes it visible everywhere, and
> @max_count seems like an internal detail that's hard to document for
> use by non-core code.

I can do that.

> I guess you still have to export both acpi_pm_set_device_wakeup()
> (currently already exported) and acpi_pm_set_bridge_wakeup()
> (this patch effectively exports it by implementing it as an inline
> function).
>
> It'd be nice if we could distinguish the device case from the bridge
> case inside acpi_pm_set_device_wakeup() so we wouldn't have to rely on
> the caller using the correct one.  But I assume you already considered
> that and found it impractical.

That's correct.