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

[Nirmal Patel <nirmal.patel@linux.intel.com>]

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