PCI: vmd: Enable PCI PM's L1 substates of remapped PCIe Root Port and NVMe

PATCH Superseded Jian-Hong Pan 2 participants 6 messages Sep 27, 2024 – Sep 30, 2024

Subsystems (3)

PCI PCI native host bridge and endpoint drivers PCI driver for Intel volume management device (vmd)

lore

Share & tools

lore

Jian-Hong Pan

author

2024-09-27 10:33 UTC #0 lore raw

v10 cover PCI: vmd: Enable PCI PM's L1 substates of remapped PCIe Root Port and NVMe

Notice the VMD remapped PCIe Root Port and NVMe have PCI PM L1 substates capability, but they are disabled originally.

Here is a failed example on ASUS B1400CEAE with enabled VMD:

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

According to "PCIe r6.0, sec 5.5.4", to config the link between the PCIe Root Port and the child device correctly:
* Ensure both devices are in D0 before enabling PCI-PM L1 PM Substates.
* Ensure L1.2 parameters: Common_Mode_Restore_Times, T_POWER_ON and LTR_L1.2_THRESHOLD are programmed properly on both devices before enable bits for L1.2.

Prepare this series to fix that.

Jian-Hong Pan (3):
PCI: vmd: Set PCI devices to D0 before enable PCI PM's L1 substates PCI/ASPM: Add notes about enabling PCI-PM L1SS to
pci_enable_link_state(_locked)
PCI: ASPM: Make pci_save_aspm_l1ss_state save both child and parent's
L1SS configuration

 drivers/pci/controller/vmd.c | 13 +++++++----
 drivers/pci/pcie/aspm.c      | 45 +++++++++++++++++++++++++++++-------
 2 files changed, 46 insertions(+), 12 deletions(-)

Signature · 1 line

2.46.2

Jian-Hong Pan

author

2024-09-27 10:36 UTC #1 lore raw

PATCH v10 1/3 PCI: vmd: Set PCI devices to D0 before enable PCI PM's L1 substates

The remapped PCIe Root Port and the child device have PCI PM L1 substates capability, but they are disabled originally.

Here is a failed example on ASUS B1400CEAE:

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

Power on all of the VMD remapped PCI devices to D0 before enable PCI-PM L1 PM Substates by following "PCIe r6.0, sec 5.5.4".

Link https://bugzilla.kernel.org/show_bug.cgi?id=218394

Signed-off-by Jian-Hong Pan <[email protected]>

Reviewed-by Kuppuswamy Sathyanarayanan <[email protected]>

---
v2:
- Power on the VMD remapped devices with pci_set_power_state_locked()
- Prepare the PCIe LTR parameters before enable L1 Substates
- Add note into the comments of both pci_enable_link_state() and pci_enable_link_state_locked() for kernel-doc.
- The original patch set can be split as individual patches.

v3:
- Re-send for the missed version information.
- Split drivers/pci/pcie/aspm.c modification into following patches.
- Fix the comment for enasuring the PCI devices in D0.

v4:
- The same

v5:
- Tweak the commit title and message
- Change the goto label from out_enable_link_state to out_state_change

v6~8:
- The same

v9:
- Update L1 PM Substates information against kernel v6.11 in commit message

v10:
- The same

 drivers/pci/controller/vmd.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

drivers/pci/controller/vmd.c +9 -4

index 264a180403a0..11870d1fc818 100644
@@ -740,11 +740,9 @@ static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
 	if (!(features & VMD_FEAT_BIOS_PM_QUIRK))
 		return 0;
 
-	pci_enable_link_state_locked(pdev, PCIE_LINK_STATE_ALL);
-
 	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR);
 	if (!pos)
-		return 0;
+		goto out_state_change;
 
 	/*
 	 * Skip if the max snoop LTR is non-zero, indicating BIOS has set it

@@ -752,7 +750,7 @@ static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
 	 */
 	pci_read_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, &ltr_reg);
 	if (!!(ltr_reg & (PCI_LTR_VALUE_MASK | PCI_LTR_SCALE_MASK)))
-		return 0;
+		goto out_state_change;
 
 	/*
 	 * Set the default values to the maximum required by the platform to

@@ -764,6 +762,13 @@ static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
 	pci_write_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, ltr_reg);
 	pci_info(pdev, "VMD: Default LTR value set by driver\n");
 
+out_state_change:
+	/*
+	 * Ensure devices are in D0 before enabling PCI-PM L1 PM Substates, per
+	 * PCIe r6.0, sec 5.5.4.
+	 */
+	pci_set_power_state_locked(pdev, PCI_D0);
+	pci_enable_link_state_locked(pdev, PCIE_LINK_STATE_ALL);
 	return 0;
 }

Signature · 1 line

2.46.2

Jian-Hong Pan

author

2024-09-27 10:37 UTC #2 lore raw

PATCH v10 2/3 PCI/ASPM: Add notes about enabling PCI-PM L1SS to pci_enable_link_state(_locked)

In reply to Jian-Hong Pan

According to "PCIe r6.0, sec 5.5.4", add note about D0 requirement in pci_enable_link_state() kernel-doc.

Signed-off-by Jian-Hong Pan <[email protected]>

Acked-by Bjorn Helgaas <[email protected]>

Reviewed-by Kuppuswamy Sathyanarayanan <[email protected]>

---
v3:
- Fix as readable comments

v4:
- The same

v5:
- Tweak and simplify the commit message

v6~10:
- The same

 drivers/pci/pcie/aspm.c | 6 ++++++
 1 file changed, 6 insertions(+)

drivers/pci/pcie/aspm.c +6 -0

index cee2365e54b8..bd0a8a05647e 100644
@@ -1442,6 +1442,9 @@ static int __pci_enable_link_state(struct pci_dev *pdev, int state, bool locked)
  * touch the LNKCTL register. Also note that this does not enable states
  * disabled by pci_disable_link_state(). Return 0 or a negative errno.
  *
+ * Note: Ensure devices are in D0 before enabling PCI-PM L1 PM Substates, per
+ * PCIe r6.0, sec 5.5.4.
+ *
  * @pdev: PCI device
  * @state: Mask of ASPM link states to enable
  */

@@ -1458,6 +1461,9 @@ EXPORT_SYMBOL(pci_enable_link_state);
  * can't touch the LNKCTL register. Also note that this does not enable states
  * disabled by pci_disable_link_state(). Return 0 or a negative errno.
  *
+ * Note: Ensure devices are in D0 before enabling PCI-PM L1 PM Substates, per
+ * PCIe r6.0, sec 5.5.4.
+ *
  * @pdev: PCI device
  * @state: Mask of ASPM link states to enable
  *

Signature · 1 line

2.46.2

Jian-Hong Pan

author

2024-09-27 10:37 UTC #3 lore raw

PATCH v10 3/3 PCI/ASPM: Make pci_save_aspm_l1ss_state save both child and parent's L1SS configuration

In reply to Jian-Hong Pan

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. Because bus [e1] has only one
device: 10000:e1:00.0 NVMe. The PCIe bridge is missed. However, when it restores the NVMe's state, it also restores the ASPM L1SS between the PCIe bridge and the NVMe by pci_restore_aspm_l1ss_state(). The NVMe's L1SS is restored correctly. But, the PCIe bridge's L1SS is restored with the wrong value 0x0 [1]. Because, the parent device (PCIe bridge)'s L1SS is not saved by pci_save_aspm_l1ss_state() before reset. That is why pci_restore_aspm_l1ss_state() gets the parent device (PCIe bridge)'s saved state capability data and restores L1SS with value 0.

So, if the PCI device has a parent, make pci_save_aspm_l1ss_state() save the parent's L1SS configuration, too. This is symmetric on pci_restore_aspm_l1ss_state().

[1]: https://lore.kernel.org/linux-pci/CAPpJ_eexU0gCHMbXw_z924WxXw0+B6SdS4eG9oGpEX1wmnMLkQ@mail.gmail.com/

Link https://bugzilla.kernel.org/show_bug.cgi?id=218394

Signed-off-by Jian-Hong Pan <[email protected]>

---
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()

 drivers/pci/pcie/aspm.c | 39 +++++++++++++++++++++++++++++++--------
 1 file changed, 31 insertions(+), 8 deletions(-)

drivers/pci/pcie/aspm.c +31 -8

index bd0a8a05647e..823aaf813978 100644
@@ -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_cap_saved_state *save_state;
-	u16 l1ss = pdev->l1ss;
+	struct pci_cap_saved_state *pl_save_state, *cl_save_state;
+	struct pci_dev *parent;
 	u32 *cap;
 
 	/*
 	 * Save L1 substate configuration. The ASPM L0s/L1 configuration
 	 * in PCI_EXP_LNKCTL_ASPMC is saved by pci_save_pcie_state().
 	 */
-	if (!l1ss)
+	if (!pdev->l1ss)
 		return;
 
-	save_state = pci_find_saved_ext_cap(pdev, PCI_EXT_CAP_ID_L1SS);
-	if (!save_state)
+	cl_save_state = pci_find_saved_ext_cap(pdev, PCI_EXT_CAP_ID_L1SS);
+	if (!cl_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++);
+	cap = &cl_save_state->cap.data[0];
+	pci_read_config_dword(pdev, pdev->l1ss + PCI_L1SS_CTL2, cap++);
+	pci_read_config_dword(pdev, pdev->l1ss + PCI_L1SS_CTL1, cap++);
+
+	/*
+	 * If here is a parent device and it has not saved state, save parent's
+	 * L1 substate configuration, too. This is symmetric on
+	 * pci_restore_aspm_l1ss_state().
+	 */
+	if (!pdev->bus || !pdev->bus->self)
+		return;
+
+	parent = pdev->bus->self;
+	if (!parent->l1ss)
+		return;
+
+	pl_save_state = pci_find_saved_ext_cap(parent, PCI_EXT_CAP_ID_L1SS);
+	if (!pl_save_state)
+		return;
+
+	if (parent->state_saved)
+		return;
+
+	cap = &pl_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)

Signature · 1 line

2.46.2

Ilpo Järvinen

2024-09-27 11:17 UTC #4 lore raw

Re: [PATCH v10 3/3] PCI/ASPM: Make pci_save_aspm_l1ss_state save both child and parent's L1SS configuration

Quoted 34 lines · Jian-Hong Pan

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

Quoted 5 lines

When pci_reset_bus() resets the bus [e1] of the NVMe, it only saves and restores NVMe's state before and after reset.
Because bus [e1] has only one
device: 10000:e1:00.0 NVMe.

This is incomplete sentence. And I don't understand why the number of devices is relevant. Perhaps just drop it?

Quoted 1 line

The PCIe bridge is missed.

Unclear what this refers to (I know what you mean but please write it so that other reading this 10 years from now will also get what is missed).

Quoted 3 lines

However, when it
restores the NVMe's state, it also restores the ASPM L1SS between the PCIe bridge and the NVMe by pci_restore_aspm_l1ss_state().

"it also restores ..." -> "ASPM code restores L1SS for both the parent bridge and the NVMe in pci_restore_aspm_l1ss_state()."

Quoted 3 lines

The NVMe's L1SS is
restored correctly. But, the PCIe bridge's L1SS is restored with the wrong value 0x0 [1]. Because, the parent device (PCIe bridge)'s L1SS is not saved

Join these sentences without . and drop the comma.

I'd say "was not saved" because at that point it occurred clearly before the restore.

Quoted 3 lines

by pci_save_aspm_l1ss_state() before reset. That is why pci_restore_aspm_l1ss_state() gets the parent device (PCIe bridge)'s saved state capability data and restores L1SS with value 0.

This last sentence seems duplicated.

Quoted 5 lines

So, if the PCI device has a parent, make pci_save_aspm_l1ss_state() save the parent's L1SS configuration, too. This is symmetric on pci_restore_aspm_l1ss_state().
[1]: https://lore.kernel.org/linux-pci/CAPpJ_eexU0gCHMbXw_z924WxXw0+B6SdS4eG9oGpEX1wmnMLkQ@mail.gmail.com/

Just put this into Link tag.

Quoted 1 line

Link https://bugzilla.kernel.org/show_bug.cgi?id=218394

Closes ?

Fixes tag is also missing.

Add:

Quoted 56 lines

Signed-off-by Jian-Hong Pan <[email protected]>

---
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()

 drivers/pci/pcie/aspm.c | 39 +++++++++++++++++++++++++++++++--------
 1 file changed, 31 insertions(+), 8 deletions(-)

drivers/pci/pcie/aspm.c +17 -8

index bd0a8a05647e..823aaf813978 100644
@@ -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_cap_saved_state *save_state;
-	u16 l1ss = pdev->l1ss;
+	struct pci_cap_saved_state *pl_save_state, *cl_save_state;
+	struct pci_dev *parent;
 	u32 *cap;
 
 	/*
 	 * Save L1 substate configuration. The ASPM L0s/L1 configuration
 	 * in PCI_EXP_LNKCTL_ASPMC is saved by pci_save_pcie_state().
 	 */
-	if (!l1ss)
+	if (!pdev->l1ss)
 		return;
 
-	save_state = pci_find_saved_ext_cap(pdev, PCI_EXT_CAP_ID_L1SS);
-	if (!save_state)
+	cl_save_state = pci_find_saved_ext_cap(pdev, PCI_EXT_CAP_ID_L1SS);
+	if (!cl_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++);
+	cap = &cl_save_state->cap.data[0];
+	pci_read_config_dword(pdev, pdev->l1ss + PCI_L1SS_CTL2, cap++);
+	pci_read_config_dword(pdev, pdev->l1ss + PCI_L1SS_CTL1, cap++);
+
+	/*
+	 * If here is a parent device and it has not saved state, save parent's
+	 * L1 substate configuration, too. This is symmetric on
+	 * pci_restore_aspm_l1ss_state().
+	 */
+	if (!pdev->bus || !pdev->bus->self)
+		return;
+

drivers/pci/pcie/aspm.c void pci_configure_aspm_l1ss(struct pci_dev *pdev) · line 120

Quoted 10 lines

drivers/pci/pcie/aspm.c +10 -0

+	parent = pdev->bus->self;
+	if (!parent->l1ss)
+		return;
+
+	pl_save_state = pci_find_saved_ext_cap(parent, PCI_EXT_CAP_ID_L1SS);
+	if (!pl_save_state)
+		return;
+
+	if (parent->state_saved)
+		return;

This decision can be made before even reading the pl_saved_state.

drivers/pci/pcie/aspm.c void pci_configure_aspm_l1ss(struct pci_dev *pdev) · line 124

Quoted 4 lines

drivers/pci/pcie/aspm.c +4 -0

+
+	cap = &pl_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++);

This approach duplicates code and that seems unnecessary to me.

Could you instead leave the current function (nearly?) as is and use it as a helper for a new pci_save_aspm_l1ss_state() which calls the helper first for the pdev and then for its parent (if needed). Or do I miss something why that is not possible?

Signature · 1 line

i.

Jian-Hong Pan

author

2024-09-30 09:31 UTC #5 lore raw

Re: [PATCH v10 3/3] PCI/ASPM: Make pci_save_aspm_l1ss_state save both child and parent's L1SS configuration

Ilpo Järvinen <[email protected]> 於 2024年9月27日週五下午7:17寫道：

Quoted 166 lines · Jian-Hong Pan

Quoted 35 lines
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
Quoted 5 lines
When pci_reset_bus() resets the bus [e1] of the NVMe, it only saves and restores NVMe's state before and after reset.
Because bus [e1] has only one
device: 10000:e1:00.0 NVMe.
This is incomplete sentence. And I don't understand why the number of devices is relevant. Perhaps just drop it?
Quoted 1 line
The PCIe bridge is missed.
Unclear what this refers to (I know what you mean but please write it so that other reading this 10 years from now will also get what is missed).
Quoted 3 lines
However, when it
restores the NVMe's state, it also restores the ASPM L1SS between the PCIe bridge and the NVMe by pci_restore_aspm_l1ss_state().
"it also restores ..." -> "ASPM code restores L1SS for both the parent bridge and the NVMe in pci_restore_aspm_l1ss_state()."
Quoted 3 lines
The NVMe's L1SS is
restored correctly. But, the PCIe bridge's L1SS is restored with the wrong value 0x0 [1]. Because, the parent device (PCIe bridge)'s L1SS is not saved
Join these sentences without . and drop the comma.
I'd say "was not saved" because at that point it occurred clearly before the restore.
Quoted 3 lines
by pci_save_aspm_l1ss_state() before reset. That is why pci_restore_aspm_l1ss_state() gets the parent device (PCIe bridge)'s saved state capability data and restores L1SS with value 0.
This last sentence seems duplicated.
Quoted 5 lines
So, if the PCI device has a parent, make pci_save_aspm_l1ss_state() save the parent's L1SS configuration, too. This is symmetric on pci_restore_aspm_l1ss_state().
[1]: https://lore.kernel.org/linux-pci/CAPpJ_eexU0gCHMbXw_z924WxXw0+B6SdS4eG9oGpEX1wmnMLkQ@mail.gmail.com/
Just put this into Link tag.
Quoted 1 line
Link https://bugzilla.kernel.org/show_bug.cgi?id=218394
Closes ?
Fixes tag is also missing.
Add:
Suggested-by Ilpo Järvinen <[email protected]>
Quoted 56 lines
Signed-off-by Jian-Hong Pan <[email protected]>
---
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()
 drivers/pci/pcie/aspm.c | 39 +++++++++++++++++++++++++++++++--------
 1 file changed, 31 insertions(+), 8 deletions(-)
drivers/pci/pcie/aspm.c +17 -8
index bd0a8a05647e..823aaf813978 100644
@@ -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_cap_saved_state *save_state;
-     u16 l1ss = pdev->l1ss;
+     struct pci_cap_saved_state *pl_save_state, *cl_save_state;
+     struct pci_dev *parent;
      u32 *cap;
 
      /*
       * Save L1 substate configuration. The ASPM L0s/L1 configuration
       * in PCI_EXP_LNKCTL_ASPMC is saved by pci_save_pcie_state().
       */
-     if (!l1ss)
+     if (!pdev->l1ss)
              return;
 
-     save_state = pci_find_saved_ext_cap(pdev, PCI_EXT_CAP_ID_L1SS);
-     if (!save_state)
+     cl_save_state = pci_find_saved_ext_cap(pdev, PCI_EXT_CAP_ID_L1SS);
+     if (!cl_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++);
+     cap = &cl_save_state->cap.data[0];
+     pci_read_config_dword(pdev, pdev->l1ss + PCI_L1SS_CTL2, cap++);
+     pci_read_config_dword(pdev, pdev->l1ss + PCI_L1SS_CTL1, cap++);
+
+     /*
+      * If here is a parent device and it has not saved state, save parent's
+      * L1 substate configuration, too. This is symmetric on
+      * pci_restore_aspm_l1ss_state().
+      */
+     if (!pdev->bus || !pdev->bus->self)
+             return;
+
drivers/pci/pcie/aspm.c void pci_configure_aspm_l1ss(struct pci_dev *pdev) · line 120
Quoted 10 lines
drivers/pci/pcie/aspm.c +10 -0
+     parent = pdev->bus->self;
+     if (!parent->l1ss)
+             return;
+
+     pl_save_state = pci_find_saved_ext_cap(parent, PCI_EXT_CAP_ID_L1SS);
+     if (!pl_save_state)
+             return;
+
+     if (parent->state_saved)
+             return;
This decision can be made before even reading the pl_saved_state.
drivers/pci/pcie/aspm.c void pci_configure_aspm_l1ss(struct pci_dev *pdev) · line 124
Quoted 4 lines
drivers/pci/pcie/aspm.c +4 -0
+
+     cap = &pl_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++);

Quoted 6 lines

This approach duplicates code and that seems unnecessary to me.
Could you instead leave the current function (nearly?) as is and use it as a helper for a new pci_save_aspm_l1ss_state() which calls the helper first for the pdev and then for its parent (if needed). Or do I miss something why that is not possible?

I was thinking of invoking it recursively the first time. But, that will go to parent's parent and so on ...
So, I flattened the code in pci_save_aspm_l1ss_state().

Having a helper doing what original pci_save_aspm_l1ss_state()'s work sounds good!

Sent the v11 patch, including the modified commit message.

Jian-Hong Pan