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);
--
2.45.2On 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
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
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/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
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 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
© 2016 - 2026 Red Hat, Inc.