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

[Hans Zhang <18255117159@163.com>]

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