Re: [PATCH v3] PCI: rcar-gen4: Limit Max_Read_Request_Size and Max_Payload_Size to 256 Bytes

[Marek Vasut <marek.vasut@mailbox.org>]

Hello Bjorn,

On 9/3/26 7:32 PM, Bjorn Helgaas wrote:
> [+cc Ziyao, Rong, Huacai for similar Loongson MRRS issue]
> 
> On Fri, Aug 21, 2026 at 04:05:51AM +0200, Marek Vasut wrote:
>> R-Car Gen4 PCIe controller has a hardware limitation of 256 Bytes
>> Max_Payload_Size (MPS). PCIe specification indicates that the MPS
>> must not exceed minimum MPS of any element along the packet path.
>> Force limit Max_Payload_Size to at most 256 Bytes for each device
>> connected to this PCIe controller.
> 
> IIUC the PCI core already enforces this limit, and what this patch
> does is double-check that this limit is observed with this check,
> right?

That is correct, this was changed in V3, I missed the commit message 
update, sorry.

Would you like me to respin the patch one more time with an updated 
commit message, or would you be willing to fix it up in tree ?

>    +	WARN_ON(pcie_get_mps(dev) > 256);
> 
> More below.


[...]

>> +	bridge->no_inc_mrrs = 1;
>> +	if (pcie_get_readrq(dev) > 256) {
>> +		pci_info(dev, "Limiting MRRS to 256 bytes\n");
>> +		pcie_set_readrq(dev, 256);
>> +	}
> 
> It would be nice if all the platforms that need no_inc_mrrs could
> apply it the same way, but I assume you saw loongson_mrrs_quirk() and
> loongson_set_min_mrrs_quirk() in the process of finding no_inc_mrrs,
> and chose a different implementation strategy for some reason, e.g.,
> this way doesn't have to include device IDs for all the Root Ports?

The loongson quirk won't work if the PCIe controller driver is built as 
a module, which the R-Car Gen4 PCIe driver can be, and in fact is often 
built as a module, because it depends on firmware which is loaded from 
filesystem.

If the controller driver is built as a module, then 
DECLARE_PCI_FIXUP_ENABLE() is not applied, the 
DECLARE_PCI_FIXUP_ENABLE() is applied only on boot and therefore only 
for built-in drivers.

I got burnt by DECLARE_PCI_FIXUP_ENABLE() in V1 of this patch.

However, there is also another part to this -- the 
rcar_gen4_pcie_enable_device() is called for every device on the bus and 
applies the MRRS limitation to every device on the bus that is 
downstream of the controller, not only the controller. This is necessary 
on this controller variant, else hardware like PCIe SSDs with MRRS 
higher than the controller break.

> Maybe we should rework no_inc_mrrs in such a way that drivers could
> set a max MRRS in the struct pci_host_bridge and make
> pcie_write_mrrs() and pcie_set_readrq() pay attention to it?  That
> might let us get rid of the FIXUP approach.
In light of the last paragraph above, that the MRRS has to be limited 
also on all devices downstream of this particular controller, I would 
like to ask -- does the Loongson controller have the same limitation or 
not ? If not, then I would argue this quirk should be isolated to this 
controller variant ; else, I am happy to start on the core patches.

[...]

Thank you for your help !