drivers/pci/pcie/aspm.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)
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.
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.
Link: https://lore.kernel.org/linux-pci/CAPpJ_eexU0gCHMbXw_z924WxXw0+B6SdS4eG9oGpEX1wmnMLkQ@mail.gmail.com/
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218394
Fixes: 17423360a27a ("PCI/ASPM: Save L1 PM Substates Capability for suspend/resume")
Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Signed-off-by: Jian-Hong Pan <jhp@endlessos.org>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Reviewed-by: David E. Box <david.e.box@linux.intel.com>
---
v9:
- Drop the v8 fix about drivers/pci/pcie/aspm.c. Use this in VMD instead.
v10:
- Drop the v9 fix about drivers/pci/controller/vmd.c
- Fix in PCIe ASPM to make it symmetric between pci_save_aspm_l1ss_state()
and pci_restore_aspm_l1ss_state()
v11:
- Introduce __pci_save_aspm_l1ss_state as a resusable helper function
which is same as the original pci_configure_aspm_l1ss
- Make pci_save_aspm_l1ss_state invoke __pci_save_aspm_l1ss_state for
both child and parent devices
- Smooth the commit message
v12:
- Update the commit message
v13:
- Tweak the commit message to make it more like a general fix
- When pci_alloc_dev() prepares the pci_dev, it sets the pci_dev's bus.
So, let pci_save_aspm_l1ss_state() access pdev's bus directly.
- Add comment in pci_save_aspm_l1ss_state() to describe why it does not
save both the PCIe device and the parent's L1SS config like
pci_restore_aspm_l1ss_state() directly.
drivers/pci/pcie/aspm.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 28567d457613..0bcd060aab32 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -79,7 +79,7 @@ void pci_configure_aspm_l1ss(struct pci_dev *pdev)
ERR_PTR(rc));
}
-void pci_save_aspm_l1ss_state(struct pci_dev *pdev)
+static void __pci_save_aspm_l1ss_state(struct pci_dev *pdev)
{
struct pci_cap_saved_state *save_state;
u16 l1ss = pdev->l1ss;
@@ -101,6 +101,22 @@ void pci_save_aspm_l1ss_state(struct pci_dev *pdev)
pci_read_config_dword(pdev, l1ss + PCI_L1SS_CTL1, cap++);
}
+void pci_save_aspm_l1ss_state(struct pci_dev *pdev)
+{
+ struct pci_dev *parent = pdev->bus->self;
+
+ __pci_save_aspm_l1ss_state(pdev);
+
+ /*
+ * Save parent's L1 substate configuration, if the parent has not saved
+ * state. It avoids pci_restore_aspm_l1ss_state() restore wrong value to
+ * parent's L1 substate configuration. However, the parent might be
+ * nothing, if pdev is a PCI bridge.
+ */
+ if (parent && !parent->state_saved)
+ __pci_save_aspm_l1ss_state(parent);
+}
+
void pci_restore_aspm_l1ss_state(struct pci_dev *pdev)
{
struct pci_cap_saved_state *pl_save_state, *cl_save_state;
--
2.47.0
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@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]
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)
Bjorn Helgaas <helgaas@kernel.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@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]
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@endlessos.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)
On Fri, Dec 13, 2024 at 12:37:24PM +0800, Jian-Hong Pan wrote:
> Bjorn Helgaas <helgaas@kernel.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@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]
>
> 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@endlessos.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.
> > 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)
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@kernel.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@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]
> >
> > 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@endlessos.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@linux.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)
>
[+to Alex, +cc kvm]
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.
17423360a27a ("PCI/ASPM: Save L1 PM Substates Capability for
suspend/resume") appeared in v6.9 and added a bug in the save/restore
path: we save the device L1SS state, but restore both the device L1SS
state and the parent's L1SS state (when the parent state may be junk)
afterwards.
Jian-Hong sees this on VMD, which makes sense because vmd uses
pci_reset_bus() when enabling the VMD domain.
But this should be a problem for any device reset, so I added you,
Alex, in case you've seen this with the resets VFIO does?
We also save/restore for suspend, but I suppose we don't notice the
problem there because in that case we save state for *all* devices,
so the parent state should be valid when we restore.
> 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
> 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
> 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
I think T_PwrOn should also be the same for both devices, FWIW.
In fact, I think L1SS should be configured identically for both ends
of the link, with the exceptions of Link Activation and
Common_Mode_Restore_Time, which are RsvdP for the Upstream Port.
> 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.
>
> 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.
>
> Link: https://lore.kernel.org/linux-pci/CAPpJ_eexU0gCHMbXw_z924WxXw0+B6SdS4eG9oGpEX1wmnMLkQ@mail.gmail.com/
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218394
> Fixes: 17423360a27a ("PCI/ASPM: Save L1 PM Substates Capability for suspend/resume")
> Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Signed-off-by: Jian-Hong Pan <jhp@endlessos.org>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Reviewed-by: David E. Box <david.e.box@linux.intel.com>
> ---
> v9:
> - Drop the v8 fix about drivers/pci/pcie/aspm.c. Use this in VMD instead.
>
> v10:
> - Drop the v9 fix about drivers/pci/controller/vmd.c
> - Fix in PCIe ASPM to make it symmetric between pci_save_aspm_l1ss_state()
> and pci_restore_aspm_l1ss_state()
>
> v11:
> - Introduce __pci_save_aspm_l1ss_state as a resusable helper function
> which is same as the original pci_configure_aspm_l1ss
> - Make pci_save_aspm_l1ss_state invoke __pci_save_aspm_l1ss_state for
> both child and parent devices
> - Smooth the commit message
>
> v12:
> - Update the commit message
>
> v13:
> - Tweak the commit message to make it more like a general fix
> - When pci_alloc_dev() prepares the pci_dev, it sets the pci_dev's bus.
> So, let pci_save_aspm_l1ss_state() access pdev's bus directly.
> - Add comment in pci_save_aspm_l1ss_state() to describe why it does not
> save both the PCIe device and the parent's L1SS config like
> pci_restore_aspm_l1ss_state() directly.
>
> drivers/pci/pcie/aspm.c | 18 +++++++++++++++++-
> 1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 28567d457613..0bcd060aab32 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -79,7 +79,7 @@ void pci_configure_aspm_l1ss(struct pci_dev *pdev)
> ERR_PTR(rc));
> }
>
> -void pci_save_aspm_l1ss_state(struct pci_dev *pdev)
> +static void __pci_save_aspm_l1ss_state(struct pci_dev *pdev)
> {
> struct pci_cap_saved_state *save_state;
> u16 l1ss = pdev->l1ss;
> @@ -101,6 +101,22 @@ void pci_save_aspm_l1ss_state(struct pci_dev *pdev)
> pci_read_config_dword(pdev, l1ss + PCI_L1SS_CTL1, cap++);
> }
>
> +void pci_save_aspm_l1ss_state(struct pci_dev *pdev)
> +{
> + struct pci_dev *parent = pdev->bus->self;
> +
> + __pci_save_aspm_l1ss_state(pdev);
> +
> + /*
> + * Save parent's L1 substate configuration, if the parent has not saved
> + * state. It avoids pci_restore_aspm_l1ss_state() restore wrong value to
> + * parent's L1 substate configuration. However, the parent might be
> + * nothing, if pdev is a PCI bridge.
> + */
> + if (parent && !parent->state_saved)
> + __pci_save_aspm_l1ss_state(parent);
> +}
> +
> void pci_restore_aspm_l1ss_state(struct pci_dev *pdev)
> {
> struct pci_cap_saved_state *pl_save_state, *cl_save_state;
> --
> 2.47.0
>
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.
>
> 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.
>
> Link: https://lore.kernel.org/linux-pci/CAPpJ_eexU0gCHMbXw_z924WxXw0+B6SdS4eG9oGpEX1wmnMLkQ@mail.gmail.com/
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218394
> Fixes: 17423360a27a ("PCI/ASPM: Save L1 PM Substates Capability for suspend/resume")
> Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Signed-off-by: Jian-Hong Pan <jhp@endlessos.org>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Reviewed-by: David E. Box <david.e.box@linux.intel.com>
> ---
> v9:
> - Drop the v8 fix about drivers/pci/pcie/aspm.c. Use this in VMD instead.
>
> v10:
> - Drop the v9 fix about drivers/pci/controller/vmd.c
> - Fix in PCIe ASPM to make it symmetric between pci_save_aspm_l1ss_state()
> and pci_restore_aspm_l1ss_state()
>
> v11:
> - Introduce __pci_save_aspm_l1ss_state as a resusable helper function
> which is same as the original pci_configure_aspm_l1ss
> - Make pci_save_aspm_l1ss_state invoke __pci_save_aspm_l1ss_state for
> both child and parent devices
> - Smooth the commit message
>
> v12:
> - Update the commit message
>
> v13:
> - Tweak the commit message to make it more like a general fix
> - When pci_alloc_dev() prepares the pci_dev, it sets the pci_dev's bus.
> So, let pci_save_aspm_l1ss_state() access pdev's bus directly.
> - Add comment in pci_save_aspm_l1ss_state() to describe why it does not
> save both the PCIe device and the parent's L1SS config like
> pci_restore_aspm_l1ss_state() directly.
>
> drivers/pci/pcie/aspm.c | 18 +++++++++++++++++-
> 1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 28567d457613..0bcd060aab32 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -79,7 +79,7 @@ void pci_configure_aspm_l1ss(struct pci_dev *pdev)
> ERR_PTR(rc));
> }
>
> -void pci_save_aspm_l1ss_state(struct pci_dev *pdev)
> +static void __pci_save_aspm_l1ss_state(struct pci_dev *pdev)
> {
> struct pci_cap_saved_state *save_state;
> u16 l1ss = pdev->l1ss;
> @@ -101,6 +101,22 @@ void pci_save_aspm_l1ss_state(struct pci_dev *pdev)
> pci_read_config_dword(pdev, l1ss + PCI_L1SS_CTL1, cap++);
> }
>
> +void pci_save_aspm_l1ss_state(struct pci_dev *pdev)
> +{
> + struct pci_dev *parent = pdev->bus->self;
> +
> + __pci_save_aspm_l1ss_state(pdev);
> +
> + /*
> + * Save parent's L1 substate configuration, if the parent has not saved
> + * state. It avoids pci_restore_aspm_l1ss_state() restore wrong value to
> + * parent's L1 substate configuration. However, the parent might be
> + * nothing, if pdev is a PCI bridge.
> + */
> + if (parent && !parent->state_saved)
> + __pci_save_aspm_l1ss_state(parent);
> +}
> +
> void pci_restore_aspm_l1ss_state(struct pci_dev *pdev)
> {
> struct pci_cap_saved_state *pl_save_state, *cl_save_state;
> --
> 2.47.0
>
>
Thanks for sending this patch! I tested on a Pixel device with 6.6
kernel. I verified that the root port and the endpoint device were
being restored with the L1ss configuration which was determined on
endpoint enumeration. Feel free to include
Tested-by: Ajay Agarwal <ajayagarwal@google.com>
© 2016 - 2026 Red Hat, Inc.