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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 6/25/2024 3:31 AM, Jian-Hong Pan wrote:
Paul M Stillwell Jr <paul.m.stillwell.jr@xxxxxxxxx> 於 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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
---
v6:
- Introduced based on the discussion
https://lore.kernel.org/linux-pci/CAPpJ_efYWWxGBopbSQHB=Y2+1RrXFR2XWeqEhGTgdiw3XX0Jmw@xxxxxxxxxxxxxx/

   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




[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux