Linux-PCI Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] PCI: PM: Skip devices in D0 for suspend-to-idle
@ 2019-06-12 22:14 Rafael J. Wysocki
  2019-06-13  6:13 ` Kai-Heng Feng
                   ` (3 more replies)
  0 siblings, 4 replies; 6+ messages in thread
From: Rafael J. Wysocki @ 2019-06-12 22:14 UTC (permalink / raw)
  To: Linux PCI
  Cc: Linux PM, Linux ACPI, LKML, Bjorn Helgaas, Mika Westerberg,
	Keith Busch, Kai-Heng Feng

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Commit d491f2b75237 ("PCI: PM: Avoid possible suspend-to-idle issue")
attempted to avoid a problem with devices whose drivers want them to
stay in D0 over suspend-to-idle and resume, but it did not go as far
as it should with that.

Namely, first of all, it is questionable to change the power state
of a PCI bridge with a device in D0 under it, but that is not
actively prevented from happening during system-wide PM transitions,
so use the skip_bus_pm flag introduced by commit d491f2b75237 for
that.

Second, the configuration of devices left in D0 (whatever the reason)
during suspend-to-idle need not be changed and attempting to put them
into D0 again by force may confuse some firmware, so explicitly avoid
doing that.

Fixes: d491f2b75237 ("PCI: PM: Avoid possible suspend-to-idle issue")
Reported-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

Tested on Dell XPS13 9360 with no issues.

---
 drivers/pci/pci-driver.c |   47 +++++++++++++++++++++++++++++++++++------------
 1 file changed, 35 insertions(+), 12 deletions(-)

Index: linux-pm/drivers/pci/pci-driver.c
===================================================================
--- linux-pm.orig/drivers/pci/pci-driver.c
+++ linux-pm/drivers/pci/pci-driver.c
@@ -524,7 +524,6 @@ static void pci_pm_default_resume_early(
 	pci_power_up(pci_dev);
 	pci_restore_state(pci_dev);
 	pci_pme_restore(pci_dev);
-	pci_fixup_device(pci_fixup_resume_early, pci_dev);
 }
 
 /*
@@ -842,18 +841,16 @@ static int pci_pm_suspend_noirq(struct d
 
 	if (pci_dev->skip_bus_pm) {
 		/*
-		 * The function is running for the second time in a row without
+		 * Either the device is a bridge with a child in D0 below it, or
+		 * the function is running for the second time in a row without
 		 * going through full resume, which is possible only during
-		 * suspend-to-idle in a spurious wakeup case.  Moreover, the
-		 * device was originally left in D0, so its power state should
-		 * not be changed here and the device register values saved
-		 * originally should be restored on resume again.
+		 * suspend-to-idle in a spurious wakeup case.  The device should
+		 * be in D0 at this point, but if it is a bridge, it may be
+		 * necessary to save its state.
 		 */
-		pci_dev->state_saved = true;
-	} else if (pci_dev->state_saved) {
-		if (pci_dev->current_state == PCI_D0)
-			pci_dev->skip_bus_pm = true;
-	} else {
+		if (!pci_dev->state_saved)
+			pci_save_state(pci_dev);
+	} else if (!pci_dev->state_saved) {
 		pci_save_state(pci_dev);
 		if (pci_power_manageable(pci_dev))
 			pci_prepare_to_sleep(pci_dev);
@@ -862,6 +859,22 @@ static int pci_pm_suspend_noirq(struct d
 	dev_dbg(dev, "PCI PM: Suspend power state: %s\n",
 		pci_power_name(pci_dev->current_state));
 
+	if (pci_dev->current_state == PCI_D0) {
+		pci_dev->skip_bus_pm = true;
+		/*
+		 * Changing the power state of a PCI bridge with a device in D0
+		 * below it is questionable, so avoid doing that by setting the
+		 * skip_bus_pm flag for the parent bridge.
+		 */
+		if (pci_dev->bus->self)
+			pci_dev->bus->self->skip_bus_pm = true;
+	}
+
+	if (pci_dev->skip_bus_pm && !pm_suspend_via_firmware()) {
+		dev_dbg(dev, "PCI PM: Skipped\n");
+		goto Fixup;
+	}
+
 	pci_pm_set_unknown_state(pci_dev);
 
 	/*
@@ -909,7 +922,16 @@ static int pci_pm_resume_noirq(struct de
 	if (dev_pm_smart_suspend_and_suspended(dev))
 		pm_runtime_set_active(dev);
 
-	pci_pm_default_resume_early(pci_dev);
+	/*
+	 * In the suspend-to-idle case, devices left in D0 during suspend will
+	 * stay in D0, so it is not necessary to restore or update their
+	 * configuration here and attempting to put them into D0 again may
+	 * confuse some firmware, so avoid doing that.
+	 */
+	if (!pci_dev->skip_bus_pm || pm_suspend_via_firmware())
+		pci_pm_default_resume_early(pci_dev);
+
+	pci_fixup_device(pci_fixup_resume_early, pci_dev);
 
 	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_resume_early(dev);
@@ -1200,6 +1222,7 @@ static int pci_pm_restore_noirq(struct d
 	}
 
 	pci_pm_default_resume_early(pci_dev);
+	pci_fixup_device(pci_fixup_resume_early, pci_dev);
 
 	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_resume_early(dev);




^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] PCI: PM: Skip devices in D0 for suspend-to-idle
  2019-06-12 22:14 [PATCH] PCI: PM: Skip devices in D0 for suspend-to-idle Rafael J. Wysocki
@ 2019-06-13  6:13 ` Kai-Heng Feng
  2019-06-13 12:57 ` Mika Westerberg
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 6+ messages in thread
From: Kai-Heng Feng @ 2019-06-13  6:13 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Linux PCI, Linux PM, Linux ACPI, LKML, Bjorn Helgaas,
	Mika Westerberg, Keith Busch

at 06:14, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:

> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Commit d491f2b75237 ("PCI: PM: Avoid possible suspend-to-idle issue")
> attempted to avoid a problem with devices whose drivers want them to
> stay in D0 over suspend-to-idle and resume, but it did not go as far
> as it should with that.
>
> Namely, first of all, it is questionable to change the power state
> of a PCI bridge with a device in D0 under it, but that is not
> actively prevented from happening during system-wide PM transitions,
> so use the skip_bus_pm flag introduced by commit d491f2b75237 for
> that.
>
> Second, the configuration of devices left in D0 (whatever the reason)
> during suspend-to-idle need not be changed and attempting to put them
> into D0 again by force may confuse some firmware, so explicitly avoid
> doing that.
>
> Fixes: d491f2b75237 ("PCI: PM: Avoid possible suspend-to-idle issue")
> Reported-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Thanks! This patch solves the issue I reported earlier.

Tested-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

> ---
>
> Tested on Dell XPS13 9360 with no issues.
>
> ---
>  drivers/pci/pci-driver.c |   47 +++++++++++++++++++++++++++++++++++------------
>  1 file changed, 35 insertions(+), 12 deletions(-)
>
> Index: linux-pm/drivers/pci/pci-driver.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/pci-driver.c
> +++ linux-pm/drivers/pci/pci-driver.c
> @@ -524,7 +524,6 @@ static void pci_pm_default_resume_early(
>  	pci_power_up(pci_dev);
>  	pci_restore_state(pci_dev);
>  	pci_pme_restore(pci_dev);
> -	pci_fixup_device(pci_fixup_resume_early, pci_dev);
>  }
>
>  /*
> @@ -842,18 +841,16 @@ static int pci_pm_suspend_noirq(struct d
>
>  	if (pci_dev->skip_bus_pm) {
>  		/*
> -		 * The function is running for the second time in a row without
> +		 * Either the device is a bridge with a child in D0 below it, or
> +		 * the function is running for the second time in a row without
>  		 * going through full resume, which is possible only during
> -		 * suspend-to-idle in a spurious wakeup case.  Moreover, the
> -		 * device was originally left in D0, so its power state should
> -		 * not be changed here and the device register values saved
> -		 * originally should be restored on resume again.
> +		 * suspend-to-idle in a spurious wakeup case.  The device should
> +		 * be in D0 at this point, but if it is a bridge, it may be
> +		 * necessary to save its state.
>  		 */
> -		pci_dev->state_saved = true;
> -	} else if (pci_dev->state_saved) {
> -		if (pci_dev->current_state == PCI_D0)
> -			pci_dev->skip_bus_pm = true;
> -	} else {
> +		if (!pci_dev->state_saved)
> +			pci_save_state(pci_dev);
> +	} else if (!pci_dev->state_saved) {
>  		pci_save_state(pci_dev);
>  		if (pci_power_manageable(pci_dev))
>  			pci_prepare_to_sleep(pci_dev);
> @@ -862,6 +859,22 @@ static int pci_pm_suspend_noirq(struct d
>  	dev_dbg(dev, "PCI PM: Suspend power state: %s\n",
>  		pci_power_name(pci_dev->current_state));
>
> +	if (pci_dev->current_state == PCI_D0) {
> +		pci_dev->skip_bus_pm = true;
> +		/*
> +		 * Changing the power state of a PCI bridge with a device in D0
> +		 * below it is questionable, so avoid doing that by setting the
> +		 * skip_bus_pm flag for the parent bridge.
> +		 */
> +		if (pci_dev->bus->self)
> +			pci_dev->bus->self->skip_bus_pm = true;
> +	}
> +
> +	if (pci_dev->skip_bus_pm && !pm_suspend_via_firmware()) {
> +		dev_dbg(dev, "PCI PM: Skipped\n");
> +		goto Fixup;
> +	}
> +
>  	pci_pm_set_unknown_state(pci_dev);
>
>  	/*
> @@ -909,7 +922,16 @@ static int pci_pm_resume_noirq(struct de
>  	if (dev_pm_smart_suspend_and_suspended(dev))
>  		pm_runtime_set_active(dev);
>
> -	pci_pm_default_resume_early(pci_dev);
> +	/*
> +	 * In the suspend-to-idle case, devices left in D0 during suspend will
> +	 * stay in D0, so it is not necessary to restore or update their
> +	 * configuration here and attempting to put them into D0 again may
> +	 * confuse some firmware, so avoid doing that.
> +	 */
> +	if (!pci_dev->skip_bus_pm || pm_suspend_via_firmware())
> +		pci_pm_default_resume_early(pci_dev);
> +
> +	pci_fixup_device(pci_fixup_resume_early, pci_dev);
>
>  	if (pci_has_legacy_pm_support(pci_dev))
>  		return pci_legacy_resume_early(dev);
> @@ -1200,6 +1222,7 @@ static int pci_pm_restore_noirq(struct d
>  	}
>
>  	pci_pm_default_resume_early(pci_dev);
> +	pci_fixup_device(pci_fixup_resume_early, pci_dev);
>
>  	if (pci_has_legacy_pm_support(pci_dev))
>  		return pci_legacy_resume_early(dev);



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] PCI: PM: Skip devices in D0 for suspend-to-idle
  2019-06-12 22:14 [PATCH] PCI: PM: Skip devices in D0 for suspend-to-idle Rafael J. Wysocki
  2019-06-13  6:13 ` Kai-Heng Feng
@ 2019-06-13 12:57 ` Mika Westerberg
  2019-06-13 18:49 ` Rafael J. Wysocki
  2019-06-13 21:38 ` Bjorn Helgaas
  3 siblings, 0 replies; 6+ messages in thread
From: Mika Westerberg @ 2019-06-13 12:57 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Linux PCI, Linux PM, Linux ACPI, LKML, Bjorn Helgaas,
	Keith Busch, Kai-Heng Feng

On Thu, Jun 13, 2019 at 12:14:02AM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Commit d491f2b75237 ("PCI: PM: Avoid possible suspend-to-idle issue")
> attempted to avoid a problem with devices whose drivers want them to
> stay in D0 over suspend-to-idle and resume, but it did not go as far
> as it should with that.
> 
> Namely, first of all, it is questionable to change the power state
> of a PCI bridge with a device in D0 under it, but that is not
> actively prevented from happening during system-wide PM transitions,
> so use the skip_bus_pm flag introduced by commit d491f2b75237 for
> that.
> 
> Second, the configuration of devices left in D0 (whatever the reason)
> during suspend-to-idle need not be changed and attempting to put them
> into D0 again by force may confuse some firmware, so explicitly avoid
> doing that.
> 
> Fixes: d491f2b75237 ("PCI: PM: Avoid possible suspend-to-idle issue")
> Reported-by: Kai-Heng Feng <kai.heng.feng@canonical.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] 6+ messages in thread

* Re: [PATCH] PCI: PM: Skip devices in D0 for suspend-to-idle
  2019-06-12 22:14 [PATCH] PCI: PM: Skip devices in D0 for suspend-to-idle Rafael J. Wysocki
  2019-06-13  6:13 ` Kai-Heng Feng
  2019-06-13 12:57 ` Mika Westerberg
@ 2019-06-13 18:49 ` Rafael J. Wysocki
  2019-06-13 21:38 ` Bjorn Helgaas
  3 siblings, 0 replies; 6+ messages in thread
From: Rafael J. Wysocki @ 2019-06-13 18:49 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Linux PCI, Linux PM, Linux ACPI, LKML, Mika Westerberg,
	Keith Busch, Kai-Heng Feng

On Thursday, June 13, 2019 12:14:02 AM CEST Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Commit d491f2b75237 ("PCI: PM: Avoid possible suspend-to-idle issue")
> attempted to avoid a problem with devices whose drivers want them to
> stay in D0 over suspend-to-idle and resume, but it did not go as far
> as it should with that.
> 
> Namely, first of all, it is questionable to change the power state
> of a PCI bridge with a device in D0 under it, but that is not
> actively prevented from happening during system-wide PM transitions,
> so use the skip_bus_pm flag introduced by commit d491f2b75237 for
> that.
> 
> Second, the configuration of devices left in D0 (whatever the reason)
> during suspend-to-idle need not be changed and attempting to put them
> into D0 again by force may confuse some firmware, so explicitly avoid
> doing that.
> 
> Fixes: d491f2b75237 ("PCI: PM: Avoid possible suspend-to-idle issue")
> Reported-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> Tested on Dell XPS13 9360 with no issues.
> 
> ---
>  drivers/pci/pci-driver.c |   47 +++++++++++++++++++++++++++++++++++------------
>  1 file changed, 35 insertions(+), 12 deletions(-)
> 
> Index: linux-pm/drivers/pci/pci-driver.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/pci-driver.c
> +++ linux-pm/drivers/pci/pci-driver.c
> @@ -524,7 +524,6 @@ static void pci_pm_default_resume_early(
>  	pci_power_up(pci_dev);
>  	pci_restore_state(pci_dev);
>  	pci_pme_restore(pci_dev);
> -	pci_fixup_device(pci_fixup_resume_early, pci_dev);
>  }
>  
>  /*
> @@ -842,18 +841,16 @@ static int pci_pm_suspend_noirq(struct d
>  
>  	if (pci_dev->skip_bus_pm) {
>  		/*
> -		 * The function is running for the second time in a row without
> +		 * Either the device is a bridge with a child in D0 below it, or
> +		 * the function is running for the second time in a row without
>  		 * going through full resume, which is possible only during
> -		 * suspend-to-idle in a spurious wakeup case.  Moreover, the
> -		 * device was originally left in D0, so its power state should
> -		 * not be changed here and the device register values saved
> -		 * originally should be restored on resume again.
> +		 * suspend-to-idle in a spurious wakeup case.  The device should
> +		 * be in D0 at this point, but if it is a bridge, it may be
> +		 * necessary to save its state.
>  		 */
> -		pci_dev->state_saved = true;
> -	} else if (pci_dev->state_saved) {
> -		if (pci_dev->current_state == PCI_D0)
> -			pci_dev->skip_bus_pm = true;
> -	} else {
> +		if (!pci_dev->state_saved)
> +			pci_save_state(pci_dev);
> +	} else if (!pci_dev->state_saved) {
>  		pci_save_state(pci_dev);
>  		if (pci_power_manageable(pci_dev))
>  			pci_prepare_to_sleep(pci_dev);
> @@ -862,6 +859,22 @@ static int pci_pm_suspend_noirq(struct d
>  	dev_dbg(dev, "PCI PM: Suspend power state: %s\n",
>  		pci_power_name(pci_dev->current_state));
>  
> +	if (pci_dev->current_state == PCI_D0) {
> +		pci_dev->skip_bus_pm = true;
> +		/*
> +		 * Changing the power state of a PCI bridge with a device in D0
> +		 * below it is questionable, so avoid doing that by setting the
> +		 * skip_bus_pm flag for the parent bridge.
> +		 */
> +		if (pci_dev->bus->self)
> +			pci_dev->bus->self->skip_bus_pm = true;
> +	}
> +
> +	if (pci_dev->skip_bus_pm && !pm_suspend_via_firmware()) {
> +		dev_dbg(dev, "PCI PM: Skipped\n");
> +		goto Fixup;
> +	}
> +
>  	pci_pm_set_unknown_state(pci_dev);
>  
>  	/*
> @@ -909,7 +922,16 @@ static int pci_pm_resume_noirq(struct de
>  	if (dev_pm_smart_suspend_and_suspended(dev))
>  		pm_runtime_set_active(dev);
>  
> -	pci_pm_default_resume_early(pci_dev);
> +	/*
> +	 * In the suspend-to-idle case, devices left in D0 during suspend will
> +	 * stay in D0, so it is not necessary to restore or update their
> +	 * configuration here and attempting to put them into D0 again may
> +	 * confuse some firmware, so avoid doing that.
> +	 */
> +	if (!pci_dev->skip_bus_pm || pm_suspend_via_firmware())
> +		pci_pm_default_resume_early(pci_dev);
> +
> +	pci_fixup_device(pci_fixup_resume_early, pci_dev);
>  
>  	if (pci_has_legacy_pm_support(pci_dev))
>  		return pci_legacy_resume_early(dev);
> @@ -1200,6 +1222,7 @@ static int pci_pm_restore_noirq(struct d
>  	}
>  
>  	pci_pm_default_resume_early(pci_dev);
> +	pci_fixup_device(pci_fixup_resume_early, pci_dev);
>  
>  	if (pci_has_legacy_pm_support(pci_dev))
>  		return pci_legacy_resume_early(dev);
> 

Bjorn, please let me know if you have any reservations here.

Since this has been confirmed to fix a reported issue, I'm about to queue it up for 5.2-rc6.

Cheers,
Rafael




^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] PCI: PM: Skip devices in D0 for suspend-to-idle
  2019-06-12 22:14 [PATCH] PCI: PM: Skip devices in D0 for suspend-to-idle Rafael J. Wysocki
                   ` (2 preceding siblings ...)
  2019-06-13 18:49 ` Rafael J. Wysocki
@ 2019-06-13 21:38 ` Bjorn Helgaas
  2019-06-13 21:46   ` Rafael J. Wysocki
  3 siblings, 1 reply; 6+ messages in thread
From: Bjorn Helgaas @ 2019-06-13 21:38 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Linux PCI, Linux PM, Linux ACPI, LKML, Mika Westerberg,
	Keith Busch, Kai-Heng Feng

On Thu, Jun 13, 2019 at 12:14:02AM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Commit d491f2b75237 ("PCI: PM: Avoid possible suspend-to-idle issue")
> attempted to avoid a problem with devices whose drivers want them to
> stay in D0 over suspend-to-idle and resume, but it did not go as far
> as it should with that.
> 
> Namely, first of all, it is questionable to change the power state
> of a PCI bridge with a device in D0 under it, but that is not
> actively prevented from happening during system-wide PM transitions,
> so use the skip_bus_pm flag introduced by commit d491f2b75237 for
> that.

I think it's more than questionable.  I think a bridge is *required*
to be in D0 if any downstream device is in D0.  Based on the PCI PM
spec r1.2, sec 6, table 6-1, if the bridge is not in D0, there can be
no PCI transactions on its secondary bus.

> Second, the configuration of devices left in D0 (whatever the reason)
> during suspend-to-idle need not be changed and attempting to put them
> into D0 again by force may confuse some firmware, so explicitly avoid
> doing that.

I don't know what to do with "may confuse some firmware"; it doesn't
say what firmware is affected or why, so it sort of leads to "we can
never touch this code because we don't know what might break."

But IMO the first reason by itself is more than enough to keep a
bridge in D0 if any downstream device is in D0.

> Fixes: d491f2b75237 ("PCI: PM: Avoid possible suspend-to-idle issue")
> Reported-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> Tested on Dell XPS13 9360 with no issues.
> 
> ---
>  drivers/pci/pci-driver.c |   47 +++++++++++++++++++++++++++++++++++------------
>  1 file changed, 35 insertions(+), 12 deletions(-)
> 
> Index: linux-pm/drivers/pci/pci-driver.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/pci-driver.c
> +++ linux-pm/drivers/pci/pci-driver.c
> @@ -524,7 +524,6 @@ static void pci_pm_default_resume_early(
>  	pci_power_up(pci_dev);
>  	pci_restore_state(pci_dev);
>  	pci_pme_restore(pci_dev);
> -	pci_fixup_device(pci_fixup_resume_early, pci_dev);
>  }
>  
>  /*
> @@ -842,18 +841,16 @@ static int pci_pm_suspend_noirq(struct d
>  
>  	if (pci_dev->skip_bus_pm) {
>  		/*
> -		 * The function is running for the second time in a row without
> +		 * Either the device is a bridge with a child in D0 below it, or
> +		 * the function is running for the second time in a row without
>  		 * going through full resume, which is possible only during
> -		 * suspend-to-idle in a spurious wakeup case.  Moreover, the
> -		 * device was originally left in D0, so its power state should
> -		 * not be changed here and the device register values saved
> -		 * originally should be restored on resume again.
> +		 * suspend-to-idle in a spurious wakeup case.  The device should
> +		 * be in D0 at this point, but if it is a bridge, it may be
> +		 * necessary to save its state.
>  		 */
> -		pci_dev->state_saved = true;
> -	} else if (pci_dev->state_saved) {
> -		if (pci_dev->current_state == PCI_D0)
> -			pci_dev->skip_bus_pm = true;
> -	} else {
> +		if (!pci_dev->state_saved)
> +			pci_save_state(pci_dev);
> +	} else if (!pci_dev->state_saved) {
>  		pci_save_state(pci_dev);
>  		if (pci_power_manageable(pci_dev))
>  			pci_prepare_to_sleep(pci_dev);
> @@ -862,6 +859,22 @@ static int pci_pm_suspend_noirq(struct d
>  	dev_dbg(dev, "PCI PM: Suspend power state: %s\n",
>  		pci_power_name(pci_dev->current_state));
>  
> +	if (pci_dev->current_state == PCI_D0) {
> +		pci_dev->skip_bus_pm = true;
> +		/*
> +		 * Changing the power state of a PCI bridge with a device in D0
> +		 * below it is questionable, so avoid doing that by setting the
> +		 * skip_bus_pm flag for the parent bridge.

Maybe "Per PCI PM r1.2, table 6-1, a bridge must be in D0 if any
downstream device is in D0"?

> +		 */
> +		if (pci_dev->bus->self)
> +			pci_dev->bus->self->skip_bus_pm = true;
> +	}
> +
> +	if (pci_dev->skip_bus_pm && !pm_suspend_via_firmware()) {
> +		dev_dbg(dev, "PCI PM: Skipped\n");
> +		goto Fixup;
> +	}
> +
>  	pci_pm_set_unknown_state(pci_dev);
>  
>  	/*
> @@ -909,7 +922,16 @@ static int pci_pm_resume_noirq(struct de
>  	if (dev_pm_smart_suspend_and_suspended(dev))
>  		pm_runtime_set_active(dev);
>  
> -	pci_pm_default_resume_early(pci_dev);
> +	/*
> +	 * In the suspend-to-idle case, devices left in D0 during suspend will
> +	 * stay in D0, so it is not necessary to restore or update their
> +	 * configuration here and attempting to put them into D0 again may
> +	 * confuse some firmware, so avoid doing that.
> +	 */
> +	if (!pci_dev->skip_bus_pm || pm_suspend_via_firmware())
> +		pci_pm_default_resume_early(pci_dev);
> +
> +	pci_fixup_device(pci_fixup_resume_early, pci_dev);
>  
>  	if (pci_has_legacy_pm_support(pci_dev))
>  		return pci_legacy_resume_early(dev);
> @@ -1200,6 +1222,7 @@ static int pci_pm_restore_noirq(struct d
>  	}
>  
>  	pci_pm_default_resume_early(pci_dev);
> +	pci_fixup_device(pci_fixup_resume_early, pci_dev);
>  
>  	if (pci_has_legacy_pm_support(pci_dev))
>  		return pci_legacy_resume_early(dev);
> 
> 
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] PCI: PM: Skip devices in D0 for suspend-to-idle
  2019-06-13 21:38 ` Bjorn Helgaas
@ 2019-06-13 21:46   ` Rafael J. Wysocki
  0 siblings, 0 replies; 6+ messages in thread
From: Rafael J. Wysocki @ 2019-06-13 21:46 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Rafael J. Wysocki, Linux PCI, Linux PM, Linux ACPI, LKML,
	Mika Westerberg, Keith Busch, Kai-Heng Feng

On Thu, Jun 13, 2019 at 11:38 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Thu, Jun 13, 2019 at 12:14:02AM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Commit d491f2b75237 ("PCI: PM: Avoid possible suspend-to-idle issue")
> > attempted to avoid a problem with devices whose drivers want them to
> > stay in D0 over suspend-to-idle and resume, but it did not go as far
> > as it should with that.
> >
> > Namely, first of all, it is questionable to change the power state
> > of a PCI bridge with a device in D0 under it, but that is not
> > actively prevented from happening during system-wide PM transitions,
> > so use the skip_bus_pm flag introduced by commit d491f2b75237 for
> > that.
>
> I think it's more than questionable.  I think a bridge is *required*
> to be in D0 if any downstream device is in D0.  Based on the PCI PM
> spec r1.2, sec 6, table 6-1, if the bridge is not in D0, there can be
> no PCI transactions on its secondary bus.

Fair enough.

> > Second, the configuration of devices left in D0 (whatever the reason)
> > during suspend-to-idle need not be changed and attempting to put them
> > into D0 again by force may confuse some firmware, so explicitly avoid
> > doing that.
>
> I don't know what to do with "may confuse some firmware"; it doesn't
> say what firmware is affected or why, so it sort of leads to "we can
> never touch this code because we don't know what might break."
>
> But IMO the first reason by itself is more than enough to keep a
> bridge in D0 if any downstream device is in D0.

OK, so I'll replace the phrase "may confuse some firmware" with "is pointless".

> > Fixes: d491f2b75237 ("PCI: PM: Avoid possible suspend-to-idle issue")
> > Reported-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >
> > Tested on Dell XPS13 9360 with no issues.
> >
> > ---
> >  drivers/pci/pci-driver.c |   47 +++++++++++++++++++++++++++++++++++------------
> >  1 file changed, 35 insertions(+), 12 deletions(-)
> >
> > Index: linux-pm/drivers/pci/pci-driver.c
> > ===================================================================
> > --- linux-pm.orig/drivers/pci/pci-driver.c
> > +++ linux-pm/drivers/pci/pci-driver.c
> > @@ -524,7 +524,6 @@ static void pci_pm_default_resume_early(
> >       pci_power_up(pci_dev);
> >       pci_restore_state(pci_dev);
> >       pci_pme_restore(pci_dev);
> > -     pci_fixup_device(pci_fixup_resume_early, pci_dev);
> >  }
> >
> >  /*
> > @@ -842,18 +841,16 @@ static int pci_pm_suspend_noirq(struct d
> >
> >       if (pci_dev->skip_bus_pm) {
> >               /*
> > -              * The function is running for the second time in a row without
> > +              * Either the device is a bridge with a child in D0 below it, or
> > +              * the function is running for the second time in a row without
> >                * going through full resume, which is possible only during
> > -              * suspend-to-idle in a spurious wakeup case.  Moreover, the
> > -              * device was originally left in D0, so its power state should
> > -              * not be changed here and the device register values saved
> > -              * originally should be restored on resume again.
> > +              * suspend-to-idle in a spurious wakeup case.  The device should
> > +              * be in D0 at this point, but if it is a bridge, it may be
> > +              * necessary to save its state.
> >                */
> > -             pci_dev->state_saved = true;
> > -     } else if (pci_dev->state_saved) {
> > -             if (pci_dev->current_state == PCI_D0)
> > -                     pci_dev->skip_bus_pm = true;
> > -     } else {
> > +             if (!pci_dev->state_saved)
> > +                     pci_save_state(pci_dev);
> > +     } else if (!pci_dev->state_saved) {
> >               pci_save_state(pci_dev);
> >               if (pci_power_manageable(pci_dev))
> >                       pci_prepare_to_sleep(pci_dev);
> > @@ -862,6 +859,22 @@ static int pci_pm_suspend_noirq(struct d
> >       dev_dbg(dev, "PCI PM: Suspend power state: %s\n",
> >               pci_power_name(pci_dev->current_state));
> >
> > +     if (pci_dev->current_state == PCI_D0) {
> > +             pci_dev->skip_bus_pm = true;
> > +             /*
> > +              * Changing the power state of a PCI bridge with a device in D0
> > +              * below it is questionable, so avoid doing that by setting the
> > +              * skip_bus_pm flag for the parent bridge.
>
> Maybe "Per PCI PM r1.2, table 6-1, a bridge must be in D0 if any
> downstream device is in D0"?

OK

> > +              */
> > +             if (pci_dev->bus->self)
> > +                     pci_dev->bus->self->skip_bus_pm = true;
> > +     }
> > +
> > +     if (pci_dev->skip_bus_pm && !pm_suspend_via_firmware()) {
> > +             dev_dbg(dev, "PCI PM: Skipped\n");
> > +             goto Fixup;
> > +     }
> > +
> >       pci_pm_set_unknown_state(pci_dev);
> >
> >       /*
> > @@ -909,7 +922,16 @@ static int pci_pm_resume_noirq(struct de
> >       if (dev_pm_smart_suspend_and_suspended(dev))
> >               pm_runtime_set_active(dev);
> >
> > -     pci_pm_default_resume_early(pci_dev);
> > +     /*
> > +      * In the suspend-to-idle case, devices left in D0 during suspend will
> > +      * stay in D0, so it is not necessary to restore or update their
> > +      * configuration here and attempting to put them into D0 again may
> > +      * confuse some firmware, so avoid doing that.
> > +      */
> > +     if (!pci_dev->skip_bus_pm || pm_suspend_via_firmware())
> > +             pci_pm_default_resume_early(pci_dev);
> > +
> > +     pci_fixup_device(pci_fixup_resume_early, pci_dev);
> >
> >       if (pci_has_legacy_pm_support(pci_dev))
> >               return pci_legacy_resume_early(dev);
> > @@ -1200,6 +1222,7 @@ static int pci_pm_restore_noirq(struct d
> >       }
> >
> >       pci_pm_default_resume_early(pci_dev);
> > +     pci_fixup_device(pci_fixup_resume_early, pci_dev);
> >
> >       if (pci_has_legacy_pm_support(pci_dev))
> >               return pci_legacy_resume_early(dev);
> >

I'll send a v2 shortly.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-12 22:14 [PATCH] PCI: PM: Skip devices in D0 for suspend-to-idle Rafael J. Wysocki
2019-06-13  6:13 ` Kai-Heng Feng
2019-06-13 12:57 ` Mika Westerberg
2019-06-13 18:49 ` Rafael J. Wysocki
2019-06-13 21:38 ` Bjorn Helgaas
2019-06-13 21:46   ` Rafael J. Wysocki

Linux-PCI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pci/0 linux-pci/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pci linux-pci/ https://lore.kernel.org/linux-pci \
		linux-pci@vger.kernel.org linux-pci@archiver.kernel.org
	public-inbox-index linux-pci


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pci


AGPL code for this site: git clone https://public-inbox.org/ public-inbox