[Regression in 6.14-rc1] System suspend/resume broken by PCI commit 1db806ec06b7c

[Regression in 6.14-rc1] System suspend/resume broken by PCI commit 1db806ec06b7c

[Rafael J. Wysocki]

Hi,

The following commit:

commit 1db806ec06b7c6e08e8af57088da067963ddf117
Author: Jian-Hong Pan <jhp@endlessos.org>
Date:   Fri Nov 15 15:22:02 2024 +0800

   PCI/ASPM: Save parent L1SS config in pci_save_aspm_l1ss_state()

   After 17423360a27a ("PCI/ASPM: Save L1 PM Substates Capability for
   suspend/resume"), pci_save_aspm_l1ss_state(dev) saves the L1SS state for
   "dev", and pci_restore_aspm_l1ss_state(dev) restores the state for both
   "dev" and its parent.

   The problem is that unless pci_save_state() has been used in some other
   path and has already saved the parent L1SS state, we will restore junk to
   the parent, which means the L1 Substates likely won't work correctly.

   Save the L1SS config for both the device and its parent in
   pci_save_aspm_l1ss_state().  When restoring, we need both because L1SS must
   be enabled at the parent (the Downstream Port) before being enabled at the
   child (the Upstream Port).

   Link: https://lore.kernel.org/r/20241115072200.37509-3-jhp@endlessos.org
   Fixes: 17423360a27a ("PCI/ASPM: Save L1 PM Substates Capability for
suspend/resume")
   Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218394
   Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
   Signed-off-by: Jian-Hong Pan <jhp@endlessos.org>
   [bhelgaas: parallel save/restore structure, simplify commit log, patch at
   https://lore.kernel.org/r/20241212230340.GA3267194@bhelgaas]
   Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
   Tested-by: Jian-Hong Pan <jhp@endlessos.org> # Asus B1400CEAE

broke system suspend/resume on my Dell XPS13 9360.  It doesn't even
pass suspend/resume testing after "echo devices > /sys/power/pm_test".

It looks like PCIe links are all down during resume after the above
commit, but it is rather hard to collect any data in that state.

Reverting the above commit on top of 6.14-rc1 makes things work again,
no problem.

I'm unsure what exactly the problem is ATM, but I'm going to check a
couple of theories.

Cheers, Rafael

Re: [Regression in 6.14-rc1] System suspend/resume broken by PCI commit 1db806ec06b7c

[Rafael J. Wysocki]

[-- Attachment #1: Type: text/plain, Size: 2493 bytes --]

On Mon, Feb 3, 2025 at 9:12 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> Hi,
>
> The following commit:
>
> commit 1db806ec06b7c6e08e8af57088da067963ddf117
> Author: Jian-Hong Pan <jhp@endlessos.org>
> Date:   Fri Nov 15 15:22:02 2024 +0800
>
>    PCI/ASPM: Save parent L1SS config in pci_save_aspm_l1ss_state()
>
>    After 17423360a27a ("PCI/ASPM: Save L1 PM Substates Capability for
>    suspend/resume"), pci_save_aspm_l1ss_state(dev) saves the L1SS state for
>    "dev", and pci_restore_aspm_l1ss_state(dev) restores the state for both
>    "dev" and its parent.
>
>    The problem is that unless pci_save_state() has been used in some other
>    path and has already saved the parent L1SS state, we will restore junk to
>    the parent, which means the L1 Substates likely won't work correctly.
>
>    Save the L1SS config for both the device and its parent in
>    pci_save_aspm_l1ss_state().  When restoring, we need both because L1SS must
>    be enabled at the parent (the Downstream Port) before being enabled at the
>    child (the Upstream Port).
>
>    Link: https://lore.kernel.org/r/20241115072200.37509-3-jhp@endlessos.org
>    Fixes: 17423360a27a ("PCI/ASPM: Save L1 PM Substates Capability for
> suspend/resume")
>    Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218394
>    Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>    Signed-off-by: Jian-Hong Pan <jhp@endlessos.org>
>    [bhelgaas: parallel save/restore structure, simplify commit log, patch at
>    https://lore.kernel.org/r/20241212230340.GA3267194@bhelgaas]
>    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>    Tested-by: Jian-Hong Pan <jhp@endlessos.org> # Asus B1400CEAE
>
> broke system suspend/resume on my Dell XPS13 9360.  It doesn't even
> pass suspend/resume testing after "echo devices > /sys/power/pm_test".
>
> It looks like PCIe links are all down during resume after the above
> commit, but it is rather hard to collect any data in that state.
>
> Reverting the above commit on top of 6.14-rc1 makes things work again,
> no problem.
>
> I'm unsure what exactly the problem is ATM, but I'm going to check a
> couple of theories.

The attached change makes it work again, FWIW, but moving the
parent->l1ss check alone below the pdev l1ss saving doesn't help.

So it is either the parent check against NULL or the
pcie_downstream_port(pdev) one that breaks it.  I guess the former,
but I'll test it tomorrow.

[-- Attachment #2: pci-l1ss-save.patch --]
[-- Type: text/x-patch, Size: 794 bytes --]

---
 drivers/pci/pcie/aspm.c |   12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -85,15 +85,7 @@
 	struct pci_cap_saved_state *save_state;
 	u32 *cap;
 
-	/*
-	 * If this is a Downstream Port, we never restore the L1SS state
-	 * directly; we only restore it when we restore the state of the
-	 * Upstream Port below it.
-	 */
-	if (pcie_downstream_port(pdev) || !parent)
-		return;
-
-	if (!pdev->l1ss || !parent->l1ss)
+	if (!pdev->l1ss)
 		return;
 
 	/*
@@ -108,7 +100,7 @@
 	pci_read_config_dword(pdev, pdev->l1ss + PCI_L1SS_CTL2, cap++);
 	pci_read_config_dword(pdev, pdev->l1ss + PCI_L1SS_CTL1, cap++);
 
-	if (parent->state_saved)
+	if (!parent || parent->state_saved || !parent->l1ss)
 		return;
 
 	/*

Re: [Regression in 6.14-rc1] System suspend/resume broken by PCI commit 1db806ec06b7c

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 3036 bytes --]

On Mon, 3 Feb 2025, Rafael J. Wysocki wrote:

> On Mon, Feb 3, 2025 at 9:12 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > Hi,

Hi Rafael,

> > The following commit:
> >
> > commit 1db806ec06b7c6e08e8af57088da067963ddf117
> > Author: Jian-Hong Pan <jhp@endlessos.org>
> > Date:   Fri Nov 15 15:22:02 2024 +0800
> >
> >    PCI/ASPM: Save parent L1SS config in pci_save_aspm_l1ss_state()
> >
> >    After 17423360a27a ("PCI/ASPM: Save L1 PM Substates Capability for
> >    suspend/resume"), pci_save_aspm_l1ss_state(dev) saves the L1SS state for
> >    "dev", and pci_restore_aspm_l1ss_state(dev) restores the state for both
> >    "dev" and its parent.
> >
> >    The problem is that unless pci_save_state() has been used in some other
> >    path and has already saved the parent L1SS state, we will restore junk to
> >    the parent, which means the L1 Substates likely won't work correctly.
> >
> >    Save the L1SS config for both the device and its parent in
> >    pci_save_aspm_l1ss_state().  When restoring, we need both because L1SS must
> >    be enabled at the parent (the Downstream Port) before being enabled at the
> >    child (the Upstream Port).
> >
> >    Link: https://lore.kernel.org/r/20241115072200.37509-3-jhp@endlessos.org
> >    Fixes: 17423360a27a ("PCI/ASPM: Save L1 PM Substates Capability for
> > suspend/resume")
> >    Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218394
> >    Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> >    Signed-off-by: Jian-Hong Pan <jhp@endlessos.org>
> >    [bhelgaas: parallel save/restore structure, simplify commit log, patch at
> >    https://lore.kernel.org/r/20241212230340.GA3267194@bhelgaas]
> >    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> >    Tested-by: Jian-Hong Pan <jhp@endlessos.org> # Asus B1400CEAE
> >
> > broke system suspend/resume on my Dell XPS13 9360.  It doesn't even
> > pass suspend/resume testing after "echo devices > /sys/power/pm_test".
> >
> > It looks like PCIe links are all down during resume after the above
> > commit, but it is rather hard to collect any data in that state.
> >
> > Reverting the above commit on top of 6.14-rc1 makes things work again,
> > no problem.
> >
> > I'm unsure what exactly the problem is ATM, but I'm going to check a
> > couple of theories.
> 
> The attached change makes it work again, FWIW, but moving the
> parent->l1ss check alone below the pdev l1ss saving doesn't help.
> 
> So it is either the parent check against NULL or the
> pcie_downstream_port(pdev) one that breaks it.  I guess the former,
> but I'll test it tomorrow.

Neither of those is the root cause but it's bit hard to see from the code 
itself because the parent->saved_state check your test patch also removed 
looks very logical on a glance (but that's the problematic line).

The fix is already here with the explanation:

https://lore.kernel.org/linux-pci/20250131152913.2507-1-ilpo.jarvinen@linux.intel.com/T/#u

-- 
 i.

Re: [Regression in 6.14-rc1] System suspend/resume broken by PCI commit 1db806ec06b7c

[Rafael J. Wysocki]

Hi Ilpo,

On Tue, Feb 4, 2025 at 8:48 AM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> On Mon, 3 Feb 2025, Rafael J. Wysocki wrote:
>
> > On Mon, Feb 3, 2025 at 9:12 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > Hi,
>
> Hi Rafael,
>
> > > The following commit:
> > >
> > > commit 1db806ec06b7c6e08e8af57088da067963ddf117
> > > Author: Jian-Hong Pan <jhp@endlessos.org>
> > > Date:   Fri Nov 15 15:22:02 2024 +0800
> > >
> > >    PCI/ASPM: Save parent L1SS config in pci_save_aspm_l1ss_state()
> > >
> > >    After 17423360a27a ("PCI/ASPM: Save L1 PM Substates Capability for
> > >    suspend/resume"), pci_save_aspm_l1ss_state(dev) saves the L1SS state for
> > >    "dev", and pci_restore_aspm_l1ss_state(dev) restores the state for both
> > >    "dev" and its parent.
> > >
> > >    The problem is that unless pci_save_state() has been used in some other
> > >    path and has already saved the parent L1SS state, we will restore junk to
> > >    the parent, which means the L1 Substates likely won't work correctly.
> > >
> > >    Save the L1SS config for both the device and its parent in
> > >    pci_save_aspm_l1ss_state().  When restoring, we need both because L1SS must
> > >    be enabled at the parent (the Downstream Port) before being enabled at the
> > >    child (the Upstream Port).
> > >
> > >    Link: https://lore.kernel.org/r/20241115072200.37509-3-jhp@endlessos.org
> > >    Fixes: 17423360a27a ("PCI/ASPM: Save L1 PM Substates Capability for
> > > suspend/resume")
> > >    Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218394
> > >    Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > >    Signed-off-by: Jian-Hong Pan <jhp@endlessos.org>
> > >    [bhelgaas: parallel save/restore structure, simplify commit log, patch at
> > >    https://lore.kernel.org/r/20241212230340.GA3267194@bhelgaas]
> > >    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > >    Tested-by: Jian-Hong Pan <jhp@endlessos.org> # Asus B1400CEAE
> > >
> > > broke system suspend/resume on my Dell XPS13 9360.  It doesn't even
> > > pass suspend/resume testing after "echo devices > /sys/power/pm_test".
> > >
> > > It looks like PCIe links are all down during resume after the above
> > > commit, but it is rather hard to collect any data in that state.
> > >
> > > Reverting the above commit on top of 6.14-rc1 makes things work again,
> > > no problem.
> > >
> > > I'm unsure what exactly the problem is ATM, but I'm going to check a
> > > couple of theories.
> >
> > The attached change makes it work again, FWIW, but moving the
> > parent->l1ss check alone below the pdev l1ss saving doesn't help.
> >
> > So it is either the parent check against NULL or the
> > pcie_downstream_port(pdev) one that breaks it.  I guess the former,
> > but I'll test it tomorrow.
>
> Neither of those is the root cause

Well, not quite.

> but it's bit hard to see from the code
> itself because the parent->saved_state check your test patch also removed

My patch hasn't removed that check.

Besides, suspend/resume works on my system without commit
1db806ec06b7c6e0 and the parent->saved_state only affects the parent,
so it clearly cannot be the culprit here.

> looks very logical on a glance (but that's the problematic line).
>
> The fix is already here with the explanation:
>
> https://lore.kernel.org/linux-pci/20250131152913.2507-1-ilpo.jarvinen@linux.intel.com/T/#u

So it turns out that the minimum fux that works here is what I posted.
That is, the upfront pcie_downstream_port(pdev) check needs to be
dropped and the !parent check needs to be moved after saving the
pdev's state.

IOW, it looks like on this platform, it is necessary to save the l1ss
state for a Root Port.

Re: [Regression in 6.14-rc1] System suspend/resume broken by PCI commit 1db806ec06b7c

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 4512 bytes --]

On Tue, 4 Feb 2025, Rafael J. Wysocki wrote:

> Hi Ilpo,
> 
> On Tue, Feb 4, 2025 at 8:48 AM Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> wrote:
> >
> > On Mon, 3 Feb 2025, Rafael J. Wysocki wrote:
> >
> > > On Mon, Feb 3, 2025 at 9:12 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > >
> > > > Hi,
> >
> > Hi Rafael,
> >
> > > > The following commit:
> > > >
> > > > commit 1db806ec06b7c6e08e8af57088da067963ddf117
> > > > Author: Jian-Hong Pan <jhp@endlessos.org>
> > > > Date:   Fri Nov 15 15:22:02 2024 +0800
> > > >
> > > >    PCI/ASPM: Save parent L1SS config in pci_save_aspm_l1ss_state()
> > > >
> > > >    After 17423360a27a ("PCI/ASPM: Save L1 PM Substates Capability for
> > > >    suspend/resume"), pci_save_aspm_l1ss_state(dev) saves the L1SS state for
> > > >    "dev", and pci_restore_aspm_l1ss_state(dev) restores the state for both
> > > >    "dev" and its parent.
> > > >
> > > >    The problem is that unless pci_save_state() has been used in some other
> > > >    path and has already saved the parent L1SS state, we will restore junk to
> > > >    the parent, which means the L1 Substates likely won't work correctly.
> > > >
> > > >    Save the L1SS config for both the device and its parent in
> > > >    pci_save_aspm_l1ss_state().  When restoring, we need both because L1SS must
> > > >    be enabled at the parent (the Downstream Port) before being enabled at the
> > > >    child (the Upstream Port).
> > > >
> > > >    Link: https://lore.kernel.org/r/20241115072200.37509-3-jhp@endlessos.org
> > > >    Fixes: 17423360a27a ("PCI/ASPM: Save L1 PM Substates Capability for
> > > > suspend/resume")
> > > >    Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218394
> > > >    Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > >    Signed-off-by: Jian-Hong Pan <jhp@endlessos.org>
> > > >    [bhelgaas: parallel save/restore structure, simplify commit log, patch at
> > > >    https://lore.kernel.org/r/20241212230340.GA3267194@bhelgaas]
> > > >    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > > >    Tested-by: Jian-Hong Pan <jhp@endlessos.org> # Asus B1400CEAE
> > > >
> > > > broke system suspend/resume on my Dell XPS13 9360.  It doesn't even
> > > > pass suspend/resume testing after "echo devices > /sys/power/pm_test".
> > > >
> > > > It looks like PCIe links are all down during resume after the above
> > > > commit, but it is rather hard to collect any data in that state.
> > > >
> > > > Reverting the above commit on top of 6.14-rc1 makes things work again,
> > > > no problem.
> > > >
> > > > I'm unsure what exactly the problem is ATM, but I'm going to check a
> > > > couple of theories.
> > >
> > > The attached change makes it work again, FWIW, but moving the
> > > parent->l1ss check alone below the pdev l1ss saving doesn't help.
> > >
> > > So it is either the parent check against NULL or the
> > > pcie_downstream_port(pdev) one that breaks it.  I guess the former,
> > > but I'll test it tomorrow.
> >
> > Neither of those is the root cause
> 
> Well, not quite.
> 
> > but it's bit hard to see from the code
> > itself because the parent->saved_state check your test patch also removed
> 
> My patch hasn't removed that check.

Ah, I'm sorry, I read too quickly and assumed the first checks were just 
moved to where the parent->state_saved check is, replacing it.

> Besides, suspend/resume works on my system without commit
> 1db806ec06b7c6e0 and the parent->saved_state only affects the parent,
> so it clearly cannot be the culprit here.
> 
> > looks very logical on a glance (but that's the problematic line).
> >
> > The fix is already here with the explanation:
> >
> > https://lore.kernel.org/linux-pci/20250131152913.2507-1-ilpo.jarvinen@linux.intel.com/T/#u
> 
> So it turns out that the minimum fux that works here is what I posted.
> That is, the upfront pcie_downstream_port(pdev) check needs to be
> dropped and the !parent check needs to be moved after saving the
> pdev's state.
> 
> IOW, it looks like on this platform, it is necessary to save the l1ss
> state for a Root Port.

The restore side, though, does also contains that pcie_downstream_port() 
so nothing would be restored.

Could a downstream component attempt to restore L1SS config while never 
having called the save beforehand? In such case your patch would make some 
meaningful difference which could explain the outcome.

-- 
 i.

Re: [Regression in 6.14-rc1] System suspend/resume broken by PCI commit 1db806ec06b7c

[Rafael J. Wysocki]

On Tue, Feb 4, 2025 at 4:54 PM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> On Tue, 4 Feb 2025, Rafael J. Wysocki wrote:
>
> > Hi Ilpo,
> >
> > On Tue, Feb 4, 2025 at 8:48 AM Ilpo Järvinen
> > <ilpo.jarvinen@linux.intel.com> wrote:
> > >
> > > On Mon, 3 Feb 2025, Rafael J. Wysocki wrote:
> > >
> > > > On Mon, Feb 3, 2025 at 9:12 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > > >
> > > > > Hi,
> > >
> > > Hi Rafael,
> > >
> > > > > The following commit:
> > > > >
> > > > > commit 1db806ec06b7c6e08e8af57088da067963ddf117
> > > > > Author: Jian-Hong Pan <jhp@endlessos.org>
> > > > > Date:   Fri Nov 15 15:22:02 2024 +0800
> > > > >
> > > > >    PCI/ASPM: Save parent L1SS config in pci_save_aspm_l1ss_state()
> > > > >
> > > > >    After 17423360a27a ("PCI/ASPM: Save L1 PM Substates Capability for
> > > > >    suspend/resume"), pci_save_aspm_l1ss_state(dev) saves the L1SS state for
> > > > >    "dev", and pci_restore_aspm_l1ss_state(dev) restores the state for both
> > > > >    "dev" and its parent.
> > > > >
> > > > >    The problem is that unless pci_save_state() has been used in some other
> > > > >    path and has already saved the parent L1SS state, we will restore junk to
> > > > >    the parent, which means the L1 Substates likely won't work correctly.
> > > > >
> > > > >    Save the L1SS config for both the device and its parent in
> > > > >    pci_save_aspm_l1ss_state().  When restoring, we need both because L1SS must
> > > > >    be enabled at the parent (the Downstream Port) before being enabled at the
> > > > >    child (the Upstream Port).
> > > > >
> > > > >    Link: https://lore.kernel.org/r/20241115072200.37509-3-jhp@endlessos.org
> > > > >    Fixes: 17423360a27a ("PCI/ASPM: Save L1 PM Substates Capability for
> > > > > suspend/resume")
> > > > >    Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218394
> > > > >    Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > > >    Signed-off-by: Jian-Hong Pan <jhp@endlessos.org>
> > > > >    [bhelgaas: parallel save/restore structure, simplify commit log, patch at
> > > > >    https://lore.kernel.org/r/20241212230340.GA3267194@bhelgaas]
> > > > >    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > > > >    Tested-by: Jian-Hong Pan <jhp@endlessos.org> # Asus B1400CEAE
> > > > >
> > > > > broke system suspend/resume on my Dell XPS13 9360.  It doesn't even
> > > > > pass suspend/resume testing after "echo devices > /sys/power/pm_test".
> > > > >
> > > > > It looks like PCIe links are all down during resume after the above
> > > > > commit, but it is rather hard to collect any data in that state.
> > > > >
> > > > > Reverting the above commit on top of 6.14-rc1 makes things work again,
> > > > > no problem.
> > > > >
> > > > > I'm unsure what exactly the problem is ATM, but I'm going to check a
> > > > > couple of theories.
> > > >
> > > > The attached change makes it work again, FWIW, but moving the
> > > > parent->l1ss check alone below the pdev l1ss saving doesn't help.
> > > >
> > > > So it is either the parent check against NULL or the
> > > > pcie_downstream_port(pdev) one that breaks it.  I guess the former,
> > > > but I'll test it tomorrow.
> > >
> > > Neither of those is the root cause
> >
> > Well, not quite.
> >
> > > but it's bit hard to see from the code
> > > itself because the parent->saved_state check your test patch also removed
> >
> > My patch hasn't removed that check.
>
> Ah, I'm sorry, I read too quickly and assumed the first checks were just
> moved to where the parent->state_saved check is, replacing it.
>
> > Besides, suspend/resume works on my system without commit
> > 1db806ec06b7c6e0 and the parent->saved_state only affects the parent,
> > so it clearly cannot be the culprit here.
> >
> > > looks very logical on a glance (but that's the problematic line).
> > >
> > > The fix is already here with the explanation:
> > >
> > > https://lore.kernel.org/linux-pci/20250131152913.2507-1-ilpo.jarvinen@linux.intel.com/T/#u
> >
> > So it turns out that the minimum fux that works here is what I posted.
> > That is, the upfront pcie_downstream_port(pdev) check needs to be
> > dropped and the !parent check needs to be moved after saving the
> > pdev's state.
> >
> > IOW, it looks like on this platform, it is necessary to save the l1ss
> > state for a Root Port.
>
> The restore side, though, does also contains that pcie_downstream_port()
> so nothing would be restored.
>
> Could a downstream component attempt to restore L1SS config while never
> having called the save beforehand? In such case your patch would make some
> meaningful difference which could explain the outcome.

Interestingly enough, removing the parent->saved_state check alone
does help too.

What appears to happen is that after adding the upfront
pcie_downstream_port(pdev) and !parent checks, it is now necessary to
save the L1SS state of the parent along the L1SS state of the child,
because it will not be saved otherwise.

I'm assuming that the fix mentioned above is on its way to the
mainline, so problem solved.

Thanks!