linux-kernel - Re: [PATCH v13] PCI/ASPM: Make pci_save_aspm_l1ss_state save both child and parent's L1SS configuration

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <e3318648-b3cb-51c0-f879-0cb005a2ef36@linux.intel.com>
Date: Mon, 16 Dec 2024 15:12:04 +0200 (EET)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Bjorn Helgaas <helgaas@...nel.org>
cc: Jian-Hong Pan <jhp@...lessos.org>, 
    Krzysztof Wilczyński <kw@...ux.com>, 
    Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@...ux.intel.com>, 
    Nirmal Patel <nirmal.patel@...ux.intel.com>, linux-pci@...r.kernel.org, 
    LKML <linux-kernel@...r.kernel.org>, linux@...lessos.org, 
    "David E. Box" <david.e.box@...ux.intel.com>
Subject: Re: [PATCH v13] PCI/ASPM: Make pci_save_aspm_l1ss_state save both
 child and parent's L1SS configuration

On Fri, 13 Dec 2024, Bjorn Helgaas wrote:

> On Fri, Dec 13, 2024 at 12:37:24PM +0800, Jian-Hong Pan wrote:
> > Bjorn Helgaas <helgaas@...nel.org> 於 2024年12月13日 週五 上午7:03寫道：
> > > On Fri, Nov 15, 2024 at 03:22:02PM +0800, Jian-Hong Pan wrote:
> > > > PCI devices' parameters on the VMD bus have been programmed properly
> > > > originally. But, cleared after pci_reset_bus() and have not been restored
> > > > correctly. This leads the link's L1.2 between PCIe Root Port and child
> > > > device gets wrong configs.
> > ...
> 
> > > I think the important thing here is that currently
> > > pci_save_aspm_l1ss_state() saves only the child L1SS state, but
> > > pci_restore_aspm_l1ss_state() restores both parent and child, and the
> > > parent state is garbage.
> > >
> > > Obviously nothing specific to VMD or NVMe or SATA.
> > >
> > > > To avoid pci_restore_aspm_l1ss_state() restore wrong value to the parent's
> > > > L1SS config like this example, make pci_save_aspm_l1ss_state() save the
> > > > parent's L1SS config, if the PCI device has a parent.
> > >
> > > I tried to simplify the commit log and the patch so it's a little more
> > > parallel with pci_restore_aspm_l1ss_state().  Please comment and test.
> > >
> > > Bjorn
> > >
> > > commit c93935e3ac92 ("PCI/ASPM: Save parent L1SS config in pci_save_aspm_l1ss_state()")
> > > Author: Jian-Hong Pan <jhp@...lessos.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@...ux.intel.com>
> > >     Signed-off-by: Jian-Hong Pan <jhp@...lessos.org>
> > >     [bhelgaas: parallel save/restore structure, simplify commit log]
> > 
> > Thanks for the simplification!
> > Tested on my Asus B1400CEAE. Both the "dev" (NVMe) and the parent (PCI
> > bridge) keep the correct L1SS config.
> > 
> > Tested-by: Jian-Hong Pan <jhp@...lessos.org>
> 
> Thanks, I applied this on pci/aspm for v6.14 since this is a pretty
> old problem, and AFAICT it's a power consumption issue, not something
> that is functionally broken.  We might be able to make a case for
> v6.13 if my understanding is incorrect.
> 
> Ilpo, David, I dropped your reviewed-by since I changed the patch
> significantly; let me know if you see any issue or if you want to add
> your reviewed-by.

The updated version seems fine too.

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>

--
 i.

> > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> > > index 28567d457613..e0bc90597dca 100644
> > > --- a/drivers/pci/pcie/aspm.c
> > > +++ b/drivers/pci/pcie/aspm.c
> > > @@ -81,24 +81,47 @@ void pci_configure_aspm_l1ss(struct pci_dev *pdev)
> > >
> > >  void pci_save_aspm_l1ss_state(struct pci_dev *pdev)
> > >  {
> > > +       struct pci_dev *parent = pdev->bus->self;
> > >         struct pci_cap_saved_state *save_state;
> > > -       u16 l1ss = pdev->l1ss;
> > >         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)
> > > +               return;
> > > +
> > >         /*
> > >          * Save L1 substate configuration. The ASPM L0s/L1 configuration
> > >          * in PCI_EXP_LNKCTL_ASPMC is saved by pci_save_pcie_state().
> > >          */
> > > -       if (!l1ss)
> > > -               return;
> > > -
> > >         save_state = pci_find_saved_ext_cap(pdev, PCI_EXT_CAP_ID_L1SS);
> > >         if (!save_state)
> > >                 return;
> > >
> > >         cap = &save_state->cap.data[0];
> > > -       pci_read_config_dword(pdev, l1ss + PCI_L1SS_CTL2, cap++);
> > > -       pci_read_config_dword(pdev, l1ss + PCI_L1SS_CTL1, cap++);
> > > +       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)
> > > +               return;
> > > +
> > > +       /*
> > > +        * Save parent's L1 substate configuration so we have it for
> > > +        * pci_restore_aspm_l1ss_state(pdev) to restore.
> > > +        */
> > > +       save_state = pci_find_saved_ext_cap(parent, PCI_EXT_CAP_ID_L1SS);
> > > +       if (!save_state)
> > > +               return;
> > > +
> > > +       cap = &save_state->cap.data[0];
> > > +       pci_read_config_dword(parent, parent->l1ss + PCI_L1SS_CTL2, cap++);
> > > +       pci_read_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1, cap++);
> > >  }
> > >
> > >  void pci_restore_aspm_l1ss_state(struct pci_dev *pdev)
>