Re: [PATCH] pci: Allow very large resource windows

From: Bjorn Helgaas
Date: Fri May 23 2014 - 14:42:04 EST

On Fri, May 23, 2014 at 11:51 AM, Kevin Hilman <khilman@xxxxxxxxxx> wrote:
> Hi Bjorn,
>
> Bjorn Helgaas <bhelgaas@xxxxxxxxxx> writes:
>
>> On Mon, May 19, 2014 at 02:03:14PM +0100, Alan wrote:
>>> From: Alan <alan@xxxxxxxxxxxxxxx>
>>>
>>> This is needed for some of the Xeon Phi type systems.
>>>
>>> Signed-off-by: Alan Cox <alan@xxxxxxxxxxxxxxx>
>>
>> I applied this to my pci/resource branch for v3.16. Nikhil
>> posted essentially the same patch a couple years ago, so I added
>> his signed-off-by and adopted his use of ARRAY_SIZE() to connect the
>> "order > 13" test with the aligns[] declaration.
>
> This patch (with the ARRAY_SIZE changes) has hit next-20140253 (in the
> form of commit 272c5a886c57) and according to my bisect, is the cause of
> a new boot failure on a 32-bit ARM platform (Marvell Armada 370,
> Mirabox, boot log excerpt down below[2].)
>
> While debugging, I found that Alan's original patch (without the
> ARRAY_SIZE change) booted just fine so I started looking closely at the
> ARRAY_SIZE() change. After some high-tech printk debugging, I noticed
> that order was negative when doing the compare, and found that this hack
> got things booting again:
>
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 47482b27fa12..792db3477fd5 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -966,7 +966,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
> /* For bridges size != alignment */
> align = pci_resource_alignment(dev, r);
> order = __ffs(align) - 20;
> - if (order >= ARRAY_SIZE(aligns)) {
> + if (order >= (int)ARRAY_SIZE(aligns)) {
> dev_warn(&dev->dev, "disabling BAR %d: %pR "
> "(bad alignment %#llx)\n", i, r,
> (unsigned long long) align);
>
> which led me to realize that the ARRAY_SIZE() change converted the >=
> into an unsigned compare where before it was a signed one, and as an
> unsigned compare, it was always true, resulting in those BARs being
> disabled.
>
> Below is a patch[1] which fixes the problem for me, and attempts a more
> detailed description of the problem in the changelog.
>
> Kevin
>
> [1]
> From 386d39a22e586fc1b060ad18c79247a50f2c0f8c Mon Sep 17 00:00:00 2001
> From: Kevin Hilman <khilman@xxxxxxxxxx>
> Date: Fri, 23 May 2014 10:24:40 -0700
> Subject: [PATCH] PCI: pbus_mem_size(): don't disable small BAR windows
>
> Since commit 272c5a886c57 (PCI: Support BAR sizes up to 8GB), small
> BAR windows get disabled.
>
> For small BAR sizes, order (as computed by __ffs(align) - 20) may be
> negative. If order is negative, when checking if it's >=
> ARRAY_SIZE(aligns) will always be true (since the result of
> ARRAY_SIZE() is unsigned) and so those BARs will be disabled.
>
> It doesn't make sense for order to be negative, and the code already
> converts it to non-negative later on, so to fix this bug, ensure that
> order is non-negative before comapring with ARRAY_SIZE().
>
> NOTE: Before commit 272c5a886c57, this worked just fine because order
> wasn't compared with ARRAY_SIZE() but with a hard-coded int, so
> the resulting signed compare worked fine.
>
> Signed-off-by: Kevin Hilman <khilman@xxxxxxxxxx>

Ouch, sorry about that. Thanks for your detailed analysis and fix. I
folded it into the patch and added a comment in the code.

Bjorn

> ---
> drivers/pci/setup-bus.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 47482b27fa12..a4152566f531 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -966,6 +966,8 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
> /* For bridges size != alignment */
> align = pci_resource_alignment(dev, r);
> order = __ffs(align) - 20;
> + if (order < 0)
> + order = 0;
> if (order >= ARRAY_SIZE(aligns)) {
> dev_warn(&dev->dev, "disabling BAR %d: %pR "
> "(bad alignment %#llx)\n", i, r,
> @@ -974,8 +976,6 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
> continue;
> }
> size += r_size;
> - if (order < 0)
> - order = 0;
> /* Exclude ranges with size > align from
> calculation of the alignment. */
> if (r_size == align)
> --
> 1.9.2
>
>
>
> [2]
> [...]
> mvebu-pcie pcie-controller.3: PCI host bridge to bus 0000:00
> pci_bus 0000:00: root bus resource [io 0x1000-0xfffff]
> pci_bus 0000:00: root bus resource [mem 0xf8000000-0xffdfffff]
> pci_bus 0000:00: root bus resource [bus 00-ff]
> pci 0000:00:01.0: [11ab:6710] type 01 class 0x060400
> pci 0000:00:02.0: [11ab:6710] type 01 class 0x060400
> PCI: bus0: Fast back to back transfers disabled
> pci 0000:00:01.0: bridge configuration invalid ([bus 00-00]), reconfiguring
> pci 0000:00:02.0: bridge configuration invalid ([bus 00-00]), reconfiguring
> PCI: bus1: Fast back to back transfers enabled
> pci_bus 0000:01: busn_res: [bus 01-ff] end is updated to 01
> pci 0000:02:00.0: [1b73:1009] type 00 class 0x0c0330
> pci 0000:02:00.0: reg 0x10: [mem 0x42000000-0x4200ffff 64bit]
> pci 0000:02:00.0: reg 0x18: [mem 0x42010000-0x42010fff 64bit]
> pci 0000:02:00.0: reg 0x20: [mem 0x42011000-0x42011fff 64bit]
> pci 0000:02:00.0: supports D1
> pci 0000:02:00.0: PME# supported from D0 D1 D3hot
> PCI: bus2: Fast back to back transfers disabled
> pci_bus 0000:02: busn_res: [bus 02-ff] end is updated to 02
> pci 0000:02:00.0: disabling BAR 0: [mem 0x42000000-0x4200ffff 64bit] (bad alignment 0x10000)
> pci 0000:02:00.0: disabling BAR 2: [mem 0x42010000-0x42010fff 64bit] (bad alignment 0x1000)
> pci 0000:02:00.0: disabling BAR 4: [mem 0x42011000-0x42011fff 64bit] (bad alignment 0x1000)
> pci 0000:00:01.0: PCI bridge to [bus 01]
> pci 0000:00:02.0: PCI bridge to [bus 02]
> Unhandled fault: external abort on non-linefetch (0x1008) at 0xf0080010
> Internal error: : 1008 [#1] SMP ARM
> Modules linked in:
> CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.15.0-rc6-next-20140523-08540-gefd7e9bf5d99 #45
> task: ee81ab80 ti: ee83a000 task.ti: ee83a000
> PC is at quirk_usb_early_handoff+0x348/0x7dc
> LR is at ioremap_page_range+0xfc/0x1b4
> pc : [<c0297f54>] lr : [<c01886d8>] psr: a0000153
> sp : ee83bd48 ip : c0019a20 fp : c0620964
> r10: 00000100 r9 : ee83bdd0 r8 : 00010000
> r7 : f0080000 r6 : c0607e20 r5 : eeb7a800 r4 : eeb7a800
> r3 : 00000146 r2 : 00000000 r1 : f0090000 r0 : f0080000
> Flags: NzCv IRQs on FIQs off Mode SVC_32 ISA ARM Segment kernel
> Control: 10c5387d Table: 00004019 DAC: 00000015
> Process swapper/0 (pid: 1, stack limit = 0xee83a248)
> Stack: (0xee83bd48 to 0xee83c000)
> bd40: c04428d4 ee83bd80 c062ae30 c053ab4c eeb7a800 c0607e20
> bd60: c053ab4c 0000ffff ee83bdd0 00000100 c0620964 c01bc3a0 00000000 c01eda18
> bd80: eeb7a800 eeb78814 eeb78800 00000001 00010000 c01ab390 eeb7a800 c01ab534
> bda0: eeb7ac00 eeb78e14 eeb78e00 c01ab57c eeb77f80 eeb78e00 ee83be28 c0012d98
> bdc0: f0074000 c0007fc8 feef0000 00000000 eeb77f80 eeb77f80 ee8e3410 c0303450
> bde0: c01be984 c01be9cc ee83be24 00010100 00000002 00000021 00000000 c01bfb68
> be00: eea1a0b8 000007cb 00000065 00000000 eeff2cb4 ee8e3410 ee83be24 10624dd3
> be20: 00000000 eea18210 c05e69c8 00000001 ee83be24 c01be9cc c01be984 00000000
> be40: 00000000 00000000 c0303450 c01bf794 c01be818 00000000 00000000 ee8e3410
> be60: c05e6984 00000000 c05e6984 00000000 00000000 ee83a000 00000000 c01f05b0
> be80: c01f0598 ee8e3410 c062c548 c01ef280 ee8e3410 c05e6984 ee8e3444 00000000
> bea0: c059f558 c01ef434 00000000 c05e6984 c01ef3a8 c01eda80 ee80965c ee8d47b4
> bec0: c05e6984 eeb1a000 c05edcc8 c01eea48 c0505490 c05c6de8 c05e6984 c05e6984
> bee0: c05d5198 eeb70f40 c0607e00 c01efa74 00000000 c05d5198 c05d5198 c0008984
> bf00: c0610304 ee82bc00 c042e950 00000010 c0607e00 c0563398 00000000 c00f5a90
> bf20: 00000000 c05d84a4 60000153 00000001 ef7fce1b c04403d0 000000a2 c00366c0
> bf40: c0562978 00000006 ef7fce26 00000006 c05d8474 c05c6de8 00000006 c05b5f30
> bf60: c0607e00 000000a2 c05b5f3c c058b5a4 00000000 c058bd14 00000006 00000006
> bf80: c058b5a4 00000000 00000000 c0422ffc 00000000 00000000 00000000 00000000
> bfa0: 00000000 c0423004 00000000 c000e5b8 00000000 00000000 00000000 00000000
> bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000
> [<c0297f54>] (quirk_usb_early_handoff) from [<c01bc3a0>] (pci_fixup_device+0xd4/0x184)
> [<c01bc3a0>] (pci_fixup_device) from [<c01ab390>] (pci_bus_add_device+0x14/0x64)
> [<c01ab390>] (pci_bus_add_device) from [<c01ab534>] (pci_bus_add_devices+0x3c/0x9c)
> [<c01ab534>] (pci_bus_add_devices) from [<c01ab57c>] (pci_bus_add_devices+0x84/0x9c)
> [<c01ab57c>] (pci_bus_add_devices) from [<c0012d98>] (pci_common_init_dev+0x184/0x2dc)
> [<c0012d98>] (pci_common_init_dev) from [<c01bfb68>] (mvebu_pcie_probe+0x3ac/0x604)
> [<c01bfb68>] (mvebu_pcie_probe) from [<c01f05b0>] (platform_drv_probe+0x18/0x48)
> [<c01f05b0>] (platform_drv_probe) from [<c01ef280>] (driver_probe_device+0x100/0x228)
> [<c01ef280>] (driver_probe_device) from [<c01ef434>] (__driver_attach+0x8c/0x90)
> [<c01ef434>] (__driver_attach) from [<c01eda80>] (bus_for_each_dev+0x54/0x88)
> [<c01eda80>] (bus_for_each_dev) from [<c01eea48>] (bus_add_driver+0xd4/0x1d0)
> [<c01eea48>] (bus_add_driver) from [<c01efa74>] (driver_register+0x78/0xf4)
> [<c01efa74>] (driver_register) from [<c0008984>] (do_one_initcall+0x80/0x1b8)
> [<c0008984>] (do_one_initcall) from [<c058bd14>] (kernel_init_freeable+0xfc/0x1c8)
> [<c058bd14>] (kernel_init_freeable) from [<c0423004>] (kernel_init+0x8/0xec)
> [<c0423004>] (kernel_init) from [<c000e5b8>] (ret_from_fork+0x14/0x3c)
> Code: e3a02000 ebf5fd27 e2507000 0affff59 (e5973010)
> ---[ end trace 20c08b6c6486c9b6 ]---
> Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/