Re: [PATCH v5 1/2] PCI: Configure root port MPS during host probing

[Niklas Cassel <cassel@kernel.org>]

On Tue, Sep 02, 2025 at 12:48:28PM -0500, Bjorn Helgaas wrote:
> On Fri, Jun 20, 2025 at 11:55:06PM +0800, Hans Zhang wrote:
> > Current PCIe initialization logic may leave root ports operating with
> > non-optimal Maximum Payload Size (MPS) settings. While downstream device
> > configuration is handled during bus enumeration, root port MPS values
> > inherited from firmware or hardware defaults ...
> 
> Apparently Root Port MPS configuration is different from that for
> downstream devices?

pci_host_probe() will call pci_scan_root_bus_bridge(), which will call
pci_scan_single_device(), which will call pci_device_add(), which will
call pci_configure_device(), which will call pci_configure_mps().

This will be done for both bridges and endpoints.

The bridge will be scanned/added first, before devices behind the bridge.


While pci_configure_device()/pci_configure_mps() will be called for both
bridges and endpoints, pci_configure_mps() will do an early return for
devices where pci_upstream_bridge() returns NULL, i.e. for devices where
that does not have an upstream bridge, i.e. for the root bridge itself:
https://github.com/torvalds/linux/blob/v6.17-rc4/drivers/pci/probe.c#L2181-L2182

So MPS will not be touched for root bridges.

This patch ensures that MPS for root bridges gets initialized to MPSS
(Max supported MPS).

Later, when pci_configure_device()/pci_configure_mps() is called for a
device behind the bridge, if the MPSS of the device behind the bridge is
smaller than the MPS of the bridge, the code reduces the MPS of the bridge:
https://github.com/torvalds/linux/blob/v6.17-rc4/drivers/pci/probe.c#L2205


My only question with this patch is if there is a bridge behind a bridge,
will the bridge behind the bridge still have pci_pcie_type() ==
PCI_EXP_TYPE_ROOT_PORT ?

If so, perhaps we should modify this patch from:

+       if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT &&
+           pcie_bus_config != PCIE_BUS_TUNE_OFF) {
+               pcie_write_mps(dev, 128 << dev->pcie_mpss);
+       }
+
        if (!bridge || !pci_is_pcie(bridge))
                return;


to:

+       if (!bridge && pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT &&
+           pcie_bus_config != PCIE_BUS_TUNE_OFF) {
+               pcie_write_mps(dev, 128 << dev->pcie_mpss);
+       }
+
        if (!bridge || !pci_is_pcie(bridge))
                return;



> > During host controller probing phase, when PCIe bus tuning is enabled,
> > the implementation now configures root port MPS settings to their
> > hardware-supported maximum values. Specifically, when configuring the MPS
> > for a PCIe device, if the device is a root port and the bus tuning is not
> > disabled (PCIE_BUS_TUNE_OFF), the MPS is set to 128 << dev->pcie_mpss to
> > match the Root Port's maximum supported payload size. The Max Read Request
> > Size (MRRS) is subsequently adjusted through existing companion logic to
> > maintain compatibility with PCIe specifications.
> > 
> > Note that this initial setting of the root port MPS to the maximum might
> > be reduced later during the enumeration of downstream devices if any of
> > those devices do not support the maximum MPS of the root port.
> > 
> > Explicit initialization at host probing stage ensures consistent PCIe
> > topology configuration before downstream devices perform their own MPS
> > negotiations. This proactive approach addresses platform-specific
> > requirements where controller drivers depend on properly initialized
> > root port settings, while maintaining backward compatibility through
> > PCIE_BUS_TUNE_OFF conditional checks. Hardware capabilities are fully
> > utilized without altering existing device negotiation behaviors.
> 
> This last paragraph seems kind of like marketing without any real
> content.  Is there something important in there?
> 
> Nits:
> s/root port/Root Port/
> 
> Reword "implementation now configures" to be clear about whether "now"
> refers to before this patch or after.
> 
> Update the MRRS "to maintain compatibility" part.  I'm dubious about
> there being a spec compatibility issue with respect to MRRS.  Cite the
> relevant section if there is an issue.

I'm not sure why the commit message mentions MRRS at all.

Sure, pcie_write_mrrs() might set MRRS to MPS, but that is existing logic
and not really related to the change in this patch IMO.


Kind regards,
Niklas