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

[Bjorn Helgaas]

[+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?

  +	WARN_ON(pcie_get_mps(dev) > 256);

More below.

> R-Car Gen4 Reference Manual, chapter 104.4.8 Usage notes for
> MRRS (Max Read Request Size) states:
>   Please set "Max Read Request Size" to 128 bytes or 256 bytes.
>   If "Max Read Request Size" is set to anything other than the
>   above, the transferred data will not match the expected value.
> This limitation also seems the apply to devices issuing MRd TLP.
> This limitation can be triggered by using non-HMB NVMe SSD with
> Max_Read_Request_Size 512 Bytes, for example Crucial P5 Plus.
> Any write into the SSD (MRd TLP issued by the SSD) longer than
> 256 Bytes wraps around at 256 Byte boundary, and the same data
> are written into the SSD starting at offset 0 and at 256 Bytes.
> Force limit Max_Read_Request_Size to at most 256 Bytes for each
> device connected to this PCIe controller to avoid this behavior.
> 
> An non-HMB (Host Memory Buffer) NVMe SSD can be identified using
> the following command. Affected SSD reports 'hmpre' field as 0:
> "
> $ nvme id-ctrl /dev/nvme0 | grep hmpre
> hmpre     : 0
> "
> 
> The symptom is a read from the SSD which wraps around at 256 Byte
> boundary. The test for this symptom can be implemented by writing
> 512 Byte of random data into the SSD and reading the data back. If
> the read back data repeat after 256 Bytes, the device is affected.
> "
> $ dd if=/dev/urandom of=/tmp/data.bin bs=256 count=2 ; \
>   dd if=/tmp/data.bin of=/dev/nvme0n1 bs=256 count=2 ; \
>   dd if=/dev/nvme0n1 bs=256 count=2 of=/tmp/readback.bin
> "
> 
> Expected data:
> "
> $ hexdump -vC /tmp/data.bin
> 00000000  97 81 b7 3b 0e 38 2b 4d  a7 d3 e0 47 ff c2 4b ca
> 00000010  c1 85 98 f0 4a ac 03 a0  3b ab f3 19 44 dd 06 8b
> ...
> 00000100  7a ce 3c b2 e1 d5 d9 11  88 63 10 59 76 3c dc 32 <-- random
> 00000110  72 32 2a 7d a3 e1 aa 13  7c da 58 a1 7b 21 11 50 <-- data
> "
> 
> Faulty readback, collected without this change in place:
> "
> $ hexdump -vC /tmp/readback.bin
> 00000000  97 81 b7 3b 0e 38 2b 4d  a7 d3 e0 47 ff c2 4b ca <---.
> 00000010  c1 85 98 f0 4a ac 03 a0  3b ab f3 19 44 dd 06 8b <-. |
> ...                                                          | |
> 00000100  97 81 b7 3b 0e 38 2b 4d  a7 d3 e0 47 ff c2 4b ca <-:-+- repeated
> 00000110  c1 85 98 f0 4a ac 03 a0  3b ab f3 19 44 dd 06 8b <-+--- data
>      ^^^
>       |
>       '--- Repeat starts at offset 0x100 = 256 Bytes
> "
> 
> Fixes: 0d0c551011df ("PCI: rcar-gen4: Add R-Car Gen4 PCIe controller support for host mode")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Marek Vasut <marek.vasut+renesas@xxxxxxxxxxx>
> ---
> Cc: "Krzysztof Wilczyński" <kwilczynski@xxxxxxxxxx>
> Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> Cc: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> Cc: Koichiro Den <den@xxxxxxxxxxxxx>
> Cc: Lorenzo Pieralisi <lpieralisi@xxxxxxxxxx>
> Cc: Magnus Damm <magnus.damm@xxxxxxxxx>
> Cc: Manivannan Sadhasivam <mani@xxxxxxxxxx>
> Cc: Rob Herring <robh@xxxxxxxxxx>
> Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> Cc: linux-pci@xxxxxxxxxxxxxxx
> Cc: linux-renesas-soc@xxxxxxxxxxxxxxx
> ---
> V2: - Dispose of the reprogramming of MPS/MRRS altogether
>     - Dispose of the entire fixup quirk
>     - Replace both with bridge enable_device hook
>     - Limit MPS/MRRS along the entire packet path to follow
>       MRRS limitation requirement
> V3: - Set no_inc_mrrs to prevent periperhal drivers from increasing MRRS
>       over the limit set by this PCIe controller driver
>     - Warn on MPS > 256 Bytes
>     - Recheck updated documentation and update code comments to match
> ---
>  drivers/pci/controller/dwc/pcie-rcar-gen4.c | 51 +++++++++++++++++++++
>  1 file changed, 51 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> index fbe465a29068f..e38cde2257bb4 100644
> --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> @@ -411,6 +411,54 @@ static int rcar_gen4_pcie_host_msi_init(struct dw_pcie_rp *pp)
>  	return ret;
>  }
>  
> +static int rcar_gen4_pcie_enable_device(struct pci_host_bridge *bridge,
> +					struct pci_dev *dev)
> +{
> +	/*
> +	 * 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.
> +	 * The controller reports Max_Payload_Size_Supported (MPSS) 256 Bytes
> +	 * for header type 0 and 128 Bytes for header type 1. The PCIe core
> +	 * will not allow MPS to be set higher than MPSS, warn here in case
> +	 * something went very wrong in the core.
> +	 *
> +	 * For details, refer to chapter "104.1.1 Features" in either of:
> +	 * R-Car S4 R19UH0161EJ0140 Rev.1.40 Jul. 31, 2026 or
> +	 * R-Car V4H R19UH0186EJ0140 Rev.1.40 Aug. 7, 2026 or
> +	 * R-Car V4M R19UH0217EJ0110 Rev.1.10 Jun. 30, 2026.
> +	 */
> +	WARN_ON(pcie_get_mps(dev) > 256);
> +
> +	/*
> +	 * R-Car Gen4 Reference Manual, chapter 104.4.8 Usage notes for
> +	 * MRRS (Max Read Request Size) states:
> +	 *   Please set "Max Read Request Size" to 128 bytes or 256 bytes.
> +	 *   If "Max Read Request Size" is set to anything other than the
> +	 *   above, the transferred data will not match the expected value.
> +	 * This limitation also seems the apply to devices issuing MRd TLP.
> +	 * This limitation can be triggered by using non-HMB NVMe SSD with
> +	 * Max_Read_Request_Size 512 Bytes, for example Crucial P5 Plus.
> +	 * Any write into the SSD (MRd TLP issued by the SSD) longer than
> +	 * 256 Bytes wraps around at 256 Byte boundary, and the same data
> +	 * are written into the SSD starting at offset 0 and at 256 Bytes.
> +	 * Force limit Max_Read_Request_Size to at most 256 Bytes for each
> +	 * device connected to this PCIe controller to avoid this behavior.
> +	 *
> +	 * For details, refer to aforementioned chapter in either of:
> +	 * R-Car S4 R19UH0161EJ0140 Rev.1.40 Jul. 31, 2026 or
> +	 * R-Car V4H R19UH0186EJ0140 Rev.1.40 Aug. 7, 2026 or
> +	 * R-Car V4M R19UH0217EJ0110 Rev.1.10 Jun. 30, 2026.
> +	 */
> +	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?

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.

> +	return 0;
> +}
> +
>  /* Host mode */
>  static int rcar_gen4_pcie_host_init(struct dw_pcie_rp *pp)
>  {
> @@ -418,6 +466,9 @@ static int rcar_gen4_pcie_host_init(struct dw_pcie_rp *pp)
>  	struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw);
>  	int ret;
>  
> +	if (pp->bridge)
> +		pp->bridge->enable_device = rcar_gen4_pcie_enable_device;
> +
>  	gpiod_set_value_cansleep(dw->pe_rst, 1);
>  
>  	ret = rcar_gen4_pcie_common_init(rcar);
> -- 
> 2.53.0
>