* [PATCH] PCI / ACPI / PM: Resume bridges w/o drivers on suspend-to-RAM @ 2018-06-26 10:06 Rafael J. Wysocki 2018-06-26 10:33 ` Mika Westerberg ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Rafael J. Wysocki @ 2018-06-26 10:06 UTC (permalink / raw) To: Linux PCI Cc: n0000b.n000b, Bjorn Helgaas, Mika Westerberg, LKML, Linux ACPI, Linux PM, Lukas Wunner From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> 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: n0000b.n000b@gmail.com Tested-by: n0000b.n000b@gmail.com Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> --- drivers/pci/pci-acpi.c | 8 ++++++++ 1 file changed, 8 insertions(+) Index: linux-pm/drivers/pci/pci-acpi.c =================================================================== --- linux-pm.orig/drivers/pci/pci-acpi.c +++ linux-pm/drivers/pci/pci-acpi.c @@ -638,6 +638,14 @@ static bool acpi_pci_need_resume(struct if (acpi_target_system_state() == ACPI_STATE_S0) return false; + /* + * 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) + return true; + return !!adev->power.flags.dsw_present; } ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] PCI / ACPI / PM: Resume bridges w/o drivers on suspend-to-RAM 2018-06-26 10:06 [PATCH] PCI / ACPI / PM: Resume bridges w/o drivers on suspend-to-RAM Rafael J. Wysocki @ 2018-06-26 10:33 ` Mika Westerberg 2018-06-26 14:01 ` Bjorn Helgaas 2018-06-29 8:34 ` [PATCH v2] " Rafael J. Wysocki 2 siblings, 0 replies; 11+ messages in thread From: Mika Westerberg @ 2018-06-26 10:33 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Linux PCI, n0000b.n000b, Bjorn Helgaas, LKML, Linux ACPI, Linux PM, Lukas Wunner On Tue, Jun 26, 2018 at 12:06:01PM +0200, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > 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: n0000b.n000b@gmail.com > Tested-by: n0000b.n000b@gmail.com > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] PCI / ACPI / PM: Resume bridges w/o drivers on suspend-to-RAM 2018-06-26 10:06 [PATCH] PCI / ACPI / PM: Resume bridges w/o drivers on suspend-to-RAM Rafael J. Wysocki 2018-06-26 10:33 ` Mika Westerberg @ 2018-06-26 14:01 ` Bjorn Helgaas 2018-06-26 14:22 ` Rafael J. Wysocki 2018-06-29 8:34 ` [PATCH v2] " Rafael J. Wysocki 2 siblings, 1 reply; 11+ messages in thread From: Bjorn Helgaas @ 2018-06-26 14:01 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Linux PCI, n0000b.n000b, Bjorn Helgaas, Mika Westerberg, LKML, Linux ACPI, Linux PM, Lukas Wunner On Tue, Jun 26, 2018 at 12:06:01PM +0200, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > 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 "stay in D3 over system suspend"? (just to be explicit about what "in suspend" means) > 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: n0000b.n000b@gmail.com > Tested-by: n0000b.n000b@gmail.com > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/pci/pci-acpi.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > Index: linux-pm/drivers/pci/pci-acpi.c > =================================================================== > --- linux-pm.orig/drivers/pci/pci-acpi.c > +++ linux-pm/drivers/pci/pci-acpi.c > @@ -638,6 +638,14 @@ static bool acpi_pci_need_resume(struct > if (acpi_target_system_state() == ACPI_STATE_S0) > return false; > > + /* > + * 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) > + return true; It sounds like the question of whether leaving a bridge in D3 confuses the firmware has a platform-specific answer. How does the driver PM handling know how to do the right thing? Does it need to know whether it's safe to put the device in D3? Or maybe a device is never put in D3 for system suspend if it has a driver? But I'm just kibbitzing; since you merged c62ec4610c40, I'm happy if you also merge this: Acked-by: Bjorn Helgaas <bhelgaas@google.com> > return !!adev->power.flags.dsw_present; > } > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] PCI / ACPI / PM: Resume bridges w/o drivers on suspend-to-RAM 2018-06-26 14:01 ` Bjorn Helgaas @ 2018-06-26 14:22 ` Rafael J. Wysocki 2018-06-26 17:14 ` Bjorn Helgaas 0 siblings, 1 reply; 11+ messages in thread From: Rafael J. Wysocki @ 2018-06-26 14:22 UTC (permalink / raw) To: Bjorn Helgaas Cc: Rafael J. Wysocki, Linux PCI, n0000b.n000b, Bjorn Helgaas, Mika Westerberg, LKML, Linux ACPI, Linux PM, Lukas Wunner On Tue, Jun 26, 2018 at 4:01 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: > On Tue, Jun 26, 2018 at 12:06:01PM +0200, Rafael J. Wysocki wrote: >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> >> 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 > > "stay in D3 over system suspend"? (just to be explicit about what "in > suspend" means) OK >> 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: n0000b.n000b@gmail.com >> Tested-by: n0000b.n000b@gmail.com >> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> --- >> drivers/pci/pci-acpi.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> Index: linux-pm/drivers/pci/pci-acpi.c >> =================================================================== >> --- linux-pm.orig/drivers/pci/pci-acpi.c >> +++ linux-pm/drivers/pci/pci-acpi.c >> @@ -638,6 +638,14 @@ static bool acpi_pci_need_resume(struct >> if (acpi_target_system_state() == ACPI_STATE_S0) >> return false; >> >> + /* >> + * 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) >> + return true; > > It sounds like the question of whether leaving a bridge in D3 confuses > the firmware has a platform-specific answer. Well, it may confuse the platform firmware in general. > How does the driver PM handling know how to do the right thing? For endpoints this is not an issue as they always have been expected to be in D3 before passing control to the platform firmware on S3 entry, but we've never done that for bridges by default, except for PCIe ports with PM enabled (in which case the driver decides whether or not to enable it). > Does it need to know whether > it's safe to put the device in D3? Or maybe a device is never put in > D3 for system suspend if it has a driver? As per the above, endpoints should be OK (or they are quirky if not), but bridges are sort of a gray area. PCIe ports for which we enable PM should be fine, for the rest it's better to be conservative and resume them IMO. > But I'm just kibbitzing; since you merged c62ec4610c40, I'm happy if > you also merge this: > > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > >> return !!adev->power.flags.dsw_present; >> } Thanks! ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] PCI / ACPI / PM: Resume bridges w/o drivers on suspend-to-RAM 2018-06-26 14:22 ` Rafael J. Wysocki @ 2018-06-26 17:14 ` Bjorn Helgaas 2018-06-26 17:19 ` Rafael J. Wysocki 0 siblings, 1 reply; 11+ messages in thread From: Bjorn Helgaas @ 2018-06-26 17:14 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Rafael J. Wysocki, Linux PCI, n0000b.n000b, Bjorn Helgaas, Mika Westerberg, LKML, Linux ACPI, Linux PM, Lukas Wunner On Tue, Jun 26, 2018 at 04:22:00PM +0200, Rafael J. Wysocki wrote: > On Tue, Jun 26, 2018 at 4:01 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Tue, Jun 26, 2018 at 12:06:01PM +0200, Rafael J. Wysocki wrote: > >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > >> + /* > >> + * 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) > >> + return true; > > > > It sounds like the question of whether leaving a bridge in D3 confuses > > the firmware has a platform-specific answer. > > Well, it may confuse the platform firmware in general. > > > How does the driver PM handling know how to do the right thing? > > For endpoints this is not an issue as they always have been expected > to be in D3 before passing control to the platform firmware on S3 > entry, but we've never done that for bridges by default, except for > PCIe ports with PM enabled (in which case the driver decides whether > or not to enable it). If there's any spec reference for the expected power states of devices when entering S3, that would be useful here. I can't tell if there's any guidance for this or if it's just figured out experimentally. > > Does it need to know whether > > it's safe to put the device in D3? Or maybe a device is never put in > > D3 for system suspend if it has a driver? > > As per the above, endpoints should be OK (or they are quirky if not), > but bridges are sort of a gray area. PCIe ports for which we enable > PM should be fine, for the rest it's better to be conservative and > resume them IMO. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] PCI / ACPI / PM: Resume bridges w/o drivers on suspend-to-RAM 2018-06-26 17:14 ` Bjorn Helgaas @ 2018-06-26 17:19 ` Rafael J. Wysocki 2018-06-26 20:32 ` Bjorn Helgaas 0 siblings, 1 reply; 11+ messages in thread From: Rafael J. Wysocki @ 2018-06-26 17:19 UTC (permalink / raw) To: Bjorn Helgaas Cc: Rafael J. Wysocki, Rafael J. Wysocki, Linux PCI, n0000b.n000b, Bjorn Helgaas, Mika Westerberg, LKML, Linux ACPI, Linux PM, Lukas Wunner On Tue, Jun 26, 2018 at 7:14 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: > On Tue, Jun 26, 2018 at 04:22:00PM +0200, Rafael J. Wysocki wrote: >> On Tue, Jun 26, 2018 at 4:01 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: >> > On Tue, Jun 26, 2018 at 12:06:01PM +0200, Rafael J. Wysocki wrote: >> >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > >> >> + /* >> >> + * 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) >> >> + return true; >> > >> > It sounds like the question of whether leaving a bridge in D3 confuses >> > the firmware has a platform-specific answer. >> >> Well, it may confuse the platform firmware in general. >> >> > How does the driver PM handling know how to do the right thing? >> >> For endpoints this is not an issue as they always have been expected >> to be in D3 before passing control to the platform firmware on S3 >> entry, but we've never done that for bridges by default, except for >> PCIe ports with PM enabled (in which case the driver decides whether >> or not to enable it). > > If there's any spec reference for the expected power states of devices > when entering S3, that would be useful here. I can't tell if there's > any guidance for this or if it's just figured out experimentally. It is not direct, but Section 16.1.6 of ACPI 6.2 says this in step 4 of the system suspend outline: OSPM places all device drivers into their respective Dx state. If the device is enabled for wake, it enters the Dx state associated with the wake capability. If the device is not enabled to wake the system, it enters the D3 state. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] PCI / ACPI / PM: Resume bridges w/o drivers on suspend-to-RAM 2018-06-26 17:19 ` Rafael J. Wysocki @ 2018-06-26 20:32 ` Bjorn Helgaas 2018-06-27 7:16 ` Rafael J. Wysocki 0 siblings, 1 reply; 11+ messages in thread From: Bjorn Helgaas @ 2018-06-26 20:32 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Rafael J. Wysocki, Linux PCI, n0000b.n000b, Bjorn Helgaas, Mika Westerberg, LKML, Linux ACPI, Linux PM, Lukas Wunner On Tue, Jun 26, 2018 at 07:19:29PM +0200, Rafael J. Wysocki wrote: > On Tue, Jun 26, 2018 at 7:14 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Tue, Jun 26, 2018 at 04:22:00PM +0200, Rafael J. Wysocki wrote: > >> On Tue, Jun 26, 2018 at 4:01 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: > >> > On Tue, Jun 26, 2018 at 12:06:01PM +0200, Rafael J. Wysocki wrote: > >> >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > >> >> + /* > >> >> + * 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) > >> >> + return true; > >> > > >> > It sounds like the question of whether leaving a bridge in D3 confuses > >> > the firmware has a platform-specific answer. > >> > >> Well, it may confuse the platform firmware in general. > >> > >> > How does the driver PM handling know how to do the right thing? > >> > >> For endpoints this is not an issue as they always have been expected > >> to be in D3 before passing control to the platform firmware on S3 > >> entry, but we've never done that for bridges by default, except for > >> PCIe ports with PM enabled (in which case the driver decides whether > >> or not to enable it). > > > > If there's any spec reference for the expected power states of devices > > when entering S3, that would be useful here. I can't tell if there's > > any guidance for this or if it's just figured out experimentally. > > It is not direct, but Section 16.1.6 of ACPI 6.2 says this in step 4 > of the system suspend outline: > > OSPM places all device drivers into their respective Dx state. If the > device is enabled for wake, > it enters the Dx state associated with the wake capability. If the > device is not enabled to wake > the system, it enters the D3 state. Thanks, that's a very useful citation! Bjorn ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] PCI / ACPI / PM: Resume bridges w/o drivers on suspend-to-RAM 2018-06-26 20:32 ` Bjorn Helgaas @ 2018-06-27 7:16 ` Rafael J. Wysocki 0 siblings, 0 replies; 11+ messages in thread From: Rafael J. Wysocki @ 2018-06-27 7:16 UTC (permalink / raw) To: Bjorn Helgaas Cc: Rafael J. Wysocki, Rafael J. Wysocki, Linux PCI, n0000b.n000b, Bjorn Helgaas, Mika Westerberg, LKML, Linux ACPI, Linux PM, Lukas Wunner On Tue, Jun 26, 2018 at 10:32 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: > On Tue, Jun 26, 2018 at 07:19:29PM +0200, Rafael J. Wysocki wrote: >> On Tue, Jun 26, 2018 at 7:14 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: >> > On Tue, Jun 26, 2018 at 04:22:00PM +0200, Rafael J. Wysocki wrote: >> >> On Tue, Jun 26, 2018 at 4:01 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: >> >> > On Tue, Jun 26, 2018 at 12:06:01PM +0200, Rafael J. Wysocki wrote: >> >> >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> > >> >> >> + /* >> >> >> + * 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) >> >> >> + return true; >> >> > >> >> > It sounds like the question of whether leaving a bridge in D3 confuses >> >> > the firmware has a platform-specific answer. >> >> >> >> Well, it may confuse the platform firmware in general. >> >> >> >> > How does the driver PM handling know how to do the right thing? >> >> >> >> For endpoints this is not an issue as they always have been expected >> >> to be in D3 before passing control to the platform firmware on S3 >> >> entry, but we've never done that for bridges by default, except for >> >> PCIe ports with PM enabled (in which case the driver decides whether >> >> or not to enable it). >> > >> > If there's any spec reference for the expected power states of devices >> > when entering S3, that would be useful here. I can't tell if there's >> > any guidance for this or if it's just figured out experimentally. >> >> It is not direct, but Section 16.1.6 of ACPI 6.2 says this in step 4 >> of the system suspend outline: >> >> OSPM places all device drivers into their respective Dx state. If the >> device is enabled for wake, >> it enters the Dx state associated with the wake capability. If the >> device is not enabled to wake >> the system, it enters the D3 state. > > Thanks, that's a very useful citation! You're welcome! :-) I actually need to withdraw the $subject patch as it doesn't help (I misread the reporters response) and the one that does work is a bit too intrusive IMO, so I'll investigate this a bit more. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] PCI / ACPI / PM: Resume bridges w/o drivers on suspend-to-RAM 2018-06-26 10:06 [PATCH] PCI / ACPI / PM: Resume bridges w/o drivers on suspend-to-RAM Rafael J. Wysocki 2018-06-26 10:33 ` Mika Westerberg 2018-06-26 14:01 ` Bjorn Helgaas @ 2018-06-29 8:34 ` Rafael J. Wysocki 2018-06-29 19:19 ` Bjorn Helgaas 2 siblings, 1 reply; 11+ messages in thread From: Rafael J. Wysocki @ 2018-06-29 8:34 UTC (permalink / raw) To: Linux PCI, Bjorn Helgaas, Mika Westerberg Cc: n0000b.n000b, LKML, Linux ACPI, Linux PM, Lukas Wunner From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> 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: n0000b.n000b@gmail.com Tested-by: n0000b.n000b@gmail.com Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> Acked-by: Bjorn Helgaas <bhelgaas@google.com> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> --- -> 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(+) 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; ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] PCI / ACPI / PM: Resume bridges w/o drivers on suspend-to-RAM 2018-06-29 8:34 ` [PATCH v2] " Rafael J. Wysocki @ 2018-06-29 19:19 ` Bjorn Helgaas 2018-06-30 21:15 ` Rafael J. Wysocki 0 siblings, 1 reply; 11+ messages in thread From: Bjorn Helgaas @ 2018-06-29 19:19 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Linux PCI, Bjorn Helgaas, Mika Westerberg, n0000b.n000b, LKML, Linux ACPI, Linux PM, Lukas Wunner On Fri, Jun 29, 2018 at 10:34:40AM +0200, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > 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: n0000b.n000b@gmail.com > Tested-by: n0000b.n000b@gmail.com > Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > > -> 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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] PCI / ACPI / PM: Resume bridges w/o drivers on suspend-to-RAM 2018-06-29 19:19 ` Bjorn Helgaas @ 2018-06-30 21:15 ` Rafael J. Wysocki 0 siblings, 0 replies; 11+ messages in thread From: Rafael J. Wysocki @ 2018-06-30 21:15 UTC (permalink / raw) To: Bjorn Helgaas Cc: Rafael J. Wysocki, Linux PCI, Bjorn Helgaas, Mika Westerberg, n0000b.n000b, LKML, Linux ACPI, Linux PM, Lukas Wunner On Fri, Jun 29, 2018 at 9:19 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: > On Fri, Jun 29, 2018 at 10:34:40AM +0200, Rafael J. Wysocki wrote: >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> >> 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: n0000b.n000b@gmail.com >> Tested-by: n0000b.n000b@gmail.com >> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> >> Acked-by: Bjorn Helgaas <bhelgaas@google.com> >> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> --- >> >> -> 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-06-30 21:15 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-06-26 10:06 [PATCH] PCI / ACPI / PM: Resume bridges w/o drivers on suspend-to-RAM Rafael J. Wysocki 2018-06-26 10:33 ` Mika Westerberg 2018-06-26 14:01 ` Bjorn Helgaas 2018-06-26 14:22 ` Rafael J. Wysocki 2018-06-26 17:14 ` Bjorn Helgaas 2018-06-26 17:19 ` Rafael J. Wysocki 2018-06-26 20:32 ` Bjorn Helgaas 2018-06-27 7:16 ` Rafael J. Wysocki 2018-06-29 8:34 ` [PATCH v2] " Rafael J. Wysocki 2018-06-29 19:19 ` Bjorn Helgaas 2018-06-30 21:15 ` Rafael J. Wysocki
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).