From: Rafael J. Wysocki <[email protected]>
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 <[email protected]>
Signed-off-by: Rafael J. Wysocki <[email protected]>
Reviewed-by: Mika Westerberg <[email protected]>
Tested-by: Kai-Heng Feng <[email protected]>
---
drivers/pci/pci-driver.c | 47 +++++++++++++++++++++++++++++++++++------------
1 file changed, 35 insertions(+), 12 deletions(-)
Index: linux-pm/drivers/pci/pci-driver.c
===================================================================
--- linux-pm.orig/drivers/pci/pci-driver.c
+++ linux-pm/drivers/pci/pci-driver.c
@@ -524,7 +524,6 @@ static void pci_pm_default_resume_early(
pci_power_up(pci_dev);
pci_restore_state(pci_dev);
pci_pme_restore(pci_dev);
- pci_fixup_device(pci_fixup_resume_early, pci_dev);
}
/*
@@ -842,18 +841,16 @@ static int pci_pm_suspend_noirq(struct d
if (pci_dev->skip_bus_pm) {
/*
- * The function is running for the second time in a row without
+ * Either the device is a bridge with a child in D0 below it, or
+ * the function is running for the second time in a row without
* going through full resume, which is possible only during
- * suspend-to-idle in a spurious wakeup case. Moreover, the
- * device was originally left in D0, so its power state should
- * not be changed here and the device register values saved
- * originally should be restored on resume again.
+ * suspend-to-idle in a spurious wakeup case. The device should
+ * be in D0 at this point, but if it is a bridge, it may be
+ * necessary to save its state.
*/
- pci_dev->state_saved = true;
- } else if (pci_dev->state_saved) {
- if (pci_dev->current_state == PCI_D0)
- pci_dev->skip_bus_pm = true;
- } else {
+ if (!pci_dev->state_saved)
+ pci_save_state(pci_dev);
+ } else if (!pci_dev->state_saved) {
pci_save_state(pci_dev);
if (pci_power_manageable(pci_dev))
pci_prepare_to_sleep(pci_dev);
@@ -862,6 +859,22 @@ static int pci_pm_suspend_noirq(struct d
dev_dbg(dev, "PCI PM: Suspend power state: %s\n",
pci_power_name(pci_dev->current_state));
+ if (pci_dev->current_state == PCI_D0) {
+ pci_dev->skip_bus_pm = true;
+ /*
+ * 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);
Hi Rafael,
On 13/06/2019 22:59, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> Reviewed-by: Mika Westerberg <[email protected]>
> Tested-by: Kai-Heng Feng <[email protected]>
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
--
nvpublic
On Mon, Jun 24, 2019 at 2:43 PM Jon Hunter <[email protected]> wrote:
>
> Hi Rafael,
>
> On 13/06/2019 22:59, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > Reviewed-by: Mika Westerberg <[email protected]>
> > Tested-by: Kai-Heng Feng <[email protected]>
>
> 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 <[email protected]> wrote:
>
> On Mon, Jun 24, 2019 at 2:43 PM Jon Hunter <[email protected]> wrote:
> >
> > Hi Rafael,
> >
> > On 13/06/2019 22:59, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <[email protected]>
> > >
> > > 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 <[email protected]>
> > > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > > Reviewed-by: Mika Westerberg <[email protected]>
> > > Tested-by: Kai-Heng Feng <[email protected]>
> >
> > 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 <[email protected]> wrote:
> >
> > On Mon, Jun 24, 2019 at 2:43 PM Jon Hunter <[email protected]> wrote:
> > >
> > > Hi Rafael,
> > >
> > > On 13/06/2019 22:59, Rafael J. Wysocki wrote:
> > > > From: Rafael J. Wysocki <[email protected]>
> > > >
> > > > 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 <[email protected]>
> > > > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > > > Reviewed-by: Mika Westerberg <[email protected]>
> > > > Tested-by: Kai-Heng Feng <[email protected]>
> > >
> > > 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 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 <[email protected]> wrote:
>>>
>>> On Mon, Jun 24, 2019 at 2:43 PM Jon Hunter <[email protected]> wrote:
>>>>
>>>> Hi Rafael,
>>>>
>>>> On 13/06/2019 22:59, Rafael J. Wysocki wrote:
>>>>> From: Rafael J. Wysocki <[email protected]>
>>>>>
>>>>> 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 <[email protected]>
>>>>> Signed-off-by: Rafael J. Wysocki <[email protected]>
>>>>> Reviewed-by: Mika Westerberg <[email protected]>
>>>>> Tested-by: Kai-Heng Feng <[email protected]>
>>>>
>>>> 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
--
nvpublic
On 24/06/2019 22:37, Rafael J. Wysocki wrote:
> On Mon, Jun 24, 2019 at 2:43 PM Jon Hunter <[email protected]> wrote:
>>
>> Hi Rafael,
>>
>> On 13/06/2019 22:59, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <[email protected]>
>>>
>>> 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 <[email protected]>
>>> Signed-off-by: Rafael J. Wysocki <[email protected]>
>>> Reviewed-by: Mika Westerberg <[email protected]>
>>> Tested-by: Kai-Heng Feng <[email protected]>
>>
>> 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
--
nvpublic
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 <[email protected]> wrote:
>>>
>>> On Mon, Jun 24, 2019 at 2:43 PM Jon Hunter <[email protected]> wrote:
>>>>
>>>> Hi Rafael,
>>>>
>>>> On 13/06/2019 22:59, Rafael J. Wysocki wrote:
>>>>> From: Rafael J. Wysocki <[email protected]>
>>>>>
>>>>> 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 <[email protected]>
>>>>> Signed-off-by: Rafael J. Wysocki <[email protected]>
>>>>> Reviewed-by: Mika Westerberg <[email protected]>
>>>>> Tested-by: Kai-Heng Feng <[email protected]>
>>>>
>>>> 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 <[email protected]>
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
--
nvpublic
On Tue, Jun 25, 2019 at 1:09 AM Rafael J. Wysocki <[email protected]> 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 <[email protected]> wrote:
> > >
> > > On Mon, Jun 24, 2019 at 2:43 PM Jon Hunter <[email protected]> wrote:
> > > >
> > > > Hi Rafael,
> > > >
> > > > On 13/06/2019 22:59, Rafael J. Wysocki wrote:
> > > > > From: Rafael J. Wysocki <[email protected]>
> > > > >
> > > > > 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 <[email protected]>
> > > > > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > > > > Reviewed-by: Mika Westerberg <[email protected]>
> > > > > Tested-by: Kai-Heng Feng <[email protected]>
> > > >
> > > > 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.