CNS3xxx: 3 regressions identified in v3.14-rc4+

ARM Superseded Krzysztof Hałasa 2 participants 12 messages Feb 28, 2014 – Mar 18, 2014

Subsystems (2)

Arm (32-bit) Arm sub-architectures

lore

Share & tools

lore

Krzysztof Hałasa

author

2014-02-28 11:00 UTC #0 lore raw

Hello,

Linux version 3.14.0-rc4+ (current tip, no extra patches), CPU is Cavium Econa CNS3420, board is Gateworks Laguna GW2388-4 (masqueraded as CNS3420VB).

Issue #1 ###################################################

kernel BUG at mm/vmalloc.c:1132!
PC is at vm_area_add_early+0x20/0x84
LR is at add_static_vm_early+0xc/0x60

The problem is cns3xxx_pcie_init() (device_initcall) calls the "early" iotable_init(). I'll attach the patch.

Issue #2 ###################################################

PCI hangs system completely while trying to access any PCI MMIO region (plain IO not tested).

The guilty commit is 928bea964827d7824b548c1f8e06eccbbc4d0d7d:

PCI: Delay enabling bridges until they're needed

We currently enable PCI bridges after scanning a bus and assigning resources. This is often done in arch code.

This patch changes this so we don't enable a bridge until necessary, i.e., until we enable a PCI device behind the bridge. We do this in the generic pci_enable_device() path, so this also removes the arch-specific code to enable bridges.

Reverting changes in arch/arm/kernel/bios32.c, drivers/pci/bus.c and include/linux/pci.h (= essentially adding pci_enable_bridges(bus) in ARM's pci_common_init_dev()) makes it work again.

Options?

Issue #3 ###################################################

NR_IRQS:16 nr_irqs:96 96

WARNING: at drivers/irqchip/irq-gic.c:952 gic_init_bases+0xe4/0x2b8()
Cannot allocate irq_descs @ IRQ16, assuming pre-allocated
Backtrace:
gic_init_bases    from cns3xxx_init_irq+0x24/0x34
cns3xxx_init_irq  from init_IRQ+0x24/0x2c
init_IRQ          from start_kernel+0x1a8/0x338

start_kernel from 0x2000806c

I'm having problems understanding how is machine_desc->nr_irqs supposed to work with CONFIG_ARM_GIC and CONFIG_SPARSE_IRQ set.

machine_desc->nr_irqs is set to NR_IRQS_CNS3XXX =
IRQ_TC11MP_GIC_START + 64 = 32 + 64 = 96.

At start, machine_desc->nr_irqs are pre-allocated via start_kernel() -> early_irq_init().

Then, gic_init(0, 29, ...) -> gic_init_bases(0, 29, ...) tries this (and fails):

	/*
	 * For primary GICs, skip over SGIs.
	 * For secondary GICs, skip over PPIs, too.
	 */
	irq_start = (effectively) 16;

	irq_base = irq_alloc_descs(irq_start, 16, gic_irqs, numa_node_id());
	if (IS_ERR_VALUE(irq_base)) {
		WARN(1, "Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n",
		     irq_start);

Does this mean machine_desc->nr_irqs is to be kept at 16 (NR_IRQS_LEGACY) or less, so it doesn't conflict with gic_init()?

Or perhaps gic_init() shouldn't warn about this?

Signature · 4 lines

Krzysztof Halasa

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

Krzysztof Hałasa

author

2014-02-28 11:12 UTC #1 lore raw

PATCH 1/1 CNS3xxx: Fix PCIe early iotable_init().

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

arch/arm/mach-cns3xxx/cns3420vb.c +1 -1

@@ -240,7 +240,7 @@ static void __init cns3420_map_io(void)
 {
 	cns3xxx_map_io();
 	iotable_init(cns3420_io_desc, ARRAY_SIZE(cns3420_io_desc));
-
+	cns3xxx_pcie_iotable_init();
 	cns3420_early_serial_setup();
 }

arch/arm/mach-cns3xxx/core.h +1 -0

@@ -14,6 +14,7 @@
 #include <linux/reboot.h>
 
 extern void cns3xxx_timer_init(void);
+extern void cns3xxx_pcie_iotable_init(void);
 
 #ifdef CONFIG_CACHE_L2X0
 void __init cns3xxx_l2x0_init(void);

arch/arm/mach-cns3xxx/pcie.c +9 -2

@@ -357,6 +357,15 @@ static int cns3xxx_pcie_abort_handler(unsigned long addr, unsigned int fsr,
 	return 0;
 }
 
+void __init cns3xxx_pcie_iotable_init()
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(cns3xxx_pcie); i++)
+		iotable_init(cns3xxx_pcie[i].cfg_bases,
+			     ARRAY_SIZE(cns3xxx_pcie[i].cfg_bases));
+}
+
 static int __init cns3xxx_pcie_init(void)
 {
 	int i;

@@ -368,8 +377,6 @@ static int __init cns3xxx_pcie_init(void)
 			"imprecise external abort");
 
 	for (i = 0; i < ARRAY_SIZE(cns3xxx_pcie); i++) {
-		iotable_init(cns3xxx_pcie[i].cfg_bases,
-			     ARRAY_SIZE(cns3xxx_pcie[i].cfg_bases));
 		cns3xxx_pwr_clk_en(0x1 << PM_CLK_GATE_REG_OFFSET_PCIE(i));
 		cns3xxx_pwr_soft_rst(0x1 << PM_SOFT_RST_REG_OFFST_PCIE(i));
 		cns3xxx_pcie_check_link(&cns3xxx_pcie[i]);

Arnd Bergmann

2014-02-28 11:15 UTC #2 lore raw

Re: [PATCH] CNS3xxx: Fix PCIe early iotable_init().

Quoted 2 lines · On Friday 28 February 2014 12:12:45 Krzysztof Ha?asa wrote:

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

Patch looks good, but please add the changeset description from your first patch.

Arnd

Krzysztof Hałasa

author

2014-03-04 06:46 UTC #3 lore raw

Re: [PATCH] CNS3xxx: Fix PCIe early iotable_init().

Quoted 2 lines · Arnd Bergmann

Patch looks good, but please add the changeset description from your first patch.

I wonder if another approach would be better. I don't like the .pfn messing and the bugs that function introduces.

Signature · 4 lines

Krzysztof Halasa

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

Arnd Bergmann

2014-02-28 11:31 UTC #4 lore raw

In reply to Krzysztof Hałasa

Quoted 16 lines · On Friday 28 February 2014 12:00:12 Krzysztof Ha?asa wrote:

PCI hangs system completely while trying to access any PCI MMIO region (plain IO not tested).
The guilty commit is 928bea964827d7824b548c1f8e06eccbbc4d0d7d:
PCI: Delay enabling bridges until they're needed
We currently enable PCI bridges after scanning a bus and assigning resources. This is often done in arch code.
This patch changes this so we don't enable a bridge until necessary, i.e., until we enable a PCI device behind the bridge. We do this in the generic pci_enable_device() path, so this also removes the arch-specific code to enable bridges.

Quoted 5 lines

Reverting changes in arch/arm/kernel/bios32.c, drivers/pci/bus.c and include/linux/pci.h (= essentially adding pci_enable_bridges(bus) in ARM's pci_common_init_dev()) makes it work again.
Options?

Hmm, this was in 3.12 already, right?

Can you try adding the code in a host specific callback such as hw->add_bus?

Quoted 35 lines

Issue #3 ###################################################
NR_IRQS:16 nr_irqs:96 96
WARNING: at drivers/irqchip/irq-gic.c:952 gic_init_bases+0xe4/0x2b8()
Cannot allocate irq_descs @ IRQ16, assuming pre-allocated
Backtrace:
gic_init_bases    from cns3xxx_init_irq+0x24/0x34
cns3xxx_init_irq  from init_IRQ+0x24/0x2c
init_IRQ          from start_kernel+0x1a8/0x338
start_kernel from 0x2000806c
I'm having problems understanding how is machine_desc->nr_irqs supposed to work with CONFIG_ARM_GIC and CONFIG_SPARSE_IRQ set.
machine_desc->nr_irqs is set to NR_IRQS_CNS3XXX =
IRQ_TC11MP_GIC_START + 64 = 32 + 64 = 96.
At start, machine_desc->nr_irqs are pre-allocated via start_kernel() -> early_irq_init().
Then, gic_init(0, 29, ...) -> gic_init_bases(0, 29, ...) tries this (and fails):
        /*
         * For primary GICs, skip over SGIs.
         * For secondary GICs, skip over PPIs, too.
         */
        irq_start = (effectively) 16;

        irq_base = irq_alloc_descs(irq_start, 16, gic_irqs, numa_node_id());
        if (IS_ERR_VALUE(irq_base)) {
                WARN(1, "Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n",
                     irq_start);

Quoted 4 lines

Does this mean machine_desc->nr_irqs is to be kept at 16 (NR_IRQS_LEGACY) or less, so it doesn't conflict with gic_init()?
Or perhaps gic_init() shouldn't warn about this?

I think the problem is that both the board file and the gic code try to set up legacy interrupts. Try just removing the 'nr_irqs=' line.

Arnd

Krzysztof Hałasa

author

2014-03-04 07:37 UTC #5 lore raw

PATCH v2 1/1 CNS3xxx: Fix PCIe early iotable_init().

In reply to Krzysztof Hałasa

This patch fixes the following BUG:

Quoted 6 lines

kernel BUG at mm/vmalloc.c:1132!
PC is at vm_area_add_early+0x20/0x84
LR is at add_static_vm_early+0xc/0x60
The problem is cns3xxx_pcie_init() (device_initcall) calls the "early" iotable_init().

Instead of merely calling the PCIe iotable_init() from machine_desc->map_io(), this patch adds the required mappings to the main CNS3xxx mapping table. This means we don't convert .pfn back to virtual addresses in pcie.c. The size of the address space consumed for PCIe control is reduced from 96 MiB (6 * 16 MiB) to about 32 MiB (this doesn't include MMIO address space required by PCI devices):

- Size of the PCIe "host" mapping is reduced from 16 MiB to 4 KiB. It's a PCI configuration address space for the local (on-chip) PCIe bridge.

- Size of the "CFG0" mapping is reduced from 16 MiB to 64 KiB. It's for Type 0 Configuration accesses, i.e., accesses to the single device (with possible "functions") on the other end of the PCIe link. We really only need a 4 KiB page at 0x8000 (see the offset calculation in cns3xxx_pci_cfg_base()). There is still a potential problem with PCI bus numbers > 0xF, are they supported?

- The code in cns3xxx_pci_cfg_base() and cns3xxx_pcie_hw_init() should be clearer now.

- The maximum address space allocated for PCI MMIO is now correctly shown in /proc/iomem as 176 MiB (per each of the two PCI "domains"
- previously only 16 MiB were reserved).

This patch has been tested on Gateworks Laguna board, masqueraded as CNS3420VB.

arch/arm/mach-cns3xxx/core.c +32 -0

@@ -47,6 +47,38 @@ static struct map_desc cns3xxx_io_desc[] __initdata = {
 		.pfn		= __phys_to_pfn(CNS3XXX_PM_BASE),
 		.length		= SZ_4K,
 		.type		= MT_DEVICE,
+#ifdef CONFIG_PCI
+	}, {
+		.virtual	= CNS3XXX_PCIE0_HOST_BASE_VIRT,
+		.pfn		= __phys_to_pfn(CNS3XXX_PCIE0_HOST_BASE),
+		.length		= SZ_4K,
+		.type		= MT_DEVICE,
+	}, {
+		.virtual	= CNS3XXX_PCIE0_CFG0_BASE_VIRT,
+		.pfn		= __phys_to_pfn(CNS3XXX_PCIE0_CFG0_BASE),
+		.length		= SZ_64K, /* really 4 KiB at offset 32 KiB */
+		.type		= MT_DEVICE,
+	}, {
+		.virtual	= CNS3XXX_PCIE0_CFG1_BASE_VIRT,
+		.pfn		= __phys_to_pfn(CNS3XXX_PCIE0_CFG1_BASE),
+		.length		= SZ_16M,
+		.type		= MT_DEVICE,
+	}, {
+		.virtual	= CNS3XXX_PCIE1_HOST_BASE_VIRT,
+		.pfn		= __phys_to_pfn(CNS3XXX_PCIE1_HOST_BASE),
+		.length		= SZ_4K,
+		.type		= MT_DEVICE,
+	}, {
+		.virtual	= CNS3XXX_PCIE1_CFG0_BASE_VIRT,
+		.pfn		= __phys_to_pfn(CNS3XXX_PCIE1_CFG0_BASE),
+		.length		= SZ_64K, /* really 4 KiB at offset 32 KiB */
+		.type		= MT_DEVICE,
+	}, {
+		.virtual	= CNS3XXX_PCIE1_CFG1_BASE_VIRT,
+		.pfn		= __phys_to_pfn(CNS3XXX_PCIE1_CFG1_BASE),
+		.length		= SZ_16M,
+		.type		= MT_DEVICE,
+#endif
 	},
 };

arch/arm/mach-cns3xxx/pcie.c +32 ~~-73~~

178 lines not on this page · raw message

Signature · 4 lines

Krzysztof Halasa

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

Krzysztof Hałasa

author

2014-03-04 09:08 UTC #6 lore raw

Re: [PATCH v2] CNS3xxx: Fix PCIe early iotable_init().

Quoted 8 lines · khalasa at piap.pl (Krzysztof Ha?asa)

This patch fixes the following BUG:
Quoted 6 lines
kernel BUG at mm/vmalloc.c:1132!
PC is at vm_area_add_early+0x20/0x84
LR is at add_static_vm_early+0xc/0x60
The problem is cns3xxx_pcie_init() (device_initcall) calls the "early" iotable_init().

That's obviously:

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

Signature · 4 lines

Krzysztof Halasa

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

Arnd Bergmann

Applied

2014-03-17 14:37 UTC #7 lore raw

Re: [PATCH v2] CNS3xxx: Fix PCIe early iotable_init().

Quoted 8 lines · Krzysztof Ha?asa

Quoted 6 lines · khalasa at piap.pl (Krzysztof Ha?asa)
This patch fixes the following BUG:
Quoted 4 lines
kernel BUG at mm/vmalloc.c:1132!
PC is at vm_area_add_early+0x20/0x84
LR is at add_static_vm_early+0xc/0x60

Quoted 7 lines

Quoted 2 lines
Quoted 2 lines
The problem is cns3xxx_pcie_init() (device_initcall) calls the "early" iotable_init().
That's obviously:
Signed-off-by Krzysztof Ha?asa <[email protected]>

Applied to next/fixes-non-critical.

Since the problem is not new and you didn't mark the patch as 'stable', this seems to be the right place. Let us know if you need backports to stable kernels, also fo the other patch.

In general, when submitting arm-soc patches for inclusion, please add arm at kernel.org to Cc, so we don't miss it.

Thanks,

Arnd

Krzysztof Hałasa

author

2014-03-18 09:36 UTC #8 lore raw

Re: [PATCH v2] CNS3xxx: Fix PCIe early iotable_init().

Quoted 5 lines · Arnd Bergmann

Applied to next/fixes-non-critical.
Since the problem is not new and you didn't mark the patch as 'stable', this seems to be the right place. Let us know if you need backports to stable kernels, also fo the other patch.

Sure, BTW there is also that problem with PCI bridge configuration or something (maybe on certain boards only) which I can't fix at the moment, but will do eventually. So absolutely no rush with this/these patch(es) alone. I just want to make CNS3xxx some valid platform (the same with IXP4xx and I also have some i.MX6 stuff) but it has to wait for now.

Thanks,

Signature · 4 lines

Krzysztof Halasa

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

Krzysztof Hałasa

author

2014-03-04 10:37 UTC #9 lore raw

[PATCH2] CNS3xxx: Fix a WARN() related to IRQ allocation.

In reply to Krzysztof Hałasa

WARNING: at drivers/irqchip/irq-gic.c:952 gic_init_bases+0xe4/0x2b8()
Cannot allocate irq_descs @ IRQ16, assuming pre-allocated
Backtrace:
gic_init_bases    from cns3xxx_init_irq+0x24/0x34
cns3xxx_init_irq  from init_IRQ+0x24/0x2c
init_IRQ          from start_kernel+0x1a8/0x338

start_kernel from 0x2000806c

The problem is that 64 CNS3xxx CPU interrupts, starting at 32, are allocated by the ARM platform-independent code (as requested by machine_desc->nr_irqs = 96), and then the GIC code tries to allocate them again.

Tested on Gateworks Laguna board, masqueraded as CNS3420VB.

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

Signature · 4 lines

Krzysztof Halasa

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

Krzysztof Hałasa

author

2014-03-04 10:50 UTC #10 lore raw

[PATCH2] CNS3xxx: Fix a WARN() related to IRQ allocation.

In reply to Krzysztof Hałasa

WARNING: at drivers/irqchip/irq-gic.c:952 gic_init_bases+0xe4/0x2b8()
Cannot allocate irq_descs @ IRQ16, assuming pre-allocated
Backtrace:
gic_init_bases    from cns3xxx_init_irq+0x24/0x34
cns3xxx_init_irq  from init_IRQ+0x24/0x2c
init_IRQ          from start_kernel+0x1a8/0x338

start_kernel from 0x2000806c

Tested on Gateworks Laguna board, masqueraded as CNS3420VB.

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

arch/arm/mach-cns3xxx/cns3420vb.c +0 -1

@@ -246,7 +246,6 @@ static void __init cns3420_map_io(void)
 
 MACHINE_START(CNS3420VB, "Cavium Networks CNS3420 Validation Board")
 	.atag_offset	= 0x100,
-	.nr_irqs	= NR_IRQS_CNS3XXX,
 	.map_io		= cns3420_map_io,
 	.init_irq	= cns3xxx_init_irq,
 	.init_time	= cns3xxx_timer_init,

arch/arm/mach-cns3xxx/core.c +0 -1

@@ -400,7 +400,6 @@ static const char *cns3xxx_dt_compat[] __initdata = {
 
 DT_MACHINE_START(CNS3XXX_DT, "Cavium Networks CNS3xxx")
 	.dt_compat	= cns3xxx_dt_compat,
-	.nr_irqs	= NR_IRQS_CNS3XXX,
 	.map_io		= cns3xxx_map_io,
 	.init_irq	= cns3xxx_init_irq,
 	.init_time	= cns3xxx_timer_init,

Signature · 4 lines

Krzysztof Halasa

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

Arnd Bergmann

Applied

2014-03-17 14:33 UTC #11 lore raw

Re: [PATCH2] CNS3xxx: Fix a WARN() related to IRQ allocation.

Quoted 8 lines · Krzysztof Ha?asa

WARNING: at drivers/irqchip/irq-gic.c:952 gic_init_bases+0xe4/0x2b8()
Cannot allocate irq_descs @ IRQ16, assuming pre-allocated
Backtrace:
gic_init_bases    from cns3xxx_init_irq+0x24/0x34
cns3xxx_init_irq  from init_IRQ+0x24/0x2c
init_IRQ          from start_kernel+0x1a8/0x338

start_kernel from 0x2000806c

Quoted 8 lines

The problem is that 64 CNS3xxx CPU interrupts, starting at 32, are allocated by the ARM platform-independent code (as requested by machine_desc->nr_irqs = 96), and then the GIC code tries to allocate them again.
Tested on Gateworks Laguna board, masqueraded as CNS3420VB.
Signed-off-by Krzysztof Ha?asa <[email protected]>

Applied to next/fixes-non-critical.

Arnd

Quoted 20 lines

arch/arm/mach-cns3xxx/cns3420vb.c +0 -1

@@ -246,7 +246,6 @@ static void __init cns3420_map_io(void)
 
 MACHINE_START(CNS3420VB, "Cavium Networks CNS3420 Validation Board")
        .atag_offset    = 0x100,
-       .nr_irqs        = NR_IRQS_CNS3XXX,
        .map_io         = cns3420_map_io,
        .init_irq       = cns3xxx_init_irq,
        .init_time      = cns3xxx_timer_init,

arch/arm/mach-cns3xxx/core.c +0 -1

@@ -400,7 +400,6 @@ static const char *cns3xxx_dt_compat[] __initdata = {
 
 DT_MACHINE_START(CNS3XXX_DT, "Cavium Networks CNS3xxx")
        .dt_compat      = cns3xxx_dt_compat,
-       .nr_irqs        = NR_IRQS_CNS3XXX,
        .map_io         = cns3xxx_map_io,
        .init_irq       = cns3xxx_init_irq,
        .init_time      = cns3xxx_timer_init,