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

PATCH Superseded Jian-Hong Pan 3 participants 10 messages Jun 24, 2024 – Jun 25, 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-06-24 08:11 UTC #0 lore raw

v6 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: Intel Corporation 11th Gen Core Processor PCIe Controller (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=45us LTR1.2_Threshold=101376ns
        L1SubCtl2: T_PwrOn=50us

10000:e1:00.0 Non-Volatile memory controller: Sandisk Corp WD Blue SN550 NVMe SSD (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=0ns
        L1SubCtl2: T_PwrOn=10us

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: vmd: Drop resetting PCI bus action after scan mapped PCI child
bus

 drivers/pci/controller/vmd.c | 33 +++++++++------------------------
 drivers/pci/pcie/aspm.c      |  6 ++++++
 2 files changed, 15 insertions(+), 24 deletions(-)

Signature · 1 line

2.45.2

Jian-Hong Pan

author

2024-06-24 08:15 UTC #1 lore raw

PATCH v6 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=0ns
        L1SubCtl2: T_PwrOn=10us

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:
- The same

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

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

index 87b7856f375a..5309afbe31f9 100644
@@ -751,11 +751,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

@@ -763,7 +761,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

@@ -775,6 +773,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.45.2

Jian-Hong Pan

author

2024-06-24 08:18 UTC #2 lore raw

PATCH v6 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:
- 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.45.2

Jian-Hong Pan

author

2024-06-24 08:22 UTC #3 lore raw

PATCH v6 3/3 PCI: vmd: Drop resetting PCI bus action after scan mapped PCI child bus

In reply to Jian-Hong Pan

According to "PCIe r6.0, sec 5.5.4", before enabling ASPM L1.2 on the PCIe Root Port and the child device, they should be programmed with the same LTR1.2_Threshold value. However, they have different values on VMD mapped PCI child bus. For example, Asus B1400CEAE's VMD mapped PCI bridge and NVMe SSD controller have different LTR1.2_Threshold values:

10000:e0:06.0 PCI bridge: Intel Corporation 11th Gen Core Processor PCIe Controller (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=45us LTR1.2_Threshold=101376ns
        L1SubCtl2: T_PwrOn=50us

10000:e1:00.0 Non-Volatile memory controller: Sandisk Corp WD Blue SN550 NVMe SSD (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=0ns
        L1SubCtl2: T_PwrOn=10us

After debug in detail, both of the VMD mapped PCI bridge and the NVMe SSD controller have been configured properly with the same LTR1.2_Threshold value. But, become misconfigured after reset the VMD mapped PCI bus which is introduced from commit 0a584655ef89 ("PCI: vmd: Fix secondary bus reset for Intel bridges") and commit 6aab5622296b ("PCI: vmd: Clean up domain before enumeration"). So, drop the resetting PCI bus action after scan VMD mapped PCI child bus.

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

---
v6:
- Introduced based on the discussion https://lore.kernel.org/linux-pci/CAPpJ_efYWWxGBopbSQHB=Y2+1RrXFR2XWeqEhGTgdiw3XX0Jmw@mail.gmail.com/

 drivers/pci/controller/vmd.c | 20 --------------------
 1 file changed, 20 deletions(-)

drivers/pci/controller/vmd.c +0 ~~-20~~

index 5309afbe31f9..af413cdb4f4e 100644
@@ -793,7 +793,6 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 	resource_size_t offset[2] = {0};
 	resource_size_t membar2_offset = 0x2000;
 	struct pci_bus *child;
-	struct pci_dev *dev;
 	int ret;
 
 	/*

@@ -935,25 +934,6 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 	pci_scan_child_bus(vmd->bus);
 	vmd_domain_reset(vmd);
 
-	/* When Intel VMD is enabled, the OS does not discover the Root Ports
-	 * owned by Intel VMD within the MMCFG space. pci_reset_bus() applies
-	 * a reset to the parent of the PCI device supplied as argument. This
-	 * is why we pass a child device, so the reset can be triggered at
-	 * the Intel bridge level and propagated to all the children in the
-	 * hierarchy.
-	 */
-	list_for_each_entry(child, &vmd->bus->children, node) {
-		if (!list_empty(&child->devices)) {
-			dev = list_first_entry(&child->devices,
-					       struct pci_dev, bus_list);
-			ret = pci_reset_bus(dev);
-			if (ret)
-				pci_warn(dev, "can't reset device: %d\n", ret);
-
-			break;
-		}
-	}
-
 	pci_assign_unassigned_bus_resources(vmd->bus);
 
 	pci_walk_bus(vmd->bus, vmd_pm_enable_quirk, &features);

Signature · 1 line

2.45.2

Nirmal Patel

maintainer

2024-06-24 15:25 UTC #4 lore raw

Re: [PATCH v6 3/3] PCI: vmd: Drop resetting PCI bus action after scan mapped PCI child bus

Quoted 79 lines · On Mon, 24 Jun 2024 16:21:45 +0800 Jian-Hong Pan <[email protected]> wrote:

According to "PCIe r6.0, sec 5.5.4", before enabling ASPM L1.2 on the PCIe Root Port and the child device, they should be programmed with the same LTR1.2_Threshold value. However, they have different values on VMD mapped PCI child bus. For example, Asus B1400CEAE's VMD mapped PCI bridge and NVMe SSD controller have different LTR1.2_Threshold values:
10000:e0:06.0 PCI bridge: Intel Corporation 11th Gen Core Processor PCIe Controller (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=45us LTR1.2_Threshold=101376ns
        L1SubCtl2: T_PwrOn=50us
10000:e1:00.0 Non-Volatile memory controller: Sandisk Corp WD Blue SN550 NVMe SSD (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=0ns
        L1SubCtl2: T_PwrOn=10us
After debug in detail, both of the VMD mapped PCI bridge and the NVMe SSD controller have been configured properly with the same LTR1.2_Threshold value. But, become misconfigured after reset the VMD mapped PCI bus which is introduced from commit 0a584655ef89 ("PCI:
vmd: Fix secondary bus reset for Intel bridges") and commit 6aab5622296b ("PCI: vmd: Clean up domain before enumeration"). So, drop the resetting PCI bus action after scan VMD mapped PCI child bus.
Signed-off-by Jian-Hong Pan <[email protected]>
---
v6:
- Introduced based on the discussion
https://lore.kernel.org/linux-pci/CAPpJ_efYWWxGBopbSQHB=Y2+1RrXFR2XWeqEhGTgdiw3XX0Jmw@mail.gmail.com/
 drivers/pci/controller/vmd.c | 20 --------------------
 1 file changed, 20 deletions(-)
(quoted diff) +0 -0
b/drivers/pci/controller/vmd.c index 5309afbe31f9..af413cdb4f4e 100644
drivers/pci/controller/vmd.c +0 -2
@@ -793,7 +793,6 @@ static int vmd_enable_domain(struct vmd_dev *vmd,
 unsigned long features) resource_size_t offset[2] = {0};
 	resource_size_t membar2_offset = 0x2000;
 	struct pci_bus *child;
-	struct pci_dev *dev;
 	int ret;
 
 	/*
@@ -935,25 +934,6 @@ static int vmd_enable_domain(struct vmd_dev
 *vmd, unsigned long features) pci_scan_child_bus(vmd->bus);
 	vmd_domain_reset(vmd);
 
-	/* When Intel VMD is enabled, the OS does not discover the
Root Ports
drivers/pci/controller/vmd.c +0 -1
-	 * owned by Intel VMD within the MMCFG space.
pci_reset_bus() applies
drivers/pci/controller/vmd.c +0 -1
-	 * a reset to the parent of the PCI device supplied as
argument. This
drivers/pci/controller/vmd.c +0 -1
-	 * is why we pass a child device, so the reset can be
triggered at
drivers/pci/controller/vmd.c +0 -1
-	 * the Intel bridge level and propagated to all the children
in the
drivers/pci/controller/vmd.c +0 -6
-	 * hierarchy.
-	 */
-	list_for_each_entry(child, &vmd->bus->children, node) {
-		if (!list_empty(&child->devices)) {
-			dev = list_first_entry(&child->devices,
-					       struct pci_dev,
bus_list);
drivers/pci/controller/vmd.c +0 -3
-			ret = pci_reset_bus(dev);
-			if (ret)
-				pci_warn(dev, "can't reset device:
%d\n", ret); -

drivers/pci/controller/vmd.c static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features) · line 939

Quoted 7 lines

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

-			break;
-		}
-	}
-
 	pci_assign_unassigned_bus_resources(vmd->bus);
 
 	pci_walk_bus(vmd->bus, vmd_pm_enable_quirk, &features);

Thanks for the patch.

pci_reset_bus is required to avoid failure in vmd domain creation during multiple soft reboots test. So I believe we can't just remove it without proper testing. vmd_pm_enable_quirk happens after pci_reset_bus, then how is it resetting LTR1.2_Threshold value?

Thanks
-nirmal

Paul M Stillwell Jr

2024-06-24 15:39 UTC #5 lore raw

Re: [PATCH v6 3/3] PCI: vmd: Drop resetting PCI bus action after scan mapped PCI child bus

Quoted 92 lines · Nirmal Patel

Quoted 79 lines · On Mon, 24 Jun 2024 16:21:45 +0800 Jian-Hong Pan <[email protected]> wrote:
According to "PCIe r6.0, sec 5.5.4", before enabling ASPM L1.2 on the PCIe Root Port and the child device, they should be programmed with the same LTR1.2_Threshold value. However, they have different values on VMD mapped PCI child bus. For example, Asus B1400CEAE's VMD mapped PCI bridge and NVMe SSD controller have different LTR1.2_Threshold values:
10000:e0:06.0 PCI bridge: Intel Corporation 11th Gen Core Processor PCIe Controller (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=45us LTR1.2_Threshold=101376ns
         L1SubCtl2: T_PwrOn=50us
10000:e1:00.0 Non-Volatile memory controller: Sandisk Corp WD Blue SN550 NVMe SSD (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=0ns
         L1SubCtl2: T_PwrOn=10us
After debug in detail, both of the VMD mapped PCI bridge and the NVMe SSD controller have been configured properly with the same LTR1.2_Threshold value. But, become misconfigured after reset the VMD mapped PCI bus which is introduced from commit 0a584655ef89 ("PCI:
vmd: Fix secondary bus reset for Intel bridges") and commit 6aab5622296b ("PCI: vmd: Clean up domain before enumeration"). So, drop the resetting PCI bus action after scan VMD mapped PCI child bus.
Signed-off-by Jian-Hong Pan <[email protected]>
---
v6:
- Introduced based on the discussion
https://lore.kernel.org/linux-pci/CAPpJ_efYWWxGBopbSQHB=Y2+1RrXFR2XWeqEhGTgdiw3XX0Jmw@mail.gmail.com/
  drivers/pci/controller/vmd.c | 20 --------------------
  1 file changed, 20 deletions(-)
(quoted diff) +0 -0
b/drivers/pci/controller/vmd.c index 5309afbe31f9..af413cdb4f4e 100644
drivers/pci/controller/vmd.c +0 -2
@@ -793,7 +793,6 @@ static int vmd_enable_domain(struct vmd_dev *vmd,
 unsigned long features) resource_size_t offset[2] = {0};
  	resource_size_t membar2_offset = 0x2000;
  	struct pci_bus *child;
-	struct pci_dev *dev;
  	int ret;
 
  	/*
@@ -935,25 +934,6 @@ static int vmd_enable_domain(struct vmd_dev
 *vmd, unsigned long features) pci_scan_child_bus(vmd->bus);
  	vmd_domain_reset(vmd);
 
-	/* When Intel VMD is enabled, the OS does not discover the
Root Ports
drivers/pci/controller/vmd.c +0 -1
-	 * owned by Intel VMD within the MMCFG space.
pci_reset_bus() applies
drivers/pci/controller/vmd.c +0 -1
-	 * a reset to the parent of the PCI device supplied as
argument. This
drivers/pci/controller/vmd.c +0 -1
-	 * is why we pass a child device, so the reset can be
triggered at
drivers/pci/controller/vmd.c +0 -1
-	 * the Intel bridge level and propagated to all the children
in the
drivers/pci/controller/vmd.c +0 -6
-	 * hierarchy.
-	 */
-	list_for_each_entry(child, &vmd->bus->children, node) {
-		if (!list_empty(&child->devices)) {
-			dev = list_first_entry(&child->devices,
-					       struct pci_dev,
bus_list);
drivers/pci/controller/vmd.c +0 -3
-			ret = pci_reset_bus(dev);
-			if (ret)
-				pci_warn(dev, "can't reset device:
%d\n", ret); -
drivers/pci/controller/vmd.c static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features) · line 939
Quoted 7 lines
drivers/pci/controller/vmd.c +0 -4
-			break;
-		}
-	}
-
  	pci_assign_unassigned_bus_resources(vmd->bus);
 
  	pci_walk_bus(vmd->bus, vmd_pm_enable_quirk, &features);
Thanks for the patch.

Quoted 7 lines

pci_reset_bus is required to avoid failure in vmd domain creation during multiple soft reboots test. So I believe we can't just remove it without proper testing. vmd_pm_enable_quirk happens after pci_reset_bus, then how is it resetting LTR1.2_Threshold value?
Thanks
-nirmal

To follow up on what Nirmal said: why can't you set the threshold value in vmd_pm_enable_quirk() since we look at LTR there?

Paul

Jian-Hong Pan

author

2024-06-25 10:32 UTC #6 lore raw

Re: [PATCH v6 3/3] PCI: vmd: Drop resetting PCI bus action after scan mapped PCI child bus

Paul M Stillwell Jr <[email protected]> 於 2024年6月24日週一下午11:39寫道：

Quoted 99 lines · Nirmal Patel

Quoted 97 lines
Quoted 79 lines · On Mon, 24 Jun 2024 16:21:45 +0800 Jian-Hong Pan <[email protected]> wrote:
According to "PCIe r6.0, sec 5.5.4", before enabling ASPM L1.2 on the PCIe Root Port and the child device, they should be programmed with the same LTR1.2_Threshold value. However, they have different values on VMD mapped PCI child bus. For example, Asus B1400CEAE's VMD mapped PCI bridge and NVMe SSD controller have different LTR1.2_Threshold values:
10000:e0:06.0 PCI bridge: Intel Corporation 11th Gen Core Processor PCIe Controller (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=45us LTR1.2_Threshold=101376ns
         L1SubCtl2: T_PwrOn=50us
10000:e1:00.0 Non-Volatile memory controller: Sandisk Corp WD Blue SN550 NVMe SSD (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=0ns
         L1SubCtl2: T_PwrOn=10us
After debug in detail, both of the VMD mapped PCI bridge and the NVMe SSD controller have been configured properly with the same LTR1.2_Threshold value. But, become misconfigured after reset the VMD mapped PCI bus which is introduced from commit 0a584655ef89 ("PCI:
vmd: Fix secondary bus reset for Intel bridges") and commit 6aab5622296b ("PCI: vmd: Clean up domain before enumeration"). So, drop the resetting PCI bus action after scan VMD mapped PCI child bus.
Signed-off-by Jian-Hong Pan <[email protected]>
---
v6:
- Introduced based on the discussion
https://lore.kernel.org/linux-pci/CAPpJ_efYWWxGBopbSQHB=Y2+1RrXFR2XWeqEhGTgdiw3XX0Jmw@mail.gmail.com/
  drivers/pci/controller/vmd.c | 20 --------------------
  1 file changed, 20 deletions(-)
(quoted diff) +0 -0
b/drivers/pci/controller/vmd.c index 5309afbe31f9..af413cdb4f4e 100644
drivers/pci/controller/vmd.c +0 -2
@@ -793,7 +793,6 @@ static int vmd_enable_domain(struct vmd_dev *vmd,
 unsigned long features) resource_size_t offset[2] = {0};
     resource_size_t membar2_offset = 0x2000;
     struct pci_bus *child;
-    struct pci_dev *dev;
     int ret;
 
     /*
@@ -935,25 +934,6 @@ static int vmd_enable_domain(struct vmd_dev
 *vmd, unsigned long features) pci_scan_child_bus(vmd->bus);
     vmd_domain_reset(vmd);
 
-    /* When Intel VMD is enabled, the OS does not discover the
Root Ports
- * owned by Intel VMD within the MMCFG space.
pci_reset_bus() applies
- * a reset to the parent of the PCI device supplied as
argument. This
- * is why we pass a child device, so the reset can be
triggered at
- * the Intel bridge level and propagated to all the children
in the
drivers/pci/controller/vmd.c +0 -6
-     * hierarchy.
-     */
-    list_for_each_entry(child, &vmd->bus->children, node) {
-            if (!list_empty(&child->devices)) {
-                    dev = list_first_entry(&child->devices,
-                                           struct pci_dev,
bus_list);
drivers/pci/controller/vmd.c +0 -3
-                    ret = pci_reset_bus(dev);
-                    if (ret)
-                            pci_warn(dev, "can't reset device:
%d\n", ret); -
drivers/pci/controller/vmd.c static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features) · line 939
Quoted 7 lines
drivers/pci/controller/vmd.c +0 -4
-                    break;
-            }
-    }
-
     pci_assign_unassigned_bus_resources(vmd->bus);
 
     pci_walk_bus(vmd->bus, vmd_pm_enable_quirk, &features);
Thanks for the patch.
pci_reset_bus is required to avoid failure in vmd domain creation during multiple soft reboots test. So I believe we can't just remove it without proper testing. vmd_pm_enable_quirk happens after pci_reset_bus, then how is it resetting LTR1.2_Threshold value?

Quoted 5 lines

Quoted 2 lines
Thanks
-nirmal
To follow up on what Nirmal said: why can't you set the threshold value in vmd_pm_enable_quirk() since we look at LTR there?

Looks like setting the threshold value again in vmd_pm_enable_quirk() is the preferred direction?
If so, I can prepare for that in the next version patch.

Jian-Hong Pan

Paul M Stillwell Jr

2024-06-25 20:32 UTC #7 lore raw

Re: [PATCH v6 3/3] PCI: vmd: Drop resetting PCI bus action after scan mapped PCI child bus

Quoted 103 lines · Jian-Hong Pan

Paul M Stillwell Jr <[email protected]> 於 2024年6月24日週一下午11:39寫道：
Quoted 102 lines · Nirmal Patel
Quoted 99 lines
Quoted 79 lines · On Mon, 24 Jun 2024 16:21:45 +0800 Jian-Hong Pan <[email protected]> wrote:
According to "PCIe r6.0, sec 5.5.4", before enabling ASPM L1.2 on the PCIe Root Port and the child device, they should be programmed with the same LTR1.2_Threshold value. However, they have different values on VMD mapped PCI child bus. For example, Asus B1400CEAE's VMD mapped PCI bridge and NVMe SSD controller have different LTR1.2_Threshold values:
10000:e0:06.0 PCI bridge: Intel Corporation 11th Gen Core Processor PCIe Controller (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=45us LTR1.2_Threshold=101376ns
          L1SubCtl2: T_PwrOn=50us
10000:e1:00.0 Non-Volatile memory controller: Sandisk Corp WD Blue SN550 NVMe SSD (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=0ns
          L1SubCtl2: T_PwrOn=10us
After debug in detail, both of the VMD mapped PCI bridge and the NVMe SSD controller have been configured properly with the same LTR1.2_Threshold value. But, become misconfigured after reset the VMD mapped PCI bus which is introduced from commit 0a584655ef89 ("PCI:
vmd: Fix secondary bus reset for Intel bridges") and commit 6aab5622296b ("PCI: vmd: Clean up domain before enumeration"). So, drop the resetting PCI bus action after scan VMD mapped PCI child bus.
Signed-off-by Jian-Hong Pan <[email protected]>
---
v6:
- Introduced based on the discussion
https://lore.kernel.org/linux-pci/CAPpJ_efYWWxGBopbSQHB=Y2+1RrXFR2XWeqEhGTgdiw3XX0Jmw@mail.gmail.com/
   drivers/pci/controller/vmd.c | 20 --------------------
   1 file changed, 20 deletions(-)
(quoted diff) +0 -0
b/drivers/pci/controller/vmd.c index 5309afbe31f9..af413cdb4f4e 100644
drivers/pci/controller/vmd.c +0 -2
@@ -793,7 +793,6 @@ static int vmd_enable_domain(struct vmd_dev *vmd,
 unsigned long features) resource_size_t offset[2] = {0};
      resource_size_t membar2_offset = 0x2000;
      struct pci_bus *child;
-    struct pci_dev *dev;
      int ret;
 
      /*
@@ -935,25 +934,6 @@ static int vmd_enable_domain(struct vmd_dev
 *vmd, unsigned long features) pci_scan_child_bus(vmd->bus);
      vmd_domain_reset(vmd);
 
-    /* When Intel VMD is enabled, the OS does not discover the
Root Ports
- * owned by Intel VMD within the MMCFG space.
pci_reset_bus() applies
- * a reset to the parent of the PCI device supplied as
argument. This
- * is why we pass a child device, so the reset can be
triggered at
- * the Intel bridge level and propagated to all the children
in the
drivers/pci/controller/vmd.c +0 -6
-     * hierarchy.
-     */
-    list_for_each_entry(child, &vmd->bus->children, node) {
-            if (!list_empty(&child->devices)) {
-                    dev = list_first_entry(&child->devices,
-                                           struct pci_dev,
bus_list);
drivers/pci/controller/vmd.c +0 -3
-                    ret = pci_reset_bus(dev);
-                    if (ret)
-                            pci_warn(dev, "can't reset device:
%d\n", ret); -
drivers/pci/controller/vmd.c static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features) · line 939
Quoted 7 lines
drivers/pci/controller/vmd.c +0 -4
-                    break;
-            }
-    }
-
      pci_assign_unassigned_bus_resources(vmd->bus);
 
      pci_walk_bus(vmd->bus, vmd_pm_enable_quirk, &features);
Thanks for the patch.
pci_reset_bus is required to avoid failure in vmd domain creation during multiple soft reboots test. So I believe we can't just remove it without proper testing. vmd_pm_enable_quirk happens after pci_reset_bus, then how is it resetting LTR1.2_Threshold value?
Thanks
-nirmal

Quoted 9 lines

Quoted 2 lines
To follow up on what Nirmal said: why can't you set the threshold value in vmd_pm_enable_quirk() since we look at LTR there?
Looks like setting the threshold value again in vmd_pm_enable_quirk() is the preferred direction?
If so, I can prepare for that in the next version patch.
Jian-Hong Pan

Yes, I think keeping all the LTR code together is the best thing.

Paul

Jian-Hong Pan

author

2024-06-25 09:53 UTC #8 lore raw

Re: [PATCH v6 3/3] PCI: vmd: Drop resetting PCI bus action after scan mapped PCI child bus

In reply to Nirmal Patel

Nirmal Patel <[email protected]> 於 2024年6月24日週一下午11:25寫道：

Quoted 89 lines

Quoted 85 lines · On Mon, 24 Jun 2024 16:21:45 +0800 Jian-Hong Pan <[email protected]> wrote:
According to "PCIe r6.0, sec 5.5.4", before enabling ASPM L1.2 on the PCIe Root Port and the child device, they should be programmed with the same LTR1.2_Threshold value. However, they have different values on VMD mapped PCI child bus. For example, Asus B1400CEAE's VMD mapped PCI bridge and NVMe SSD controller have different LTR1.2_Threshold values:
10000:e0:06.0 PCI bridge: Intel Corporation 11th Gen Core Processor PCIe Controller (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=45us LTR1.2_Threshold=101376ns
        L1SubCtl2: T_PwrOn=50us
10000:e1:00.0 Non-Volatile memory controller: Sandisk Corp WD Blue SN550 NVMe SSD (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=0ns
        L1SubCtl2: T_PwrOn=10us
After debug in detail, both of the VMD mapped PCI bridge and the NVMe SSD controller have been configured properly with the same LTR1.2_Threshold value. But, become misconfigured after reset the VMD mapped PCI bus which is introduced from commit 0a584655ef89 ("PCI:
vmd: Fix secondary bus reset for Intel bridges") and commit 6aab5622296b ("PCI: vmd: Clean up domain before enumeration"). So, drop the resetting PCI bus action after scan VMD mapped PCI child bus.
Signed-off-by Jian-Hong Pan <[email protected]>
---
v6:
- Introduced based on the discussion
https://lore.kernel.org/linux-pci/CAPpJ_efYWWxGBopbSQHB=Y2+1RrXFR2XWeqEhGTgdiw3XX0Jmw@mail.gmail.com/
 drivers/pci/controller/vmd.c | 20 --------------------
 1 file changed, 20 deletions(-)
(quoted diff) +0 -0
b/drivers/pci/controller/vmd.c index 5309afbe31f9..af413cdb4f4e 100644
drivers/pci/controller/vmd.c +0 -2
@@ -793,7 +793,6 @@ static int vmd_enable_domain(struct vmd_dev *vmd,
 unsigned long features) resource_size_t offset[2] = {0};
      resource_size_t membar2_offset = 0x2000;
      struct pci_bus *child;
-     struct pci_dev *dev;
      int ret;
 
      /*
@@ -935,25 +934,6 @@ static int vmd_enable_domain(struct vmd_dev
 *vmd, unsigned long features) pci_scan_child_bus(vmd->bus);
      vmd_domain_reset(vmd);
 
-     /* When Intel VMD is enabled, the OS does not discover the
Root Ports
- * owned by Intel VMD within the MMCFG space.
pci_reset_bus() applies
- * a reset to the parent of the PCI device supplied as
argument. This
- * is why we pass a child device, so the reset can be
triggered at
- * the Intel bridge level and propagated to all the children
in the
drivers/pci/controller/vmd.c +0 -6
-      * hierarchy.
-      */
-     list_for_each_entry(child, &vmd->bus->children, node) {
-             if (!list_empty(&child->devices)) {
-                     dev = list_first_entry(&child->devices,
-                                            struct pci_dev,
bus_list);
drivers/pci/controller/vmd.c +0 -3
-                     ret = pci_reset_bus(dev);
-                     if (ret)
-                             pci_warn(dev, "can't reset device:
%d\n", ret); -
drivers/pci/controller/vmd.c +0 -4
-                     break;
-             }
-     }
-
      pci_assign_unassigned_bus_resources(vmd->bus);

Quoted 8 lines

Quoted 1 line
      pci_walk_bus(vmd->bus, vmd_pm_enable_quirk, &features);
Thanks for the patch.
pci_reset_bus is required to avoid failure in vmd domain creation during multiple soft reboots test. So I believe we can't just remove it without proper testing. vmd_pm_enable_quirk happens after pci_reset_bus, then how is it resetting LTR1.2_Threshold value?

uh! I mean that drop pci_reset_bus(dev) after pci_scan_child_bus(vmd->bus) Not pci_walk_bus() with vmd_pm_enable_quirk.

Jian-Hong Pan

Nirmal Patel

maintainer

2024-06-25 19:42 UTC #9 lore raw

Re: [PATCH v6 3/3] PCI: vmd: Drop resetting PCI bus action after scan mapped PCI child bus

Quoted 96 lines · On Tue, 25 Jun 2024 17:52:55 +0800 Jian-Hong Pan <[email protected]> wrote:

Nirmal Patel <[email protected]> 於 2024年6月24日週一下午11:25寫道：
Quoted 94 lines
Quoted 80 lines · On Mon, 24 Jun 2024 16:21:45 +0800 Jian-Hong Pan <[email protected]> wrote:
According to "PCIe r6.0, sec 5.5.4", before enabling ASPM L1.2 on the PCIe Root Port and the child device, they should be programmed with the same LTR1.2_Threshold value. However, they have different values on VMD mapped PCI child bus. For example, Asus B1400CEAE's VMD mapped PCI bridge and NVMe SSD controller have different LTR1.2_Threshold values:
10000:e0:06.0 PCI bridge: Intel Corporation 11th Gen Core Processor PCIe Controller (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=45us LTR1.2_Threshold=101376ns
        L1SubCtl2: T_PwrOn=50us
10000:e1:00.0 Non-Volatile memory controller: Sandisk Corp WD Blue SN550 NVMe SSD (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=0ns
        L1SubCtl2: T_PwrOn=10us
After debug in detail, both of the VMD mapped PCI bridge and the NVMe SSD controller have been configured properly with the same LTR1.2_Threshold value. But, become misconfigured after reset the VMD mapped PCI bus which is introduced from commit 0a584655ef89 ("PCI: vmd: Fix secondary bus reset for Intel bridges") and commit 6aab5622296b ("PCI: vmd: Clean up domain before enumeration"). So, drop the resetting PCI bus action after scan VMD mapped PCI child bus.
Signed-off-by Jian-Hong Pan <[email protected]>
---
v6:
- Introduced based on the discussion
https://lore.kernel.org/linux-pci/CAPpJ_efYWWxGBopbSQHB=Y2+1RrXFR2XWeqEhGTgdiw3XX0Jmw@mail.gmail.com/
 drivers/pci/controller/vmd.c | 20 --------------------
 1 file changed, 20 deletions(-)
(quoted diff) +0 -0
b/drivers/pci/controller/vmd.c index 5309afbe31f9..af413cdb4f4e 100644 --- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
drivers/pci/controller/vmd.c +0 -2
@@ -793,7 +793,6 @@ static int vmd_enable_domain(struct vmd_dev
 *vmd, unsigned long features) resource_size_t offset[2] = {0};
      resource_size_t membar2_offset = 0x2000;
      struct pci_bus *child;
-     struct pci_dev *dev;
      int ret;
 
      /*
@@ -935,25 +934,6 @@ static int vmd_enable_domain(struct vmd_dev
 *vmd, unsigned long features) pci_scan_child_bus(vmd->bus);
      vmd_domain_reset(vmd);
 
-     /* When Intel VMD is enabled, the OS does not discover the
Root Ports
- * owned by Intel VMD within the MMCFG space.
pci_reset_bus() applies
- * a reset to the parent of the PCI device supplied as
argument. This
- * is why we pass a child device, so the reset can be
triggered at
- * the Intel bridge level and propagated to all the children
in the
drivers/pci/controller/vmd.c +0 -6
-      * hierarchy.
-      */
-     list_for_each_entry(child, &vmd->bus->children, node) {
-             if (!list_empty(&child->devices)) {
-                     dev = list_first_entry(&child->devices,
-                                            struct pci_dev,
bus_list);
drivers/pci/controller/vmd.c +0 -3
-                     ret = pci_reset_bus(dev);
-                     if (ret)
-                             pci_warn(dev, "can't reset device:
%d\n", ret); -
drivers/pci/controller/vmd.c static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features) · line 939
Quoted 7 lines
drivers/pci/controller/vmd.c +0 -4
-                     break;
-             }
-     }
-
      pci_assign_unassigned_bus_resources(vmd->bus);
 
      pci_walk_bus(vmd->bus, vmd_pm_enable_quirk, &features);  
Thanks for the patch.

Quoted 8 lines

Quoted 4 lines
pci_reset_bus is required to avoid failure in vmd domain creation during multiple soft reboots test. So I believe we can't just remove it without proper testing. vmd_pm_enable_quirk happens after pci_reset_bus, then how is it resetting LTR1.2_Threshold value?
uh! I mean that drop pci_reset_bus(dev) after pci_scan_child_bus(vmd->bus) Not pci_walk_bus() with vmd_pm_enable_quirk.

That is what I am saying; we cannot remove/drop pci_reset_bus it is very useful for the case I mentioned in my previous comment and in case of direct assign or Guest OS PCIe device enumeration.

-nirmal

Quoted 5 lines

Jian-Hong Pan
Jian-Hong Pan