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

[sashiko-bot@kernel.org]

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