Re: [RFC PATCH 3/4] PCI: Configure Root Port MPS after scanning its hierarchy

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] pci: ignoring pcie_set_mps() failures causes illegal MPS topology
--

commit 6dac99f476ff730ab7dd14051b62e9402806b339
Author: Niklas Cassel <cassel@kernel.org>

PCI: Configure Root Port MPS after scanning its hierarchy

This commit introduces logic to raise the Maximum Payload Size (MPS) for Root
Ports and their downstream hierarchies to the largest supported value during
scanning or rescan. It applies to PCIE_BUS_DEFAULT configurations to improve
efficiency. It also enables the Intel read completion coalescing quirk for the
default MPS strategy.

> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index d8e58e5ef7309..5f37b480b51df 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c

[ ... ]

> @@ -3083,6 +3083,82 @@ static int pcie_bus_configure_set(struct pci_dev *dev, void *data)
>  	return 0;
>  }
>  
> +static int pcie_raise_mps(struct pci_dev *dev, void *data)
> +{
> +	int mps = *(int *)data;
> +
> +	/* MPS is of type 'RsvdP' for VFs */
> +	if (!pci_is_pcie(dev) || dev->is_virtfn)
> +		return 0;
> +
> +	if (pcie_get_mps(dev) < mps && pcie_set_mps(dev, mps))
> +		pci_err(dev, "can't set Max Payload Size to %d\n", mps);
> +
> +	return 0;
> +}

[Severity: Medium]
When pcie_set_mps() fails in pcie_raise_mps(), this error is logged but
swallowed by returning 0 to the pci_walk_bus() callback loop. Does this allow
the loop to continue and potentially create an illegal MPS topology where a
downstream device inherits a larger MPS than its upstream bridge?

> +
> +/*
> + * With PCIE_BUS_DEFAULT, pci_configure_mps() only matches each device to its

[ ... ]

> + * leave hierarchies alone once any of their devices may have a driver bound.
> + */
> +static void pcie_bus_raise_default_mps(struct pci_bus *bus)
> +{
> +	struct pci_dev *rp = bus->self;
> +	u8 smpss = rp->pcie_mpss;
> +	int mps, old_mps;
> +
> +	if (pci_pcie_type(rp) != PCI_EXP_TYPE_ROOT_PORT ||
> +	    list_empty(&bus->devices) || pci_bus_in_use(bus))
> +		return;
> +
> +	pci_walk_bus(bus, pcie_find_smpss, &smpss);
> +	mps = 128 << smpss;
> +	old_mps = pcie_get_mps(rp);
> +	if (mps <= old_mps)
> +		return;
> +
> +	pcie_raise_mps(rp, &mps);
> +	pci_walk_bus(bus, pcie_raise_mps, &mps);

[Severity: Medium]
If the call to pcie_raise_mps() for the Root Port (rp) fails to set its MPS,
does this dangerously proceed to call pci_walk_bus() and set the MPS of
downstream devices anyway? Could this result in a PCIe protocol violation
leading to Malformed TLP errors and packet drops?

> +	pci_info(rp, "Max Payload Size of hierarchy set to %d (was %d)\n",
> +		 mps, old_mps);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930145017.1356088-6-cassel@kernel.org?part=3

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