Fixes for Armada 370/XP PCIe

From: Willy Tarreau <[email protected]>

mvebu_pcie_handle_membase_change() and mvebu_pcie_handle_iobase_change() do not correctly compute the window size. PCI uses an inclusive start/end address pair, which requires a +1 when converting to size.

This only worked because a bug in the mbus driver allowed it to silently accept and round up bogus sizes.

Fix this by adding one to the computed size.

Fixes 45361a4fe4464180815157654aabbd2afb4848ad ('PCIe driver for Marvell Armada 370/XP systems')

Cc <[email protected]> # v3.11+

Signed-off-by Willy Tarreau <[email protected]>

Reviewed-by Jason Gunthorpe <[email protected]>

Signed-off-by Thomas Petazzoni <[email protected]>

Tested-by Neil Greatorex <[email protected]>

---
 drivers/pci/host/pci-mvebu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/pci/host/pci-mvebu.c +2 -2

index d3d1cfd..4829921 100644
@@ -331,7 +331,7 @@ static void mvebu_pcie_handle_iobase_change(struct mvebu_pcie_port *port)
 	port->iowin_base = port->pcie->io.start + iobase;
 	port->iowin_size = ((0xFFF | ((port->bridge.iolimit & 0xF0) << 8) |
 			    (port->bridge.iolimitupper << 16)) -
-			    iobase);
+			    iobase) + 1;
 
 	mvebu_mbus_add_window_remap_by_id(port->io_target, port->io_attr,
 					  port->iowin_base, port->iowin_size,

@@ -364,7 +364,7 @@ static void mvebu_pcie_handle_membase_change(struct mvebu_pcie_port *port)
 	port->memwin_base  = ((port->bridge.membase & 0xFFF0) << 16);
 	port->memwin_size  =
 		(((port->bridge.memlimit & 0xFFF0) << 16) | 0xFFFFF) -
-		port->memwin_base;
+		port->memwin_base + 1;
 
 	mvebu_mbus_add_window_by_id(port->mem_target, port->mem_attr,
 				    port->memwin_base, port->memwin_size);

Signature · 1 line

1.9.2

Quoted 10 lines · Thomas Petazzoni

From: Willy Tarreau <[email protected]>

mvebu_pcie_handle_membase_change() and mvebu_pcie_handle_iobase_change() do not correctly compute the window size. PCI uses an inclusive start/end address pair, which requires a +1 when converting to size.

This only worked because a bug in the mbus driver allowed it to silently accept and round up bogus sizes.

Quoted 8 lines

Fix this by adding one to the computed size.

Fixes 45361a4fe4464180815157654aabbd2afb4848ad ('PCIe driver for Marvell Armada 370/XP systems')

Cc <[email protected]> # v3.11+

Signed-off-by Willy Tarreau <[email protected]>

Reviewed-by Jason Gunthorpe <[email protected]>

Signed-off-by Thomas Petazzoni <[email protected]>

Tested-by Neil Greatorex <[email protected]>

Acked-by Bjorn Helgaas <[email protected]>

If I were merging via my tree, I would tweak the subject capitalization so "git log --oneline drivers/pci/host/pci-mvebu.c" looks consistent.

MBus windows are used on Marvell platforms to map certain peripherals in the physical address space. In the PCIe context, MBus windows are needed to map PCIe I/O and memory regions in the physical address.

However, those MBus windows can only have power of two sizes, while PCIe BAR do not necessarily guarantee this. For this reason, the current pci-mvebu breaks on platforms where PCIe devices have BARs that don't sum up to a power of two size at the emulated bridge level.

This commit fixes this by allowing the pci-mvebu driver to create multiple contiguous MBus windows (each having a power of two size) to cover a given PCIe BAR.

To achieve this, two functions are added: mvebu_pcie_add_windows() and mvebu_pcie_del_windows() to respectively add and remove all the MBus windows that are needed to map the provided PCIe region base and size. The emulated PCI bridge code now calls those functions, instead of directly calling the mvebu-mbus driver functions.

Fixes 45361a4fe4464180815157654aabbd2afb4848ad ('pci: PCIe driver for Marvell Armada 370/XP systems')

Cc <[email protected]> # v3.11+

Signed-off-by Thomas Petazzoni <[email protected]>

Tested-by Neil Greatorex <[email protected]>

---
 drivers/pci/host/pci-mvebu.c | 88 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 74 insertions(+), 14 deletions(-)

drivers/pci/host/pci-mvebu.c +74 -14

index 4829921..e384e25 100644
@@ -293,6 +293,58 @@ static int mvebu_pcie_hw_wr_conf(struct mvebu_pcie_port *port,
 	return PCIBIOS_SUCCESSFUL;
 }
 
+/*
+ * Remove windows, starting from the largest ones to the smallest
+ * ones.
+ */
+static void mvebu_pcie_del_windows(struct mvebu_pcie_port *port,
+				   phys_addr_t base, size_t size)
+{
+	while (size) {
+		size_t sz = 1 << (fls(size) - 1);
+
+		mvebu_mbus_del_window(base, sz);
+		base += sz;
+		size -= sz;
+	}
+}
+
+/*
+ * MBus windows can only have a power of two size, but PCI BARs do not
+ * have this constraint. Therefore, we have to split the PCI BAR into
+ * areas each having a power of two size. We start from the largest
+ * one (i.e highest order bit set in the size).
+ */
+static void mvebu_pcie_add_windows(struct mvebu_pcie_port *port,
+				   unsigned int target, unsigned int attribute,
+				   phys_addr_t base, size_t size,
+				   phys_addr_t remap)
+{
+	size_t size_mapped = 0;
+
+	while (size) {
+		size_t sz = 1 << (fls(size) - 1);
+		int ret;
+
+		ret = mvebu_mbus_add_window_remap_by_id(target, attribute, base,
+							sz, remap);
+		if (ret) {
+			dev_err(&port->pcie->pdev->dev,
+				"Could not create MBus window at 0x%x, size 0x%x: %d\n",
+				base, sz, ret);
+			mvebu_pcie_del_windows(port, base - size_mapped,
+					       size_mapped);
+			return;
+		}
+
+		size -= sz;
+		size_mapped += sz;
+		base += sz;
+		if (remap != MVEBU_MBUS_NO_REMAP)
+			remap += sz;
+	}
+}
+
 static void mvebu_pcie_handle_iobase_change(struct mvebu_pcie_port *port)
 {
 	phys_addr_t iobase;

@@ -304,8 +356,8 @@ static void mvebu_pcie_handle_iobase_change(struct mvebu_pcie_port *port)
 
 		/* If a window was configured, remove it */
 		if (port->iowin_base) {
-			mvebu_mbus_del_window(port->iowin_base,
-					      port->iowin_size);
+			mvebu_pcie_del_windows(port, port->iowin_base,
+					       port->iowin_size);
 			port->iowin_base = 0;
 			port->iowin_size = 0;
 		}

@@ -333,9 +385,9 @@ static void mvebu_pcie_handle_iobase_change(struct mvebu_pcie_port *port)
 			    (port->bridge.iolimitupper << 16)) -
 			    iobase) + 1;
 
-	mvebu_mbus_add_window_remap_by_id(port->io_target, port->io_attr,
-					  port->iowin_base, port->iowin_size,
-					  iobase);
+	mvebu_pcie_add_windows(port, port->io_target, port->io_attr,
+			       port->iowin_base, port->iowin_size,
+			       iobase);
 }
 
 static void mvebu_pcie_handle_membase_change(struct mvebu_pcie_port *port)

@@ -346,8 +398,8 @@ static void mvebu_pcie_handle_membase_change(struct mvebu_pcie_port *port)
 
 		/* If a window was configured, remove it */
 		if (port->memwin_base) {
-			mvebu_mbus_del_window(port->memwin_base,
-					      port->memwin_size);
+			mvebu_pcie_del_windows(port, port->memwin_base,
+					       port->memwin_size);
 			port->memwin_base = 0;
 			port->memwin_size = 0;
 		}

@@ -366,8 +418,9 @@ static void mvebu_pcie_handle_membase_change(struct mvebu_pcie_port *port)
 		(((port->bridge.memlimit & 0xFFF0) << 16) | 0xFFFFF) -
 		port->memwin_base + 1;
 
-	mvebu_mbus_add_window_by_id(port->mem_target, port->mem_attr,
-				    port->memwin_base, port->memwin_size);
+	mvebu_pcie_add_windows(port, port->mem_target, port->mem_attr,
+			       port->memwin_base, port->memwin_size,
+			       MVEBU_MBUS_NO_REMAP);
 }
 
 /*

@@ -743,14 +796,21 @@ static resource_size_t mvebu_pcie_align_resource(struct pci_dev *dev,
 
 	/*
 	 * On the PCI-to-PCI bridge side, the I/O windows must have at
-	 * least a 64 KB size and be aligned on their size, and the
-	 * memory windows must have at least a 1 MB size and be
-	 * aligned on their size
+	 * least a 64 KB size and the memory windows must have at
+	 * least a 1 MB size. Moreover, MBus windows need to have a
+	 * base address aligned on their size, and their size must be
+	 * a power of two. This means that if the BAR doesn't have a
+	 * power of two size, several MBus windows will actually be
+	 * created. We need to ensure that the biggest MBus window
+	 * (which will be the first one) is aligned on its size, which
+	 * explains the rounddown_pow_of_two() being done here.
 	 */
 	if (res->flags & IORESOURCE_IO)
-		return round_up(start, max_t(resource_size_t, SZ_64K, size));
+		return round_up(start, max_t(resource_size_t, SZ_64K,
+					     rounddown_pow_of_two(size)));
 	else if (res->flags & IORESOURCE_MEM)
-		return round_up(start, max_t(resource_size_t, SZ_1M, size));
+		return round_up(start, max_t(resource_size_t, SZ_1M,
+					     rounddown_pow_of_two(size)));
 	else
 		return start;
 }

Signature · 1 line

1.9.2

Hello,

This set of commits fixes a number of problems in the PCIe support of the Armada 370 and Armada XP SoCs, allowing to use PCIe devices that were not properly supported until now.

Due to the interaction of PCIe with other subsystems, the fixes are not limited to drivers/pci, but also touch drivers/bus and drivers/irqchip.

Here are the details of the patches:

* The first three patches are fixes in the MSI handling. They fix problems with PCIe device drivers trying to use MSI-X (which we don't support), and incorrect freeing of MSIs causing kernel panics when PCIe device drivers try to allocate/free MSIs several times.

They touch drivers/irqchip/ only, and they are independent from the rest of the series, both from a build and a runtime point of view.

These bugs exist since the MSI support was added, in v3.13. The commits carry the necessary Fixes and Cc to stable informations.

* The fourth patch fixes an off-by-one in the computed size of MBus windows. This only worked because the mvebu-mbus driver was silently accepting invalid sizes. I've marked it for stable because it really is bug, but even though it's not visible by itself, it is needed for other patches in the series.

This patch touches drivers/pci/host, and should probably be taken by the mvebu maintainers, with the Ack of the PCI maintainer, as it is a runtime dependency for the next patch.

This patch is marked for stable all the way to when the PCI driver was introduced.

* The fifth patch make the mvebu-mbus driver check (and loudly complain) if it is asked to create invalid regions (whose base address or size are not compatible with the documented MBus requirements).

This patch touches drivers/bus, and should probably be taken by the mvebu maintainers. As explained above, it is mandatory to have PATCH 4 applied before PATCH 5: there is a runtime dependency between the two (but no build time dependency).

This patch is not marked for stable, as it only adds some additional debugging information, which while very useful, is not technically stable material.

* The sixth patch relaxes a check in mvebu-mbus driver about conflicting windows, which is necessary

This patch touches drivers/bus, and should probably be taken by the mvebu maintainers. It is not a fix by itself, but it is required for the last patch, which is a fix. Therefore, it is marked for stable, all the way to when the mvebu-mbus driver was introduced.

* The seventh patch improves the pci-mvebu driver to allow the creation of multiple MBus windows when the PCI BAR calculated by the Linux PCI core has a non power-of-two size. This is needed because MBus windows can only have power-of-two sizes. Until now, we were configuring invalid MBus windows, which with certain PCIe devices, was either causing kernel panics at boot time or misfunction of the device.

Jason Gunthorpe (1):
bus: mvebu-mbus: Avoid setting an undefined window size

Neil Greatorex (1):
irqchip: armada-370-xp: Fix releasing of MSIs

Thomas Petazzoni (4):
irqchip: armada-370-xp: fix invalid cast of signed value into unsigned
variable
irqchip: armada-370-xp: implement the ->check_device() msi_chip
operation
bus: mvebu-mbus: allow several windows with the same target/attribute
pci: pci-mvebu: split PCIe BARs into multiple MBus windows when needed

Willy Tarreau (1):
pci: mvebu: fix off-by-one in the computed size of the mbus windows

 drivers/bus/mvebu-mbus.c            | 22 ++++++---
 drivers/irqchip/irq-armada-370-xp.c | 17 +++++--
 drivers/pci/host/pci-mvebu.c        | 92 ++++++++++++++++++++++++++++++-------
 3 files changed, 106 insertions(+), 25 deletions(-)

Hello,

This set of commits fixes a number of problems in the PCIe support of the Armada 370 and Armada XP SoCs, allowing to use PCIe devices that were not properly supported until now.

Due to the interaction of PCIe with other subsystems, the fixes are not limited to drivers/pci, but also touch drivers/bus and drivers/irqchip.

I've tentatively pulled this series as follows:

patches 1-3 into mvebu/drivers-irqchip-fixes

patches 4-7 into mvebu/drivers-mbus_pci-fixes

with the latter depending on the former. I'm going to hold them in -next for a bit. At least until we get an Ack from Bjorn. Then I'll send them their appropriate ways.

Quoted 5 lines · Jason Cooper

I've tentatively pulled this series as follows:

patches 1-3 into mvebu/drivers-irqchip-fixes

patches 4-7 into mvebu/drivers-mbus_pci-fixes

Thanks!

Quoted 3 lines

with the latter depending on the former. I'm going to hold them in -next for a bit. At least until we get an Ack from Bjorn. Then I'll send them their appropriate ways.

There is actually no need for a dependency between the two. Neither from a build point of view, or a runtime point of view. 1-3 fix purely MSI related things, while 4-7 fix purely MBus/PCI related things. I don't if not having a dependency will make things easier for you, but I thought it was worth mentioning. The only reason why all those patches were together is that, as a whole, they allow to fix a number of problems with certain PCIe devices.