[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 at 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 at 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);
>>   	}
>