[PATCH v9 0/3] PCI: Configure Root Port MPS during host probing

[PATCH v9 0/3] PCI: Configure Root Port MPS during host probing

[Hans Zhang]

Current PCIe enumeration configures each device's Maximum Payload Size
(MPS) top-down to match its upstream bridge.  Root Ports, however, return
early from pci_configure_mps() because they have no upstream bridge, so
their MPS may remain at firmware defaults and fail to use the controller's
full capability.

There is also a hierarchy problem when a device's MPS Supported (MPSS) is
smaller than the MPS already programmed above it.  The existing reduction
only lowers a Root Port.  If a Switch is in between, the Switch ports keep
the larger MPS and the endpoint is left mismatched, which can cause
Malformed TLPs.

This series addresses both issues:

1. Patch 1 fixes pci_configure_mps() so that when a device's MPSS is
   smaller than the MPS already in use above it, the whole Root Port
   hierarchy is reduced as necessary, including Switch ports and
   already-programmed sibling devices.

2. Patch 2 safely initializes Root Port MPS to its maximum supported value
   during host probing.  Patch 1 ensures this larger initial value is
   reduced again when downstream devices require a smaller MPS.
   PCIE_BUS_TUNE_OFF and PCIE_BUS_PEER2PEER are left untouched.

3. Patch 3 removes the now-redundant Meson PCIe driver MPS programming and
   cleans up the resulting dead macros and misleading MRRS helper name.

---
Changes in v9:
- Add patch 1 to reduce the whole Root Port hierarchy when a downstream device
  has a smaller MPSS, addressing the Switch case reported by sashiko.
- Patch 2 keeps the Root Port max-MPS initialization, adds error handling, and
  relies on patch 1 for later reduction.
- Patch 3 drops PCIE_CAP_MAX_PAYLOAD_SIZE and MAX_PAYLOAD_SIZE, renames
  meson_size_to_payload() to meson_size_to_mrrs(), and updates the warning.

Changes in v8:
https://patchwork.kernel.org/project/linux-pci/patch/20260911132145.211380-1-18255117159@163.com/

- Rebase to v7.3-rc1.

Changes in v7:
https://patchwork.kernel.org/project/linux-pci/cover/20251127170908.14850-1-18255117159@163.com/

- Exclude PCIE_BUS_PEER2PEER mode from Root Port MPS configuration
- Remove redundant check for upstream bridge (Root Ports don't have one)
- Improve commit message and code comments as per Bjorn.

Changes for v6:
https://patchwork.kernel.org/project/linux-pci/patch/20251104165125.174168-1-18255117159@163.com/

- Modify the commit message and comments. (Bjorn)
- Patch 1/2 code logic: Add !bridge check to configure MPS only for Root Ports
  without an upstream bridge (root bridges), avoiding incorrect handling of
  non-root-bridge Root Ports (Niklas).

Changes for v5:
https://patchwork.kernel.org/project/linux-pci/patch/20250620155507.1022099-1-18255117159@163.com/

- Use pcie_set_mps directly instead of pcie_write_mps.
- The patch 1 commit message were modified.

Changes for v4:
https://patchwork.kernel.org/project/linux-pci/patch/20250510155607.390687-1-18255117159@163.com/

- The patch [v4 1/2] add a comment to explain why it was done this way.
- The patch [v4 2/2] have not been modified.
- Drop patch [v3 3/3]. The Maintainer of the pci-aardvark.c file suggests
  that this patch cannot be submitted. In addition, Mani also suggests
  dropping this patch until this series of issues is resolved.

Changes for v3:
https://patchwork.kernel.org/project/linux-pci/patch/20250506173439.292460-1-18255117159@163.com/

- The new split is patch 2/3 and 3/3.
- Modify the patch 1/3 according to Niklas' suggestion.

Changes for v2:
https://patchwork.kernel.org/project/linux-pci/patch/20250425095708.32662-1-18255117159@163.com/

- According to the Maintainer's suggestion, limit the setting of MPS
  changes to platforms with controller drivers.
- Delete the MPS code set by the SOC manufacturer.
---

Hans Zhang (3):
  PCI: Match the hierarchy's MPS to a device's MPSS as necessary
  PCI: Configure Root Port MPS during host probing
  PCI: dwc: Remove redundant MPS configuration

 drivers/pci/controller/dwc/pci-meson.c | 25 ++----------
 drivers/pci/probe.c                    | 53 ++++++++++++++++++++++++--
 2 files changed, 52 insertions(+), 26 deletions(-)


base-commit: 08df884136f1c1197bab2a27814404fd329d9aac
-- 
2.34.1


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH v9 1/3] PCI: Match the hierarchy's MPS to a device's MPSS as necessary

[Hans Zhang]

pci_configure_mps() enumerates top-down and programs each device's Maximum
Payload Size (MPS) to match its upstream bridge.  When a device's MPS
Supported (MPSS) is too small to match, commit 9f0e89359775 ("PCI: Match
Root Port's MPS to endpoint's MPSS as necessary") reduces the upstream
bridge instead, but only when that bridge is a Root Port.

That covers an endpoint directly below a Root Port and nothing else.  With
a Switch in between, the Switch ports have already inherited the Root
Port's larger MPS, the reduction is skipped because the upstream bridge is
a Switch Downstream Port, and pcie_set_mps() then fails with -EINVAL for
the endpoint.  The endpoint is left at 128 bytes below a port programmed
for more, so any larger TLP it receives is treated as Malformed.

Multi-function devices hit the same hole from the other direction:
reducing the Root Port for a function with a small MPSS leaves the sibling
functions already programmed to the larger value.

Walk the hierarchy from the Root Port down and reduce every device that is
above the new value.  Reducing only the ports between the device and the
Root Port is not sufficient, because a Switch may not fragment or
repackage TLPs: an already programmed sibling left at the larger MPS could
emit a TLP too large for its egress port.

This only affects PCIE_BUS_DEFAULT.  PCIE_BUS_SAFE already converges the
hierarchy on the smallest MPSS in pcie_bus_configure_settings(), while
PCIE_BUS_TUNE_OFF and PCIE_BUS_PEER2PEER return before this point.

Fixes: 9f0e89359775 ("PCI: Match Root Port's MPS to endpoint's MPSS as necessary")
Co-developed-by: Niklas Cassel <cassel@kernel.org>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
Signed-off-by: Hans Zhang <18255117159@163.com>
---
 drivers/pci/probe.c | 38 ++++++++++++++++++++++++++++++++++----
 1 file changed, 34 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 27008e2ea5af..232bce2819f0 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2200,9 +2200,28 @@ int pci_setup_device(struct pci_dev *dev)
 	return 0;
 }
 
+static int pcie_reduce_mps(struct pci_dev *dev, void *data)
+{
+	int mps = *(int *)data;
+	int ret;
+
+	/* MPS is of type 'RsvdP' for VFs */
+	if (!pci_is_pcie(dev) || dev->is_virtfn)
+		return 0;
+
+	if (pcie_get_mps(dev) > mps) {
+		ret = pcie_set_mps(dev, mps);
+		if (ret)
+			dev_warn(&dev->dev, "failed to set MPS\n");
+	}
+
+	return 0;
+}
+
 static void pci_configure_mps(struct pci_dev *dev)
 {
 	struct pci_dev *bridge = pci_upstream_bridge(dev);
+	struct pci_dev *rp;
 	int mps, mpss, p_mps, rc;
 
 	if (!pci_is_pcie(dev))
@@ -2252,10 +2271,21 @@ static void pci_configure_mps(struct pci_dev *dev)
 		return;
 
 	mpss = 128 << dev->pcie_mpss;
-	if (mpss < p_mps && pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT) {
-		pcie_set_mps(bridge, mpss);
-		pci_info(dev, "Upstream bridge's Max Payload Size set to %d (was %d, max %d)\n",
-			 mpss, p_mps, 128 << bridge->pcie_mpss);
+	rp = pcie_find_root_port(bridge);
+	if (mpss < p_mps && rp) {
+		/*
+		 * dev cannot be programmed to the MPS already in use above
+		 * it, so reduce the hierarchy to what dev supports.  A Switch
+		 * may not repackage TLPs, so reducing only the upstream
+		 * bridge is not enough: every port up to the Root Port has to
+		 * come down as well, and so do the devices already programmed
+		 * below that Root Port, which would otherwise be left sending
+		 * TLPs too large for their egress port.
+		 */
+		pcie_reduce_mps(rp, &mpss);
+		pci_walk_bus(rp->subordinate, pcie_reduce_mps, &mpss);
+		pci_info(dev, "Max Payload Size of %s hierarchy set to %d (was %d)\n",
+			 pci_name(rp), mpss, p_mps);
 		p_mps = pcie_get_mps(bridge);
 	}
 
-- 
2.34.1


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH v9 2/3] PCI: Configure Root Port MPS during host probing

[Hans Zhang]

Current PCIe initialization logic may leave Root Ports operating with
non-optimal Maximum Payload Size (MPS) settings. The existing code in
pci_configure_mps() returns early for devices without an upstream bridge
which includes Root Ports, so their MPS values remain at firmware
defaults. This fails to utilize the controller's full capabilities,
leading to suboptimal data transfer efficiency across the PCIe hierarchy.

With this patch, during the host controller probing phase:
- When PCIe bus tuning is enabled (not PCIE_BUS_TUNE_OFF) and not
  PCIE_BUS_PEER2PEER (which requires the default 128 bytes for optimal
  peer-to-peer operation), and
- The device is a Root Port, the Root Port's MPS is set to its maximum
  supported value.

Note that this initial maximum MPS setting may be reduced later, during
downstream device enumeration, if any downstream device does not support
the Root Port's maximum MPS.

This change ensures Root Ports are initialized to their maximum MPS before
downstream devices negotiate MPS, while maintaining backward compatibility
via the PCIE_BUS_TUNE_OFF check and not interfering with the
PCIE_BUS_PEER2PEER strategy.

Suggested-by: Niklas Cassel <cassel@kernel.org>
Suggested-by: Manivannan Sadhasivam <mani@kernel.org>
Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
Signed-off-by: Hans Zhang <18255117159@163.com>
Tested-by: Mahesh Vaidya <mahesh.vaidya@altera.com>
Tested-by: Ricardo Pardini <ricardo@pardini.net> # on Odroid-HC4
Tested-by: Shawn Lin <shawn.lin@rock-chips.com>
---
 drivers/pci/probe.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 232bce2819f0..08af75aa7366 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2248,6 +2248,21 @@ static void pci_configure_mps(struct pci_dev *dev)
 		return;
 	}
 
+	/*
+	 * Unless MPS strategy is PCIE_BUS_TUNE_OFF (don't touch MPS at all) or
+	 * PCIE_BUS_PEER2PEER (use minimum MPS for peer-to-peer), set Root Ports'
+	 * MPS to their maximum supported value. Depending on the MPS strategy
+	 * and MPSS of downstream devices, a Root Port's MPS may be reduced
+	 * later during device enumeration.
+	 */
+	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT &&
+	    pcie_bus_config != PCIE_BUS_TUNE_OFF &&
+	    pcie_bus_config != PCIE_BUS_PEER2PEER) {
+		rc = pcie_set_mps(dev, 128 << dev->pcie_mpss);
+		if (rc)
+			dev_warn(&dev->dev, "failed to set root port MPS\n");
+	}
+
 	if (!bridge || !pci_is_pcie(bridge))
 		return;
 
-- 
2.34.1


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH v9 3/3] PCI: dwc: Remove redundant MPS configuration

[Hans Zhang]

The Meson PCIe controller driver manually configures maximum payload
size (MPS) through meson_set_max_payload, duplicating functionality now
centralized in the PCI core.  Deprecating redundant code simplifies the
driver and aligns it with the consolidated MPS management strategy,
improving long-term maintainability.

With meson_set_max_payload() gone, meson_size_to_payload() is only used
for MRRS.  Rename it to meson_size_to_mrrs(), update the warning message,
and drop the now-unused PCIE_CAP_MAX_PAYLOAD_SIZE and MAX_PAYLOAD_SIZE
macros.

Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
Signed-off-by: Hans Zhang <18255117159@163.com>
---
 drivers/pci/controller/dwc/pci-meson.c | 25 +++----------------------
 1 file changed, 3 insertions(+), 22 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-meson.c b/drivers/pci/controller/dwc/pci-meson.c
index 8559d132dcde..1d3687566dda 100644
--- a/drivers/pci/controller/dwc/pci-meson.c
+++ b/drivers/pci/controller/dwc/pci-meson.c
@@ -21,7 +21,6 @@
 
 #define to_meson_pcie(x) dev_get_drvdata((x)->dev)
 
-#define PCIE_CAP_MAX_PAYLOAD_SIZE(x)	((x) << 5)
 #define PCIE_CAP_MAX_READ_REQ_SIZE(x)	((x) << 12)
 
 /* PCIe specific config registers */
@@ -37,7 +36,6 @@
 #define PM_CURRENT_STATE(x)		(((x) >> 7) & 0x1)
 
 #define PORT_CLK_RATE			100000000UL
-#define MAX_PAYLOAD_SIZE		256
 #define MAX_READ_REQ_SIZE		256
 #define PCIE_RESET_DELAY		500
 #define PCIE_SHARED_RESET		1
@@ -256,7 +254,7 @@ static void meson_pcie_ltssm_enable(struct meson_pcie *mp)
 	meson_cfg_writel(mp, val, PCIE_CFG0);
 }
 
-static int meson_size_to_payload(struct meson_pcie *mp, int size)
+static int meson_size_to_mrrs(struct meson_pcie *mp, int size)
 {
 	struct device *dev = mp->pci.dev;
 
@@ -266,35 +264,19 @@ static int meson_size_to_payload(struct meson_pcie *mp, int size)
 	 * than 2^12, just set to default size 2^(1+7).
 	 */
 	if (!is_power_of_2(size) || size < 128 || size > 4096) {
-		dev_warn(dev, "payload size %d, set to default 256\n", size);
+		dev_warn(dev, "MRRS %d, set to default 256\n", size);
 		return 1;
 	}
 
 	return fls(size) - 8;
 }
 
-static void meson_set_max_payload(struct meson_pcie *mp, int size)
-{
-	struct dw_pcie *pci = &mp->pci;
-	u32 val;
-	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
-	int max_payload_size = meson_size_to_payload(mp, size);
-
-	val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_DEVCTL);
-	val &= ~PCI_EXP_DEVCTL_PAYLOAD;
-	dw_pcie_writel_dbi(pci, offset + PCI_EXP_DEVCTL, val);
-
-	val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_DEVCTL);
-	val |= PCIE_CAP_MAX_PAYLOAD_SIZE(max_payload_size);
-	dw_pcie_writel_dbi(pci, offset + PCI_EXP_DEVCTL, val);
-}
-
 static void meson_set_max_rd_req_size(struct meson_pcie *mp, int size)
 {
 	struct dw_pcie *pci = &mp->pci;
 	u32 val;
 	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
-	int max_rd_req_size = meson_size_to_payload(mp, size);
+	int max_rd_req_size = meson_size_to_mrrs(mp, size);
 
 	val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_DEVCTL);
 	val &= ~PCI_EXP_DEVCTL_READRQ;
@@ -363,7 +345,6 @@ static int meson_pcie_host_init(struct dw_pcie_rp *pp)
 
 	pp->bridge->ops = &meson_pci_ops;
 
-	meson_set_max_payload(mp, MAX_PAYLOAD_SIZE);
 	meson_set_max_rd_req_size(mp, MAX_READ_REQ_SIZE);
 
 	return 0;
-- 
2.34.1


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v9 3/3] PCI: dwc: Remove redundant MPS configuration

[sashiko-bot]

> The Meson PCIe controller driver manually configures maximum payload
> size (MPS) through meson_set_max_payload, duplicating functionality now
> centralized in the PCI core.  Deprecating redundant code simplifies the
> driver and aligns it with the consolidated MPS management strategy,
> improving long-term maintainability.
> 
> With meson_set_max_payload() gone, meson_size_to_payload() is only used
> for MRRS.  Rename it to meson_size_to_mrrs(), update the warning message,
> and drop the now-unused PCIE_CAP_MAX_PAYLOAD_SIZE and MAX_PAYLOAD_SIZE
> macros.
> 
> Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
> Signed-off-by: Hans Zhang <18255117159@163.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260916153907.60344-1-18255117159@163.com?part=3


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v9 2/3] PCI: Configure Root Port MPS during host probing

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] pci: dangerous dynamic MPS reduction under PCIE_BUS_DEFAULT
--

commit 32945143b32b8b42f1dd3e1f419414a5bf8f9acf
Author: Hans Zhang <18255117159@163.com>

PCI: Configure Root Port MPS during host probing

This commit updates the PCIe initialization logic to set a Root Port's Maximum
Payload Size (MPS) to its maximum supported value during host controller
probing. It does this as long as the MPS strategy is not PCIE_BUS_TUNE_OFF or
PCIE_BUS_PEER2PEER, meaning it applies under the default PCIE_BUS_DEFAULT
strategy.

> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 232bce2819f06..08af75aa7366b 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2248,6 +2248,21 @@ static void pci_configure_mps(struct pci_dev *dev)
>  		return;
>  	}
>  
> +	/*
> +	 * Unless MPS strategy is PCIE_BUS_TUNE_OFF (don't touch MPS at all) or
> +	 * PCIE_BUS_PEER2PEER (use minimum MPS for peer-to-peer), set Root Ports'
> +	 * MPS to their maximum supported value. Depending on the MPS strategy
> +	 * and MPSS of downstream devices, a Root Port's MPS may be reduced
> +	 * later during device enumeration.
> +	 */
> +	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT &&
> +	    pcie_bus_config != PCIE_BUS_TUNE_OFF &&
> +	    pcie_bus_config != PCIE_BUS_PEER2PEER) {
> +		rc = pcie_set_mps(dev, 128 << dev->pcie_mpss);

[Severity: High]
Does this code introduce a regression where active downstream topologies are
exposed to dangerous dynamic MPS reductions under the default PCIE_BUS_DEFAULT
strategy?

By forcing the Root Port MPS to its maximum supported value here, it overrides
safe firmware settings. If a device with a smaller MPSS is subsequently
enumerated or hotplugged, pci_configure_mps() will detect that the downstream
MPSS is smaller than the Root Port's MPS, triggering pcie_reduce_mps() on the
entire hierarchy.

Because pcie_reduce_mps() modifies the Root Port first and sequentially walks
downstream without quiescing active devices, this creates a dangerous window.

If an existing downstream device (such as an active NIC or early
firmware-driven display controller) is actively performing DMA, could it
transmit a payload exceeding the newly reduced MPS of the Root Port,
potentially leading to Malformed TLP Fatal Errors and a system crash?

> +		if (rc)
> +			dev_warn(&dev->dev, "failed to set root port MPS\n");
> +	}
> +
>  	if (!bridge || !pci_is_pcie(bridge))
>  		return;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260916153907.60344-1-18255117159@163.com?part=2

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v9 1/3] PCI: Match the hierarchy's MPS to a device's MPSS as necessary

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] pci: pci_walk_bus() modifies MPS of active devices causing Malformed TLPs
- [High] pci: dynamically modified MPS in pcie_set_mps() is lost on system resume
- [High] pci: unlocked RMW of PCI_EXP_DEVCTL in pcie_set_mps() causes data races
--

commit 5af1f52d57f95fe2335fa9124a2d504949bbc917
Author: Hans Zhang <18255117159@163.com>

PCI: Match the hierarchy's MPS to a device's MPSS as necessary

When a device's MPSS is too small to match the upstream bridge, 
pci_configure_mps() now walks the hierarchy from the Root Port down
and reduces every device that is above the new value to avoid
Malformed TLPs on Switch egress ports.

> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 27008e2ea5afc..232bce2819f06 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2200,9 +2200,28 @@ int pci_setup_device(struct pci_dev *dev)
>  	return 0;
>  }
>  
> +static int pcie_reduce_mps(struct pci_dev *dev, void *data)
> +{
> +	int mps = *(int *)data;
> +	int ret;
> +
> +	/* MPS is of type 'RsvdP' for VFs */
> +	if (!pci_is_pcie(dev) || dev->is_virtfn)
> +		return 0;
> +
> +	if (pcie_get_mps(dev) > mps) {
> +		ret = pcie_set_mps(dev, mps);
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: High]
Does bypassing the PM software save state here cause the dynamically modified
MPS to be lost on system resume?

If the sibling driver previously called pci_save_state() during its probe,
dev->state_saved becomes true. During the next suspend, the PM core skips
saving the new hardware state. On resume, the old, larger MPS would be
restored, potentially desynchronizing the hierarchy.

[Severity: High]
Also, does modifying the hardware MPS of active third-party endpoints using
an unlocked Read-Modify-Write of PCI_EXP_DEVCTL cause data races?

pcie_set_mps() performs an unlocked RMW of PCI_EXP_DEVCTL. If the sibling
driver or userspace (via VFIO) modifies the same register concurrently
(e.g., via pcie_set_readrq()), could it corrupt the configuration state of
active endpoints?

> +		if (ret)
> +			dev_warn(&dev->dev, "failed to set MPS\n");
> +	}
> +
> +	return 0;
> +}
> +
>  static void pci_configure_mps(struct pci_dev *dev)
>  {
>  	struct pci_dev *bridge = pci_upstream_bridge(dev);

[ ... ]

> @@ -2252,10 +2271,21 @@ static void pci_configure_mps(struct pci_dev *dev)
>  		return;
>  
>  	mpss = 128 << dev->pcie_mpss;
> -	if (mpss < p_mps && pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT) {
> -		pcie_set_mps(bridge, mpss);
> -		pci_info(dev, "Upstream bridge's Max Payload Size set to %d (was %d, max %d)\n",
> -			 mpss, p_mps, 128 << bridge->pcie_mpss);
> +	rp = pcie_find_root_port(bridge);
> +	if (mpss < p_mps && rp) {
> +		/*
> +		 * dev cannot be programmed to the MPS already in use above
> +		 * it, so reduce the hierarchy to what dev supports.  A Switch
> +		 * may not repackage TLPs, so reducing only the upstream
> +		 * bridge is not enough: every port up to the Root Port has to
> +		 * come down as well, and so do the devices already programmed
> +		 * below that Root Port, which would otherwise be left sending
> +		 * TLPs too large for their egress port.
> +		 */
> +		pcie_reduce_mps(rp, &mpss);
> +		pci_walk_bus(rp->subordinate, pcie_reduce_mps, &mpss);
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: Critical]
Does unconditionally walking the bus and dynamically reducing the MPS of active
siblings cause Malformed TLPs?

PCIe specifications prohibit changing MPS while a device is active. Active
devices will continue generating TLPs based on their driver's internal state
(or previously negotiated packet sizes). When these larger TLPs hit the
dynamically reduced upstream bridge, won't they be dropped as Malformed
TLPs, potentially leading to a system crash or bus reset?

> +		pci_info(dev, "Max Payload Size of %s hierarchy set to %d (was %d)\n",
> +			 pci_name(rp), mpss, p_mps);
>  		p_mps = pcie_get_mps(bridge);
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260916153907.60344-1-18255117159@163.com?part=1

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v9 1/3] PCI: Match the hierarchy's MPS to a device's MPSS as necessary

[Hans Zhang]

Hi,

Thanks for the review.

On 9/16/26 23:52, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [Critical] pci: pci_walk_bus() modifies MPS of active devices causing Malformed TLPs
> - [High] pci: dynamically modified MPS in pcie_set_mps() is lost on system resume
> - [High] pci: unlocked RMW of PCI_EXP_DEVCTL in pcie_set_mps() causes data races
> --
> 
> commit 5af1f52d57f95fe2335fa9124a2d504949bbc917
> Author: Hans Zhang <18255117159@163.com>
> 
> PCI: Match the hierarchy's MPS to a device's MPSS as necessary
> 
> When a device's MPSS is too small to match the upstream bridge,
> pci_configure_mps() now walks the hierarchy from the Root Port down
> and reduces every device that is above the new value to avoid
> Malformed TLPs on Switch egress ports.
> 
>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> index 27008e2ea5afc..232bce2819f06 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -2200,9 +2200,28 @@ int pci_setup_device(struct pci_dev *dev)
>>   	return 0;
>>   }
>>   
>> +static int pcie_reduce_mps(struct pci_dev *dev, void *data)
>> +{
>> +	int mps = *(int *)data;
>> +	int ret;
>> +
>> +	/* MPS is of type 'RsvdP' for VFs */
>> +	if (!pci_is_pcie(dev) || dev->is_virtfn)
>> +		return 0;
>> +
>> +	if (pcie_get_mps(dev) > mps) {
>> +		ret = pcie_set_mps(dev, mps);
>                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> [Severity: High]
> Does bypassing the PM software save state here cause the dynamically modified
> MPS to be lost on system resume?
> 
> If the sibling driver previously called pci_save_state() during its probe,
> dev->state_saved becomes true. During the next suspend, the PM core skips
> saving the new hardware state. On resume, the old, larger MPS would be
> restored, potentially desynchronizing the hierarchy.
> 

pci_save_state() is called from the driver's probe path, which runs 
after enumeration has completed.  pcie_reduce_mps() runs during 
enumeration, i.e. before any driver can have called pci_save_state(). 
The saved state therefore already contains the reduced MPS and it is 
restored correctly on resume.


> [Severity: High]
> Also, does modifying the hardware MPS of active third-party endpoints using
> an unlocked Read-Modify-Write of PCI_EXP_DEVCTL cause data races?
> 
> pcie_set_mps() performs an unlocked RMW of PCI_EXP_DEVCTL. If the sibling
> driver or userspace (via VFIO) modifies the same register concurrently
> (e.g., via pcie_set_readrq()), could it corrupt the configuration state of
> active endpoints?


pcie_set_mps() does not perform an unlocked RMW.  It calls
pcie_capability_clear_and_set_word(), which serializes config space
access through pci_lock.  Concurrent access from userspace (VFIO) or 
from pcie_set_readrq() cannot corrupt PCI_EXP_DEVCTL.

> 
>> +		if (ret)
>> +			dev_warn(&dev->dev, "failed to set MPS\n");
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   static void pci_configure_mps(struct pci_dev *dev)
>>   {
>>   	struct pci_dev *bridge = pci_upstream_bridge(dev);
> 
> [ ... ]
> 
>> @@ -2252,10 +2271,21 @@ static void pci_configure_mps(struct pci_dev *dev)
>>   		return;
>>   
>>   	mpss = 128 << dev->pcie_mpss;
>> -	if (mpss < p_mps && pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT) {
>> -		pcie_set_mps(bridge, mpss);
>> -		pci_info(dev, "Upstream bridge's Max Payload Size set to %d (was %d, max %d)\n",
>> -			 mpss, p_mps, 128 << bridge->pcie_mpss);
>> +	rp = pcie_find_root_port(bridge);
>> +	if (mpss < p_mps && rp) {
>> +		/*
>> +		 * dev cannot be programmed to the MPS already in use above
>> +		 * it, so reduce the hierarchy to what dev supports.  A Switch
>> +		 * may not repackage TLPs, so reducing only the upstream
>> +		 * bridge is not enough: every port up to the Root Port has to
>> +		 * come down as well, and so do the devices already programmed
>> +		 * below that Root Port, which would otherwise be left sending
>> +		 * TLPs too large for their egress port.
>> +		 */
>> +		pcie_reduce_mps(rp, &mpss);
>> +		pci_walk_bus(rp->subordinate, pcie_reduce_mps, &mpss);
>                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> [Severity: Critical]
> Does unconditionally walking the bus and dynamically reducing the MPS of active
> siblings cause Malformed TLPs?
> 
> PCIe specifications prohibit changing MPS while a device is active. Active
> devices will continue generating TLPs based on their driver's internal state
> (or previously negotiated packet sizes). When these larger TLPs hit the
> dynamically reduced upstream bridge, won't they be dropped as Malformed
> TLPs, potentially leading to a system crash or bus reset?

The concern is valid in principle, but pci_configure_mps() is only
called from pci_configure_device() during pci_setup_device(), i.e. while
the device is being enumerated and before its driver has probed.  No
device below the Root Port can be actively driving TLPs at that point,
so the "active NIC or early firmware-driven display controller" scenario
does not apply on this path.

The same applies to the hotplug case: the newly added device goes 
throughpci_setup_device() and pci_configure_mps() before it is bound to 
a driver.  Reducing the Root Port's MPS when a downstream device has a
smaller MPSS is also pre-existing behaviour from 9f0e89359775; this
series extends it to Switch ports and already-programmed siblings, which
is required to fix the Switch mismatch reported in v8.  I will document
this precondition in the commit message.




Best regards,
Hans

> 
>> +		pci_info(dev, "Max Payload Size of %s hierarchy set to %d (was %d)\n",
>> +			 pci_name(rp), mpss, p_mps);
>>   		p_mps = pcie_get_mps(bridge);
>>   	}
> 


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v9 2/3] PCI: Configure Root Port MPS during host probing

[Hans Zhang]

Hi,

Thanks for the review.



On 9/16/26 23:51, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] pci: dangerous dynamic MPS reduction under PCIE_BUS_DEFAULT
> --
> 
> commit 32945143b32b8b42f1dd3e1f419414a5bf8f9acf
> Author: Hans Zhang <18255117159@163.com>
> 
> PCI: Configure Root Port MPS during host probing
> 
> This commit updates the PCIe initialization logic to set a Root Port's Maximum
> Payload Size (MPS) to its maximum supported value during host controller
> probing. It does this as long as the MPS strategy is not PCIE_BUS_TUNE_OFF or
> PCIE_BUS_PEER2PEER, meaning it applies under the default PCIE_BUS_DEFAULT
> strategy.
> 
>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> index 232bce2819f06..08af75aa7366b 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -2248,6 +2248,21 @@ static void pci_configure_mps(struct pci_dev *dev)
>>   		return;
>>   	}
>>   
>> +	/*
>> +	 * Unless MPS strategy is PCIE_BUS_TUNE_OFF (don't touch MPS at all) or
>> +	 * PCIE_BUS_PEER2PEER (use minimum MPS for peer-to-peer), set Root Ports'
>> +	 * MPS to their maximum supported value. Depending on the MPS strategy
>> +	 * and MPSS of downstream devices, a Root Port's MPS may be reduced
>> +	 * later during device enumeration.
>> +	 */
>> +	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT &&
>> +	    pcie_bus_config != PCIE_BUS_TUNE_OFF &&
>> +	    pcie_bus_config != PCIE_BUS_PEER2PEER) {
>> +		rc = pcie_set_mps(dev, 128 << dev->pcie_mpss);
> 
> [Severity: High]
> Does this code introduce a regression where active downstream topologies are
> exposed to dangerous dynamic MPS reductions under the default PCIE_BUS_DEFAULT
> strategy?
> 
> By forcing the Root Port MPS to its maximum supported value here, it overrides
> safe firmware settings. If a device with a smaller MPSS is subsequently
> enumerated or hotplugged, pci_configure_mps() will detect that the downstream
> MPSS is smaller than the Root Port's MPS, triggering pcie_reduce_mps() on the
> entire hierarchy.
> 
> Because pcie_reduce_mps() modifies the Root Port first and sequentially walks
> downstream without quiescing active devices, this creates a dangerous window.
> 
> If an existing downstream device (such as an active NIC or early
> firmware-driven display controller) is actively performing DMA, could it
> transmit a payload exceeding the newly reduced MPS of the Root Port,
> potentially leading to Malformed TLP Fatal Errors and a system crash?


The scenario you describe requires pcie_reduce_mps() to modify the MPS
of a device that is already active.  As explained in the review of patch
1/3, pci_configure_mps() is only reached from pci_setup_device() during
enumeration, before the device is bound to its driver and before it can
generate TLPs.  The reduction is therefore performed while the hierarchy
is still quiescent.

The pre-existing behaviour from 9f0e89359775 already lowers the Root
Port's MPS when a downstream device has a smaller MPSS, so this series
does not introduce dynamic reduction of an active hierarchy.  Patch 1/3
additionally covers Switch ports and already-programmed siblings, which
the old code missed; that is what fixes the Switch case reported in v8.

> 
>> +		if (rc)
>> +			dev_warn(&dev->dev, "failed to set root port MPS\n");
>> +	}
>> +
>>   	if (!bridge || !pci_is_pcie(bridge))
>>   		return;
> 


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic