linux-kernel - Re: [PATCH v4 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] [thread-next>] [day] [month] [year] [list]
Message-ID: <aEwRAZgLJUECbGz6@ryzen>
Date: Fri, 13 Jun 2025 13:52:33 +0200
From: Niklas Cassel <cassel@...nel.org>
To: Manivannan Sadhasivam <mani@...nel.org>
Cc: Hans Zhang <18255117159@....com>, lpieralisi@...nel.org, kw@...ux.com,
	bhelgaas@...gle.com, heiko@...ech.de,
	manivannan.sadhasivam@...aro.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 v4 1/2] PCI: Configure root port MPS during host probing

On Fri, Jun 13, 2025 at 12:08:31PM +0530, Manivannan Sadhasivam wrote:
> On Sat, May 10, 2025 at 11:56: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 might not utilize the full
> > capabilities supported by the controller hardware. This can result is
> > uboptimal data transfer efficiency across the PCIe hierarchy.
> > 
> > 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. By iterating through bridge devices
> > under the root bus and identifying PCIe root ports, each port's MPS is
> > set to 128 << pcie_mpss to match the device's maximum supported payload
> > size.
> 
> I don't think the above statement is accurate. This patch is not iterating
> through the bridges and you cannot identify root ports using that. What this
> patch does is, it checks whether the device is root port or not and if it is,
> then it sets the MPS to MPSS (hw maximum) if PCIE_BUS_TUNE_OFF is not set.

Correct.
Later, when the bus is walked, if any downstream device does not support
the MPS value currently configured in the root port, pci_configure_mps()
will reduce the MPS in the root port to the max supported by the downstream
device.

So even we start off by setting MPS in the root port to the max supported
by the root port, it might get reduced later on.


> 
> > The Max Read Request Size (MRRS) is subsequently adjusted through
> > existing companion logic to maintain compatibility with PCIe
> > specifications.
> > 
> > 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.
> > 
> > Suggested-by: Niklas Cassel <cassel@...nel.org>
> > Signed-off-by: Hans Zhang <18255117159@....com>
> > ---
> >  drivers/pci/probe.c | 72 ++++++++++++++++++++++++++-------------------
> >  1 file changed, 41 insertions(+), 31 deletions(-)
> > 
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index 364fa2a514f8..1f67c03d170a 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -2149,6 +2149,37 @@ int pci_setup_device(struct pci_dev *dev)
> >  	return 0;
> >  }
> >  
> > +static void pcie_write_mps(struct pci_dev *dev, int mps)
> > +{
> > +	int rc;
> > +
> > +	if (pcie_bus_config == PCIE_BUS_PERFORMANCE) {
> > +		mps = 128 << dev->pcie_mpss;
> > +
> > +		if (pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT &&
> > +		    dev->bus->self)
> > +
> > +			/*
> > +			 * For "Performance", the assumption is made that
> > +			 * downstream communication will never be larger than
> > +			 * the MRRS.  So, the MPS only needs to be configured
> > +			 * for the upstream communication.  This being the case,
> > +			 * walk from the top down and set the MPS of the child
> > +			 * to that of the parent bus.
> > +			 *
> > +			 * Configure the device MPS with the smaller of the
> > +			 * device MPSS or the bridge MPS (which is assumed to be
> > +			 * properly configured at this point to the largest
> > +			 * allowable MPS based on its parent bus).
> > +			 */
> > +			mps = min(mps, pcie_get_mps(dev->bus->self));
> > +	}
> > +
> > +	rc = pcie_set_mps(dev, mps);
> > +	if (rc)
> > +		pci_err(dev, "Failed attempting to set the MPS\n");
> > +}
> > +
> >  static void pci_configure_mps(struct pci_dev *dev)
> >  {
> >  	struct pci_dev *bridge = pci_upstream_bridge(dev);
> > @@ -2178,6 +2209,16 @@ static void pci_configure_mps(struct pci_dev *dev)
> >  		return;
> >  	}
> >  
> > +	/*
> > +	 * Unless MPS strategy is PCIE_BUS_TUNE_OFF (don't touch MPS at all),
> > +	 * start off by setting root ports' MPS to MPSS. Depending on the MPS
> > +	 * strategy, and the MPSS of the devices below the root port, the MPS
> > +	 * of the root port might get overridden later.
> > +	 */
> > +	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);
> 
> I believe you can just use "pcie_set_mps(dev, 128 << dev->pcie_mpss)" directly
> and avoid moving pcie_write_mps() around.

+1


Kind regards,
Niklas