ARM: cns3xxx: pci: avoid potential stack overflow

PATCH Superseded Arnd Bergmann 2 participants 6 messages May 19, 2015 – Oct 8, 2015

Subsystems (3)

Arm (32-bit) Arm sub-architectures PCI

lore

Share & tools

lore

Arnd Bergmann

author

2015-05-19 15:14 UTC #0 lore raw

PATCH 1/1 ARM: cns3xxx: pci: avoid potential stack overflow

The cns3xxx_pcie_hw_init function uses excessive kernel stack space because of a hack that puts a fake struct pci_sys_data and struct pci_bus on the stack in order to call the generic pci_bus_read_config accessors, which causes a warning in ARM allmodconfig builds:

arch/arm/mach-cns3xxx/pcie.c:266:1: warning: the frame size of 1080 bytes is larger than 1024 bytes

This rewrites the code in question to use a private implementation of the config space access for the same purpose, getting rid of the local variables and the warning in the process. As part of this, we have to use an open-coded version of pci_bus_find_capability(), which unfortunately complicates the implementation.

Signed-off-by Arnd Bergmann <[email protected]>

arch/arm/mach-cns3xxx/pcie.c +49 ~~-35~~

index c622c306c390..f411664639a2 100644
@@ -106,6 +106,30 @@ static int cns3xxx_pci_read_config(struct pci_bus *bus, unsigned int devfn,
 	return ret;
 }
 
+static u32 cns3xxx_pci_raw_read_config(void __iomem *base, int where,
+				       int size)
+{
+	u32 mask = (0x1ull << (size * 8)) - 1;
+	int shift = (where % 4) * 8;
+
+	return (__raw_readl(base + (where & 0xffc)) >> shift) & mask;
+}
+
+static void cns3xxx_pci_raw_write_config(void __iomem *base, int where,
+					 int size, u32 val)
+{
+	u32 v;
+	u32 mask = (0x1ull << (size * 8)) - 1;
+	int shift = (where % 4) * 8;
+
+	v = __raw_readl(base + (where & 0xffc));
+
+	v &= ~(mask << shift);
+	v |= (val & mask) << shift;
+
+	__raw_writel(v, base + (where & 0xffc));
+}
+
 static int cns3xxx_pci_setup(int nr, struct pci_sys_data *sys)
 {
 	struct cns3xxx_pcie *cnspci = sysdata_to_cnspci(sys);

@@ -213,56 +237,46 @@ static void __init cns3xxx_pcie_check_link(struct cns3xxx_pcie *cnspci)
 
 static void __init cns3xxx_pcie_hw_init(struct cns3xxx_pcie *cnspci)
 {
-	int port = cnspci->port;
-	struct pci_sys_data sd = {
-		.private_data = cnspci,
-	};
-	struct pci_bus bus = {
-		.number = 0,
-		.ops = &cns3xxx_pcie_ops,
-		.sysdata = &sd,
-	};
+	void __iomem *regs = cnspci->host_regs;
 	u16 mem_base  = cnspci->res_mem.start >> 16;
 	u16 mem_limit = cnspci->res_mem.end   >> 16;
 	u16 io_base   = cnspci->res_io.start  >> 16;
 	u16 io_limit  = cnspci->res_io.end    >> 16;
-	u32 devfn = 0;
-	u8 tmp8;
 	u16 pos;
 	u16 dc;
 
-	pci_bus_write_config_byte(&bus, devfn, PCI_PRIMARY_BUS, 0);
-	pci_bus_write_config_byte(&bus, devfn, PCI_SECONDARY_BUS, 1);
-	pci_bus_write_config_byte(&bus, devfn, PCI_SUBORDINATE_BUS, 1);
+	cns3xxx_pci_raw_write_config(regs, PCI_PRIMARY_BUS, 1, 0);
+	cns3xxx_pci_raw_write_config(regs, PCI_SECONDARY_BUS, 1, 1);
+	cns3xxx_pci_raw_write_config(regs, PCI_SUBORDINATE_BUS, 1, 1);
 
-	pci_bus_read_config_byte(&bus, devfn, PCI_PRIMARY_BUS, &tmp8);
-	pci_bus_read_config_byte(&bus, devfn, PCI_SECONDARY_BUS, &tmp8);
-	pci_bus_read_config_byte(&bus, devfn, PCI_SUBORDINATE_BUS, &tmp8);
+	cns3xxx_pci_raw_read_config(regs, PCI_PRIMARY_BUS, 1);
+	cns3xxx_pci_raw_read_config(regs, PCI_SECONDARY_BUS, 1);
+	cns3xxx_pci_raw_read_config(regs, PCI_SUBORDINATE_BUS, 1);
 
-	pci_bus_write_config_word(&bus, devfn, PCI_MEMORY_BASE, mem_base);
-	pci_bus_write_config_word(&bus, devfn, PCI_MEMORY_LIMIT, mem_limit);
-	pci_bus_write_config_word(&bus, devfn, PCI_IO_BASE_UPPER16, io_base);
-	pci_bus_write_config_word(&bus, devfn, PCI_IO_LIMIT_UPPER16, io_limit);
+	cns3xxx_pci_raw_write_config(regs, PCI_MEMORY_BASE, 2, mem_base);
+	cns3xxx_pci_raw_write_config(regs, PCI_MEMORY_LIMIT, 2, mem_limit);
+	cns3xxx_pci_raw_write_config(regs, PCI_IO_BASE_UPPER16, 2, io_base);
+	cns3xxx_pci_raw_write_config(regs, PCI_IO_LIMIT_UPPER16, 2, io_limit);
 
 	if (!cnspci->linked)
 		return;
 
+	regs = cnspci->cfg0_regs + (PCI_DEVFN(1, 0) << 12);
+
 	/* Set Device Max_Read_Request_Size to 128 byte */
-	bus.number = 1; /* directly connected PCIe device */
-	devfn = PCI_DEVFN(0, 0);
-	pos = pci_bus_find_capability(&bus, devfn, PCI_CAP_ID_EXP);
-	pci_bus_read_config_word(&bus, devfn, pos + PCI_EXP_DEVCTL, &dc);
-	if (dc & PCI_EXP_DEVCTL_READRQ) {
-		dc &= ~PCI_EXP_DEVCTL_READRQ;
-		pci_bus_write_config_word(&bus, devfn, pos + PCI_EXP_DEVCTL, dc);
-		pci_bus_read_config_word(&bus, devfn, pos + PCI_EXP_DEVCTL, &dc);
-		if (dc & PCI_EXP_DEVCTL_READRQ)
-			pr_warn("PCIe: Unable to set device Max_Read_Request_Size\n");
-		else
-			pr_info("PCIe: Max_Read_Request_Size set to 128 bytes\n");
-	}
+	pos = cns3xxx_pci_raw_read_config(regs, PCI_CAPABILITY_LIST, 1);
+	while (cns3xxx_pci_raw_read_config(regs, pos, 1) != PCI_CAP_ID_EXP)
+		pos = cns3xxx_pci_raw_read_config(regs, pos + PCI_CAP_LIST_NEXT, 1);
+
+	dc = cns3xxx_pci_raw_read_config(regs, pos + PCI_EXP_DEVCTL, 2);
+	dc &= ~(0x3 << 12);	/* Clear Device Control Register [14:12] */
+	cns3xxx_pci_raw_write_config(regs, pos + PCI_EXP_DEVCTL, 2, dc);
+	dc = cns3xxx_pci_raw_read_config(regs, pos + PCI_EXP_DEVCTL, 2);
+	if (!(dc & (0x3 << 12)))
+		pr_info("PCIe: Set Device Max_Read_Request_Size to 128 byte\n");
+
 	/* Disable PCIe0 Interrupt Mask INTA to INTD */
-	__raw_writel(~0x3FFF, MISC_PCIE_INT_MASK(port));
+	__raw_writel(~0x3FFF, MISC_PCIE_INT_MASK(cnspci->port));
 }
 
 static int cns3xxx_pcie_abort_handler(unsigned long addr, unsigned int fsr,

Krzysztof Hałasa

2015-05-27 06:45 UTC #1 lore raw

Quoted 6 lines · Arnd Bergmann

The cns3xxx_pcie_hw_init function uses excessive kernel stack space because of a hack that puts a fake struct pci_sys_data and struct pci_bus on the stack in order to call the generic pci_bus_read_config accessors, which causes a warning in ARM allmodconfig builds:

Quoted 8 lines

arch/arm/mach-cns3xxx/pcie.c:266:1: warning: the frame size of 1080 bytes is larger than 1024 bytes
This rewrites the code in question to use a private implementation of the config space access for the same purpose, getting rid of the local variables and the warning in the process. As part of this, we have to use an open-coded version of pci_bus_find_capability(), which unfortunately complicates the implementation.

Wouldn't it be better to simply use static structs for this purpose? The hack isn't pretty, though.

Review

Quoted 2 lines

(quoted diff) +2 -0

+	regs = cnspci->cfg0_regs + (PCI_DEVFN(1, 0) << 12);
+

Some comment about would be helpful. Such as this:

arch/arm/mach-cns3xxx/pcie.c static void __init cns3xxx_pcie_check_link(struct cns3xxx_pcie *cnspci) · line 251

Quoted 1 line

arch/arm/mach-cns3xxx/pcie.c +0 -1
-	bus.number = 1; /* directly connected PCIe device */

                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

arch/arm/mach-cns3xxx/pcie.c static void __init cns3xxx_pcie_check_link(struct cns3xxx_pcie *cnspci) · line 270

Quoted 4 lines

arch/arm/mach-cns3xxx/pcie.c +4 -0

+	pos = cns3xxx_pci_raw_read_config(regs, PCI_CAPABILITY_LIST, 1);
+	while (cns3xxx_pci_raw_read_config(regs, pos, 1) != PCI_CAP_ID_EXP)
+		pos = cns3xxx_pci_raw_read_config(regs, pos + PCI_CAP_LIST_NEXT, 1);
+

I wonder if this can fail (i.e., no PCI_CAP_ID_EXP capability).

arch/arm/mach-cns3xxx/pcie.c static void __init cns3xxx_pcie_check_link(struct cns3xxx_pcie *cnspci) · line 277

Quoted 7 lines

arch/arm/mach-cns3xxx/pcie.c +7 -0

+	dc = cns3xxx_pci_raw_read_config(regs, pos + PCI_EXP_DEVCTL, 2);
+	dc &= ~(0x3 << 12);	/* Clear Device Control Register [14:12] */
+	cns3xxx_pci_raw_write_config(regs, pos + PCI_EXP_DEVCTL, 2, dc);
+	dc = cns3xxx_pci_raw_read_config(regs, pos + PCI_EXP_DEVCTL, 2);
+	if (!(dc & (0x3 << 12)))
+		pr_info("PCIe: Set Device Max_Read_Request_Size to 128 byte\n");
+

This seems to revert 367dc4b75f4349d5363bc3ebdc030939db944786. Why do you want to do it?

Signature · 4 lines

Krzysztof Halasa

Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Arnd Bergmann

author

2015-10-07 20:01 UTC #2 lore raw

Quoted 17 lines · Krzysztof Ha?asa

Quoted 6 lines · Arnd Bergmann
The cns3xxx_pcie_hw_init function uses excessive kernel stack space because of a hack that puts a fake struct pci_sys_data and struct pci_bus on the stack in order to call the generic pci_bus_read_config accessors, which causes a warning in ARM allmodconfig builds:
Quoted 8 lines
arch/arm/mach-cns3xxx/pcie.c:266:1: warning: the frame size of 1080 bytes is larger than 1024 bytes
This rewrites the code in question to use a private implementation of the config space access for the same purpose, getting rid of the local variables and the warning in the process. As part of this, we have to use an open-coded version of pci_bus_find_capability(), which unfortunately complicates the implementation.

Quoted 2 lines

Wouldn't it be better to simply use static structs for this purpose? The hack isn't pretty, though.

Hi Krzysztof,

sorry for the late reply. I sent the patch shortly before my parental leave and have only now picked up this work again.

I've looked at the driver once more and have come up with a modified approach that should hopefully address all the concerns.

Quoted 8 lines

Review
Quoted 2 lines
(quoted diff) +2 -0
+	regs = cnspci->cfg0_regs + (PCI_DEVFN(1, 0) << 12);
+
Some comment about would be helpful. Such as this:
arch/arm/mach-cns3xxx/pcie.c static void __init cns3xxx_pcie_check_link(struct cns3xxx_pcie *cnspci) · line 251
Quoted 1 line
arch/arm/mach-cns3xxx/pcie.c +0 -1
-	bus.number = 1; /* directly connected PCIe device */
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Agreed. I think I've managed to get rid of this part completely though.

Quoted 6 lines

arch/arm/mach-cns3xxx/pcie.c static void __init cns3xxx_pcie_check_link(struct cns3xxx_pcie *cnspci) · line 270
Quoted 4 lines
arch/arm/mach-cns3xxx/pcie.c +4 -0
+	pos = cns3xxx_pci_raw_read_config(regs, PCI_CAPABILITY_LIST, 1);
+	while (cns3xxx_pci_raw_read_config(regs, pos, 1) != PCI_CAP_ID_EXP)
+		pos = cns3xxx_pci_raw_read_config(regs, pos + PCI_CAP_LIST_NEXT, 1);
+
I wonder if this can fail (i.e., no PCI_CAP_ID_EXP capability).

This is now gone too, so we no longer have to worry about it. I was mistakingly assuming that these were registers inside of the SoC rather than in the device that gets attached.

Quoted 10 lines

arch/arm/mach-cns3xxx/pcie.c static void __init cns3xxx_pcie_check_link(struct cns3xxx_pcie *cnspci) · line 277
Quoted 7 lines
arch/arm/mach-cns3xxx/pcie.c +7 -0
+	dc = cns3xxx_pci_raw_read_config(regs, pos + PCI_EXP_DEVCTL, 2);
+	dc &= ~(0x3 << 12);	/* Clear Device Control Register [14:12] */
+	cns3xxx_pci_raw_write_config(regs, pos + PCI_EXP_DEVCTL, 2, dc);
+	dc = cns3xxx_pci_raw_read_config(regs, pos + PCI_EXP_DEVCTL, 2);
+	if (!(dc & (0x3 << 12)))
+		pr_info("PCIe: Set Device Max_Read_Request_Size to 128 byte\n");
+
This seems to revert 367dc4b75f4349d5363bc3ebdc030939db944786. Why do you want to do it?

My mistake. Gone too now.

Thanks for your careful review back then!

Arnd

Arnd Bergmann

author

2015-10-07 20:05 UTC #3 lore raw

PATCH v2 1/1 ARM: cns3xxx: pci: avoid potential stack overflow

arch/arm/mach-cns3xxx/pcie.c:266:1: warning: the frame size of 1080 bytes is larger than 1024 bytes

I've spent a few hours trying to find out what exactly this code is wants to achieve here. The obvious part is setting up the host_regs using config space accessors, and this can simply be changed to use direct MMIO accesses, as I do in this patch.

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.

With those two changes in place, we no longer need the fake pci_sys_data/pci_bus structures for faking config space writes, and the stack usage goes down as well.

Signed-off-by Arnd Bergmann <[email protected]>

---
New approach based on Krzysztof's feedback about the previous version, should be cleaner now, but still needs critical review

 arch/arm/mach-cns3xxx/pcie.c | 71 +++++++++++++++++++++++++++++----------------------------------------
 1 file changed, 30 insertions(+), 41 deletions(-)

arch/arm/mach-cns3xxx/pcie.c +30 ~~-41~~

index c622c306c390..47905a50e075 100644
@@ -65,8 +65,9 @@ static void __iomem *cns3xxx_pci_map_bus(struct pci_bus *bus,
 
 	/*
 	 * The CNS PCI bridge doesn't fit into the PCI hierarchy, though
-	 * we still want to access it. For this to work, we must place
-	 * the first device on the same bus as the CNS PCI bridge.
+	 * we still want to access it.
+	 * We place the host bridge on bus 0, and the directly connected
+	 * device on bus 1, slot 0.
 	 */
 	if (busno == 0) { /* internal PCIe bus, host bridge device */
 		if (devfn == 0) /* device# and function# are ignored by hw */

@@ -211,58 +212,46 @@ static void __init cns3xxx_pcie_check_link(struct cns3xxx_pcie *cnspci)
 	}
 }
 
+static void cns3xxx_write_config(struct cns3xxx_pcie *cnspci,
+					 int where, int size, u32 val)
+{
+	void __iomem *base = cnspci->host_regs + (where & 0xffc);
+	u32 v;
+	u32 mask = (0x1ull << (size * 8)) - 1;
+	int shift = (where % 4) * 8;
+
+	v = readl_relaxed(base + (where & 0xffc));
+
+	v &= ~(mask << shift);
+	v |= (val & mask) << shift;
+
+	writel_relaxed(v, base + (where & 0xffc));
+	readl_relaxed(base + (where & 0xffc));
+}
+
 static void __init cns3xxx_pcie_hw_init(struct cns3xxx_pcie *cnspci)
 {
-	int port = cnspci->port;
-	struct pci_sys_data sd = {
-		.private_data = cnspci,
-	};
-	struct pci_bus bus = {
-		.number = 0,
-		.ops = &cns3xxx_pcie_ops,
-		.sysdata = &sd,
-	};
 	u16 mem_base  = cnspci->res_mem.start >> 16;
 	u16 mem_limit = cnspci->res_mem.end   >> 16;
 	u16 io_base   = cnspci->res_io.start  >> 16;
 	u16 io_limit  = cnspci->res_io.end    >> 16;
-	u32 devfn = 0;
-	u8 tmp8;
-	u16 pos;
-	u16 dc;
-
-	pci_bus_write_config_byte(&bus, devfn, PCI_PRIMARY_BUS, 0);
-	pci_bus_write_config_byte(&bus, devfn, PCI_SECONDARY_BUS, 1);
-	pci_bus_write_config_byte(&bus, devfn, PCI_SUBORDINATE_BUS, 1);
 
-	pci_bus_read_config_byte(&bus, devfn, PCI_PRIMARY_BUS, &tmp8);
-	pci_bus_read_config_byte(&bus, devfn, PCI_SECONDARY_BUS, &tmp8);
-	pci_bus_read_config_byte(&bus, devfn, PCI_SUBORDINATE_BUS, &tmp8);
-
-	pci_bus_write_config_word(&bus, devfn, PCI_MEMORY_BASE, mem_base);
-	pci_bus_write_config_word(&bus, devfn, PCI_MEMORY_LIMIT, mem_limit);
-	pci_bus_write_config_word(&bus, devfn, PCI_IO_BASE_UPPER16, io_base);
-	pci_bus_write_config_word(&bus, devfn, PCI_IO_LIMIT_UPPER16, io_limit);
+	cns3xxx_write_config(cnspci, PCI_PRIMARY_BUS, 1, 0);
+	cns3xxx_write_config(cnspci, PCI_SECONDARY_BUS, 1, 1);
+	cns3xxx_write_config(cnspci, PCI_SUBORDINATE_BUS, 1, 1);
+	cns3xxx_write_config(cnspci, PCI_MEMORY_BASE, 2, mem_base);
+	cns3xxx_write_config(cnspci, PCI_MEMORY_LIMIT, 2, mem_limit);
+	cns3xxx_write_config(cnspci, PCI_IO_BASE_UPPER16, 2, io_base);
+	cns3xxx_write_config(cnspci, PCI_IO_LIMIT_UPPER16, 2, io_limit);
 
 	if (!cnspci->linked)
 		return;
 
 	/* Set Device Max_Read_Request_Size to 128 byte */
-	bus.number = 1; /* directly connected PCIe device */
-	devfn = PCI_DEVFN(0, 0);
-	pos = pci_bus_find_capability(&bus, devfn, PCI_CAP_ID_EXP);
-	pci_bus_read_config_word(&bus, devfn, pos + PCI_EXP_DEVCTL, &dc);
-	if (dc & PCI_EXP_DEVCTL_READRQ) {
-		dc &= ~PCI_EXP_DEVCTL_READRQ;
-		pci_bus_write_config_word(&bus, devfn, pos + PCI_EXP_DEVCTL, dc);
-		pci_bus_read_config_word(&bus, devfn, pos + PCI_EXP_DEVCTL, &dc);
-		if (dc & PCI_EXP_DEVCTL_READRQ)
-			pr_warn("PCIe: Unable to set device Max_Read_Request_Size\n");
-		else
-			pr_info("PCIe: Max_Read_Request_Size set to 128 bytes\n");
-	}
+	pcie_bus_config = PCIE_BUS_PEER2PEER;
+
 	/* Disable PCIe0 Interrupt Mask INTA to INTD */
-	__raw_writel(~0x3FFF, MISC_PCIE_INT_MASK(port));
+	__raw_writel(~0x3FFF, MISC_PCIE_INT_MASK(cnspci->port));
 }
 
 static int cns3xxx_pcie_abort_handler(unsigned long addr, unsigned int fsr,

Krzysztof Hałasa

Acked-by on v2 1/1

2015-10-08 10:03 UTC #4 lore raw

Hi,

Quoted 5 lines · Arnd Bergmann

With those two changes in place, we no longer need the fake pci_sys_data/pci_bus structures for faking config space writes, and the stack usage goes down as well.
 arch/arm/mach-cns3xxx/pcie.c | 71 +++++++++++++++++++++++++++++----------------------------------------

I'm ATM unable to test this change, but will do that at some point. Meanwhile, I guess there is nothing I can say against this patch. Thanks.

Acked-by Krzysztof Ha?asa <[email protected]>

Signature · 4 lines

Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Arnd Bergmann

author

2015-10-08 14:38 UTC #5 lore raw

Quoted 8 lines · On Thursday 08 October 2015 12:03:18 Krzysztof Ha?asa wrote:

Hi,
Quoted 3 lines · Arnd Bergmann
With those two changes in place, we no longer need the fake pci_sys_data/pci_bus structures for faking config space writes, and the stack usage goes down as well.

Quoted 8 lines

Quoted 1 line
 arch/arm/mach-cns3xxx/pcie.c | 71 +++++++++++++++++++++++++++++----------------------------------------
I'm ATM unable to test this change, but will do that at some point. Meanwhile, I guess there is nothing I can say against this patch. Thanks.
Acked-by Krzysztof Ha?asa <[email protected]>

Thanks! I've queued it up in next/fixes-non-critical for linux-4.4 now, if you find something wrong later, we can revert or fix up.

There is no urgency for this one, especially since you still want to test it, so no reason to put it in 4.3.

This was the last pre-2015 build warning for the ARM defconfigs and allmodconfig, all remaining warnings are regressions or new code, and I've submitted patches for those. Let's see if we can get to a warning-free kernel build in 4.4.

Arnd