Re: [PATCH v8 1/2] PCI: Configure Root Port MPS during host probing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Logic Error / PCIe MPS Mismatch. Unconditionally configuring the Root Port MPS to its maximum supported value breaks devices connected via PCIe switches under the default MPS strategy (PCIE_BUS_DEFAULT).
- [Medium] Missing Error Handling: The return value of `pcie_set_mps()` is silently ignored when configuring the Root Port MPS.
--

commit 13816df0ca2fe3070f644d4bbf35fd712be22285
Author: Hans Zhang <18255117159@163.com>

PCI: Configure Root Port MPS during host probing

Current PCIe initialization logic may leave Root Ports operating with
non-optimal Maximum Payload Size (MPS) settings. This change ensures
Root Ports are initialized to their maximum MPS before downstream devices
negotiate MPS, while not interfering with the PCIE_BUS_PEER2PEER strategy.

> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 27008e2ea5afc..dc47478589182 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2229,6 +2229,18 @@ 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)
> +		pcie_set_mps(dev, 128 << dev->pcie_mpss);

[Severity: High]
Does unconditionally setting the Root Port MPS to maximum break devices
connected via PCIe switches under the default MPS strategy (PCIE_BUS_DEFAULT)?

If a system has a PCIe switch and the Root Port supports a larger MPS
(e.g., 256B) than the endpoint behind the switch (e.g., 128B), it appears this
would cause a mismatch.

During top-down enumeration, this new code unconditionally sets the Root Port
MPS to max (256B), and the switch ports inherit 256B. When the endpoint is
later enumerated in pci_configure_mps(), it hits this pre-existing logic
further down in the function:

    mpss = 128 << dev->pcie_mpss;
    if (mpss < p_mps && pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT) {
        pcie_set_mps(bridge, mpss);

Because the upstream bridge is a Switch Downstream Port and not a Root Port,
the condition pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT evaluates to
false.

Since the MPS reduction strictly limits itself to Root Ports, the MPS of the
switch is not reduced, leaving it at 256B. The endpoint then fails to set its
MPS to 256B, returning -EINVAL and permanently leaving a mismatch which could
lead to dropped packets as Malformed TLPs.

[Severity: Medium]
Are we missing an error check here?

If the configuration write fails due to hardware quirks or an invalid value,
the return value of pcie_set_mps() is silently ignored, unlike similar calls
in pci_configure_mps() which log a warning or handle the failure.

> +
>  	if (!bridge || !pci_is_pcie(bridge))
>  		return;
>

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

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