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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <aLlmV8Qiaph1PHFY@ryzen>
Date: Thu, 4 Sep 2025 12:13:43 +0200
From: Niklas Cassel <cassel@...nel.org>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: Hans Zhang <18255117159@....com>, lpieralisi@...nel.org,
	kwilczynski@...nel.org, bhelgaas@...gle.com, heiko@...ech.de,
	mani@...nel.org, yue.wang@...ogic.com, pali@...nel.org,
	neil.armstrong@...aro.org, robh@...nel.org, jingoohan1@...il.com,
	khilman@...libre.com, jbrunet@...libre.com,
	martin.blumenstingl@...glemail.com, linux-pci@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-amlogic@...ts.infradead.org,
	linux-rockchip@...ts.infradead.org
Subject: Re: [PATCH v5 1/2] PCI: Configure root port MPS during host probing

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