[PATCH] Revert "ARM: cns3xxx: pci: avoid potential stack overflow"

[PATCH] Revert "ARM: cns3xxx: pci: avoid potential stack overflow"

[Bjorn Helgaas]

This reverts commit 498a92d42596a7a32c042319eb62a4c3d8081cf1.

Krzysztof reported that this change broke Cavium CNS3xxx, ARMv6 (Laguna
GW-2388) because the MRRS setting is never written to the hardware.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
CC: Arnd Bergmann <arnd@arndb.de>
CC: Krzysztof Hałasa <khalasa@piap.pl>
---
 arch/arm/mach-cns3xxx/pcie.c |   71 ++++++++++++++++++++++++------------------
 1 file changed, 41 insertions(+), 30 deletions(-)

diff --git a/arch/arm/mach-cns3xxx/pcie.c b/arch/arm/mach-cns3xxx/pcie.c
index 318394e..c622c30 100644
--- a/arch/arm/mach-cns3xxx/pcie.c
+++ b/arch/arm/mach-cns3xxx/pcie.c
@@ -65,9 +65,8 @@ 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.
-	 * We place the host bridge on bus 0, and the directly connected
-	 * device on bus 1, slot 0.
+	 * 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.
 	 */
 	if (busno == 0) { /* internal PCIe bus, host bridge device */
 		if (devfn == 0) /* device# and function# are ignored by hw */
@@ -212,46 +211,58 @@ 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);
-
-	v &= ~(mask << shift);
-	v |= (val & mask) << shift;
-
-	writel_relaxed(v, base);
-	readl_relaxed(base);
-}
-
 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);
 
-	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);
+	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);
 
 	if (!cnspci->linked)
 		return;
 
 	/* Set Device Max_Read_Request_Size to 128 byte */
-	pcie_bus_config = PCIE_BUS_PEER2PEER;
-
+	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");
+	}
 	/* Disable PCIe0 Interrupt Mask INTA to INTD */
-	__raw_writel(~0x3FFF, MISC_PCIE_INT_MASK(cnspci->port));
+	__raw_writel(~0x3FFF, MISC_PCIE_INT_MASK(port));
 }
 
 static int cns3xxx_pcie_abort_handler(unsigned long addr, unsigned int fsr,

Re: [PATCH] Revert "ARM: cns3xxx: pci: avoid potential stack overflow"

[Bjorn Helgaas]

[+cc Russell, linux-arm-kernel]

On Tue, May 31, 2016 at 04:58:02PM -0500, Bjorn Helgaas wrote:
> This reverts commit 498a92d42596a7a32c042319eb62a4c3d8081cf1.
> 
> Krzysztof reported that this change broke Cavium CNS3xxx, ARMv6 (Laguna
> GW-2388) because the MRRS setting is never written to the hardware.

Krzysztof, can you test this and see whether it fixes the problem for
you?

> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> CC: Arnd Bergmann <arnd@arndb.de>
> CC: Krzysztof Hałasa <khalasa@piap.pl>
> ---
>  arch/arm/mach-cns3xxx/pcie.c |   71 ++++++++++++++++++++++++------------------
>  1 file changed, 41 insertions(+), 30 deletions(-)
> 
> diff --git a/arch/arm/mach-cns3xxx/pcie.c b/arch/arm/mach-cns3xxx/pcie.c
> index 318394e..c622c30 100644
> --- a/arch/arm/mach-cns3xxx/pcie.c
> +++ b/arch/arm/mach-cns3xxx/pcie.c
> @@ -65,9 +65,8 @@ 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.
> -	 * We place the host bridge on bus 0, and the directly connected
> -	 * device on bus 1, slot 0.
> +	 * 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.
>  	 */
>  	if (busno == 0) { /* internal PCIe bus, host bridge device */
>  		if (devfn == 0) /* device# and function# are ignored by hw */
> @@ -212,46 +211,58 @@ 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);
> -
> -	v &= ~(mask << shift);
> -	v |= (val & mask) << shift;
> -
> -	writel_relaxed(v, base);
> -	readl_relaxed(base);
> -}
> -
>  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);
>  
> -	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);
> +	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);
>  
>  	if (!cnspci->linked)
>  		return;
>  
>  	/* Set Device Max_Read_Request_Size to 128 byte */
> -	pcie_bus_config = PCIE_BUS_PEER2PEER;
> -
> +	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");
> +	}
>  	/* Disable PCIe0 Interrupt Mask INTA to INTD */
> -	__raw_writel(~0x3FFF, MISC_PCIE_INT_MASK(cnspci->port));
> +	__raw_writel(~0x3FFF, MISC_PCIE_INT_MASK(port));
>  }
>  
>  static int cns3xxx_pcie_abort_handler(unsigned long addr, unsigned int fsr,
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Revert "ARM: cns3xxx: pci: avoid potential stack overflow"

[Krzysztof Hałasa]

Bjorn Helgaas <bhelgaas@google.com> writes:

> This reverts commit 498a92d42596a7a32c042319eb62a4c3d8081cf1.
>
> Krzysztof reported that this change broke Cavium CNS3xxx, ARMv6 (Laguna
> GW-2388) because the MRRS setting is never written to the hardware.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> CC: Arnd Bergmann <arnd@arndb.de>
> CC: Krzysztof Hałasa <khalasa@piap.pl>
> ---
>  arch/arm/mach-cns3xxx/pcie.c |   71 ++++++++++++++++++++++++------------------
>  1 file changed, 41 insertions(+), 30 deletions(-)

This, applied to v4.7-rc1, fixes the problem on my Laguna boards.

Tested-by: Krzysztof Hałasa <khalasa@piap.pl>

And as well

Acked-by: Krzysztof Hałasa <khalasa@piap.pl>

Thanks.
-- 
Krzysztof Halasa

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

Re: [PATCH] Revert "ARM: cns3xxx: pci: avoid potential stack overflow"

[Arnd Bergmann]

On Wednesday, June 1, 2016 1:08:59 PM CEST Krzysztof Hałasa wrote:
> Bjorn Helgaas <bhelgaas@google.com> writes:
> 
> > This reverts commit 498a92d42596a7a32c042319eb62a4c3d8081cf1.
> >
> > Krzysztof reported that this change broke Cavium CNS3xxx, ARMv6 (Laguna
> > GW-2388) because the MRRS setting is never written to the hardware.
> >
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > CC: Arnd Bergmann <arnd@arndb.de>
> > CC: Krzysztof Hałasa <khalasa@piap.pl>
> > ---
> >  arch/arm/mach-cns3xxx/pcie.c |   71 ++++++++++++++++++++++++------------------
> >  1 file changed, 41 insertions(+), 30 deletions(-)
> 
> This, applied to v4.7-rc1, fixes the problem on my Laguna boards.
> 
> Tested-by: Krzysztof Hałasa <khalasa@piap.pl>
> 
> And as well
> 
> Acked-by: Krzysztof Hałasa <khalasa@piap.pl>

Thank!

Obviously, I'd rather not bring back the gcc warning or the potential
stack overflow (however unlikely).

What exactly is the problem we are seeing, and is there a way to fix
it on top of my patch? Are we perhaps just missing a call to
pcie_bus_configure_settings()?

Note that cns3xxx is in a bit of an odd state, as only half of the
platform code is even present in the kernel, and there is no effort
to change that. As far as I know, the board that this was tested on
is not present in the mainline kernel, and the board we support
is a development system that few people even own at this point.

	Arnd

Re: [PATCH] Revert "ARM: cns3xxx: pci: avoid potential stack overflow"

[Krzysztof Hałasa]

Arnd Bergmann <arnd@arndb.de> writes:

> What exactly is the problem we are seeing, and is there a way to fix
> it on top of my patch? Are we perhaps just missing a call to
> pcie_bus_configure_settings()?

From: khalasa@piap.pl (Krzysztof Halasa)
Subject: [PATCH] Extend PCIE_BUS_PEER2PEER to set MRSS=128 to fix CNS3xxx BM DMA.
To: Bjorn Helgaas <bhelgaas@google.com>
Cc: Arnd Bergmann <arnd@arndb.de>, linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org
Date: Mon, 21 Mar 2016 10:39:52 +0100 (11 weeks, 2 days, 19 hours ago)

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

Signed-off-by: Krzysztof Hałasa <khalasa@piap.pl>
Fixes: 498a92d42596 ("ARM: cns3xxx: pci: avoid potential stack overflow")

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 6d7ab9b..91713b6 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -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
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 2771625..6f5088a 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -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;

> Note that cns3xxx is in a bit of an odd state, as only half of the
> platform code is even present in the kernel, and there is no effort
> to change that. As far as I know, the board that this was tested on
> is not present in the mainline kernel, and the board we support
> is a development system that few people even own at this point.

The boards I use (Gateworks Laguna) are basically equivalent to the
devel board (from the platform code POV).
The kernel lacks support for SMP and the Ethernet driver (and things
like GPIO), though there are patches available and I plan to integrate
them, when the existing issues are resolved.

Also, this is practically a non-DT arch but I guess a conversion to DT
would be a good thing as it would eliminate a need for board-specific
code. That's why there is no platform code for Laguna. Unfortunately
there is no DT file for CNS3xxx, and I'm not an DT expert.
-- 
Krzysztof Halasa

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

Re: [PATCH] Revert "ARM: cns3xxx: pci: avoid potential stack overflow"

[Arnd Bergmann]

On Thursday, June 9, 2016 7:42:24 AM CEST Krzysztof Hałasa wrote:
> Arnd Bergmann <arnd@arndb.de> writes:
> 
> > What exactly is the problem we are seeing, and is there a way to fix
> > it on top of my patch? Are we perhaps just missing a call to
> > pcie_bus_configure_settings()?
> 
> From: khalasa@piap.pl (Krzysztof Halasa)
> Subject: [PATCH] Extend PCIE_BUS_PEER2PEER to set MRSS=128 to fix CNS3xxx BM DMA.
> To: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Arnd Bergmann <arnd@arndb.de>, linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org
> Date: Mon, 21 Mar 2016 10:39:52 +0100 (11 weeks, 2 days, 19 hours ago)
> 
> 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
> 
> Signed-off-by: Krzysztof Hałasa <khalasa@piap.pl>
> Fixes: 498a92d42596 ("ARM: cns3xxx: pci: avoid potential stack overflow")

I see now, thanks for the quote. I guess I missed how PCIE_BUS_PEER2PEER is
documented as /* set MPS = 128 for all devices */ unlike PCIE_BUS_PERFORMANCE,
which is documented as setting both MPS and MRRS.

It seems the current behavior was introduced by
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=ed2888e90

Before that, we were always setting both mrrs and mps. As we don't know
who uses PCIE_BUS_PEER2PEER, maybe another option would be to add yet
another pcie_bus_config value for this particular quirk?

> > Note that cns3xxx is in a bit of an odd state, as only half of the
> > platform code is even present in the kernel, and there is no effort
> > to change that. As far as I know, the board that this was tested on
> > is not present in the mainline kernel, and the board we support
> > is a development system that few people even own at this point.
> 
> The boards I use (Gateworks Laguna) are basically equivalent to the
> devel board (from the platform code POV).
> The kernel lacks support for SMP and the Ethernet driver (and things
> like GPIO), though there are patches available and I plan to integrate
> them, when the existing issues are resolved.

Ok, good to know.

> Also, this is practically a non-DT arch but I guess a conversion to DT
> would be a good thing as it would eliminate a need for board-specific
> code. That's why there is no platform code for Laguna. Unfortunately
> there is no DT file for CNS3xxx, and I'm not an DT expert.

I started the DT conversion a long time ago (see the DT parsing in
arch/arm/mach-cns3xxx/core.c) but I never had any hardware to test
on, and it was at a time when we didn't even have DT support in all
the subsystems.

I'd definitely help you get the rest of the DT support in place if
you can test it. This is now the only SMP platform and one of
the last users of GIC and l2x0 that does not use DT, so I'd love
to see that converted just so we can remove the legacy probing from
those drivers.

Converting what we have in mainline should be fairly straightforward,
but there is more code in 
target/linux/cns3xxx/files/arch/arm/mach-cns3xxx/laguna.c that requires
more work, in particular we need to come up with a way to handle
the laguna_net_data and laguna_info structures, which have some of
the same data that is normall in DT. Also, the gpio driver doesn't
have a trivial conversion to DT and requires some work to define
a binding and implement that.

	Arnd

Re: [PATCH] Revert "ARM: cns3xxx: pci: avoid potential stack overflow"

[Krzysztof Hałasa]

Arnd Bergmann <arnd@arndb.de> writes:

> Before that, we were always setting both mrrs and mps. As we don't know
> who uses PCIE_BUS_PEER2PEER, maybe another option would be to add yet
> another pcie_bus_config value for this particular quirk?

It would be a safe approach.
Or, maybe another non-pcie_bus_config thing, I don't know (so
the pcie_bus_config is left for the user).

> I started the DT conversion a long time ago (see the DT parsing in
> arch/arm/mach-cns3xxx/core.c) but I never had any hardware to test
> on, and it was at a time when we didn't even have DT support in all
> the subsystems.
>
> I'd definitely help you get the rest of the DT support in place if
> you can test it. This is now the only SMP platform and one of
> the last users of GIC and l2x0 that does not use DT, so I'd love
> to see that converted just so we can remove the legacy probing from
> those drivers.

Ok. Is there a DT skeleton file somewhere, so I can try to boot the
board (without Laguna extras) in DT mode?
At first, I only need CPU + RAM + console serial port.

> Converting what we have in mainline should be fairly straightforward,
> but there is more code in 
> target/linux/cns3xxx/files/arch/arm/mach-cns3xxx/laguna.c that requires
> more work, in particular we need to come up with a way to handle
> the laguna_net_data and laguna_info structures, which have some of
> the same data that is normall in DT.

I assume adding this to U-Boot should be acceptable (for Gateworks,
too). They are already doing this to their i.MX6 line Ventana.

> Also, the gpio driver doesn't
> have a trivial conversion to DT and requires some work to define
> a binding and implement that.

GPIO is a bit less important ATM, since the boards can boot without it.
-- 
Krzysztof Halasa

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

Re: [PATCH] Revert "ARM: cns3xxx: pci: avoid potential stack overflow"

[Arnd Bergmann]

On Friday, June 10, 2016 12:10:14 PM CEST Krzysztof Hałasa wrote:
> Arnd Bergmann <arnd@arndb.de> writes:
> 
> > Before that, we were always setting both mrrs and mps. As we don't know
> > who uses PCIE_BUS_PEER2PEER, maybe another option would be to add yet
> > another pcie_bus_config value for this particular quirk?
> 
> It would be a safe approach.
> Or, maybe another non-pcie_bus_config thing, I don't know (so
> the pcie_bus_config is left for the user).
> 
> > I started the DT conversion a long time ago (see the DT parsing in
> > arch/arm/mach-cns3xxx/core.c) but I never had any hardware to test
> > on, and it was at a time when we didn't even have DT support in all
> > the subsystems.
> >
> > I'd definitely help you get the rest of the DT support in place if
> > you can test it. This is now the only SMP platform and one of
> > the last users of GIC and l2x0 that does not use DT, so I'd love
> > to see that converted just so we can remove the legacy probing from
> > those drivers.
> 
> Ok. Is there a DT skeleton file somewhere, so I can try to boot the
> board (without Laguna extras) in DT mode?
> At first, I only need CPU + RAM + console serial port.

I'd start by copying the relevant nodes from
arch/arm/boot/dts/arm-realview-pb11mp.dts, which is the closest
I can think of. I've put together something completely untested
below.

The key part is to have the correct "compatible" property in the
root node, which must list the actual machine before listing
"cavium,cns3420" to match the machine descriptor in
arch/arm/mach-cns3xxx/core.c

The RAM will be filled from the atags compatibility code
(but it makes sense to list it anyway), and the serial port
is not even needed in the first iteration if you heave
CONFIG_DEBUG_CNS3XXX set.


> > Converting what we have in mainline should be fairly straightforward,
> > but there is more code in 
> > target/linux/cns3xxx/files/arch/arm/mach-cns3xxx/laguna.c that requires
> > more work, in particular we need to come up with a way to handle
> > the laguna_net_data and laguna_info structures, which have some of
> > the same data that is normall in DT.
> 
> I assume adding this to U-Boot should be acceptable (for Gateworks,
> too). They are already doing this to their i.MX6 line Ventana.

Ok, if U-Boot can convert the configuration into the right DT
properties, that is ideal.

> > Also, the gpio driver doesn't
> > have a trivial conversion to DT and requires some work to define
> > a binding and implement that.
> 
> GPIO is a bit less important ATM, since the boards can boot without it.

Ok.


	Arnd

---
/dts-v1/;
#include <dt-bindings/interrupt-controller/irq.h>
#include <dt-bindings/gpio/gpio.h>
#include "skeleton.dtsi"

/ {
	model = "Cavium CNS3420 validation board";
	compatible = "cavium,cns3420";
	interrupt-parent = <&gic>;

	chosen { };

	aliases {
		serial0 = &serial;
	};

	memory {
		/* 256MB at address 0 */
		reg = <0x00000000 0x10000000>;
	};

	cpus {
		#address-cells = <1>;
		#size-cells = <0>;
		enable-method = "cavium,cns3420-smp";

		cpu@0 {
			device_type = "cpu";
			compatible = "arm,arm11mpcore";
			reg = <0>;
			next-level-cache = <&L2>;
		};

		cpu@1 {
			device_type = "cpu";
			compatible = "arm,arm11mpcore";
			reg = <1>;
			next-level-cache = <&L2>;
		};
	};

	/* Primary TestChip GIC synthesized with the CPU */
	gic: interrupt-controller@1f000100 {
		compatible = "arm,arm11mp-gic";
		#interrupt-cells = <3>;
		#address-cells = <1>;
		interrupt-controller;
		reg = <0x90001000 0x1000>,
		      <0x90000100 0x100>;
	};

	L2: l2-cache {
		compatible = "arm,l220-cache";
		reg = <0x92002000 0x1000>;
		interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
		cache-unified;
		cache-level = <2>;
		/* all of the below are probably wrong and
		   have to be fixed before we can use l2x0_of_init */
		cache-size = <1048576>; // 1MB
		cache-sets = <4096>;
		cache-line-size = <32>;
		arm,shared-override;
		arm,parity-enable;
		arm,outer-sync-disable;
	};

	scu@1f000000 {
		compatible = "arm,arm11mp-scu";
		reg = <0x90000000 0x100>;
	};

	flash@10000000 {
		/* 128MiB NOR Flash memory */
		compatible = "cfi-flash";
		reg = <0x10000000 0x08000000>;
		bank-width = <2>;

		partition@00000000 {
			label = "uboot";
			reg = <0 0x00040000>;
		};

		partition@00040000 {
			label = "kernel";
			reg = <0x00040000 0x004c0000>;
        	};

		partition@00500000 {
			label = "filesystem";
			reg = <0x00500000 0x07000000>;
        	};

		partition@07500000 {
			label = "filesystem2";
			reg = <0x07500000 0x00ae0000>;
        	};

		partition@07fe0000 {
			label = "ubootenv";
			reg = <0x07fe0000 0x00020000>;
        	};
        };

	soc {
		#address-cells = <1>;
		#size-cells = <1>;
		compatible = "simple-bus";
		ranges;

		serial0: serial@78000000 {
			compatible = "ns16550a";
			reg = <0x78000000 0x1000>;
			interrupts = <GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>;
			regshift = <2>;
			clocks = <&uartclk>;
		};
	};
};

Re: [PATCH] Revert "ARM: cns3xxx: pci: avoid potential stack overflow"

[Krzysztof Hałasa]

Arnd Bergmann <arnd@arndb.de> writes:

> I'd start by copying the relevant nodes from
> arch/arm/boot/dts/arm-realview-pb11mp.dts, which is the closest
> I can think of. I've put together something completely untested
> below.

Thanks. I will try to handle that. 3+ weeks, unfortunately.

Now, what do we do with the PCIe MRRS?
-- 
Krzysztof Halasa

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

Re: [PATCH] Revert "ARM: cns3xxx: pci: avoid potential stack overflow"

[Bjorn Helgaas]

On Wed, Jun 01, 2016 at 11:09:47PM +0200, Arnd Bergmann wrote:
> On Wednesday, June 1, 2016 1:08:59 PM CEST Krzysztof Hałasa wrote:
> > Bjorn Helgaas <bhelgaas@google.com> writes:
> > 
> > > This reverts commit 498a92d42596a7a32c042319eb62a4c3d8081cf1.
> > >
> > > Krzysztof reported that this change broke Cavium CNS3xxx, ARMv6 (Laguna
> > > GW-2388) because the MRRS setting is never written to the hardware.
> > >
> > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > > CC: Arnd Bergmann <arnd@arndb.de>
> > > CC: Krzysztof Hałasa <khalasa@piap.pl>
> > > ---
> > >  arch/arm/mach-cns3xxx/pcie.c |   71 ++++++++++++++++++++++++------------------
> > >  1 file changed, 41 insertions(+), 30 deletions(-)
> > 
> > This, applied to v4.7-rc1, fixes the problem on my Laguna boards.
> > 
> > Tested-by: Krzysztof Hałasa <khalasa@piap.pl>
> > 
> > And as well
> > 
> > Acked-by: Krzysztof Hałasa <khalasa@piap.pl>
> 
> Thank!
> 
> Obviously, I'd rather not bring back the gcc warning or the potential
> stack overflow (however unlikely).
> 
> What exactly is the problem we are seeing, and is there a way to fix
> it on top of my patch? Are we perhaps just missing a call to
> pcie_bus_configure_settings()?
> 
> Note that cns3xxx is in a bit of an odd state, as only half of the
> platform code is even present in the kernel, and there is no effort
> to change that. As far as I know, the board that this was tested on
> is not present in the mainline kernel, and the board we support
> is a development system that few people even own at this point.

I'm sure there's a good, simple fix for this that would be better than
the revert.  Unfortunately I don't have time to work on it right now.
I'll be on vacation for the last 2-3 weeks before v4.7 releases, so
I'm scrambling to get the big chunks merged before 6/22 or so.

Bjorn