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

[Bjorn Helgaas]

On Thu, Sep 03, 2026 at 08:51:06PM +0200, Marek Vasut wrote:
> 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 ?

I fixed the commit log, no problem.

> >    +	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.

Ouch, that does hurt.

> 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.

I don't know if we'll get a real answer for Loongson (there's no
maintainer listed for it, hint hint :)), but my guess is that it does
apply to all devices downstream of the Loongson controller.

I think MRRS is mostly interesting for DMA because MMIO from CPUs is
usually small sizes, far below the 128-byte or larger transfers that
devices may do.