[v2] PCI / ACPI / PM: Resume bridges w/o drivers on suspend-to-RAM

Commit Message

Rafael J. Wysocki June 29, 2018, 8:34 a.m. UTC

From: Rafael J. Wysocki <[e-mail]>

It is reported that commit c62ec4610c40 (PM / core: Fix direct_complete
handling for devices with no callbacks) introduced a system suspend
regression on Samsung 305V4A by allowing a PCI bridge (not a PCIe
port) to stay in D3 over suspend-to-RAM, which is a side effect of
setting power.direct_complete for the children of that bridge that
have no PM callbacks.

On the majority of systems PCI bridges are not allowed to be
runtime-suspended (the power/control sysfs attribute is set to "on"
for them by default), but user space can change that setting and if
it does so and a given bridge has no children with PM callbacks, the
direct_complete optimization will be applied to it and it will stay
in suspend over system suspend.  Apparently, that confuses the
platform firmware on the affected machine and that may very well
happen elsewhere, so avoid the direct_complete optimization for
PCI bridges with no drivers (if there is a driver, it should take
care of the PM handling) on suspend-to-RAM altogether (that should
not matter for suspend-to-idle as platform firmware is not involved
in it).

Fixes: c62ec4610c40 (PM / core: Fix direct_complete handling for devices with no callbacks)
Link: https://bugzilla.kernel.org/show_bug.cgi?id=199941
Reported-by: [e-mail]
Tested-by: [e-mail]
Reviewed-by: Mika Westerberg <[e-mail]>
Acked-by: Bjorn Helgaas <[e-mail]>
Signed-off-by: Rafael J. Wysocki <[e-mail]>
---

-> v2: Make the patch work for PCI bridges without ACPI companion objects.

Since that doesn't really change the idea etc, but only makes the check
to be made earlier, I've added the Reviewed-by and Acked-by tags given to
the previous version.  If that's not right, please let me know.

---
 drivers/pci/pci-acpi.c |    9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Bjorn Helgaas June 29, 2018, 7:19 p.m. UTC | #1

On Fri, Jun 29, 2018 at 10:34:40AM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[e-mail]>
> 
> It is reported that commit c62ec4610c40 (PM / core: Fix direct_complete
> handling for devices with no callbacks) introduced a system suspend
> regression on Samsung 305V4A by allowing a PCI bridge (not a PCIe
> port) to stay in D3 over suspend-to-RAM, which is a side effect of
> setting power.direct_complete for the children of that bridge that
> have no PM callbacks.
> 
> On the majority of systems PCI bridges are not allowed to be
> runtime-suspended (the power/control sysfs attribute is set to "on"
> for them by default), but user space can change that setting and if
> it does so and a given bridge has no children with PM callbacks, the
> direct_complete optimization will be applied to it and it will stay
> in suspend over system suspend.  Apparently, that confuses the
> platform firmware on the affected machine and that may very well
> happen elsewhere, so avoid the direct_complete optimization for
> PCI bridges with no drivers (if there is a driver, it should take
> care of the PM handling) on suspend-to-RAM altogether (that should
> not matter for suspend-to-idle as platform firmware is not involved
> in it).
> 
> Fixes: c62ec4610c40 (PM / core: Fix direct_complete handling for devices with no callbacks)
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=199941
> Reported-by: [e-mail]
> Tested-by: [e-mail]
> Reviewed-by: Mika Westerberg <[e-mail]>
> Acked-by: Bjorn Helgaas <[e-mail]>
> Signed-off-by: Rafael J. Wysocki <[e-mail]>
> ---
> 
> -> v2: Make the patch work for PCI bridges without ACPI companion objects.
> 
> Since that doesn't really change the idea etc, but only makes the check
> to be made earlier, I've added the Reviewed-by and Acked-by tags given to
> the previous version.  If that's not right, please let me know.

Keeping my ack is fine, and I assume you'll apply this.

It would be nice to see that ACPI spec reference (the spec version and
section number, not necessarily all the text) in the changelog or
maybe even a comment in the code.

> ---
>  drivers/pci/pci-acpi.c |    9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> Index: linux-pm/drivers/pci/pci-acpi.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/pci-acpi.c
> +++ linux-pm/drivers/pci/pci-acpi.c
> @@ -629,6 +629,15 @@ static bool acpi_pci_need_resume(struct
>  {
>  	struct acpi_device *adev = ACPI_COMPANION(&dev->dev);
>  
> +	/*
> +	 * In some cases (eg. Samsung 305V4A) leaving a bridge in suspend
> +	 * confuses the platform firmware, so avoid doing that, unless the
> +	 * bridge has a driver that should take care of PM handling.
> +	 */
> +	if (pci_is_bridge(dev) && !dev->driver &&
> +	    acpi_target_system_state() != ACPI_STATE_S0)
> +		return true;
> +
>  	if (!adev || !acpi_device_power_manageable(adev))
>  		return false;
>  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Rafael J. Wysocki June 30, 2018, 9:15 p.m. UTC | #2

On Fri, Jun 29, 2018 at 9:19 PM, Bjorn Helgaas <[e-mail]> wrote:
> On Fri, Jun 29, 2018 at 10:34:40AM +0200, Rafael J. Wysocki wrote:
>> From: Rafael J. Wysocki <[e-mail]>
>>
>> It is reported that commit c62ec4610c40 (PM / core: Fix direct_complete
>> handling for devices with no callbacks) introduced a system suspend
>> regression on Samsung 305V4A by allowing a PCI bridge (not a PCIe
>> port) to stay in D3 over suspend-to-RAM, which is a side effect of
>> setting power.direct_complete for the children of that bridge that
>> have no PM callbacks.
>>
>> On the majority of systems PCI bridges are not allowed to be
>> runtime-suspended (the power/control sysfs attribute is set to "on"
>> for them by default), but user space can change that setting and if
>> it does so and a given bridge has no children with PM callbacks, the
>> direct_complete optimization will be applied to it and it will stay
>> in suspend over system suspend.  Apparently, that confuses the
>> platform firmware on the affected machine and that may very well
>> happen elsewhere, so avoid the direct_complete optimization for
>> PCI bridges with no drivers (if there is a driver, it should take
>> care of the PM handling) on suspend-to-RAM altogether (that should
>> not matter for suspend-to-idle as platform firmware is not involved
>> in it).
>>
>> Fixes: c62ec4610c40 (PM / core: Fix direct_complete handling for devices with no callbacks)
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=199941
>> Reported-by: [e-mail]
>> Tested-by: [e-mail]
>> Reviewed-by: Mika Westerberg <[e-mail]>
>> Acked-by: Bjorn Helgaas <[e-mail]>
>> Signed-off-by: Rafael J. Wysocki <[e-mail]>
>> ---
>>
>> -> v2: Make the patch work for PCI bridges without ACPI companion objects.
>>
>> Since that doesn't really change the idea etc, but only makes the check
>> to be made earlier, I've added the Reviewed-by and Acked-by tags given to
>> the previous version.  If that's not right, please let me know.
>
> Keeping my ack is fine, and I assume you'll apply this.

I will, thanks!

> It would be nice to see that ACPI spec reference (the spec version and
> section number, not necessarily all the text) in the changelog or
> maybe even a comment in the code.

OK

Patch

Index: linux-pm/drivers/pci/pci-acpi.c
===================================================================
--- linux-pm.orig/drivers/pci/pci-acpi.c
+++ linux-pm/drivers/pci/pci-acpi.c
@@ -629,6 +629,15 @@  static bool acpi_pci_need_resume(struct
 {
 	struct acpi_device *adev = ACPI_COMPANION(&dev->dev);
 
+	/*
+	 * In some cases (eg. Samsung 305V4A) leaving a bridge in suspend
+	 * confuses the platform firmware, so avoid doing that, unless the
+	 * bridge has a driver that should take care of PM handling.
+	 */
+	if (pci_is_bridge(dev) && !dev->driver &&
+	    acpi_target_system_state() != ACPI_STATE_S0)
+		return true;
+
 	if (!adev || !acpi_device_power_manageable(adev))
 		return false;