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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- 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.