| 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
| ||
|
Message-ID: <CAPpJ_eeHbQBtNK=0rJG-jzU-jZ=Tc1RknwrQgsFgEjr1qTqyqg@mail.gmail.com>
Date: Fri, 13 Dec 2024 12:37:24 +0800
From: Jian-Hong Pan <jhp@...lessos.org>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: 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,
linux-kernel@...r.kernel.org, linux@...lessos.org,
Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
"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
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.
> >
> > Here is a failed example on ASUS B1400CEAE with enabled VMD. Both PCIe
> > bridge and NVMe device should have the same LTR1.2_Threshold value.
> > However, they are configured as different values in this case:
> >
> > 10000:e0:06.0 PCI bridge [0604]: Intel Corporation 11th Gen Core Processor PCIe Controller [8086:9a09] (rev 01) (prog-if 00 [Normal decode])
> > ...
> > Capabilities: [200 v1] L1 PM Substates
> > L1SubCap: PCI-PM_L1.2+ PCI-PM_L1.1+ ASPM_L1.2+ ASPM_L1.1+ L1_PM_Substates+
> > PortCommonModeRestoreTime=45us PortTPowerOnTime=50us
> > L1SubCtl1: PCI-PM_L1.2- PCI-PM_L1.1- ASPM_L1.2+ ASPM_L1.1-
> > T_CommonMode=0us LTR1.2_Threshold=0ns
> > L1SubCtl2: T_PwrOn=0us
> >
> > 10000:e1:00.0 Non-Volatile memory controller [0108]: Sandisk Corp WD Blue SN550 NVMe SSD [15b7:5009] (rev 01) (prog-if 02 [NVM Express])
> > ...
> > Capabilities: [900 v1] L1 PM Substates
> > L1SubCap: PCI-PM_L1.2+ PCI-PM_L1.1- ASPM_L1.2+ ASPM_L1.1- L1_PM_Substates+
> > PortCommonModeRestoreTime=32us PortTPowerOnTime=10us
> > L1SubCtl1: PCI-PM_L1.2- PCI-PM_L1.1- ASPM_L1.2+ ASPM_L1.1-
> > T_CommonMode=0us LTR1.2_Threshold=101376ns
> > L1SubCtl2: T_PwrOn=50us
> >
> >
> > Here is VMD mapped PCI device tree:
> >
> > -+-[0000:00]-+-00.0 Intel Corporation Device 9a04
> > | ...
> > \-[10000:e0]-+-06.0-[e1]----00.0 Sandisk Corp WD Blue SN550 NVMe SSD
> > \-17.0 Intel Corporation Tiger Lake-LP SATA Controller
> >
> > When pci_reset_bus() resets the bus [e1] of the NVMe, it only saves and
> > restores NVMe's state before and after reset. Then, when it restores the
> > NVMe's state, ASPM code restores L1SS for both the parent bridge and the
> > NVMe in pci_restore_aspm_l1ss_state(). The NVMe's L1SS is restored
> > correctly. But, the parent bridge's L1SS is restored with a wrong value 0x0
> > because the parent bridge's L1SS wasn't saved by pci_save_aspm_l1ss_state()
> > before reset.
>
> 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>
> 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)
Powered by blists - more mailing lists