[PATCH] PCI: also apply D3 delay when leaving D3cold

[PATCH] PCI: also apply D3 delay when leaving D3cold

[Daniel Drake]

This delay is needed to fix resume from s2idle of the XHCI controller on
AMD Ryzen SoCs, where a 20ms delay is required (this will be quirked
in a followup patch), to avoid this failure:

  xhci_hcd 0000:03:00.4: WARN: xHC restore state timeout
  xhci_hcd 0000:03:00.4: PCI post-resume error -110!

The D3 delay is already being performed in a runtime resume from D3cold,
through the following sequence of events:

     pci_pm_runtime_resume
  -> pci_restore_standard_config
  -> pci_set_power_state(D0)
  -> __pci_start_power_transition
  -> pci_platform_power_transition
  -> pci_update_current_state

At this point, the device has been set to D0 at the platform level,
so pci_update_current_state() reads pmcsr and updates dev->current_state
to D3hot. Now when we reach pci_raw_set_power_state() the D3 delay will
be applied.

However, the D3cold resume from s2idle path is somewhat different, and
we arrive at the same function without hitting pci_update_current_state()
along the way:
     pci_pm_resume_noirq
  -> pci_pm_default_resume_early
  -> pci_power_up
  -> pci_raw_set_power_state

As dev->current_state is D3cold, the D3 delay is skipped and the XHCI
controllers fail to be powered up.

Apply the D3 delay in the s2idle resume case too, in order to fix
USB functionality after resume.

Link: http://lkml.kernel.org/r/CAD8Lp47Vh69gQjROYG69=waJgL7hs1PwnLonL9+27S_TcRhixA@mail.gmail.com
Signed-off-by: Daniel Drake <drake@endlessm.com>
---
 drivers/pci/pci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e7982af9a5d8..ab15fa5eda2c 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -883,7 +883,8 @@ static int pci_raw_set_power_state(struct pci_dev *dev, pci_power_t state)
 	 * Mandatory power management transition delays; see PCI PM 1.1
 	 * 5.6.1 table 18
 	 */
-	if (state == PCI_D3hot || dev->current_state == PCI_D3hot)
+	if (state == PCI_D3hot || dev->current_state == PCI_D3hot
+			|| dev->current_state == PCI_D3cold)
 		pci_dev_d3_sleep(dev);
 	else if (state == PCI_D2 || dev->current_state == PCI_D2)
 		udelay(PCI_PM_D2_DELAY);
-- 
2.20.1

Re: [PATCH] PCI: also apply D3 delay when leaving D3cold

[Rafael J. Wysocki]

On Friday, September 27, 2019 11:02:02 AM CEST Daniel Drake wrote:
> This delay is needed to fix resume from s2idle of the XHCI controller on
> AMD Ryzen SoCs, where a 20ms delay is required (this will be quirked
> in a followup patch), to avoid this failure:
> 
>   xhci_hcd 0000:03:00.4: WARN: xHC restore state timeout
>   xhci_hcd 0000:03:00.4: PCI post-resume error -110!
> 
> The D3 delay is already being performed in a runtime resume from D3cold,
> through the following sequence of events:
> 
>      pci_pm_runtime_resume
>   -> pci_restore_standard_config
>   -> pci_set_power_state(D0)
>   -> __pci_start_power_transition
>   -> pci_platform_power_transition
>   -> pci_update_current_state
> 
> At this point, the device has been set to D0 at the platform level,
> so pci_update_current_state() reads pmcsr and updates dev->current_state
> to D3hot. Now when we reach pci_raw_set_power_state() the D3 delay will
> be applied.
> 
> However, the D3cold resume from s2idle path is somewhat different, and
> we arrive at the same function without hitting pci_update_current_state()
> along the way:
>      pci_pm_resume_noirq
>   -> pci_pm_default_resume_early
>   -> pci_power_up
>   -> pci_raw_set_power_state
> 
> As dev->current_state is D3cold, the D3 delay is skipped and the XHCI
> controllers fail to be powered up.
> 
> Apply the D3 delay in the s2idle resume case too, in order to fix
> USB functionality after resume.
> 
> Link: http://lkml.kernel.org/r/CAD8Lp47Vh69gQjROYG69=waJgL7hs1PwnLonL9+27S_TcRhixA@mail.gmail.com
> Signed-off-by: Daniel Drake <drake@endlessm.com>
> ---
>  drivers/pci/pci.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index e7982af9a5d8..ab15fa5eda2c 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -883,7 +883,8 @@ static int pci_raw_set_power_state(struct pci_dev *dev, pci_power_t state)
>  	 * Mandatory power management transition delays; see PCI PM 1.1
>  	 * 5.6.1 table 18
>  	 */
> -	if (state == PCI_D3hot || dev->current_state == PCI_D3hot)
> +	if (state == PCI_D3hot || dev->current_state == PCI_D3hot
> +			|| dev->current_state == PCI_D3cold)
>  		pci_dev_d3_sleep(dev);
>  	else if (state == PCI_D2 || dev->current_state == PCI_D2)
>  		udelay(PCI_PM_D2_DELAY);
> 

So I think that we can use pci_restore_standard_config() in the system resume
patch too, which should address the issue as well.

Basically, there is no reason for the PM-runtime and system-wide resume code
paths to be different in that respect.

Something like this (untested):

---
 drivers/pci/pci-driver.c |   11 ++---------
 drivers/pci/pci.c        |   13 -------------
 drivers/pci/pci.h        |    1 -
 3 files changed, 2 insertions(+), 23 deletions(-)

Index: linux-pm/drivers/pci/pci-driver.c
===================================================================
--- linux-pm.orig/drivers/pci/pci-driver.c
+++ linux-pm/drivers/pci/pci-driver.c
@@ -521,13 +521,6 @@ static int pci_restore_standard_config(s
 
 #ifdef CONFIG_PM_SLEEP
 
-static void pci_pm_default_resume_early(struct pci_dev *pci_dev)
-{
-	pci_power_up(pci_dev);
-	pci_restore_state(pci_dev);
-	pci_pme_restore(pci_dev);
-}
-
 /*
  * Default "suspend" method for devices that have no driver provided suspend,
  * or not even a driver at all (second part).
@@ -938,7 +931,7 @@ static int pci_pm_resume_noirq(struct de
 	 * pointless, so avoid doing that.
 	 */
 	if (!(pci_dev->skip_bus_pm && pm_suspend_no_platform()))
-		pci_pm_default_resume_early(pci_dev);
+		pci_restore_standard_config(pci_dev);
 
 	pci_fixup_device(pci_fixup_resume_early, pci_dev);
 
@@ -1213,7 +1206,7 @@ static int pci_pm_restore_noirq(struct d
 			return error;
 	}
 
-	pci_pm_default_resume_early(pci_dev);
+	pci_restore_standard_config(pci_dev);
 	pci_fixup_device(pci_fixup_resume_early, pci_dev);
 
 	if (pci_has_legacy_pm_support(pci_dev))
Index: linux-pm/drivers/pci/pci.c
===================================================================
--- linux-pm.orig/drivers/pci/pci.c
+++ linux-pm/drivers/pci/pci.c
@@ -959,19 +959,6 @@ void pci_refresh_power_state(struct pci_
 }
 
 /**
- * pci_power_up - Put the given device into D0 forcibly
- * @dev: PCI device to power up
- */
-void pci_power_up(struct pci_dev *dev)
-{
-	if (platform_pci_power_manageable(dev))
-		platform_pci_set_power_state(dev, PCI_D0);
-
-	pci_raw_set_power_state(dev, PCI_D0);
-	pci_update_current_state(dev, PCI_D0);
-}
-
-/**
  * pci_platform_power_transition - Use platform to change device power state
  * @dev: PCI device to handle.
  * @state: State to put the device into.
Index: linux-pm/drivers/pci/pci.h
===================================================================
--- linux-pm.orig/drivers/pci/pci.h
+++ linux-pm/drivers/pci/pci.h
@@ -85,7 +85,6 @@ struct pci_platform_pm_ops {
 int pci_set_platform_pm(const struct pci_platform_pm_ops *ops);
 void pci_update_current_state(struct pci_dev *dev, pci_power_t state);
 void pci_refresh_power_state(struct pci_dev *dev);
-void pci_power_up(struct pci_dev *dev);
 void pci_disable_enabled_device(struct pci_dev *dev);
 int pci_finish_runtime_suspend(struct pci_dev *dev);
 void pcie_clear_root_pme_status(struct pci_dev *dev);

Re: [PATCH] PCI: also apply D3 delay when leaving D3cold

[Daniel Drake]

On Fri, Oct 11, 2019 at 6:14 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> So I think that we can use pci_restore_standard_config() in the system resume
> patch too, which should address the issue as well.
>
> Basically, there is no reason for the PM-runtime and system-wide resume code
> paths to be different in that respect.

Your patch works without modification when combined with
https://patchwork.kernel.org/patch/11187815/

Can you push this directly or would it be helpful if I update the
commit message and submit it by email?

Thanks!
Daniel

Re: [PATCH] PCI: also apply D3 delay when leaving D3cold

[Rafael J. Wysocki]

On Monday, October 14, 2019 8:18:06 AM CEST Daniel Drake wrote:
> On Fri, Oct 11, 2019 at 6:14 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > So I think that we can use pci_restore_standard_config() in the system resume
> > patch too, which should address the issue as well.
> >
> > Basically, there is no reason for the PM-runtime and system-wide resume code
> > paths to be different in that respect.
> 
> Your patch works without modification when combined with
> https://patchwork.kernel.org/patch/11187815/

Great, thanks for the confirmation!

> Can you push this directly or would it be helpful if I update the
> commit message and submit it by email?

It would be prudent if I submitted it properly with a changelog etc.

However, I noticed that it might cause the ACPI power state to get out of sync
during resume from suspend-to-RAM in some (arguably theoretical only) cases.

Specifically, that may happen if the kernel puts a device into D3hot on
suspend and the platform firmware powers it up during system wakeup, because in
that case the pci_update_current_state() in pci_restore_standard_config() will
notice D0 and the pci_set_power_state() in there will not be called, so the
ACPI layer will still hold on to the stale D3hot power state value.

While I still think that both the system resume and runtime resume code paths
should be as similar as reasonably possible, the above needs to be taken into
account IMO, so it is better to retain pci_pm_default_resume_early(), but make
it do a conditional "ACPI power state refresh" and then call
pci_restore_standard_config().

So something like the patch below (can you please test it too?).

---
 drivers/pci/pci-driver.c |    8 +++++---
 drivers/pci/pci.c        |   15 ---------------
 drivers/pci/pci.h        |    1 -
 3 files changed, 5 insertions(+), 19 deletions(-)

Index: linux-pm/drivers/pci/pci-driver.c
===================================================================
--- linux-pm.orig/drivers/pci/pci-driver.c
+++ linux-pm/drivers/pci/pci-driver.c
@@ -523,9 +523,10 @@ static int pci_restore_standard_config(s
 
 static void pci_pm_default_resume_early(struct pci_dev *pci_dev)
 {
-	pci_power_up(pci_dev);
-	pci_restore_state(pci_dev);
-	pci_pme_restore(pci_dev);
+	if (pm_resume_via_firmware())
+		pci_refresh_power_state(pci_dev);
+
+	pci_restore_standard_config(pci_dev);
 }
 
 /*
@@ -713,6 +714,7 @@ static void pci_pm_complete(struct devic
 		pci_power_t pre_sleep_state = pci_dev->current_state;
 
 		pci_refresh_power_state(pci_dev);
+		pci_update_current_state(pci_dev, pci_dev->current_state);
 		/*
 		 * On platforms with ACPI this check may also trigger for
 		 * devices sharing power resources if one of those power
Index: linux-pm/drivers/pci/pci.c
===================================================================
--- linux-pm.orig/drivers/pci/pci.c
+++ linux-pm/drivers/pci/pci.c
@@ -954,21 +954,6 @@ void pci_refresh_power_state(struct pci_
 {
 	if (platform_pci_power_manageable(dev))
 		platform_pci_refresh_power_state(dev);
-
-	pci_update_current_state(dev, dev->current_state);
-}
-
-/**
- * pci_power_up - Put the given device into D0 forcibly
- * @dev: PCI device to power up
- */
-void pci_power_up(struct pci_dev *dev)
-{
-	if (platform_pci_power_manageable(dev))
-		platform_pci_set_power_state(dev, PCI_D0);
-
-	pci_raw_set_power_state(dev, PCI_D0);
-	pci_update_current_state(dev, PCI_D0);
 }
 
 /**
Index: linux-pm/drivers/pci/pci.h
===================================================================
--- linux-pm.orig/drivers/pci/pci.h
+++ linux-pm/drivers/pci/pci.h
@@ -85,7 +85,6 @@ struct pci_platform_pm_ops {
 int pci_set_platform_pm(const struct pci_platform_pm_ops *ops);
 void pci_update_current_state(struct pci_dev *dev, pci_power_t state);
 void pci_refresh_power_state(struct pci_dev *dev);
-void pci_power_up(struct pci_dev *dev);
 void pci_disable_enabled_device(struct pci_dev *dev);
 int pci_finish_runtime_suspend(struct pci_dev *dev);
 void pcie_clear_root_pme_status(struct pci_dev *dev);

Re: [PATCH] PCI: also apply D3 delay when leaving D3cold

[Daniel Drake]

On Mon, Oct 14, 2019 at 5:22 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> While I still think that both the system resume and runtime resume code paths
> should be as similar as reasonably possible, the above needs to be taken into
> account IMO, so it is better to retain pci_pm_default_resume_early(), but make
> it do a conditional "ACPI power state refresh" and then call
> pci_restore_standard_config().
>
> So something like the patch below (can you please test it too?).

This is also working fine, thanks for your help!

Re: [PATCH] PCI: also apply D3 delay when leaving D3cold

[Rafael J. Wysocki]

On Mon, Oct 14, 2019 at 11:46 AM Daniel Drake <drake@endlessm.com> wrote:
>
> On Mon, Oct 14, 2019 at 5:22 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > While I still think that both the system resume and runtime resume code paths
> > should be as similar as reasonably possible, the above needs to be taken into
> > account IMO, so it is better to retain pci_pm_default_resume_early(), but make
> > it do a conditional "ACPI power state refresh" and then call
> > pci_restore_standard_config().
> >
> > So something like the patch below (can you please test it too?).
>
> This is also working fine, thanks for your help!

Thanks for the confirmation and let me submit the patch properly.

[PATCH] PCI: PM: Consolidate runtime resume and system resume paths

[Rafael J. Wysocki]

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

There is an arbitrary difference between the system resume and
runtime resume code paths for PCI devices regarding the delay to
apply when switching the devices from D3cold to D0.

Namely, pci_restore_standard_config() used in the runtime resume
code path calls pci_set_power_state() which in turn invokes
__pci_start_power_transition() to power up the device through the
platform firmware and that function applies the transition delay
(as per PCI Express Base Specification Revision 2.0, Section 6.6.1).
However, pci_pm_default_resume_early() used in the system resume
code path calls pci_power_up() which doesn't apply the delay at
all and that causes issues to occur during resume from
suspend-to-idle on some systems where the delay is required.

Since there is no reason for that difference to exist, modify
pci_pm_default_resume_early() to invoke pci_restore_standard_config()
instead of pci_power_up() and drop the latter, but in order to
prevent the ACPI power state values (cached by the ACPI layer) from
becoming stale in some cases during resume from suspend-to-RAM
(ACPI S3), as per commit cc2893b6af52 ("PCI: Ensure we re-enable
devices on resume"), refresh the ACPI power state information in
pci_pm_default_resume_early() in that case.

[Note that while this change should take the issue originally
 addressed by commit cc2893b6af52 ("PCI: Ensure we re-enable devices
 on resume") into account in a generally safer way, an alternative
 would be to make pci_power_up() use __pci_start_power_transition()
 instead of calling platform_pci_set_power_state() directly.]

Fixes: db288c9c5f9d ("PCI / PM: restore the original behavior of pci_set_power_state()")
Reported-by: Daniel Drake <drake@endlessm.com> 
Tested-by: Daniel Drake <drake@endlessm.com> 
Link: https://lore.kernel.org/linux-pm/CAD8Lp44TYxrMgPLkHCqF9hv6smEurMXvmmvmtyFhZ6Q4SE+dig@mail.gmail.com/T/#m21be74af263c6a34f36e0fc5c77c5449d9406925
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/pci/pci-driver.c |    8 +++++---
 drivers/pci/pci.c        |   15 ---------------
 drivers/pci/pci.h        |    1 -
 3 files changed, 5 insertions(+), 19 deletions(-)

Index: linux-pm/drivers/pci/pci-driver.c
===================================================================
--- linux-pm.orig/drivers/pci/pci-driver.c
+++ linux-pm/drivers/pci/pci-driver.c
@@ -523,9 +523,10 @@ static int pci_restore_standard_config(s
 
 static void pci_pm_default_resume_early(struct pci_dev *pci_dev)
 {
-	pci_power_up(pci_dev);
-	pci_restore_state(pci_dev);
-	pci_pme_restore(pci_dev);
+	if (pm_resume_via_firmware())
+		pci_refresh_power_state(pci_dev);
+
+	pci_restore_standard_config(pci_dev);
 }
 
 /*
@@ -713,6 +714,7 @@ static void pci_pm_complete(struct devic
 		pci_power_t pre_sleep_state = pci_dev->current_state;
 
 		pci_refresh_power_state(pci_dev);
+		pci_update_current_state(pci_dev, pci_dev->current_state);
 		/*
 		 * On platforms with ACPI this check may also trigger for
 		 * devices sharing power resources if one of those power
Index: linux-pm/drivers/pci/pci.c
===================================================================
--- linux-pm.orig/drivers/pci/pci.c
+++ linux-pm/drivers/pci/pci.c
@@ -954,21 +954,6 @@ void pci_refresh_power_state(struct pci_
 {
 	if (platform_pci_power_manageable(dev))
 		platform_pci_refresh_power_state(dev);
-
-	pci_update_current_state(dev, dev->current_state);
-}
-
-/**
- * pci_power_up - Put the given device into D0 forcibly
- * @dev: PCI device to power up
- */
-void pci_power_up(struct pci_dev *dev)
-{
-	if (platform_pci_power_manageable(dev))
-		platform_pci_set_power_state(dev, PCI_D0);
-
-	pci_raw_set_power_state(dev, PCI_D0);
-	pci_update_current_state(dev, PCI_D0);
 }
 
 /**
Index: linux-pm/drivers/pci/pci.h
===================================================================
--- linux-pm.orig/drivers/pci/pci.h
+++ linux-pm/drivers/pci/pci.h
@@ -85,7 +85,6 @@ struct pci_platform_pm_ops {
 int pci_set_platform_pm(const struct pci_platform_pm_ops *ops);
 void pci_update_current_state(struct pci_dev *dev, pci_power_t state);
 void pci_refresh_power_state(struct pci_dev *dev);
-void pci_power_up(struct pci_dev *dev);
 void pci_disable_enabled_device(struct pci_dev *dev);
 int pci_finish_runtime_suspend(struct pci_dev *dev);
 void pcie_clear_root_pme_status(struct pci_dev *dev);

Re: [PATCH] PCI: PM: Consolidate runtime resume and system resume paths

[Rafael J. Wysocki]

On Monday, October 14, 2019 12:51:31 PM CEST Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> There is an arbitrary difference between the system resume and
> runtime resume code paths for PCI devices regarding the delay to
> apply when switching the devices from D3cold to D0.
> 
> Namely, pci_restore_standard_config() used in the runtime resume
> code path calls pci_set_power_state() which in turn invokes
> __pci_start_power_transition() to power up the device through the
> platform firmware and that function applies the transition delay
> (as per PCI Express Base Specification Revision 2.0, Section 6.6.1).
> However, pci_pm_default_resume_early() used in the system resume
> code path calls pci_power_up() which doesn't apply the delay at
> all and that causes issues to occur during resume from
> suspend-to-idle on some systems where the delay is required.
> 
> Since there is no reason for that difference to exist, modify
> pci_pm_default_resume_early() to invoke pci_restore_standard_config()
> instead of pci_power_up() and drop the latter, but in order to
> prevent the ACPI power state values (cached by the ACPI layer) from
> becoming stale in some cases during resume from suspend-to-RAM
> (ACPI S3), as per commit cc2893b6af52 ("PCI: Ensure we re-enable
> devices on resume"), refresh the ACPI power state information in
> pci_pm_default_resume_early() in that case.
> 
> [Note that while this change should take the issue originally
>  addressed by commit cc2893b6af52 ("PCI: Ensure we re-enable devices
>  on resume") into account in a generally safer way, an alternative
>  would be to make pci_power_up() use __pci_start_power_transition()
>  instead of calling platform_pci_set_power_state() directly.]
> 
> Fixes: db288c9c5f9d ("PCI / PM: restore the original behavior of pci_set_power_state()")
> Reported-by: Daniel Drake <drake@endlessm.com> 
> Tested-by: Daniel Drake <drake@endlessm.com> 
> Link: https://lore.kernel.org/linux-pm/CAD8Lp44TYxrMgPLkHCqF9hv6smEurMXvmmvmtyFhZ6Q4SE+dig@mail.gmail.com/T/#m21be74af263c6a34f36e0fc5c77c5449d9406925
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/pci/pci-driver.c |    8 +++++---
>  drivers/pci/pci.c        |   15 ---------------
>  drivers/pci/pci.h        |    1 -
>  3 files changed, 5 insertions(+), 19 deletions(-)
> 
> Index: linux-pm/drivers/pci/pci-driver.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/pci-driver.c
> +++ linux-pm/drivers/pci/pci-driver.c
> @@ -523,9 +523,10 @@ static int pci_restore_standard_config(s
>  
>  static void pci_pm_default_resume_early(struct pci_dev *pci_dev)
>  {
> -	pci_power_up(pci_dev);
> -	pci_restore_state(pci_dev);
> -	pci_pme_restore(pci_dev);
> +	if (pm_resume_via_firmware())
> +		pci_refresh_power_state(pci_dev);

Well, this is still not going to work if the ACPI power state after the update
above is not D0, but the pci_update_current_state() in pci_restore_standard_config()
returns D0, which was the case that triggered commit cc2893b6af52 IIRC.

So scratch this one, please, and I'll submit the safer option.

> +
> +	pci_restore_standard_config(pci_dev);
>  }
>  
>  /*
> @@ -713,6 +714,7 @@ static void pci_pm_complete(struct devic
>  		pci_power_t pre_sleep_state = pci_dev->current_state;
>  
>  		pci_refresh_power_state(pci_dev);
> +		pci_update_current_state(pci_dev, pci_dev->current_state);
>  		/*
>  		 * On platforms with ACPI this check may also trigger for
>  		 * devices sharing power resources if one of those power

[PATCH] PCI: PM: Fix pci_power_up()

[Rafael J. Wysocki]

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

There is an arbitrary difference between the system resume and
runtime resume code paths for PCI devices regarding the delay to
apply when switching the devices from D3cold to D0.

Namely, pci_restore_standard_config() used in the runtime resume
code path calls pci_set_power_state() which in turn invokes
__pci_start_power_transition() to power up the device through the
platform firmware and that function applies the transition delay
(as per PCI Express Base Specification Revision 2.0, Section 6.6.1).
However, pci_pm_default_resume_early() used in the system resume
code path calls pci_power_up() which doesn't apply the delay at
all and that causes issues to occur during resume from
suspend-to-idle on some systems where the delay is required.

Since there is no reason for that difference to exist, modify
pci_power_up() to follow pci_set_power_state() more closely and
invoke __pci_start_power_transition() from there to call the
platform firmware to power up the device (in case that's necessary).

Fixes: db288c9c5f9d ("PCI / PM: restore the original behavior of pci_set_power_state()")
Reported-by: Daniel Drake <drake@endlessm.com> 
Link: https://lore.kernel.org/linux-pm/CAD8Lp44TYxrMgPLkHCqF9hv6smEurMXvmmvmtyFhZ6Q4SE+dig@mail.gmail.com/T/#m21be74af263c6a34f36e0fc5c77c5449d9406925
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

Daniel, please test this one.

---
 drivers/pci/pci.c |   24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

Index: linux-pm/drivers/pci/pci.c
===================================================================
--- linux-pm.orig/drivers/pci/pci.c
+++ linux-pm/drivers/pci/pci.c
@@ -959,19 +959,6 @@ void pci_refresh_power_state(struct pci_
 }
 
 /**
- * pci_power_up - Put the given device into D0 forcibly
- * @dev: PCI device to power up
- */
-void pci_power_up(struct pci_dev *dev)
-{
-	if (platform_pci_power_manageable(dev))
-		platform_pci_set_power_state(dev, PCI_D0);
-
-	pci_raw_set_power_state(dev, PCI_D0);
-	pci_update_current_state(dev, PCI_D0);
-}
-
-/**
  * pci_platform_power_transition - Use platform to change device power state
  * @dev: PCI device to handle.
  * @state: State to put the device into.
@@ -1154,6 +1141,17 @@ int pci_set_power_state(struct pci_dev *
 EXPORT_SYMBOL(pci_set_power_state);
 
 /**
+ * pci_power_up - Put the given device into D0 forcibly
+ * @dev: PCI device to power up
+ */
+void pci_power_up(struct pci_dev *dev)
+{
+	__pci_start_power_transition(dev, PCI_D0);
+	pci_raw_set_power_state(dev, PCI_D0);
+	pci_update_current_state(dev, PCI_D0);
+}
+
+/**
  * pci_choose_state - Choose the power state of a PCI device
  * @dev: PCI device to be suspended
  * @state: target sleep state for the whole system. This is the value

Re: [PATCH] PCI: PM: Fix pci_power_up()

[Daniel Drake]

On Mon, Oct 14, 2019 at 7:25 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> Since there is no reason for that difference to exist, modify
> pci_power_up() to follow pci_set_power_state() more closely and
> invoke __pci_start_power_transition() from there to call the
> platform firmware to power up the device (in case that's necessary).
>
> Fixes: db288c9c5f9d ("PCI / PM: restore the original behavior of pci_set_power_state()")
> Reported-by: Daniel Drake <drake@endlessm.com>
> Link: https://lore.kernel.org/linux-pm/CAD8Lp44TYxrMgPLkHCqF9hv6smEurMXvmmvmtyFhZ6Q4SE+dig@mail.gmail.com/T/#m21be74af263c6a34f36e0fc5c77c5449d9406925
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>
> Daniel, please test this one.

This one is working too, thanks

Daniel

Re: [PATCH] PCI: PM: Fix pci_power_up()

[Rafael J. Wysocki]

On Tue, Oct 15, 2019 at 7:11 AM Daniel Drake <drake@endlessm.com> wrote:
>
> On Mon, Oct 14, 2019 at 7:25 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > Since there is no reason for that difference to exist, modify
> > pci_power_up() to follow pci_set_power_state() more closely and
> > invoke __pci_start_power_transition() from there to call the
> > platform firmware to power up the device (in case that's necessary).
> >
> > Fixes: db288c9c5f9d ("PCI / PM: restore the original behavior of pci_set_power_state()")
> > Reported-by: Daniel Drake <drake@endlessm.com>
> > Link: https://lore.kernel.org/linux-pm/CAD8Lp44TYxrMgPLkHCqF9hv6smEurMXvmmvmtyFhZ6Q4SE+dig@mail.gmail.com/T/#m21be74af263c6a34f36e0fc5c77c5449d9406925
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >
> > Daniel, please test this one.
>
> This one is working too, thanks

Thank you!

Bjorn, any concerns?

Re: [PATCH] PCI: PM: Fix pci_power_up()

[Bjorn Helgaas]

On Mon, Oct 14, 2019 at 01:25:00PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> There is an arbitrary difference between the system resume and
> runtime resume code paths for PCI devices regarding the delay to
> apply when switching the devices from D3cold to D0.
> 
> Namely, pci_restore_standard_config() used in the runtime resume
> code path calls pci_set_power_state() which in turn invokes
> __pci_start_power_transition() to power up the device through the
> platform firmware and that function applies the transition delay
> (as per PCI Express Base Specification Revision 2.0, Section 6.6.1).
> However, pci_pm_default_resume_early() used in the system resume
> code path calls pci_power_up() which doesn't apply the delay at
> all and that causes issues to occur during resume from
> suspend-to-idle on some systems where the delay is required.
> 
> Since there is no reason for that difference to exist, modify
> pci_power_up() to follow pci_set_power_state() more closely and
> invoke __pci_start_power_transition() from there to call the
> platform firmware to power up the device (in case that's necessary).
> 
> Fixes: db288c9c5f9d ("PCI / PM: restore the original behavior of pci_set_power_state()")
> Reported-by: Daniel Drake <drake@endlessm.com> 
> Link: https://lore.kernel.org/linux-pm/CAD8Lp44TYxrMgPLkHCqF9hv6smEurMXvmmvmtyFhZ6Q4SE+dig@mail.gmail.com/T/#m21be74af263c6a34f36e0fc5c77c5449d9406925
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> Daniel, please test this one.
> 
> ---
>  drivers/pci/pci.c |   24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
> 
> Index: linux-pm/drivers/pci/pci.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/pci.c
> +++ linux-pm/drivers/pci/pci.c
> @@ -959,19 +959,6 @@ void pci_refresh_power_state(struct pci_
>  }
>  
>  /**
> - * pci_power_up - Put the given device into D0 forcibly
> - * @dev: PCI device to power up
> - */
> -void pci_power_up(struct pci_dev *dev)
> -{
> -	if (platform_pci_power_manageable(dev))
> -		platform_pci_set_power_state(dev, PCI_D0);
> -
> -	pci_raw_set_power_state(dev, PCI_D0);
> -	pci_update_current_state(dev, PCI_D0);
> -}
> -
> -/**
>   * pci_platform_power_transition - Use platform to change device power state
>   * @dev: PCI device to handle.
>   * @state: State to put the device into.
> @@ -1154,6 +1141,17 @@ int pci_set_power_state(struct pci_dev *
>  EXPORT_SYMBOL(pci_set_power_state);
>  
>  /**
> + * pci_power_up - Put the given device into D0 forcibly

Not specifically for this patch, but what does "forcibly" mean?

> + * @dev: PCI device to power up
> + */
> +void pci_power_up(struct pci_dev *dev)
> +{
> +	__pci_start_power_transition(dev, PCI_D0);
> +	pci_raw_set_power_state(dev, PCI_D0);
> +	pci_update_current_state(dev, PCI_D0);

There's not very much difference between:

  pci_power_up(dev);

and

  pci_set_power_state(dev, PCI_D0);

It looks like the main difference is that pci_set_power_state() calls
__pci_complete_power_transition(), which ultimately calls
acpi_pci_set_power_state() (for ACPI systems).

So maybe "forcibly" means something like "ignoring any platform power
management methods"?  It's not obvious to me when we should skip the
platform stuff or whether the skipping should be done at the high
level (like calling either pci_power_up() or pci_set_power_state()) or
at a lower level (e.g., if everybody called pci_set_power_state() and
it could internally tell whether we're skipping the platform part).

If we could unify the paths as much as possible, that would be nice,
but if it's not feasible, it's not feasible.  If you'd like me to push
this for v5.4, let me know, otherwise you can apply my:

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

> +}
> +
> +/**
>   * pci_choose_state - Choose the power state of a PCI device
>   * @dev: PCI device to be suspended
>   * @state: target sleep state for the whole system. This is the value
> 
> 
> 

Re: [PATCH] PCI: PM: Fix pci_power_up()

[Rafael J. Wysocki]

On Tue, Oct 15, 2019 at 9:20 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Mon, Oct 14, 2019 at 01:25:00PM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > There is an arbitrary difference between the system resume and
> > runtime resume code paths for PCI devices regarding the delay to
> > apply when switching the devices from D3cold to D0.
> >
> > Namely, pci_restore_standard_config() used in the runtime resume
> > code path calls pci_set_power_state() which in turn invokes
> > __pci_start_power_transition() to power up the device through the
> > platform firmware and that function applies the transition delay
> > (as per PCI Express Base Specification Revision 2.0, Section 6.6.1).
> > However, pci_pm_default_resume_early() used in the system resume
> > code path calls pci_power_up() which doesn't apply the delay at
> > all and that causes issues to occur during resume from
> > suspend-to-idle on some systems where the delay is required.
> >
> > Since there is no reason for that difference to exist, modify
> > pci_power_up() to follow pci_set_power_state() more closely and
> > invoke __pci_start_power_transition() from there to call the
> > platform firmware to power up the device (in case that's necessary).
> >
> > Fixes: db288c9c5f9d ("PCI / PM: restore the original behavior of pci_set_power_state()")
> > Reported-by: Daniel Drake <drake@endlessm.com>
> > Link: https://lore.kernel.org/linux-pm/CAD8Lp44TYxrMgPLkHCqF9hv6smEurMXvmmvmtyFhZ6Q4SE+dig@mail.gmail.com/T/#m21be74af263c6a34f36e0fc5c77c5449d9406925
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >
> > Daniel, please test this one.
> >
> > ---
> >  drivers/pci/pci.c |   24 +++++++++++-------------
> >  1 file changed, 11 insertions(+), 13 deletions(-)
> >
> > Index: linux-pm/drivers/pci/pci.c
> > ===================================================================
> > --- linux-pm.orig/drivers/pci/pci.c
> > +++ linux-pm/drivers/pci/pci.c
> > @@ -959,19 +959,6 @@ void pci_refresh_power_state(struct pci_
> >  }
> >
> >  /**
> > - * pci_power_up - Put the given device into D0 forcibly
> > - * @dev: PCI device to power up
> > - */
> > -void pci_power_up(struct pci_dev *dev)
> > -{
> > -     if (platform_pci_power_manageable(dev))
> > -             platform_pci_set_power_state(dev, PCI_D0);
> > -
> > -     pci_raw_set_power_state(dev, PCI_D0);
> > -     pci_update_current_state(dev, PCI_D0);
> > -}
> > -
> > -/**
> >   * pci_platform_power_transition - Use platform to change device power state
> >   * @dev: PCI device to handle.
> >   * @state: State to put the device into.
> > @@ -1154,6 +1141,17 @@ int pci_set_power_state(struct pci_dev *
> >  EXPORT_SYMBOL(pci_set_power_state);
> >
> >  /**
> > + * pci_power_up - Put the given device into D0 forcibly
>
> Not specifically for this patch, but what does "forcibly" mean?
>
> > + * @dev: PCI device to power up
> > + */
> > +void pci_power_up(struct pci_dev *dev)
> > +{
> > +     __pci_start_power_transition(dev, PCI_D0);
> > +     pci_raw_set_power_state(dev, PCI_D0);
> > +     pci_update_current_state(dev, PCI_D0);
>
> There's not very much difference between:
>
>   pci_power_up(dev);
>
> and
>
>   pci_set_power_state(dev, PCI_D0);
>
> It looks like the main difference is that pci_set_power_state() calls
> __pci_complete_power_transition(), which ultimately calls
> acpi_pci_set_power_state() (for ACPI systems).

Yes, it does, for power states deeper than D0, which is not the case here.

The main difference is the dev->current_state == state check in
pci_set_power_state(), but in the resume case specifically
dev->current_state == PCI_D0 doesn't matter, because the real power
state of the device may be different.

> So maybe "forcibly" means something like "ignoring any platform power
> management methods"?

It means "go into D0 no matter what the current cached value is".

>  It's not obvious to me when we should skip the
> platform stuff or whether the skipping should be done at the high
> level (like calling either pci_power_up() or pci_set_power_state()) or
> at a lower level (e.g., if everybody called pci_set_power_state() and
> it could internally tell whether we're skipping the platform part).

For transitions into D0 __pci_start_power_transition() is the platform
stuff, so we don't skip it and the other things that are present in
pci_set_power_state() and are not there in pci_power_up() are simply
unnecessary for transitions to D0.

> If we could unify the paths as much as possible, that would be nice,
> but if it's not feasible, it's not feasible.

It kind of is, but I'd prefer to do it on top of this patch.

First, the pci_update_current_state() in pci_power_up() can be moved
to pci_pm_default_resume_early() which is the only caller of
pci_power_up(). [The role of that pci_update_current_state() is to
change the current_state value to D3cold if the device is not
accessible (or the platform firmware says that it is D3cold, which may
be the case after a failing attempt to use it to switch the device
over to D0).]  Next, if pci_power_up() is modified to return the
return value of pci_raw_set_power_state(), pci_set_power_state() can
be implemented (roughly) as

sanitize the state argument

if (dev->current_state == state)
        return 0;

if (state == PCI_D0)
        return pci_power_up();

carry out a transition into a deeper power state.

And so pci_power_up() will be used by pci_set_power_state(), for
transitions into D0, and (directly) by pci_pm_default_resume_early().

How does that sound?

> If you'd like me to push this for v5.4, let me know, otherwise you
> can apply my:
>
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>

I will, thanks!