| Message ID | 1668247.RaJIPSxJUN@kreacher (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Rafael Wysocki |
| Headers | show |
| Series | [v2] PCI: PM: Skip devices in D0 for suspend-to-idle | expand |
Hi Rafael, On 13/06/2019 22:59, 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, the power state of a PCI bridge with a > downstream device in D0 must be 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), but that is not actively enforced > 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 is pointless, 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> > Tested-by: Kai-Heng Feng <kai.heng.feng@canonical.com> I have noticed a regression in both the mainline and -next branches on one of our boards when testing suspend. The bisect is point to this commit and reverting on top of mainline does fix the problem. So far I have not looked at this in close detail but kernel log is showing ... [ 52.775138] PM: suspend entry (deep) [ 52.779040] Filesystems sync: 0.000 seconds [ 52.783476] Freezing user space processes ... (elapsed 0.001 seconds) done. [ 52.791891] OOM killer disabled. [ 52.795174] Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done. [ 52.803752] printk: Suspending console(s) (use no_console_suspend to debug) [ 52.823750] r8169 0000:01:00.0 eth0: Link is Down [ 52.823908] pci_generic_config_write32: 22 callbacks suppressed [ 52.823914] pci_bus 0000:00: 2-byte config write to 0000:00:02.0 offset 0x9c may corrupt adjacent RW1C bits [ 53.045383] Disabling non-boot CPUs ... [ 53.048481] Entering suspend state LP1 [ 53.048508] Enabling non-boot CPUs ... [ 53.049402] CPU1 is up [ 53.050182] CPU2 is up [ 53.051017] CPU3 is up [ 53.051613] tegra-pcie 1003000.pcie: probing port 1, using 1 lanes [ 53.069689] pcieport 0000:00:02.0: nv_msi_ht_cap_quirk didn't locate host bridge [ 53.093751] r8169 0000:01:00.0: Refused to change power state, currently in D3 [ 53.156586] tegra-pcie 1003000.pcie: Slot present pin change, signature: 00000004 [ 53.156593] tegra-pcie 1003000.pcie: Response decoding error, signature: 10010045 [ 53.156596] tegra-pcie 1003000.pcie: FPCI address: fe10010044 [ 53.156711] tegra-pcie 1003000.pcie: Response decoding error, signature: 2000000c [ 53.156714] tegra-pcie 1003000.pcie: FPCI address: 2000000c [ 53.156719] tegra-pcie 1003000.pcie: Response decoding error, signature: 20000001 [ 53.156722] tegra-pcie 1003000.pcie: FPCI address: 20000000 [ 53.177372] pci_bus 0000:00: 2-byte config write to 0000:00:02.0 offset 0x88 may corrupt adjacent RW1C bits [ 53.177379] pci_bus 0000:00: 2-byte config write to 0000:00:02.0 offset 0x90 may corrupt adjacent RW1C bits [ 53.177384] pci_bus 0000:00: 2-byte config write to 0000:00:02.0 offset 0x98 may corrupt adjacent RW1C bits [ 53.177389] pci_bus 0000:00: 2-byte config write to 0000:00:02.0 offset 0x9c may corrupt adjacent RW1C bits [ 53.177394] pci_bus 0000:00: 2-byte config write to 0000:00:02.0 offset 0xa8 may corrupt adjacent RW1C bits [ 53.177399] pci_bus 0000:00: 2-byte config write to 0000:00:02.0 offset 0xb0 may corrupt adjacent RW1C bits [ 53.177451] pci_bus 0000:00: 2-byte config write to 0000:00:02.0 offset 0x52 may corrupt adjacent RW1C bits [ 53.177461] pci_bus 0000:00: 2-byte config write to 0000:00:02.0 offset 0x52 may corrupt adjacent RW1C bits [ 53.177470] pci_bus 0000:00: 2-byte config write to 0000:00:02.0 offset 0x5c may corrupt adjacent RW1C bits [ 53.187746] r8169 0000:01:00.0 eth0: rtl_eriar_cond == 1 (loop: 100, delay: 100). [ 53.188030] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 53.224105] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 53.224394] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 53.224679] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 53.224966] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 53.225247] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 53.225528] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 53.225813] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 53.226089] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 53.226372] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 53.226654] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 53.226934] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 53.227213] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 53.227495] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 53.227773] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 53.228050] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 53.228326] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 53.228609] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 53.228896] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 53.229181] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 54.064108] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 54.064429] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 54.064713] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 54.064996] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 54.065013] Generic Realtek PHY r8169-100:00: Master/Slave resolution failed, maybe conflicting manual settings? [ 54.065016] ------------[ cut here ]------------ [ 54.065025] WARNING: CPU: 1 PID: 617 at /home/jonathanh/workdir/tegra/mlt-linux_torvalds/kernel/drivers/net/phy/phy.c:735 phy_error+0x1c/0x54 [ 54.065028] Modules linked in: ttm [ 54.065039] CPU: 1 PID: 617 Comm: kworker/1:2 Not tainted 5.2.0-rc6 #1 [ 54.065041] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree) [ 54.065049] Workqueue: events_power_efficient phy_state_machine [ 54.065068] [<c0112244>] (unwind_backtrace) from [<c010cad8>] (show_stack+0x10/0x14) [ 54.065075] [<c010cad8>] (show_stack) from [<c0a60624>] (dump_stack+0xb4/0xc8) [ 54.065082] [<c0a60624>] (dump_stack) from [<c0123cbc>] (__warn+0xe0/0xf8) [ 54.065090] [<c0123cbc>] (__warn) from [<c0123dec>] (warn_slowpath_null+0x40/0x48) [ 54.065095] [<c0123dec>] (warn_slowpath_null) from [<c06173f8>] (phy_error+0x1c/0x54) [ 54.065101] [<c06173f8>] (phy_error) from [<c06184ec>] (phy_state_machine+0x64/0x1c0) [ 54.065112] [<c06184ec>] (phy_state_machine) from [<c013e744>] (process_one_work+0x204/0x578) [ 54.065119] [<c013e744>] (process_one_work) from [<c013f444>] (worker_thread+0x44/0x584) [ 54.065123] [<c013f444>] (worker_thread) from [<c01445d4>] (kthread+0x148/0x150) [ 54.065128] [<c01445d4>] (kthread) from [<c01010e8>] (ret_from_fork+0x14/0x2c) [ 54.065131] Exception stack(0xe91b3fb0 to 0xe91b3ff8) [ 54.065134] 3fa0: 00000000 00000000 00000000 00000000 [ 54.065138] 3fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 [ 54.065141] 3fe0: 00000000 00000000 00000000 00000000 00000013 00000000 [ 54.065145] ---[ end trace f59188238fc6fed4 ]--- [ 54.065168] r8169 0000:01:00.0 eth0: Link is Down [ 54.075411] r8169 0000:01:00.0 eth0: rtl_chipcmd_cond == 1 (loop: 100, delay: 100). [ 54.085652] r8169 0000:01:00.0 eth0: rtl_eriar_cond == 1 (loop: 100, delay: 100). [ 54.095833] r8169 0000:01:00.0 eth0: rtl_eriar_cond == 1 (loop: 100, delay: 100). [ 54.106017] r8169 0000:01:00.0 eth0: rtl_eriar_cond == 1 (loop: 100, delay: 100). [ 54.116214] r8169 0000:01:00.0 eth0: rtl_eriar_cond == 1 (loop: 100, delay: 100). [ 54.126419] r8169 0000:01:00.0 eth0: rtl_eriar_cond == 1 (loop: 100, delay: 100). [ 54.136613] r8169 0000:01:00.0 eth0: rtl_eriar_cond == 1 (loop: 100, delay: 100). [ 54.146829] r8169 0000:01:00.0 eth0: rtl_eriar_cond == 1 (loop: 100, delay: 100). [ 54.157030] r8169 0000:01:00.0 eth0: rtl_eriar_cond == 1 (loop: 100, delay: 100). [ 54.167223] r8169 0000:01:00.0 eth0: rtl_eriar_cond == 1 (loop: 100, delay: 100). [ 54.177433] r8169 0000:01:00.0 eth0: rtl_eriar_cond == 1 (loop: 100, delay: 100). [ 54.187635] r8169 0000:01:00.0 eth0: rtl_eriar_cond == 1 (loop: 100, delay: 100). [ 54.197840] r8169 0000:01:00.0 eth0: rtl_eriar_cond == 1 (loop: 100, delay: 100). [ 54.199008] r8169 0000:01:00.0 eth0: rtl_ephyar_cond == 1 (loop: 100, delay: 10). [ 54.200173] r8169 0000:01:00.0 eth0: rtl_ephyar_cond == 1 (loop: 100, delay: 10). [ 54.201336] r8169 0000:01:00.0 eth0: rtl_ephyar_cond == 1 (loop: 100, delay: 10). [ 54.202502] r8169 0000:01:00.0 eth0: rtl_ephyar_cond == 1 (loop: 100, delay: 10). [ 54.203018] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 54.546178] ata1: SATA link down (SStatus 0 SControl 300) [ 56.810813] OOM killer enabled. [ 56.813953] Restarting tasks ... done. [ 56.819622] PM: suspend exit [ 72.504381] nfs: server 192.168.99.1 not responding, still trying [ 78.104369] nfs: server 192.168.99.1 not responding, still trying Let me know if you have any thoughts. Cheers Jon
On Mon, Jun 24, 2019 at 2:43 PM Jon Hunter <jonathanh@nvidia.com> wrote: > > Hi Rafael, > > On 13/06/2019 22:59, 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, the power state of a PCI bridge with a > > downstream device in D0 must be 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), but that is not actively enforced > > 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 is pointless, 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> > > Tested-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > > I have noticed a regression in both the mainline and -next branches on > one of our boards when testing suspend. The bisect is point to this > commit and reverting on top of mainline does fix the problem. So far I > have not looked at this in close detail but kernel log is showing ... Can you please collect a log like that, but with dynamic debug in pci-driver.c enabled? Note that reverting this commit is rather out of the question, so we need to get to the bottom of the failure.
On Mon, Jun 24, 2019 at 11:37 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Mon, Jun 24, 2019 at 2:43 PM Jon Hunter <jonathanh@nvidia.com> wrote: > > > > Hi Rafael, > > > > On 13/06/2019 22:59, 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, the power state of a PCI bridge with a > > > downstream device in D0 must be 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), but that is not actively enforced > > > 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 is pointless, 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> > > > Tested-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > > > > I have noticed a regression in both the mainline and -next branches on > > one of our boards when testing suspend. The bisect is point to this > > commit and reverting on top of mainline does fix the problem. So far I > > have not looked at this in close detail but kernel log is showing ... > > Can you please collect a log like that, but with dynamic debug in > pci-driver.c enabled? > > Note that reverting this commit is rather out of the question, so we > need to get to the bottom of the failure. I suspect that there is a problem with the pm_suspend_via_firmware() check which returns 'false' on the affected board, but the platform actually removes power from devices left in D0 during suspend. I guess it would be more appropriate to check something like pm_suspend_no_platform() which would return 'true' in the suspend-to-idle patch w/ ACPI.
On Tuesday, June 25, 2019 12:20:26 AM CEST Rafael J. Wysocki wrote: > On Mon, Jun 24, 2019 at 11:37 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > On Mon, Jun 24, 2019 at 2:43 PM Jon Hunter <jonathanh@nvidia.com> wrote: > > > > > > Hi Rafael, > > > > > > On 13/06/2019 22:59, 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, the power state of a PCI bridge with a > > > > downstream device in D0 must be 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), but that is not actively enforced > > > > 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 is pointless, 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> > > > > Tested-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > > > > > > I have noticed a regression in both the mainline and -next branches on > > > one of our boards when testing suspend. The bisect is point to this > > > commit and reverting on top of mainline does fix the problem. So far I > > > have not looked at this in close detail but kernel log is showing ... > > > > Can you please collect a log like that, but with dynamic debug in > > pci-driver.c enabled? > > > > Note that reverting this commit is rather out of the question, so we > > need to get to the bottom of the failure. > > I suspect that there is a problem with the pm_suspend_via_firmware() > check which returns 'false' on the affected board, but the platform > actually removes power from devices left in D0 during suspend. > > I guess it would be more appropriate to check something like > pm_suspend_no_platform() which would return 'true' in the > suspend-to-idle patch w/ ACPI. So I wonder if the patch below makes any difference? --- drivers/pci/pci-driver.c | 8 ++++---- include/linux/suspend.h | 26 ++++++++++++++++++++++++-- kernel/power/suspend.c | 3 +++ 3 files changed, 31 insertions(+), 6 deletions(-) Index: linux-pm/include/linux/suspend.h =================================================================== --- linux-pm.orig/include/linux/suspend.h +++ linux-pm/include/linux/suspend.h @@ -209,8 +209,9 @@ extern int suspend_valid_only_mem(suspen extern unsigned int pm_suspend_global_flags; -#define PM_SUSPEND_FLAG_FW_SUSPEND (1 << 0) -#define PM_SUSPEND_FLAG_FW_RESUME (1 << 1) +#define PM_SUSPEND_FLAG_FW_SUSPEND BIT(0) +#define PM_SUSPEND_FLAG_FW_RESUME BIT(1) +#define PM_SUSPEND_FLAG_NO_PLATFORM BIT(2) static inline void pm_suspend_clear_flags(void) { @@ -227,6 +228,11 @@ static inline void pm_set_resume_via_fir pm_suspend_global_flags |= PM_SUSPEND_FLAG_FW_RESUME; } +static inline void pm_set_suspend_no_platform(void) +{ + pm_suspend_global_flags |= PM_SUSPEND_FLAG_NO_PLATFORM; +} + /** * pm_suspend_via_firmware - Check if platform firmware will suspend the system. * @@ -268,6 +274,22 @@ static inline bool pm_resume_via_firmwar return !!(pm_suspend_global_flags & PM_SUSPEND_FLAG_FW_RESUME); } +/** + * pm_suspend_no_platform - Check if platform may change device power states. + * + * To be called during system-wide power management transitions to sleep states + * or during the subsequent system-wide transitions back to the working state. + * + * Return 'true' if the power states of devices remain under full control of the + * kernel throughout the system-wide suspend and resume cycle in progress (that + * is, if a device is put into a certain power state during suspend, it can be + * expected to remain in that state during resume). + */ +static inline bool pm_suspend_no_platform(void) +{ + return !!(pm_suspend_global_flags & PM_SUSPEND_FLAG_NO_PLATFORM); +} + /* Suspend-to-idle state machnine. */ enum s2idle_states { S2IDLE_STATE_NONE, /* Not suspended/suspending. */ Index: linux-pm/kernel/power/suspend.c =================================================================== --- linux-pm.orig/kernel/power/suspend.c +++ linux-pm/kernel/power/suspend.c @@ -493,6 +493,9 @@ int suspend_devices_and_enter(suspend_st pm_suspend_target_state = state; + if (state == PM_SUSPEND_TO_IDLE) + pm_set_suspend_no_platform(); + error = platform_suspend_begin(state); if (error) goto Close; Index: linux-pm/drivers/pci/pci-driver.c =================================================================== --- linux-pm.orig/drivers/pci/pci-driver.c +++ linux-pm/drivers/pci/pci-driver.c @@ -870,7 +870,7 @@ static int pci_pm_suspend_noirq(struct d pci_dev->bus->self->skip_bus_pm = true; } - if (pci_dev->skip_bus_pm && !pm_suspend_via_firmware()) { + if (pci_dev->skip_bus_pm && pm_suspend_no_platform()) { dev_dbg(dev, "PCI PM: Skipped\n"); goto Fixup; } @@ -925,10 +925,10 @@ static int pci_pm_resume_noirq(struct de /* * 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. + * configuration here and attempting to put them into D0 again is + * pointless, so avoid doing that. */ - if (!pci_dev->skip_bus_pm || pm_suspend_via_firmware()) + if (!(pci_dev->skip_bus_pm && pm_suspend_no_platform())) pci_pm_default_resume_early(pci_dev); pci_fixup_device(pci_fixup_resume_early, pci_dev);
On 24/06/2019 22:37, Rafael J. Wysocki wrote: > On Mon, Jun 24, 2019 at 2:43 PM Jon Hunter <jonathanh@nvidia.com> wrote: >> >> Hi Rafael, >> >> On 13/06/2019 22:59, 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, the power state of a PCI bridge with a >>> downstream device in D0 must be 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), but that is not actively enforced >>> 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 is pointless, 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> >>> Tested-by: Kai-Heng Feng <kai.heng.feng@canonical.com> >> >> I have noticed a regression in both the mainline and -next branches on >> one of our boards when testing suspend. The bisect is point to this >> commit and reverting on top of mainline does fix the problem. So far I >> have not looked at this in close detail but kernel log is showing ... > > Can you please collect a log like that, but with dynamic debug in > pci-driver.c enabled? Yes, here you go ... [ 52.939258] PM: suspend entry (deep) [ 52.942963] Filesystems sync: 0.000 seconds [ 52.947596] Freezing user space processes ... (elapsed 0.001 seconds) done. [ 52.956145] OOM killer disabled. [ 52.959371] Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done. [ 52.968088] printk: Suspending console(s) (use no_console_suspend to debug) [ 52.992168] r8169 0000:01:00.0 eth0: Link is Down [ 52.992245] pci_generic_config_write32: 22 callbacks suppressed [ 52.992250] pci_bus 0000:00: 2-byte config write to 0000:00:02.0 offset 0x9c may corrupt adjacent RW1C bits [ 53.204186] r8169 0000:01:00.0: PCI PM: Suspend power state: D3hot [ 53.204221] pcieport 0000:00:02.0: PCI PM: Suspend power state: D0 [ 53.204224] pcieport 0000:00:02.0: PCI PM: Skipped [ 53.215716] Disabling non-boot CPUs ... [ 53.218833] Entering suspend state LP1 [ 53.218860] Enabling non-boot CPUs ... [ 53.219731] CPU1 is up [ 53.220482] CPU2 is up [ 53.221289] CPU3 is up [ 53.221850] tegra-pcie 1003000.pcie: probing port 1, using 1 lanes [ 53.239925] pcieport 0000:00:02.0: nv_msi_ht_cap_quirk didn't locate host bridge [ 53.264145] r8169 0000:01:00.0: Refused to change power state, currently in D3 [ 53.326969] tegra-pcie 1003000.pcie: Slot present pin change, signature: 00000004 [ 53.326975] tegra-pcie 1003000.pcie: Response decoding error, signature: 10010045 [ 53.326978] tegra-pcie 1003000.pcie: FPCI address: fe10010044 [ 53.327091] tegra-pcie 1003000.pcie: Response decoding error, signature: 2000000c [ 53.327095] tegra-pcie 1003000.pcie: FPCI address: 2000000c [ 53.327099] tegra-pcie 1003000.pcie: Response decoding error, signature: 20000001 [ 53.327102] tegra-pcie 1003000.pcie: FPCI address: 20000000 [ 53.347944] pci_bus 0000:00: 2-byte config write to 0000:00:02.0 offset 0x88 may corrupt adjacent RW1C bits [ 53.347955] pci_bus 0000:00: 2-byte config write to 0000:00:02.0 offset 0x90 may corrupt adjacent RW1C bits [ 53.347962] pci_bus 0000:00: 2-byte config write to 0000:00:02.0 offset 0x98 may corrupt adjacent RW1C bits [ 53.347969] pci_bus 0000:00: 2-byte config write to 0000:00:02.0 offset 0x9c may corrupt adjacent RW1C bits [ 53.347977] pci_bus 0000:00: 2-byte config write to 0000:00:02.0 offset 0xa8 may corrupt adjacent RW1C bits [ 53.347984] pci_bus 0000:00: 2-byte config write to 0000:00:02.0 offset 0xb0 may corrupt adjacent RW1C bits [ 53.348025] pci_bus 0000:00: 2-byte config write to 0000:00:02.0 offset 0x52 may corrupt adjacent RW1C bits [ 53.348033] pci_bus 0000:00: 2-byte config write to 0000:00:02.0 offset 0x52 may corrupt adjacent RW1C bits [ 53.348043] pci_bus 0000:00: 2-byte config write to 0000:00:02.0 offset 0x5c may corrupt adjacent RW1C bits [ 53.358310] r8169 0000:01:00.0 eth0: rtl_eriar_cond == 1 (loop: 100, delay: 100). [ 53.358592] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 53.394498] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 53.394789] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 53.395072] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 53.395352] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 53.395635] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 53.395919] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 53.396209] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 53.396488] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 53.396771] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 53.397055] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 53.397330] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 53.397608] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 53.397884] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 53.398162] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 53.398441] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 53.398721] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 53.399006] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 53.399295] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 53.399579] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 54.234501] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 54.234819] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 54.235104] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 54.235386] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 54.235403] Generic Realtek PHY r8169-100:00: Master/Slave resolution failed, maybe conflicting manual settings? [ 54.235406] ------------[ cut here ]------------ [ 54.235416] WARNING: CPU: 3 PID: 112 at /home/jonathanh/workdir/tegra/mlt-linux_torvalds/kernel/drivers/net/phy/phy.c:735 phy_error+0x1c/0x54 [ 54.235419] Modules linked in: ttm [ 54.235429] CPU: 3 PID: 112 Comm: kworker/3:1 Not tainted 5.2.0-rc6-dirty #3 [ 54.235431] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree) [ 54.235441] Workqueue: events_power_efficient phy_state_machine [ 54.235455] [<c0112244>] (unwind_backtrace) from [<c010cad8>] (show_stack+0x10/0x14) [ 54.235463] [<c010cad8>] (show_stack) from [<c0a606a4>] (dump_stack+0xb4/0xc8) [ 54.235471] [<c0a606a4>] (dump_stack) from [<c0123cbc>] (__warn+0xe0/0xf8) [ 54.235477] [<c0123cbc>] (__warn) from [<c0123dec>] (warn_slowpath_null+0x40/0x48) [ 54.235482] [<c0123dec>] (warn_slowpath_null) from [<c0617470>] (phy_error+0x1c/0x54) [ 54.235488] [<c0617470>] (phy_error) from [<c0618564>] (phy_state_machine+0x64/0x1c0) [ 54.235498] [<c0618564>] (phy_state_machine) from [<c013e744>] (process_one_work+0x204/0x578) [ 54.235503] [<c013e744>] (process_one_work) from [<c013f444>] (worker_thread+0x44/0x584) [ 54.235507] [<c013f444>] (worker_thread) from [<c01445d4>] (kthread+0x148/0x150) [ 54.235512] [<c01445d4>] (kthread) from [<c01010e8>] (ret_from_fork+0x14/0x2c) [ 54.235515] Exception stack(0xe9ea1fb0 to 0xe9ea1ff8) [ 54.235518] 1fa0: 00000000 00000000 00000000 00000000 [ 54.235522] 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 [ 54.235525] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 [ 54.235528] ---[ end trace 772a7ce78ffff5e6 ]--- [ 54.235551] r8169 0000:01:00.0 eth0: Link is Down [ 54.245804] r8169 0000:01:00.0 eth0: rtl_chipcmd_cond == 1 (loop: 100, delay: 100). [ 54.256058] r8169 0000:01:00.0 eth0: rtl_eriar_cond == 1 (loop: 100, delay: 100). [ 54.266257] r8169 0000:01:00.0 eth0: rtl_eriar_cond == 1 (loop: 100, delay: 100). [ 54.276454] r8169 0000:01:00.0 eth0: rtl_eriar_cond == 1 (loop: 100, delay: 100). [ 54.286656] r8169 0000:01:00.0 eth0: rtl_eriar_cond == 1 (loop: 100, delay: 100). [ 54.296860] r8169 0000:01:00.0 eth0: rtl_eriar_cond == 1 (loop: 100, delay: 100). [ 54.307064] r8169 0000:01:00.0 eth0: rtl_eriar_cond == 1 (loop: 100, delay: 100). [ 54.317263] r8169 0000:01:00.0 eth0: rtl_eriar_cond == 1 (loop: 100, delay: 100). [ 54.327464] r8169 0000:01:00.0 eth0: rtl_eriar_cond == 1 (loop: 100, delay: 100). [ 54.337660] r8169 0000:01:00.0 eth0: rtl_eriar_cond == 1 (loop: 100, delay: 100). [ 54.347902] r8169 0000:01:00.0 eth0: rtl_eriar_cond == 1 (loop: 100, delay: 100). [ 54.358102] r8169 0000:01:00.0 eth0: rtl_eriar_cond == 1 (loop: 100, delay: 100). [ 54.368303] r8169 0000:01:00.0 eth0: rtl_eriar_cond == 1 (loop: 100, delay: 100). [ 54.369471] r8169 0000:01:00.0 eth0: rtl_ephyar_cond == 1 (loop: 100, delay: 10). [ 54.370637] r8169 0000:01:00.0 eth0: rtl_ephyar_cond == 1 (loop: 100, delay: 10). [ 54.371799] r8169 0000:01:00.0 eth0: rtl_ephyar_cond == 1 (loop: 100, delay: 10). [ 54.372961] r8169 0000:01:00.0 eth0: rtl_ephyar_cond == 1 (loop: 100, delay: 10). [ 54.373416] r8169 0000:01:00.0 eth0: rtl_ocp_gphy_cond == 1 (loop: 10, delay: 25). [ 54.716510] ata1: SATA link down (SStatus 0 SControl 300) [ 56.998780] OOM killer enabled. [ 57.001909] Restarting tasks ... done. [ 57.007392] PM: suspend exit [ 73.144767] nfs: server 192.168.99.1 not responding, still trying [ 77.624567] nfs: server 192.168.99.1 not responding, still trying Cheers Jon
On 25/06/2019 00:09, Rafael J. Wysocki wrote: > On Tuesday, June 25, 2019 12:20:26 AM CEST Rafael J. Wysocki wrote: >> On Mon, Jun 24, 2019 at 11:37 PM Rafael J. Wysocki <rafael@kernel.org> wrote: >>> >>> On Mon, Jun 24, 2019 at 2:43 PM Jon Hunter <jonathanh@nvidia.com> wrote: >>>> >>>> Hi Rafael, >>>> >>>> On 13/06/2019 22:59, 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, the power state of a PCI bridge with a >>>>> downstream device in D0 must be 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), but that is not actively enforced >>>>> 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 is pointless, 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> >>>>> Tested-by: Kai-Heng Feng <kai.heng.feng@canonical.com> >>>> >>>> I have noticed a regression in both the mainline and -next branches on >>>> one of our boards when testing suspend. The bisect is point to this >>>> commit and reverting on top of mainline does fix the problem. So far I >>>> have not looked at this in close detail but kernel log is showing ... >>> >>> Can you please collect a log like that, but with dynamic debug in >>> pci-driver.c enabled? >>> >>> Note that reverting this commit is rather out of the question, so we >>> need to get to the bottom of the failure. >> >> I suspect that there is a problem with the pm_suspend_via_firmware() >> check which returns 'false' on the affected board, but the platform >> actually removes power from devices left in D0 during suspend. >> >> I guess it would be more appropriate to check something like >> pm_suspend_no_platform() which would return 'true' in the >> suspend-to-idle patch w/ ACPI. > > So I wonder if the patch below makes any difference? Thanks. I will try this now and let you know. Cheers! Jon
On 25/06/2019 00:09, Rafael J. Wysocki wrote: > On Tuesday, June 25, 2019 12:20:26 AM CEST Rafael J. Wysocki wrote: >> On Mon, Jun 24, 2019 at 11:37 PM Rafael J. Wysocki <rafael@kernel.org> wrote: >>> >>> On Mon, Jun 24, 2019 at 2:43 PM Jon Hunter <jonathanh@nvidia.com> wrote: >>>> >>>> Hi Rafael, >>>> >>>> On 13/06/2019 22:59, 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, the power state of a PCI bridge with a >>>>> downstream device in D0 must be 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), but that is not actively enforced >>>>> 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 is pointless, 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> >>>>> Tested-by: Kai-Heng Feng <kai.heng.feng@canonical.com> >>>> >>>> I have noticed a regression in both the mainline and -next branches on >>>> one of our boards when testing suspend. The bisect is point to this >>>> commit and reverting on top of mainline does fix the problem. So far I >>>> have not looked at this in close detail but kernel log is showing ... >>> >>> Can you please collect a log like that, but with dynamic debug in >>> pci-driver.c enabled? >>> >>> Note that reverting this commit is rather out of the question, so we >>> need to get to the bottom of the failure. >> >> I suspect that there is a problem with the pm_suspend_via_firmware() >> check which returns 'false' on the affected board, but the platform >> actually removes power from devices left in D0 during suspend. >> >> I guess it would be more appropriate to check something like >> pm_suspend_no_platform() which would return 'true' in the >> suspend-to-idle patch w/ ACPI. > > So I wonder if the patch below makes any difference? > > --- > drivers/pci/pci-driver.c | 8 ++++---- > include/linux/suspend.h | 26 ++++++++++++++++++++++++-- > kernel/power/suspend.c | 3 +++ > 3 files changed, 31 insertions(+), 6 deletions(-) > > Index: linux-pm/include/linux/suspend.h > =================================================================== > --- linux-pm.orig/include/linux/suspend.h > +++ linux-pm/include/linux/suspend.h > @@ -209,8 +209,9 @@ extern int suspend_valid_only_mem(suspen > > extern unsigned int pm_suspend_global_flags; > > -#define PM_SUSPEND_FLAG_FW_SUSPEND (1 << 0) > -#define PM_SUSPEND_FLAG_FW_RESUME (1 << 1) > +#define PM_SUSPEND_FLAG_FW_SUSPEND BIT(0) > +#define PM_SUSPEND_FLAG_FW_RESUME BIT(1) > +#define PM_SUSPEND_FLAG_NO_PLATFORM BIT(2) > > static inline void pm_suspend_clear_flags(void) > { > @@ -227,6 +228,11 @@ static inline void pm_set_resume_via_fir > pm_suspend_global_flags |= PM_SUSPEND_FLAG_FW_RESUME; > } > > +static inline void pm_set_suspend_no_platform(void) > +{ > + pm_suspend_global_flags |= PM_SUSPEND_FLAG_NO_PLATFORM; > +} > + > /** > * pm_suspend_via_firmware - Check if platform firmware will suspend the system. > * > @@ -268,6 +274,22 @@ static inline bool pm_resume_via_firmwar > return !!(pm_suspend_global_flags & PM_SUSPEND_FLAG_FW_RESUME); > } > > +/** > + * pm_suspend_no_platform - Check if platform may change device power states. > + * > + * To be called during system-wide power management transitions to sleep states > + * or during the subsequent system-wide transitions back to the working state. > + * > + * Return 'true' if the power states of devices remain under full control of the > + * kernel throughout the system-wide suspend and resume cycle in progress (that > + * is, if a device is put into a certain power state during suspend, it can be > + * expected to remain in that state during resume). > + */ > +static inline bool pm_suspend_no_platform(void) > +{ > + return !!(pm_suspend_global_flags & PM_SUSPEND_FLAG_NO_PLATFORM); > +} > + > /* Suspend-to-idle state machnine. */ > enum s2idle_states { > S2IDLE_STATE_NONE, /* Not suspended/suspending. */ > Index: linux-pm/kernel/power/suspend.c > =================================================================== > --- linux-pm.orig/kernel/power/suspend.c > +++ linux-pm/kernel/power/suspend.c > @@ -493,6 +493,9 @@ int suspend_devices_and_enter(suspend_st > > pm_suspend_target_state = state; > > + if (state == PM_SUSPEND_TO_IDLE) > + pm_set_suspend_no_platform(); > + > error = platform_suspend_begin(state); > if (error) > goto Close; > Index: linux-pm/drivers/pci/pci-driver.c > =================================================================== > --- linux-pm.orig/drivers/pci/pci-driver.c > +++ linux-pm/drivers/pci/pci-driver.c > @@ -870,7 +870,7 @@ static int pci_pm_suspend_noirq(struct d > pci_dev->bus->self->skip_bus_pm = true; > } > > - if (pci_dev->skip_bus_pm && !pm_suspend_via_firmware()) { > + if (pci_dev->skip_bus_pm && pm_suspend_no_platform()) { > dev_dbg(dev, "PCI PM: Skipped\n"); > goto Fixup; > } > @@ -925,10 +925,10 @@ static int pci_pm_resume_noirq(struct de > /* > * 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. > + * configuration here and attempting to put them into D0 again is > + * pointless, so avoid doing that. > */ > - if (!pci_dev->skip_bus_pm || pm_suspend_via_firmware()) > + if (!(pci_dev->skip_bus_pm && pm_suspend_no_platform())) > pci_pm_default_resume_early(pci_dev); > > pci_fixup_device(pci_fixup_resume_early, pci_dev); I can confirm that above works for me. So ... Tested-by: Jon Hunter <jonathanh@nvidia.com> This looks better ... [ 52.545820] PM: suspend entry (deep) [ 52.549547] Filesystems sync: 0.000 seconds [ 52.553966] Freezing user space processes ... (elapsed 0.001 seconds) done. [ 52.562375] OOM killer disabled. [ 52.565684] Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done. [ 52.574426] printk: Suspending console(s) (use no_console_suspend to debug) [ 52.590255] r8169 0000:01:00.0 eth0: Link is Down [ 52.590483] pci_generic_config_write32: 22 callbacks suppressed [ 52.590488] pci_bus 0000:00: 2-byte config write to 0000:00:02.0 offset 0x9c may corrupt adjacent RW1C bits [ 52.794091] r8169 0000:01:00.0: PCI PM: Suspend power state: D3hot [ 52.794128] pcieport 0000:00:02.0: PCI PM: Suspend power state: D0 [ 52.805674] Disabling non-boot CPUs ... [ 52.809257] Entering suspend state LP1 [ 52.809284] Enabling non-boot CPUs ... [ 52.810184] CPU1 is up [ 52.810973] CPU2 is up [ 52.811819] CPU3 is up [ 52.822259] tegra-pcie 1003000.pcie: probing port 1, using 1 lanes [ 52.840085] pci_bus 0000:00: 2-byte config write to 0000:00:02.0 offset 0x4c may corrupt adjacent RW1C bits [ 52.840096] pci_bus 0000:00: 2-byte config write to 0000:00:02.0 offset 0x88 may corrupt adjacent RW1C bits [ 52.840101] pci_bus 0000:00: 2-byte config write to 0000:00:02.0 offset 0x90 may corrupt adjacent RW1C bits [ 52.840106] pci_bus 0000:00: 2-byte config write to 0000:00:02.0 offset 0x98 may corrupt adjacent RW1C bits [ 52.840111] pci_bus 0000:00: 2-byte config write to 0000:00:02.0 offset 0x9c may corrupt adjacent RW1C bits [ 52.840116] pci_bus 0000:00: 2-byte config write to 0000:00:02.0 offset 0xa8 may corrupt adjacent RW1C bits [ 52.840121] pci_bus 0000:00: 2-byte config write to 0000:00:02.0 offset 0xb0 may corrupt adjacent RW1C bits [ 52.840155] pci_bus 0000:00: 2-byte config write to 0000:00:02.0 offset 0x52 may corrupt adjacent RW1C bits [ 52.840160] pci_bus 0000:00: 2-byte config write to 0000:00:02.0 offset 0x52 may corrupt adjacent RW1C bits [ 52.840184] pcieport 0000:00:02.0: nv_msi_ht_cap_quirk didn't locate host bridge [ 52.864252] tegra-pcie 1003000.pcie: Slot present pin change, signature: 00000004 [ 53.024171] r8169 0000:01:00.0 eth0: Link is Down [ 53.376438] ata1: SATA link down (SStatus 0 SControl 300) [ 55.881131] r8169 0000:01:00.0 eth0: Link is Up - 1Gbps/Full - flow control rx/tx [ 56.012154] OOM killer enabled. [ 56.015295] Restarting tasks ... done. [ 56.019415] PM: suspend exit [ 56.045929] PM: suspend entry (deep) [ 56.049565] Filesystems sync: 0.000 seconds [ 56.053906] Freezing user space processes ... (elapsed 0.001 seconds) done. [ 56.062205] OOM killer disabled. [ 56.065462] Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done. [ 56.074193] printk: Suspending console(s) (use no_console_suspend to debug) [ 56.096375] r8169 0000:01:00.0 eth0: Link is Down [ 56.304070] r8169 0000:01:00.0: PCI PM: Suspend power state: D3hot [ 56.304111] pcieport 0000:00:02.0: PCI PM: Suspend power state: D0 [ 56.314134] tegra-pcie 1003000.pcie: PME Ack is not received on port: 1 [ 56.325777] Disabling non-boot CPUs ... [ 56.328540] Entering suspend state LP1 [ 56.328571] Enabling non-boot CPUs ... [ 56.329687] CPU1 is up [ 56.330694] CPU2 is up [ 56.331772] CPU3 is up [ 56.332396] tegra-pcie 1003000.pcie: probing port 1, using 1 lanes [ 56.350134] pcieport 0000:00:02.0: nv_msi_ht_cap_quirk didn't locate host bridge [ 56.374241] tegra-pcie 1003000.pcie: Slot present pin change, signature: 00000004 [ 56.534158] r8169 0000:01:00.0 eth0: Link is Down [ 56.886516] ata1: SATA link down (SStatus 0 SControl 300) [ 59.417895] OOM killer enabled. [ 59.421025] Restarting tasks ... done. [ 59.425305] PM: suspend exit [ 60.756040] r8169 0000:01:00.0 eth0: Link is Up - 1Gbps/Full - flow control rx/tx Thanks! Jon
On Tue, Jun 25, 2019 at 1:09 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > On Tuesday, June 25, 2019 12:20:26 AM CEST Rafael J. Wysocki wrote: > > On Mon, Jun 24, 2019 at 11:37 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > On Mon, Jun 24, 2019 at 2:43 PM Jon Hunter <jonathanh@nvidia.com> wrote: > > > > > > > > Hi Rafael, > > > > > > > > On 13/06/2019 22:59, 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, the power state of a PCI bridge with a > > > > > downstream device in D0 must be 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), but that is not actively enforced > > > > > 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 is pointless, 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> > > > > > Tested-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > > > > > > > > I have noticed a regression in both the mainline and -next branches on > > > > one of our boards when testing suspend. The bisect is point to this > > > > commit and reverting on top of mainline does fix the problem. So far I > > > > have not looked at this in close detail but kernel log is showing ... > > > > > > Can you please collect a log like that, but with dynamic debug in > > > pci-driver.c enabled? > > > > > > Note that reverting this commit is rather out of the question, so we > > > need to get to the bottom of the failure. > > > > I suspect that there is a problem with the pm_suspend_via_firmware() > > check which returns 'false' on the affected board, but the platform > > actually removes power from devices left in D0 during suspend. > > > > I guess it would be more appropriate to check something like > > pm_suspend_no_platform() which would return 'true' in the > > suspend-to-idle patch w/ ACPI. > > So I wonder if the patch below makes any difference? Mika, can you please test this one in combination with the other changes we've been working on? I really don't expect to see problems, but just to be sure ... > --- > drivers/pci/pci-driver.c | 8 ++++---- > include/linux/suspend.h | 26 ++++++++++++++++++++++++-- > kernel/power/suspend.c | 3 +++ > 3 files changed, 31 insertions(+), 6 deletions(-) > > Index: linux-pm/include/linux/suspend.h > =================================================================== > --- linux-pm.orig/include/linux/suspend.h > +++ linux-pm/include/linux/suspend.h > @@ -209,8 +209,9 @@ extern int suspend_valid_only_mem(suspen > > extern unsigned int pm_suspend_global_flags; > > -#define PM_SUSPEND_FLAG_FW_SUSPEND (1 << 0) > -#define PM_SUSPEND_FLAG_FW_RESUME (1 << 1) > +#define PM_SUSPEND_FLAG_FW_SUSPEND BIT(0) > +#define PM_SUSPEND_FLAG_FW_RESUME BIT(1) > +#define PM_SUSPEND_FLAG_NO_PLATFORM BIT(2) > > static inline void pm_suspend_clear_flags(void) > { > @@ -227,6 +228,11 @@ static inline void pm_set_resume_via_fir > pm_suspend_global_flags |= PM_SUSPEND_FLAG_FW_RESUME; > } > > +static inline void pm_set_suspend_no_platform(void) > +{ > + pm_suspend_global_flags |= PM_SUSPEND_FLAG_NO_PLATFORM; > +} > + > /** > * pm_suspend_via_firmware - Check if platform firmware will suspend the system. > * > @@ -268,6 +274,22 @@ static inline bool pm_resume_via_firmwar > return !!(pm_suspend_global_flags & PM_SUSPEND_FLAG_FW_RESUME); > } > > +/** > + * pm_suspend_no_platform - Check if platform may change device power states. > + * > + * To be called during system-wide power management transitions to sleep states > + * or during the subsequent system-wide transitions back to the working state. > + * > + * Return 'true' if the power states of devices remain under full control of the > + * kernel throughout the system-wide suspend and resume cycle in progress (that > + * is, if a device is put into a certain power state during suspend, it can be > + * expected to remain in that state during resume). > + */ > +static inline bool pm_suspend_no_platform(void) > +{ > + return !!(pm_suspend_global_flags & PM_SUSPEND_FLAG_NO_PLATFORM); > +} > + > /* Suspend-to-idle state machnine. */ > enum s2idle_states { > S2IDLE_STATE_NONE, /* Not suspended/suspending. */ > Index: linux-pm/kernel/power/suspend.c > =================================================================== > --- linux-pm.orig/kernel/power/suspend.c > +++ linux-pm/kernel/power/suspend.c > @@ -493,6 +493,9 @@ int suspend_devices_and_enter(suspend_st > > pm_suspend_target_state = state; > > + if (state == PM_SUSPEND_TO_IDLE) > + pm_set_suspend_no_platform(); > + > error = platform_suspend_begin(state); > if (error) > goto Close; > Index: linux-pm/drivers/pci/pci-driver.c > =================================================================== > --- linux-pm.orig/drivers/pci/pci-driver.c > +++ linux-pm/drivers/pci/pci-driver.c > @@ -870,7 +870,7 @@ static int pci_pm_suspend_noirq(struct d > pci_dev->bus->self->skip_bus_pm = true; > } > > - if (pci_dev->skip_bus_pm && !pm_suspend_via_firmware()) { > + if (pci_dev->skip_bus_pm && pm_suspend_no_platform()) { > dev_dbg(dev, "PCI PM: Skipped\n"); > goto Fixup; > } > @@ -925,10 +925,10 @@ static int pci_pm_resume_noirq(struct de > /* > * 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. > + * configuration here and attempting to put them into D0 again is > + * pointless, so avoid doing that. > */ > - if (!pci_dev->skip_bus_pm || pm_suspend_via_firmware()) > + if (!(pci_dev->skip_bus_pm && pm_suspend_no_platform())) > pci_pm_default_resume_early(pci_dev); > > pci_fixup_device(pci_fixup_resume_early, pci_dev); > > >
On Tue, Jun 25, 2019 at 06:23:46PM +0200, Rafael J. Wysocki wrote: > > So I wonder if the patch below makes any difference? > > Mika, can you please test this one in combination with the other > changes we've been working on? Sure, I'll give it a try shortly.
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; + /* + * Per PCI PM r1.2, table 6-1, a bridge must be in D0 if any + * downstream device is in D0, so avoid changing the power state + * of the parent bridge by setting the skip_bus_pm flag for it. + */ + 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);