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

["Rafael J. Wysocki" <rjw@rjwysocki.net>]

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