linux-kernel - Re: [PATCH v6 1/2] PCI: Configure Root Port MPS during host probing

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <74a61ccd-c694-44d7-a54c-a40da2df823e@163.com>
Date: Fri, 28 Nov 2025 00:58:07 +0800
From: Hans Zhang <18255117159@....com>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: lpieralisi@...nel.org, kwilczynski@...nel.org, bhelgaas@...gle.com,
 heiko@...ech.de, mani@...nel.org, yue.wang@...ogic.com, pali@...nel.org,
 neil.armstrong@...aro.org, robh@...nel.org, jingoohan1@...il.com,
 khilman@...libre.com, jbrunet@...libre.com,
 martin.blumenstingl@...glemail.com, cassel@...nel.org,
 linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-amlogic@...ts.infradead.org,
 linux-rockchip@...ts.infradead.org
Subject: Re: [PATCH v6 1/2] PCI: Configure Root Port MPS during host probing

Hi Bjorn,

Thank you very much for your reply and the problems you pointed out. The 
next version will modify it.

Best regards,
Hans

On 2025/11/27 07:54, Bjorn Helgaas wrote:
> On Wed, Nov 05, 2025 at 12:51:24AM +0800, Hans Zhang wrote:
>> Current PCIe initialization logic may leave Root Ports (root bridges)
>> operating with non-optimal Maximum Payload Size (MPS) settings. Existing
>> code in pci_configure_mps() returns early for devices without an upstream
>> bridge (!bridge) which includes Root Ports, so their MPS values remain
>> at firmware/hardware defaults. This fails to utilize the controller's full
>> capabilities, leading to suboptimal data transfer efficiency across the
>> PCIe hierarchy.
>>
>> With this patch, during the host controller probing phase:
>> - When PCIe bus tuning is enabled (not PCIE_BUS_TUNE_OFF), and
>> - The device is a Root Port without an upstream bridge (!bridge),
>> The Root Port's MPS is set to its hardware-supported maximum value
>> (128 << dev->pcie_mpss).
>>
>> Note that this initial maximum MPS setting may be reduced later, during
>> downstream device enumeration, if any downstream device does not suppor
>> the Root Port's maximum MPS.
>>
>> This change ensures Root Ports are properly initialized before downstream
>> devices negotiate MPS, while maintaining backward compatibility via the
>> PCIE_BUS_TUNE_OFF check.
> 
> "Properly" is sort of a junk word for me because all it really says is
> we were stupid before, and we're smarter now, but it doesn't explain
> exactly *what* was wrong and why this new thing is "proper."
> 
> It's obvious that the Max_Payload_Size power-on default (128 bytes) is
> suboptimal in some situations, so you don't even need to say that.
> And I think 128 bytes *is* optimal in the PCIE_BUS_PEER2PEER case.
> 
> s/Root Ports (root bridges)/Root Ports/
> s/bridge (!bridge)/bridge/     # a couple times
> s/hardware-supported//         # unnecessary
> s/(128 << dev->pcie_mpss)//    # we can read the spec
> s/suppor/support/
> 
>> Suggested-by: Niklas Cassel <cassel@...nel.org>
>> Suggested-by: Manivannan Sadhasivam <mani@...nel.org>
>> Signed-off-by: Hans Zhang <18255117159@....com>
>> ---
>>   drivers/pci/probe.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> index 0ce98e18b5a8..2459def3af9b 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -2196,6 +2196,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),
>> +	 * start off by setting Root Ports' MPS to MPSS. This only applies to
>> +	 * Root Ports without an upstream bridge (root bridges), as other Root
>> +	 * Ports will have downstream bridges.
> 
> I can't parse this sentence.  *No* Root Port has an upstream bridge.
> So I don't know what "other Root Ports" would be or why they would
> have downstream bridges (any Root Port is likely to have downstream
> endpoints or bridges).
> 
>> +	   ... Depending on the MPS strategy
>> +	 * and MPSS of downstream devices, the Root Port's MPS may be
>> +	 * overridden later.
>> +	 */
>> +	if (!bridge && pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT &&
>> +	    pcie_bus_config != PCIE_BUS_TUNE_OFF)
>> +		pcie_set_mps(dev, 128 << dev->pcie_mpss);
>> +
>>   	if (!bridge || !pci_is_pcie(bridge))
>>   		return;
>>   
>> -- 
>> 2.34.1
>>