| Message ID | 20190927090202.1468-1-drake@endlessm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI: also apply D3 delay when leaving D3cold | expand |
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);
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
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);
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!
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.
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);
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(-)