Skip to contentSkip to conversation

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

PATCH Applied Jian-Hong Pan 5 participants 14 messages Oct 1, 2024 – Nov 7, 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
#0 lore raw

v12 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      | 26 +++++++++++++++++++++++++-
 2 files changed, 34 insertions(+), 5 deletions(-)
Signature · 1 line
2.46.2
author
#1 lore raw

PATCH v12 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~12:
- 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
Applied
#2 lore raw

Re: [PATCH v12 1/3] PCI: vmd: Set PCI devices to D0 before enable PCI PM's L1 substates

Hello,

Quoted 6 lines

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

Quoted 8 lines
	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".

Applied to controller/vmd, thank you!

[01/01] PCI: vmd: Set PCI devices to D0 before enable PCI PM's L1 substates

        https://git.kernel.org/pci/pci/c/c8d39213cb70

	Krzysztof
author
#3 lore raw

PATCH v12 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~12:
- 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
author
#4 lore raw

PATCH v12 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. 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.

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

Link https://lore.kernel.org/linux-pci/CAPpJ_eexU0gCHMbXw_z924WxXw0+B6SdS4eG9oGpEX1wmnMLkQ@mail.gmail.com/ open here
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 <[email protected]>
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()

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

 drivers/pci/pcie/aspm.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)
drivers/pci/pcie/aspm.c +19 -1
index bd0a8a05647e..17cdf372f7e0 100644
@@ -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,24 @@ 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;
+
+	__pci_save_aspm_l1ss_state(pdev);
+
+	/*
+	 * To be symmetric on pci_restore_aspm_l1ss_state(), save parent's L1
+	 * substate configuration, if the parent has not saved state.
+	 */
+	if (!pdev->bus || !pdev->bus->self)
+		return;
+
+	parent = pdev->bus->self;
+	if (!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;
Signature · 1 line
2.46.2
Reviewed-by on v12 3/3
#5 lore raw

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

Quoted 46 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

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.

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

Quoted 5 lines
Link https://lore.kernel.org/linux-pci/CAPpJ_eexU0gCHMbXw_z924WxXw0+B6SdS4eG9oGpEX1wmnMLkQ@mail.gmail.com/ open here
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 <[email protected]>
Signed-off-by Jian-Hong Pan <[email protected]>
Reviewed-by Ilpo Järvinen <[email protected]>
Signature · 1 line
 i.
Quoted 61 lines

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

 drivers/pci/pcie/aspm.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)
drivers/pci/pcie/aspm.c +19 -1
index bd0a8a05647e..17cdf372f7e0 100644
@@ -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,24 @@ 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;
+
+	__pci_save_aspm_l1ss_state(pdev);
+
+	/*
+	 * To be symmetric on pci_restore_aspm_l1ss_state(), save parent's L1
+	 * substate configuration, if the parent has not saved state.
+	 */
+	if (!pdev->bus || !pdev->bus->self)
+		return;
+
+	parent = pdev->bus->self;
+	if (!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;
Reviewed-by on v12 3/3
#6 lore raw

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

In reply to Jian-Hong Pan

Hi Jian-Hong,

Quoted 112 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

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.

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

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 <[email protected]>
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()

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

 drivers/pci/pcie/aspm.c | 20 +++++++++++++++++++-

 1 file changed, 19 insertions(+), 1 deletion(-)
drivers/pci/pcie/aspm.c +0 -0
index bd0a8a05647e..17cdf372f7e0 100644
@@ -79,7 +79,7 @@ void pci_configure_aspm_l1ss(struct pci_dev *pdev)
  			ERR_PTR(rc));
  }

drivers/pci/pcie/aspm.c +1 -1
-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;

drivers/pci/pcie/aspm.c +0 -0
@@ -101,6 +101,24 @@ void pci_save_aspm_l1ss_state(struct pci_dev *pdev)
  	pci_read_config_dword(pdev, l1ss + PCI_L1SS_CTL1, cap++);
  }

drivers/pci/pcie/aspm.c +18 -0
+void pci_save_aspm_l1ss_state(struct pci_dev *pdev)
+{
+	struct pci_dev *parent;
+
+	__pci_save_aspm_l1ss_state(pdev);
+
+	/*
+	 * To be symmetric on pci_restore_aspm_l1ss_state(), save parent's L1
+	 * substate configuration, if the parent has not saved state.
+	 */
+	if (!pdev->bus || !pdev->bus->self)
+		return;
+
+	parent = pdev->bus->self;
+	if (!parent->state_saved)
+		__pci_save_aspm_l1ss_state(parent);
+}
+
Quoted 3 lines

 void pci_restore_aspm_l1ss_state(struct pci_dev *pdev) {
  struct pci_cap_saved_state *pl_save_state, *cl_save_state;

This took a while to debug and find a solution. Thanks for continuing to work on this.

Reviewed-by David E. Box <[email protected]
Reviewed-by David E. Box
#7 lore raw

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

Quoted 122 lines · David E. Box

Hi Jian-Hong,

Quoted 112 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

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.

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

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 <[email protected]>
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()

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

 drivers/pci/pcie/aspm.c | 20 +++++++++++++++++++-

 1 file changed, 19 insertions(+), 1 deletion(-)
drivers/pci/pcie/aspm.c +0 -0
index bd0a8a05647e..17cdf372f7e0 100644
@@ -79,7 +79,7 @@ void pci_configure_aspm_l1ss(struct pci_dev *pdev)
  			ERR_PTR(rc));
  }

drivers/pci/pcie/aspm.c +1 -1
-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;

drivers/pci/pcie/aspm.c +0 -0
@@ -101,6 +101,24 @@ void pci_save_aspm_l1ss_state(struct pci_dev *pdev)
  	pci_read_config_dword(pdev, l1ss + PCI_L1SS_CTL1, cap++);
  }

drivers/pci/pcie/aspm.c +18 -0
+void pci_save_aspm_l1ss_state(struct pci_dev *pdev)
+{
+	struct pci_dev *parent;
+
+	__pci_save_aspm_l1ss_state(pdev);
+
+	/*
+	 * To be symmetric on pci_restore_aspm_l1ss_state(), save parent's L1
+	 * substate configuration, if the parent has not saved state.
+	 */
+	if (!pdev->bus || !pdev->bus->self)
+		return;
+
+	parent = pdev->bus->self;
+	if (!parent->state_saved)
+		__pci_save_aspm_l1ss_state(parent);
+}
+
Quoted 3 lines

 void pci_restore_aspm_l1ss_state(struct pci_dev *pdev) {
  struct pci_cap_saved_state *pl_save_state, *cl_save_state;

This took a while to debug and find a solution. Thanks for continuing to work on this.

Quoted 1 line
Reviewed-by David E. Box <[email protected]

The tag is missing the closing >.

Repeating the correct one here for the purpose of patchwork to pick up the correct tag:

Reviewed-by David E. Box <[email protected]>

...But I suppose patchwork might now record both the wrong and the correct tag so heads up to maintainers if applying this version of the patch.

Signature · 1 line
 i.
Applied
#8 lore raw

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

In reply to Jian-Hong Pan

Hello,

Quoted 42 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

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.

Quoted 3 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().

Applied to aspm, thank you!

[01/02] PCI/ASPM: Add notes about enabling PCI-PM L1SS to pci_enable_link_state(_locked)

        https://git.kernel.org/pci/pci/c/4681da23786a

[02/02] PCI/ASPM: Make pci_save_aspm_l1ss_state save both child and parent's L1SS configuration

        https://git.kernel.org/pci/pci/c/1f37e72d586f

	Krzysztof
#9 lore raw

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

In reply to Jian-Hong Pan
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 7 lines

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.

There's nothing specific to VMD here, is there? This whole log looks like it should be made generic. The VMD *example* is OK, but the justification should not be VMD-specific. This last paragraph seems to be the kernel of the whole thing, and I don't think it's specific to either VMD or NVMe.

Quoted 45 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().

Link https://lore.kernel.org/linux-pci/CAPpJ_eexU0gCHMbXw_z924WxXw0+B6SdS4eG9oGpEX1wmnMLkQ@mail.gmail.com/ open here
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 <[email protected]>
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()

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

 drivers/pci/pcie/aspm.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)
drivers/pci/pcie/aspm.c +1 -1
index bd0a8a05647e..17cdf372f7e0 100644
@@ -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;

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

Quoted 8 lines
drivers/pci/pcie/aspm.c +5 -0
@@ -101,6 +101,24 @@ 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;
+
+	__pci_save_aspm_l1ss_state(pdev);

Is there any point in saving the "pdev" state if there's no parent?

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

Quoted 6 lines
drivers/pci/pcie/aspm.c +6 -0
+	/*
+	 * To be symmetric on pci_restore_aspm_l1ss_state(), save parent's L1
+	 * substate configuration, if the parent has not saved state.
+	 */
+	if (!pdev->bus || !pdev->bus->self)
+		return;

Is "pdev->bus == NULL" possible here even though it doesn't seem possible in pci_restore_aspm_l1ss_state()?

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

Quoted 4 lines
drivers/pci/pcie/aspm.c +4 -0
+	parent = pdev->bus->self;
+	if (!parent->state_saved)
+		__pci_save_aspm_l1ss_state(parent);
+}

I see the suggestion for a helper here, but I'm not convinced. pci_save_aspm_l1ss_state() and pci_restore_aspm_l1ss_state() should *look* similar, and a helper makes them less similar.

I think you should go to some effort to follow the pci_restore_aspm_l1ss_state() structure, as much as possible doing the same declarations, checks, and lookups in the same order, e.g.:

  struct pci_cap_saved_state *pl_save_state, *cl_save_state;
  struct pci_dev *parent = pdev->bus->self;

if (pcie_downstream_port(pdev) || !parent)

	  return;

if (!pdev->l1ss || !parent->l1ss)

	  return;

cl_save_state = pci_find_saved_ext_cap(pdev, PCI_EXT_CAP_ID_L1SS); pl_save_state = pci_find_saved_ext_cap(parent, PCI_EXT_CAP_ID_L1SS); if (!cl_save_state || !pl_save_state)

	  return;

Bjorn

#10 lore raw

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

Quoted 134 lines · Bjorn Helgaas
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 7 lines

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.

There's nothing specific to VMD here, is there? This whole log looks like it should be made generic. The VMD *example* is OK, but the justification should not be VMD-specific. This last paragraph seems to be the kernel of the whole thing, and I don't think it's specific to either VMD or NVMe.

Quoted 45 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().

Link https://lore.kernel.org/linux-pci/CAPpJ_eexU0gCHMbXw_z924WxXw0+B6SdS4eG9oGpEX1wmnMLkQ@mail.gmail.com/ open here
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 <[email protected]>
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()

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

 drivers/pci/pcie/aspm.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)
drivers/pci/pcie/aspm.c +1 -1
index bd0a8a05647e..17cdf372f7e0 100644
@@ -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;

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

Quoted 8 lines
drivers/pci/pcie/aspm.c +5 -0
@@ -101,6 +101,24 @@ 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;
+
+	__pci_save_aspm_l1ss_state(pdev);

Is there any point in saving the "pdev" state if there's no parent?

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

Quoted 6 lines
drivers/pci/pcie/aspm.c +6 -0
+	/*
+	 * To be symmetric on pci_restore_aspm_l1ss_state(), save parent's L1
+	 * substate configuration, if the parent has not saved state.
+	 */
+	if (!pdev->bus || !pdev->bus->self)
+		return;

Is "pdev->bus == NULL" possible here even though it doesn't seem possible in pci_restore_aspm_l1ss_state()?

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

Quoted 4 lines
drivers/pci/pcie/aspm.c +4 -0
+	parent = pdev->bus->self;
+	if (!parent->state_saved)
+		__pci_save_aspm_l1ss_state(parent);
+}

I see the suggestion for a helper here, but I'm not convinced. pci_save_aspm_l1ss_state() and pci_restore_aspm_l1ss_state() should *look* similar, and a helper makes them less similar.

I think you should go to some effort to follow the pci_restore_aspm_l1ss_state() structure, as much as possible doing the same declarations, checks, and lookups in the same order, e.g.:

  struct pci_cap_saved_state *pl_save_state, *cl_save_state;
  struct pci_dev *parent = pdev->bus->self;

if (pcie_downstream_port(pdev) || !parent)

	  return;
Quoted 7 lines

if (!pdev->l1ss || !parent->l1ss)

	  return;

cl_save_state = pci_find_saved_ext_cap(pdev, PCI_EXT_CAP_ID_L1SS); pl_save_state = pci_find_saved_ext_cap(parent, PCI_EXT_CAP_ID_L1SS); if (!cl_save_state || !pl_save_state)

	  return;

Hi,

I understand I'm not the one who has the final say in this, but the reason why restore has to be done the way it is (the long way), is because of the strict ordering requirement of operations it performs.

There are no similar ordering requirements on the save side AFAIK.

Signature · 1 line
 i.
#11 lore raw

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

Quoted 34 lines · Ilpo Järvinen
Quoted 5 lines · Bjorn Helgaas
Quoted 4 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.

...

Quoted 18 lines
Quoted 3 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().

I see the suggestion for a helper here, but I'm not convinced. pci_save_aspm_l1ss_state() and pci_restore_aspm_l1ss_state() should *look* similar, and a helper makes them less similar.

I think you should go to some effort to follow the pci_restore_aspm_l1ss_state() structure, as much as possible doing the same declarations, checks, and lookups in the same order, e.g.:

  struct pci_cap_saved_state *pl_save_state, *cl_save_state;
  struct pci_dev *parent = pdev->bus->self;

if (pcie_downstream_port(pdev) || !parent)

	  return;
Quoted 7 lines

if (!pdev->l1ss || !parent->l1ss)

	  return;

cl_save_state = pci_find_saved_ext_cap(pdev, PCI_EXT_CAP_ID_L1SS); pl_save_state = pci_find_saved_ext_cap(parent, PCI_EXT_CAP_ID_L1SS); if (!cl_save_state || !pl_save_state)

	  return;
Quoted 5 lines

I understand I'm not the one who has the final say in this, but the reason why restore has to be done the way it is (the long way), is because of the strict ordering requirement of operations it performs.

There are no similar ordering requirements on the save side AFAIK.

I'm not suggesting any change to the restore side. The commit log says we're making save/restore symmetric, but IMO they end up looking very asymmetric.

Bjorn

author
#12 lore raw

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

In reply to Bjorn Helgaas

Bjorn Helgaas <[email protected]> 於 2024年11月6日 週三 上午6:59寫道：

Quoted 44 lines · Jian-Hong Pan
Quoted 34 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 7 lines

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.

Quoted 5 lines

There's nothing specific to VMD here, is there? This whole log looks like it should be made generic. The VMD *example* is OK, but the justification should not be VMD-specific. This last paragraph seems to be the kernel of the whole thing, and I don't think it's specific to either VMD or NVMe.

It is a generic fix. Lets see how to modify the wording here.

Quoted 48 lines
Quoted 48 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().

Link https://lore.kernel.org/linux-pci/CAPpJ_eexU0gCHMbXw_z924WxXw0+B6SdS4eG9oGpEX1wmnMLkQ@mail.gmail.com/ open here
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 <[email protected]>
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()

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

 drivers/pci/pcie/aspm.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)
drivers/pci/pcie/aspm.c +1 -1
index bd0a8a05647e..17cdf372f7e0 100644
@@ -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,24 @@ void pci_save_aspm_l1ss_state(struct pci_dev *pdev)
      pci_read_config_dword(pdev, l1ss + PCI_L1SS_CTL1, cap++);
 }
Quoted 7 lines

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

Quoted 5 lines
drivers/pci/pcie/aspm.c +5 -0
+void pci_save_aspm_l1ss_state(struct pci_dev *pdev)
+{
+     struct pci_dev *parent;
+
+     __pci_save_aspm_l1ss_state(pdev);

Is there any point in saving the "pdev" state if there's no parent?

This is a tricky part. If the code path comes from:
pci_save_state()

    pci_save_pcie_state()
        pci_save_aspm_l1ss_state()

and the pci device is a PCIe bridge, then should the device save ASPM L1SS state?

1. This code tries to save its ASPM L1SS state directly. Then, when the child device saves ASPM L1SS state, it does not need to save the PCIe bridge's ASPM L1SS state again.

2 .However, if we shift this "__pci_save_aspm_l1ss_state(pdev);" after "if (!pdev->bus || !pdev->bus->self)" condition check, then it should save both the device and parent's ASPM L1SS state. Because, PCIe bridge does not have a parent device and will not save its ASPM L1SS state by itself.

Following the 2nd scenario, is it possible to only save & restore a PCIe bridge, and not touch children devices? In this condition, pci_restore_aspm_l1ss_state() will not restore the PCIe bridge's ASPM L1SS state itself, because it does not have a parent. Only the child device can restore the PCIe bridge's ASPM L1SS state via pci_restore_aspm_l1ss_state(). So, lets trace who invoke pci_restore_aspm_l1ss_state():
pci_restore_state()

    "dev->state_saved" condition check
    dev->state_saved()
        pci_restore_aspm_l1ss_state()

The "dev->state_saved" condition check guards it. If the child device has not been saved, then it will not go to restoration. So, the parent device's ASPM L1SS state will not be restored by 0. => Okay

Consider that ASPM L1SS only works when both the link's parent and child devices are configured and powered correctly. The 2nd scenario seems to make more sense.

Quoted 9 lines

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

Quoted 6 lines
drivers/pci/pcie/aspm.c +6 -0
+     /*
+      * To be symmetric on pci_restore_aspm_l1ss_state(), save parent's L1
+      * substate configuration, if the parent has not saved state.
+      */
+     if (!pdev->bus || !pdev->bus->self)
+             return;

Is "pdev->bus == NULL" possible here even though it doesn't seem possible in pci_restore_aspm_l1ss_state()?

After boot & test again and again, it seems the devices already have their bus at this point.

However, after I traced the code, I found two possible paths:
1. pcie_config_aspm_link() -> pci_save_aspm_l1ss_state(): Here is already the link. So, has the bus.
2. pci_save_state() -> pci_save_pcie_state() -> pci_save_aspm_l1ss_state(): pci_save_state() is an exported function which can be invoked at any point. So, I am not sure about this part. And, that is why I make it check "pdev->bus == NULL" here.

Jian-Hong Pan

Quoted 28 lines

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

Quoted 4 lines
drivers/pci/pcie/aspm.c +4 -0
+     parent = pdev->bus->self;
+     if (!parent->state_saved)
+             __pci_save_aspm_l1ss_state(parent);
+}

I see the suggestion for a helper here, but I'm not convinced. pci_save_aspm_l1ss_state() and pci_restore_aspm_l1ss_state() should *look* similar, and a helper makes them less similar.

I think you should go to some effort to follow the pci_restore_aspm_l1ss_state() structure, as much as possible doing the same declarations, checks, and lookups in the same order, e.g.:

  struct pci_cap_saved_state *pl_save_state, *cl_save_state;
  struct pci_dev *parent = pdev->bus->self;

  if (pcie_downstream_port(pdev) || !parent)
          return;

  if (!pdev->l1ss || !parent->l1ss)
          return;

  cl_save_state = pci_find_saved_ext_cap(pdev, PCI_EXT_CAP_ID_L1SS);
  pl_save_state = pci_find_saved_ext_cap(parent, PCI_EXT_CAP_ID_L1SS);
  if (!cl_save_state || !pl_save_state)
          return;

Bjorn

#13 lore raw

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

Quoted 108 lines · Jian-Hong Pan

Bjorn Helgaas <[email protected]> 於 2024年11月6日 週三 上午6:59寫道：

Quoted 43 lines · Jian-Hong Pan
Quoted 34 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 7 lines

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.

Quoted 5 lines

There's nothing specific to VMD here, is there? This whole log looks like it should be made generic. The VMD *example* is OK, but the justification should not be VMD-specific. This last paragraph seems to be the kernel of the whole thing, and I don't think it's specific to either VMD or NVMe.

It is a generic fix. Lets see how to modify the wording here.

Quoted 48 lines
Quoted 48 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().

Link https://lore.kernel.org/linux-pci/CAPpJ_eexU0gCHMbXw_z924WxXw0+B6SdS4eG9oGpEX1wmnMLkQ@mail.gmail.com/ open here
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 <[email protected]>
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()

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

 drivers/pci/pcie/aspm.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)
drivers/pci/pcie/aspm.c +1 -1
index bd0a8a05647e..17cdf372f7e0 100644
@@ -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,24 @@ void pci_save_aspm_l1ss_state(struct pci_dev *pdev)
      pci_read_config_dword(pdev, l1ss + PCI_L1SS_CTL1, cap++);
 }
Quoted 7 lines

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

Quoted 5 lines
drivers/pci/pcie/aspm.c +5 -0
+void pci_save_aspm_l1ss_state(struct pci_dev *pdev)
+{
+     struct pci_dev *parent;
+
+     __pci_save_aspm_l1ss_state(pdev);

Is there any point in saving the "pdev" state if there's no parent?

Quoted 7 lines

This is a tricky part. If the code path comes from:
pci_save_state()

    pci_save_pcie_state()
        pci_save_aspm_l1ss_state()

and the pci device is a PCIe bridge, then should the device save ASPM L1SS state?

This is a good question and is separate from the fundamental problem being solved here. If we zoom in and focus specifically on the case where we restore garbage to the bridge L1SS config, I think this will be more understandable.

Start by making the overall structure similar to pci_restore_aspm_l1ss_state(). If the early exits end up being slightly different because of this concern, that's fine, and we can add a short comment about why they are different.

Quoted 43 lines

1. This code tries to save its ASPM L1SS state directly. Then, when the child device saves ASPM L1SS state, it does not need to save the PCIe bridge's ASPM L1SS state again.

2 .However, if we shift this "__pci_save_aspm_l1ss_state(pdev);" after "if (!pdev->bus || !pdev->bus->self)" condition check, then it should save both the device and parent's ASPM L1SS state. Because, PCIe bridge does not have a parent device and will not save its ASPM L1SS state by itself.

Following the 2nd scenario, is it possible to only save & restore a PCIe bridge, and not touch children devices? In this condition, pci_restore_aspm_l1ss_state() will not restore the PCIe bridge's ASPM L1SS state itself, because it does not have a parent. Only the child device can restore the PCIe bridge's ASPM L1SS state via pci_restore_aspm_l1ss_state(). So, lets trace who invoke pci_restore_aspm_l1ss_state():
pci_restore_state()

    "dev->state_saved" condition check
    dev->state_saved()
        pci_restore_aspm_l1ss_state()

The "dev->state_saved" condition check guards it. If the child device has not been saved, then it will not go to restoration. So, the parent device's ASPM L1SS state will not be restored by 0. => Okay

Consider that ASPM L1SS only works when both the link's parent and child devices are configured and powered correctly. The 2nd scenario seems to make more sense.

Quoted 9 lines

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

Quoted 6 lines
drivers/pci/pcie/aspm.c +6 -0
+     /*
+      * To be symmetric on pci_restore_aspm_l1ss_state(), save parent's L1
+      * substate configuration, if the parent has not saved state.
+      */
+     if (!pdev->bus || !pdev->bus->self)
+             return;

Is "pdev->bus == NULL" possible here even though it doesn't seem possible in pci_restore_aspm_l1ss_state()?

After boot & test again and again, it seems the devices already have their bus at this point.

Quoted 7 lines

However, after I traced the code, I found two possible paths:
1. pcie_config_aspm_link() -> pci_save_aspm_l1ss_state(): Here is already the link. So, has the bus.
2. pci_save_state() -> pci_save_pcie_state() -> pci_save_aspm_l1ss_state(): pci_save_state() is an exported function which can be invoked at any point. So, I am not sure about this part. And, that is why I make it check "pdev->bus == NULL" here.

Is there any case where we build a pci_dev that can have pdev->bus == NULL? I don't think so.

Quoted 28 lines
Quoted 28 lines

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

Quoted 4 lines
drivers/pci/pcie/aspm.c +4 -0
+     parent = pdev->bus->self;
+     if (!parent->state_saved)
+             __pci_save_aspm_l1ss_state(parent);
+}

I see the suggestion for a helper here, but I'm not convinced. pci_save_aspm_l1ss_state() and pci_restore_aspm_l1ss_state() should *look* similar, and a helper makes them less similar.

I think you should go to some effort to follow the pci_restore_aspm_l1ss_state() structure, as much as possible doing the same declarations, checks, and lookups in the same order, e.g.:

  struct pci_cap_saved_state *pl_save_state, *cl_save_state;
  struct pci_dev *parent = pdev->bus->self;

  if (pcie_downstream_port(pdev) || !parent)
          return;

  if (!pdev->l1ss || !parent->l1ss)
          return;

  cl_save_state = pci_find_saved_ext_cap(pdev, PCI_EXT_CAP_ID_L1SS);
  pl_save_state = pci_find_saved_ext_cap(parent, PCI_EXT_CAP_ID_L1SS);
  if (!cl_save_state || !pl_save_state)
          return;

Bjorn