| Message ID | 20240624082144.10265-2-jhp@endlessos.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI: vmd: Enable PCI PM's L1 substates of remapped PCIe Root Port and NVMe | expand |
On Mon, 24 Jun 2024 16:21:45 +0800 Jian-Hong Pan <jhp@endlessos.org> 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 <jhp@endlessos.org> > --- > 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(-) > > diff --git a/drivers/pci/controller/vmd.c > b/drivers/pci/controller/vmd.c index 5309afbe31f9..af413cdb4f4e 100644 > --- a/drivers/pci/controller/vmd.c > +++ b/drivers/pci/controller/vmd.c > @@ -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); 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
On 6/24/2024 8:24 AM, Nirmal Patel wrote: > On Mon, 24 Jun 2024 16:21:45 +0800 > Jian-Hong Pan <jhp@endlessos.org> 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 <jhp@endlessos.org> >> --- >> 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(-) >> >> diff --git a/drivers/pci/controller/vmd.c >> b/drivers/pci/controller/vmd.c index 5309afbe31f9..af413cdb4f4e 100644 >> --- a/drivers/pci/controller/vmd.c >> +++ b/drivers/pci/controller/vmd.c >> @@ -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); > > 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 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
Nirmal Patel <nirmal.patel@linux.intel.com> 於 2024年6月24日 週一 下午11:25寫道： > > On Mon, 24 Jun 2024 16:21:45 +0800 > Jian-Hong Pan <jhp@endlessos.org> 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 <jhp@endlessos.org> > > --- > > 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(-) > > > > diff --git a/drivers/pci/controller/vmd.c > > b/drivers/pci/controller/vmd.c index 5309afbe31f9..af413cdb4f4e 100644 > > --- a/drivers/pci/controller/vmd.c > > +++ b/drivers/pci/controller/vmd.c > > @@ -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); > > 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 Jian-Hong Pan
Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com> 於 2024年6月24日 週一 下午11:39寫道： > > On 6/24/2024 8:24 AM, Nirmal Patel wrote: > > On Mon, 24 Jun 2024 16:21:45 +0800 > > Jian-Hong Pan <jhp@endlessos.org> 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 <jhp@endlessos.org> > >> --- > >> 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(-) > >> > >> diff --git a/drivers/pci/controller/vmd.c > >> b/drivers/pci/controller/vmd.c index 5309afbe31f9..af413cdb4f4e 100644 > >> --- a/drivers/pci/controller/vmd.c > >> +++ b/drivers/pci/controller/vmd.c > >> @@ -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); > > > > 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 > > 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
On Tue, 25 Jun 2024 17:52:55 +0800 Jian-Hong Pan <jhp@endlessos.org> wrote: > Nirmal Patel <nirmal.patel@linux.intel.com> 於 2024年6月24日 週一 > 下午11:25寫道： > > > > On Mon, 24 Jun 2024 16:21:45 +0800 > > Jian-Hong Pan <jhp@endlessos.org> 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 <jhp@endlessos.org> > > > --- > > > 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(-) > > > > > > diff --git a/drivers/pci/controller/vmd.c > > > b/drivers/pci/controller/vmd.c index 5309afbe31f9..af413cdb4f4e > > > 100644 --- a/drivers/pci/controller/vmd.c > > > +++ b/drivers/pci/controller/vmd.c > > > @@ -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); > > > > 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. 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 > > Jian-Hong Pan > > > Jian-Hong Pan
On 6/25/2024 3:31 AM, Jian-Hong Pan wrote: > Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com> 於 2024年6月24日 週一 下午11:39寫道： >> >> On 6/24/2024 8:24 AM, Nirmal Patel wrote: >>> On Mon, 24 Jun 2024 16:21:45 +0800 >>> Jian-Hong Pan <jhp@endlessos.org> 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 <jhp@endlessos.org> >>>> --- >>>> 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(-) >>>> >>>> diff --git a/drivers/pci/controller/vmd.c >>>> b/drivers/pci/controller/vmd.c index 5309afbe31f9..af413cdb4f4e 100644 >>>> --- a/drivers/pci/controller/vmd.c >>>> +++ b/drivers/pci/controller/vmd.c >>>> @@ -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); >>> >>> 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 >> >> 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
diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c index 5309afbe31f9..af413cdb4f4e 100644 --- a/drivers/pci/controller/vmd.c +++ b/drivers/pci/controller/vmd.c @@ -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);
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 <jhp@endlessos.org> --- 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(-)