On Friday, May 31, 2019 11:16:48 PM CEST Bjorn Helgaas wrote:
> On Fri, May 31, 2019 at 11:49:30AM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> >
> > The current code resumes devices in D3hot during system suspend if
> > the target power state for them is D3cold, but that is not necessary
> > in general. It only is necessary to do that if the platform firmware
> > requires the device to be resumed, but that should be covered by
> > the platform_pci_need_resume() check anyway, so rework
> > pci_dev_keep_suspended() to avoid returning 'false' for devices
> > in D3hot which need not be resumed due to platform firmware
> > requirements.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > ---
> > drivers/pci/pci.c | 15 ++++++++++++---
> > 1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > Index: linux-pm/drivers/pci/pci.c
> > ===================================================================
> > --- linux-pm.orig/drivers/pci/pci.c
> > +++ linux-pm/drivers/pci/pci.c
> > @@ -2474,10 +2474,19 @@ bool pci_dev_keep_suspended(struct pci_d
> > {
> > struct device *dev = &pci_dev->dev;
> > bool wakeup = device_may_wakeup(dev);
> > + pci_power_t target_state;
> >
> > - if (!pm_runtime_suspended(dev)
> > - || pci_target_state(pci_dev, wakeup) != pci_dev->current_state
> > - || platform_pci_need_resume(pci_dev))
> > + if (!pm_runtime_suspended(dev) || platform_pci_need_resume(pci_dev))
> > + return false;
> > +
> > + target_state = pci_target_state(pci_dev, wakeup);
>
> Nit, add a blank line here.
OK
> > + /*
> > + * If the earlier platform check has not triggered, D3cold is just power
> > + * removal on top of D3hot, so no need to resume the device in that
> > + * case.
> > + */
> > + if (target_state != pci_dev->current_state &&
> > + target_state != PCI_D3cold && pci_dev->current_state != PCI_D3hot)
> > return false;
>
> This is more a comment on the existing code than on this particular
> patch, but I find this whole function hard to understand, and I think
> one reason is that there are a lot of negative conditions, both in
> this function and in its callers. This "target_state != ... &&
> target_state != ... && current_state != ..." is one example. Another
> is the function name itself. It might be easier to read as something
> like this:
>
> bool pci_dev_need_resume(...)
> {
> if (!pm_runtime_suspended(...))
> return true;
>
> if (platform_pci_need_resume(...))
> return true;
>
> if (target_state != current_state)
> return true;
Please see the appended (untested) patch on top of the $subject one.
> ...
>
> Another reason I think it's hard to read is that
> "pci_dev_keep_suspended" suggests that this is a pure boolean function
> without side-effects, but in fact it also fiddles with the PME state
> in some cases. I don't have any ideas for that part.
Well, I can only propose to put the PME adjustment part into a separate function like
in the patch below.
---
drivers/pci/pci-driver.c | 21 ++++++++++++++++---
drivers/pci/pci.c | 50 ++++++++++++++++++++++++-----------------------
drivers/pci/pci.h | 3 +-
3 files changed, 46 insertions(+), 28 deletions(-)
Index: linux-pm/drivers/pci/pci.c
===================================================================
--- linux-pm.orig/drivers/pci/pci.c
+++ linux-pm/drivers/pci/pci.c
@@ -2459,54 +2459,56 @@ bool pci_dev_run_wake(struct pci_dev *de
EXPORT_SYMBOL_GPL(pci_dev_run_wake);
/**
- * pci_dev_keep_suspended - Check if the device can stay in the suspended state.
+ * pci_dev_need_resume - Check if the device can stay in the suspended state.
* @pci_dev: Device to check.
*
- * Return 'true' if the device is runtime-suspended, it doesn't have to be
+ * Return 'false' if the device is runtime-suspended, it doesn't have to be
* reconfigured due to wakeup settings difference between system and runtime
* suspend and the current power state of it is suitable for the upcoming
* (system) transition.
- *
- * If the device is not configured for system wakeup, disable PME for it before
- * returning 'true' to prevent it from waking up the system unnecessarily.
*/
-bool pci_dev_keep_suspended(struct pci_dev *pci_dev)
+bool pci_dev_need_resume(struct pci_dev *pci_dev)
{
struct device *dev = &pci_dev->dev;
- bool wakeup = device_may_wakeup(dev);
pci_power_t target_state;
if (!pm_runtime_suspended(dev) || platform_pci_need_resume(pci_dev))
- return false;
+ return true;
- target_state = pci_target_state(pci_dev, wakeup);
+ target_state = pci_target_state(pci_dev, device_may_wakeup(dev));
/*
* If the earlier platform check has not triggered, D3cold is just power
* removal on top of D3hot, so no need to resume the device in that
* case.
*/
- if (target_state != pci_dev->current_state &&
- target_state != PCI_D3cold && pci_dev->current_state != PCI_D3hot)
- return false;
+ return target_state != pci_dev->current_state &&
+ target_state != PCI_D3cold &&
+ pci_dev->current_state != PCI_D3hot;
+}
+
+/**
+ * pci_dev_adjust_pme - Adjust PME setting for a suspended device.
+ * @pci_dev: Device to check.
+ *
+ * If the device is not configured for system wakeup, disable PME for it to
+ * prevent it from waking up the system unnecessarily.
+ */
+void pci_dev_adjust_pme(struct pci_dev *pci_dev)
+{
+ struct device *dev = &pci_dev->dev;
- /*
- * At this point the device is good to go unless it's been configured
- * to generate PME at the runtime suspend time, but it is not supposed
- * to wake up the system. In that case, simply disable PME for it
- * (it will have to be re-enabled on exit from system resume).
- *
- * If the device's power state is D3cold and the platform check above
- * hasn't triggered, the device's configuration is suitable and we don't
- * need to manipulate it at all.
- */
spin_lock_irq(&dev->power.lock);
+ /*
+ * If the device's power state is D3cold and the platform check in
+ * pci_dev_need_resume() hasn't triggered, the device's configuration is
+ * suitable and it need not be touched.
+ */
if (pm_runtime_suspended(dev) && pci_dev->current_state < PCI_D3cold &&
- !wakeup)
+ !device_may_wakeup(dev))
__pci_pme_active(pci_dev, false);
spin_unlock_irq(&dev->power.lock);
- return true;
}
/**
Index: linux-pm/drivers/pci/pci-driver.c
===================================================================
--- linux-pm.orig/drivers/pci/pci-driver.c
+++ linux-pm/drivers/pci/pci-driver.c
@@ -679,6 +679,7 @@ static bool pci_has_legacy_pm_support(st
static int pci_pm_prepare(struct device *dev)
{
struct device_driver *drv = dev->driver;
+ struct pci_dev *pci_dev = to_pci_dev(dev);
if (drv && drv->pm && drv->pm->prepare) {
int error = drv->pm->prepare(dev);
@@ -688,7 +689,15 @@ static int pci_pm_prepare(struct device
if (!error && dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_PREPARE))
return 0;
}
- return pci_dev_keep_suspended(to_pci_dev(dev));
+ if (pci_dev_need_resume(pci_dev))
+ return 0;
+
+ /*
+ * The PME setting needs to be adjusted here in case the direct-complete
+ * optimization is used with respect to this device.
+ */
+ pci_dev_adjust_pme(pci_dev);
+ return 1;
}
static void pci_pm_complete(struct device *dev)
@@ -758,9 +767,11 @@ static int pci_pm_suspend(struct device
* better to resume the device from runtime suspend here.
*/
if (!dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND) ||
- !pci_dev_keep_suspended(pci_dev)) {
+ pci_dev_need_resume(pci_dev)) {
pm_runtime_resume(dev);
pci_dev->state_saved = false;
+ } else {
+ pci_dev_adjust_pme(pci_dev);
}
if (pm->suspend) {
@@ -1108,8 +1119,12 @@ static int pci_pm_poweroff(struct device
/* The reason to do that is the same as in pci_pm_suspend(). */
if (!dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND) ||
- !pci_dev_keep_suspended(pci_dev))
+ pci_dev_need_resume(pci_dev)) {
pm_runtime_resume(dev);
+ pci_dev->state_saved = false;
+ } else {
+ pci_dev_adjust_pme(pci_dev);
+ }
pci_dev->state_saved = false;
if (pm->poweroff) {
Index: linux-pm/drivers/pci/pci.h
===================================================================
--- linux-pm.orig/drivers/pci/pci.h
+++ linux-pm/drivers/pci/pci.h
@@ -82,7 +82,8 @@ int pci_finish_runtime_suspend(struct pc
void pcie_clear_root_pme_status(struct pci_dev *dev);
int __pci_pme_wakeup(struct pci_dev *dev, void *ign);
void pci_pme_restore(struct pci_dev *dev);
-bool pci_dev_keep_suspended(struct pci_dev *dev);
+bool pci_dev_need_resume(struct pci_dev *dev);
+void pci_dev_adjust_pme(struct pci_dev *dev);
void pci_dev_complete_resume(struct pci_dev *pci_dev);
void pci_config_pm_runtime_get(struct pci_dev *dev);
void pci_config_pm_runtime_put(struct pci_dev *dev);
