Extend PCIE_BUS_PEER2PEER to set MRSS=128 to fix CNS3xxx BM DMA.
PATCH 1/1 Extend PCIE_BUS_PEER2PEER to set MRSS=128 to fix CNS3xxx BM DMA.
I think this bug needs to be fixed, this way or another.
The platform in question is Cavium CNS3xxx, ARMv6.
A recent patch by Arnd Bergmann (498a92d42596 "ARM: cns3xxx: pci: avoid potential stack overflow") converted an explicit setting of PCI_EXP_DEVCTL_READRQ = 0 (i.e., max 128 bytes for bus-mastering PCIe DMA read request) to:
+ pcie_bus_config = PCIE_BUS_PEER2PEER;
with the following commentary:
"The second part is how the driver sets up the Max_Read_Request_Size value for the first device/function on bus 1, i.e. the device plugged directly into the PCIe root port.
For all I can tell, this is in fact incomplete, as it does not perform the same setting on devices attached to a PCIe switch, or multi-function devices.
The solution for this part fortunately is even easier: if we just set the global pcie_bus_config variable to PCIE_BUS_PEER2PEER, all PCIe devices in the system are limited to 128 byte MPS, which in turn limits the MRRS to 128 bytes for all devices, and we no longer even need to touch any devices."
The problem is the MRRS setting is never written to the hardware. I propose the following, though I'm not sure if we can do this safely, especially given the comments in probe.c. OTOH, this change may be required in other (all?) cases when the user requests PCIE_BUS_PEER2PEER.
On this Laguna GW-2388 the following patch fixes BM DMA with:
0000:00:00.0 PCI bridge: Cavium Networks Device 3400 (rev 01) 0000:01:00.0 PCI bridge: Texas Instruments XIO2001 PCI Express-to-PCI Bridge 0000:02:0e.0 (PCI devices behind the bridge, these are doing actual BM xfers) 0001:00:00.0 PCI bridge: Cavium Networks Device 3400 (rev 01 - this is
the second lane from the CPU)
pci 0000:00:00.0: Max Payload Size set to 128/ 128 (was 128), Max Read Rq 128 pci 0000:01:00.0: Max Payload Size set to 128/ 512 (was 128), Max Read Rq 128 pci 0001:00:00.0: Max Payload Size set to 128/ 128 (was 128), Max Read Rq 128
drivers/pci/probe.c +2 -1
@@ -1919,7 +1919,8 @@ static void pcie_write_mrrs(struct pci_dev *dev)/* In the "safe" case, do not configure the MRRS. There appear to be * issues with setting MRRS to 0 on a number of devices. */ -if (pcie_bus_config !=PCIE_BUS_PERFORMANCE ) +if (pcie_bus_config !=PCIE_BUS_PERFORMANCE && + pcie_bus_config !=PCIE_BUS_PEER2PEER )return ;/* For Max performance, the MRRS must be set to the largest supported
include/linux/pci.h +1 -1
@@ -756,7 +756,7 @@ enum pcie_bus_config_types {PCIE_BUS_DEFAULT ,/* ensure MPS matches upstream bridge */ PCIE_BUS_SAFE ,/* use largest MPS boot-time devices support */ PCIE_BUS_PERFORMANCE ,/* use MPS and MRRS for best performance */ - PCIE_BUS_PEER2PEER, /* set MPS = 128 for all devices */ + PCIE_BUS_PEER2PEER, /* set MPS and MRSS to 128 for all devices */ };extern enum pcie_bus_config_types pcie_bus_config;
Quoted 9 lines · Krzysztof Halasa
I think this bug needs to be fixed, this way or another.
The platform in question is Cavium CNS3xxx, ARMv6.
A recent patch by Arnd Bergmann (
498a92d42596"ARM: cns3xxx: pci: avoid potential stack overflow") converted an explicit setting ofPCI_EXP_DEVCTL_READRQ= 0 (i.e., max 128 bytes for bus-mastering PCIe DMA read request) to:
+pcie_bus_config=PCIE_BUS_PEER2PEER;
Any thoughts on this, Arnd? I can (and will) look at this, but it's a complicated area and it will take me quite a while to dig into it.
Quoted 64 lines
with the following commentary:
"The second part is how the driver sets up theMax_Read_Request_Sizevalue for the first device/function on bus 1, i.e. the device plugged directly into the PCIe root port.
For all I can tell, this is in fact incomplete, as it does not perform the same setting on devices attached to a PCIe switch, or multi-function devices.
The solution for this part fortunately is even easier: if we just set the globalpcie_bus_configvariable toPCIE_BUS_PEER2PEER, all PCIe devices in the system are limited to 128 byte MPS, which in turn limits the MRRS to 128 bytes for all devices, and we no longer even need to touch any devices."The problem is the MRRS setting is never written to the hardware. I propose the following, though I'm not sure if we can do this safely, especially given the comments in
probe.c. OTOH, this change may be required in other (all?) cases when the user requestsPCIE_BUS_PEER2PEER.On this Laguna GW-2388 the following patch fixes BM DMA with:
0000:00:00.0 PCI bridge: Cavium Networks Device 3400 (rev 01) 0000:01:00.0 PCI bridge: Texas Instruments XIO2001 PCI Express-to-PCI Bridge 0000:02:0e.0 (PCI devices behind the bridge, these are doing actual BM xfers) 0001:00:00.0 PCI bridge: Cavium Networks Device 3400 (rev 01 - this is
the second lane from the CPU)pci 0000:00:00.0: Max Payload Size set to 128/ 128 (was 128), Max Read Rq 128 pci 0000:01:00.0: Max Payload Size set to 128/ 512 (was 128), Max Read Rq 128 pci 0001:00:00.0: Max Payload Size set to 128/ 128 (was 128), Max Read Rq 128Signed-off-by Krzysztof Hałasa <[email protected]>Fixes 498a92d42596 ("ARM: cns3xxx: pci: avoid potential stack overflow")drivers/pci/probe.c +2
-1@@ -1919,7 +1919,8 @@ static void pcie_write_mrrs(struct pci_dev *dev)/* In the "safe" case, do not configure the MRRS. There appear to be * issues with setting MRRS to 0 on a number of devices. */ -if (pcie_bus_config !=PCIE_BUS_PERFORMANCE ) +if (pcie_bus_config !=PCIE_BUS_PERFORMANCE && + pcie_bus_config !=PCIE_BUS_PEER2PEER )return ;/* For Max performance, the MRRS must be set to the largest supported include/linux/pci.h +1
-2@@ -756,7 +756,7 @@ enum pcie_bus_config_types {PCIE_BUS_DEFAULT ,/* ensure MPS matches upstream bridge */ PCIE_BUS_SAFE ,/* use largest MPS boot-time devices support */ PCIE_BUS_PERFORMANCE ,/* use MPS and MRRS for best performance */ - PCIE_BUS_PEER2PEER, /* set MPS = 128 for all devices */ + PCIE_BUS_PEER2PEER, /* set MPS and MRSS to 128 for all devices */ };extern enum pcie_bus_config_types pcie_bus_config; --To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 5 lines · Bjorn Helgaas
Quoted 4 lines · Krzysztof Halasa
I think this bug needs to be fixed, this way or another.
The platform in question is Cavium CNS3xxx, ARMv6.
Quoted 8 lines
Quoted 5 lines
A recent patch by Arnd Bergmann (
498a92d42596"ARM: cns3xxx: pci: avoid potential stack overflow") converted an explicit setting ofPCI_EXP_DEVCTL_READRQ= 0 (i.e., max 128 bytes for bus-mastering PCIe DMA read request) to:
+pcie_bus_config=PCIE_BUS_PEER2PEER;Any thoughts on this, Arnd? I can (and will) look at this, but it's a complicated area and it will take me quite a while to dig into it.
What should we do here? Should we revert 498a92d42596? It appeared in v4.4, so this has been broken for a while.
It looks like 498a92d42596 merely fixed a warning, at the expense of breaking DMA on Cavium. Reverting it would bring the warning back, but that's better than broken DMA.
Quoted 68 lines
Quoted 61 lines
with the following commentary:
"The second part is how the driver sets up theMax_Read_Request_Sizevalue for the first device/function on bus 1, i.e. the device plugged directly into the PCIe root port.
For all I can tell, this is in fact incomplete, as it does not perform the same setting on devices attached to a PCIe switch, or multi-function devices.
The solution for this part fortunately is even easier: if we just set the globalpcie_bus_configvariable toPCIE_BUS_PEER2PEER, all PCIe devices in the system are limited to 128 byte MPS, which in turn limits the MRRS to 128 bytes for all devices, and we no longer even need to touch any devices."The problem is the MRRS setting is never written to the hardware. I propose the following, though I'm not sure if we can do this safely, especially given the comments in
probe.c. OTOH, this change may be required in other (all?) cases when the user requestsPCIE_BUS_PEER2PEER.On this Laguna GW-2388 the following patch fixes BM DMA with:
0000:00:00.0 PCI bridge: Cavium Networks Device 3400 (rev 01) 0000:01:00.0 PCI bridge: Texas Instruments XIO2001 PCI Express-to-PCI Bridge 0000:02:0e.0 (PCI devices behind the bridge, these are doing actual BM xfers) 0001:00:00.0 PCI bridge: Cavium Networks Device 3400 (rev 01 - this is
the second lane from the CPU)pci 0000:00:00.0: Max Payload Size set to 128/ 128 (was 128), Max Read Rq 128 pci 0000:01:00.0: Max Payload Size set to 128/ 512 (was 128), Max Read Rq 128 pci 0001:00:00.0: Max Payload Size set to 128/ 128 (was 128), Max Read Rq 128Signed-off-by Krzysztof Hałasa <[email protected]>Fixes 498a92d42596 ("ARM: cns3xxx: pci: avoid potential stack overflow")drivers/pci/probe.c +2
-1@@ -1919,7 +1919,8 @@ static void pcie_write_mrrs(struct pci_dev *dev)/* In the "safe" case, do not configure the MRRS. There appear to be * issues with setting MRRS to 0 on a number of devices. */ -if (pcie_bus_config !=PCIE_BUS_PERFORMANCE ) +if (pcie_bus_config !=PCIE_BUS_PERFORMANCE && + pcie_bus_config !=PCIE_BUS_PEER2PEER )return ;/* For Max performance, the MRRS must be set to the largest supported include/linux/pci.h +1
-2@@ -756,7 +756,7 @@ enum pcie_bus_config_types {PCIE_BUS_DEFAULT ,/* ensure MPS matches upstream bridge */ PCIE_BUS_SAFE ,/* use largest MPS boot-time devices support */ PCIE_BUS_PERFORMANCE ,/* use MPS and MRRS for best performance */ - PCIE_BUS_PEER2PEER, /* set MPS = 128 for all devices */ + PCIE_BUS_PEER2PEER, /* set MPS and MRSS to 128 for all devices */ };extern enum pcie_bus_config_types pcie_bus_config; --Quoted 3 lines
To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 3 lines · Bjorn Helgaas
It looks like
498a92d42596merely fixed a warning, at the expense of breaking DMA on Cavium. Reverting it would bring the warning back, but that's better than broken DMA.
Perhaps we should change PCIE_BUS_PEER2PEER to also write MRRS anyway.
I realize the CNS3xxx patch is some sort of clever workaround, and that PCIE_BUS_PEER2PEER (which normally comes from kernel command line parameter "pcie_bus_peer2peer") was not exactly intended for this. But if one asks for "peer2peer" (which means limiting transfers to 128 bytes), how could it all work if the bus mastering read requests are not equally limited?
BTW s/MRSS/MRRS/g
Signature · 4 lines
Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Quoted 6 lines · Krzysztof Hałasa
Quoted 3 lines · Bjorn Helgaas
It looks like
498a92d42596merely fixed a warning, at the expense of breaking DMA on Cavium. Reverting it would bring the warning back, but that's better than broken DMA.
Quoted 8 lines
Perhaps we should change
PCIE_BUS_PEER2PEERto also write MRRS anyway.I realize the CNS3xxx patch is some sort of clever workaround, and that
PCIE_BUS_PEER2PEER(which normally comes from kernel command line parameter "pcie_bus_peer2peer") was not exactly intended for this. But if one asks for "peer2peer" (which means limiting transfers to 128 bytes), how could it all work if the bus mastering read requests are not equally limited?
MPS=128 means a function will never generate a TLP exceeding 128 bytes. MRRS=128 means a function will never generate a Read Request with a size exceeding 128 bytes.
The comment in pcie_write_mrrs() claims:
... the MRRS ... cannot be configured larger than the MPS the device or the bus can support.
I think this comment is wrong, at least from a hardware point of view. Setting all devices to MRRS=512 and MPS=128 is a legal configuration that means functions may receive Read Requests for up to 512 bytes, and they will have to respond with 4 128-byte TLPs.
The spec (PCIe r3.0, 7.8.4 implementation note) says:
The Max_Read_Request_Size mechanism allows improved control of bandwidth allocation in systems where quality of service (QoS) is important for the target applications. For example, an arbitration scheme based on counting Requests (and not the sizes of those Requests) provides imprecise bandwidth allocation when some Requesters use much larger sizes than others. The Max_Read_Request_Size mechanism can be used to force more uniform allocation of bandwidth, by restricting the upper size of Read Requests.
The Linux usage of tying it to MRRS to MPS is part of a plan to use MRRS to restrict the TLP size in one direction and MPS to restrict TLP size in the other. See b03e7495a862 ("PCI: Set PCI-E Max Payload Size on fabric"). But this is not in line with the spec intention, and I'm not 100% convinced that it works reliably.
So I'm a little wary of tweaking our MPS/MRRS configuration without a more extensive analysis and (hopefully) some simplification of the code.
Bjorn
Quoted 15 lines · Bjorn Helgaas
Quoted 5 lines · Bjorn Helgaas
Quoted 4 lines · Krzysztof Halasa
I think this bug needs to be fixed, this way or another.
The platform in question is Cavium CNS3xxx, ARMv6.
Quoted 8 lines
Quoted 5 lines
A recent patch by Arnd Bergmann (
498a92d42596"ARM: cns3xxx: pci: avoid potential stack overflow") converted an explicit setting ofPCI_EXP_DEVCTL_READRQ= 0 (i.e., max 128 bytes for bus-mastering PCIe DMA read request) to:
+pcie_bus_config=PCIE_BUS_PEER2PEER;Any thoughts on this, Arnd? I can (and will) look at this, but it's a complicated area and it will take me quite a while to dig into it.
Quoted 6 lines
What should we do here? Should we revert
498a92d42596? It appeared in v4.4, so this has been broken for a while.It looks like
498a92d42596merely fixed a warning, at the expense of breaking DMA on Cavium. Reverting it would bring the warning back, but that's better than broken DMA.
I propose that we revert 498a92d42596 ("ARM: cns3xxx: pci: avoid potential stack overflow"). I haven't had a chance to look into the problem with it yet.
Quoted 72 lines
Quoted 59 lines
Quoted 59 lines
with the following commentary:
"The second part is how the driver sets up theMax_Read_Request_Sizevalue for the first device/function on bus 1, i.e. the device plugged directly into the PCIe root port.
For all I can tell, this is in fact incomplete, as it does not perform the same setting on devices attached to a PCIe switch, or multi-function devices.
The solution for this part fortunately is even easier: if we just set the globalpcie_bus_configvariable toPCIE_BUS_PEER2PEER, all PCIe devices in the system are limited to 128 byte MPS, which in turn limits the MRRS to 128 bytes for all devices, and we no longer even need to touch any devices."The problem is the MRRS setting is never written to the hardware. I propose the following, though I'm not sure if we can do this safely, especially given the comments in
probe.c. OTOH, this change may be required in other (all?) cases when the user requestsPCIE_BUS_PEER2PEER.On this Laguna GW-2388 the following patch fixes BM DMA with:
0000:00:00.0 PCI bridge: Cavium Networks Device 3400 (rev 01) 0000:01:00.0 PCI bridge: Texas Instruments XIO2001 PCI Express-to-PCI Bridge 0000:02:0e.0 (PCI devices behind the bridge, these are doing actual BM xfers) 0001:00:00.0 PCI bridge: Cavium Networks Device 3400 (rev 01 - this is
the second lane from the CPU)pci 0000:00:00.0: Max Payload Size set to 128/ 128 (was 128), Max Read Rq 128 pci 0000:01:00.0: Max Payload Size set to 128/ 512 (was 128), Max Read Rq 128 pci 0001:00:00.0: Max Payload Size set to 128/ 128 (was 128), Max Read Rq 128Signed-off-by Krzysztof Hałasa <[email protected]>Fixes 498a92d42596 ("ARM: cns3xxx: pci: avoid potential stack overflow")drivers/pci/probe.c +2
-1@@ -1919,7 +1919,8 @@ static void pcie_write_mrrs(struct pci_dev *dev)/* In the "safe" case, do not configure the MRRS. There appear to be * issues with setting MRRS to 0 on a number of devices. */ -if (pcie_bus_config !=PCIE_BUS_PERFORMANCE ) +if (pcie_bus_config !=PCIE_BUS_PERFORMANCE && + pcie_bus_config !=PCIE_BUS_PEER2PEER )return ;/* For Max performance, the MRRS must be set to the largest supported include/linux/pci.h +1
-1@@ -756,7 +756,7 @@ enum pcie_bus_config_types {PCIE_BUS_DEFAULT ,/* ensure MPS matches upstream bridge */ PCIE_BUS_SAFE ,/* use largest MPS boot-time devices support */ PCIE_BUS_PERFORMANCE ,/* use MPS and MRRS for best performance */ - PCIE_BUS_PEER2PEER, /* set MPS = 128 for all devices */ + PCIE_BUS_PEER2PEER, /* set MPS and MRSS to 128 for all devices */ };Quoted 9 lines
Quoted 5 lines
include/linux/pci.h +0
-1extern enum pcie_bus_config_types pcie_bus_config; --To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html