The mbus hardware requires a power of two size, and size aligned base.
Currently, if a non-power of two is passed in to the low level routines
they configure the register in a way that results in undefined behaviour.
Call WARN and return EINVAL instead.
Also, update the debugfs routines to show a message if there is an
invalid register setting.
All together this makes the recent problems with silent failure
of PCI very obvious, noisy and debuggable.
Signed-off-by: Jason Gunthorpe <[email protected]>
---
drivers/bus/mvebu-mbus.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
It would be good to see that these warn's trigger, my system here has
everything aligned..
diff --git a/drivers/bus/mvebu-mbus.c b/drivers/bus/mvebu-mbus.c
index 2ac754e..9578ae3 100644
--- a/drivers/bus/mvebu-mbus.c
+++ b/drivers/bus/mvebu-mbus.c
@@ -56,6 +56,7 @@
#include <linux/of.h>
#include <linux/of_address.h>
#include <linux/debugfs.h>
+#include <linux/log2.h>
/*
* DDR target is the same on all platforms.
@@ -266,6 +267,17 @@ static int mvebu_mbus_setup_window(struct mvebu_mbus_state *mbus,
mbus->soc->win_cfg_offset(win);
u32 ctrl, remap_addr;
+ if (!is_power_of_2(size)) {
+ WARN(true, "Invalid MBus window size: 0x%zx\n", size);
+ return -EINVAL;
+ }
+
+ if ((base & (phys_addr_t)(size - 1)) != 0) {
+ WARN(true, "Invalid MBus base/size: %pa len 0x%zx\n", &base,
+ size);
+ return -EINVAL;
+ }
+
ctrl = ((size - 1) & WIN_CTRL_SIZE_MASK) |
(attr << WIN_CTRL_ATTR_SHIFT) |
(target << WIN_CTRL_TGT_SHIFT) |
@@ -413,6 +425,10 @@ static int mvebu_devs_debug_show(struct seq_file *seq, void *v)
win, (unsigned long long)wbase,
(unsigned long long)(wbase + wsize), wtarget, wattr);
+ if (!is_power_of_2(wsize) ||
+ ((wbase & (u64)(wsize - 1)) != 0))
+ seq_puts(seq, " (Invalid base/size!!)");
+
if (win < mbus->soc->num_remappable_wins) {
seq_printf(seq, " (remap %016llx)\n",
(unsigned long long)wremap);
--
1.8.1.2
Hi Jason,
On Tue, Apr 08, 2014 at 05:44:14PM -0600, Jason Gunthorpe wrote:
> The mbus hardware requires a power of two size, and size aligned base.
> Currently, if a non-power of two is passed in to the low level routines
> they configure the register in a way that results in undefined behaviour.
>
> Call WARN and return EINVAL instead.
The WARN is correctly emitted for both igb ports here, but unfortunately
despite EINVAL, I still get the panic. Also I find it surprizing that it
reports sizes ending in ffff. I read the patch and it looks correct, so
we possibly have something else wrong here.
OK I just got it by adding two printk() in pci-mvebu.c. Both functions
mvebu_pcie_handle_iobase_change() and mvebu_pcie_handle_membase_change()
do pass a size which is in fact a mask (size - 1) and not the real size.
So the mbus is fed with an incorrect size which is off by one :
root@xpgp:~# modprobe igb
igb: Intel(R) Gigabit Ethernet Network Driver - version 5.0.5-k
igb: Copyright (c) 2007-2013 Intel Corporation.
PCI: enabling device 0000:00:09.0 (0140 -> 0143)
mvebu_pcie_handle_iobase_change: base=e8010000 size=00000fff
------------[ cut here ]------------
WARNING: CPU: 0 PID: 1258 at drivers/bus/mvebu-mbus.c:271 mvebu_mbus_setup_window+0xb3/0xe4()
Invalid MBus window size: 0xfff
Modules linked in: igb(+) i2c_algo_bit
CPU: 0 PID: 1258 Comm: modprobe Not tainted 3.14.0-mvebu #18
[<c0011c39>] (unwind_backtrace) from [<c000f20b>] (show_stack+0xb/0xc)
[<c000f20b>] (show_stack) from [<c02b5d1b>] (dump_stack+0x4f/0x64)
[<c02b5d1b>] (dump_stack) from [<c001a145>] (warn_slowpath_common+0x49/0x68)
[<c001a145>] (warn_slowpath_common) from [<c001a1bd>] (warn_slowpath_fmt+0x1d/0x28)
[<c001a1bd>] (warn_slowpath_fmt) from [<c017005b>] (mvebu_mbus_setup_window+0xb3/0xe4)
[<c017005b>] (mvebu_mbus_setup_window) from [<c01700f1>] (mvebu_mbus_alloc_window.constprop.7+0x65/0xc4)
[<c01700f1>] (mvebu_mbus_alloc_window.constprop.7) from [<c01865cf>] (mvebu_pcie_handle_iobase_change+0x6b/0xb0)
[<c01865cf>] (mvebu_pcie_handle_iobase_change) from [<c0186d1b>] (mvebu_pcie_wr_conf+0x2eb/0x2f4)
[<c0186d1b>] (mvebu_pcie_wr_conf) from [<c0177f39>] (pci_bus_write_config_word+0x35/0x44)
[<c0177f39>] (pci_bus_write_config_word) from [<c0010bf3>] (pcibios_enable_device+0xab/0xc0)
[<c0010bf3>] (pcibios_enable_device) from [<c017bb67>] (do_pci_enable_device+0x27/0x80)
[<c017bb67>] (do_pci_enable_device) from [<c017c727>] (pci_enable_device_flags+0x8f/0xc4)
[<c017c727>] (pci_enable_device_flags) from [<c017c67b>] (pci_enable_bridge+0x2b/0x48)
[<c017c67b>] (pci_enable_bridge) from [<c017c6e1>] (pci_enable_device_flags+0x49/0xc4)
[<c017c6e1>] (pci_enable_device_flags) from [<bf806aa1>] (igb_probe+0x1c/0xb80 [igb])
[<bf806aa1>] (igb_probe [igb]) from [<c017d1f5>] (pci_device_probe+0x45/0x6c)
[<c017d1f5>] (pci_device_probe) from [<c019bead>] (driver_probe_device+0xb5/0x174)
[<c019bead>] (driver_probe_device) from [<c019bfb7>] (__driver_attach+0x4b/0x4c)
[<c019bfb7>] (__driver_attach) from [<c019af43>] (bus_for_each_dev+0x27/0x48)
[<c019af43>] (bus_for_each_dev) from [<c019b967>] (bus_add_driver+0x87/0x128)
[<c019b967>] (bus_add_driver) from [<c019c379>] (driver_register+0x35/0x84)
[<c019c379>] (driver_register) from [<bf81e027>] (init_module+0x26/0x3f [igb])
[<bf81e027>] (init_module [igb]) from [<c0008809>] (do_one_initcall+0xa1/0xe4)
[<c0008809>] (do_one_initcall) from [<c00555e3>] (load_module+0x1067/0x1544)
[<c00555e3>] (load_module) from [<c0055b3f>] (SyS_init_module+0x7f/0x8c)
[<c0055b3f>] (SyS_init_module) from [<c000cc81>] (ret_fast_syscall+0x1/0x46)
---[ end trace 53f41ddadca51c5e ]---
mvebu_pcie_handle_membase_change: base=e0000000 size=002fffff
------------[ cut here ]------------
WARNING: CPU: 0 PID: 1258 at drivers/bus/mvebu-mbus.c:271 mvebu_mbus_setup_window+0xb3/0xe4()
Invalid MBus window size: 0x2fffff
Modules linked in: igb(+) i2c_algo_bit
CPU: 0 PID: 1258 Comm: modprobe Tainted: G W 3.14.0-mvebu #18
[<c0011c39>] (unwind_backtrace) from [<c000f20b>] (show_stack+0xb/0xc)
[<c000f20b>] (show_stack) from [<c02b5d1b>] (dump_stack+0x4f/0x64)
[<c02b5d1b>] (dump_stack) from [<c001a145>] (warn_slowpath_common+0x49/0x68)
[<c001a145>] (warn_slowpath_common) from [<c001a1bd>] (warn_slowpath_fmt+0x1d/0x28)
[<c001a1bd>] (warn_slowpath_fmt) from [<c017005b>] (mvebu_mbus_setup_window+0xb3/0xe4)
[<c017005b>] (mvebu_mbus_setup_window) from [<c0170145>] (mvebu_mbus_alloc_window.constprop.7+0xb9/0xc4)
[<c0170145>] (mvebu_mbus_alloc_window.constprop.7) from [<c0170447>] (mvebu_mbus_add_window_by_id+0xf/0x14)
[<c0170447>] (mvebu_mbus_add_window_by_id) from [<c0186cf1>] (mvebu_pcie_wr_conf+0x2c1/0x2f4)
[<c0186cf1>] (mvebu_pcie_wr_conf) from [<c0177f39>] (pci_bus_write_config_word+0x35/0x44)
[<c0177f39>] (pci_bus_write_config_word) from [<c0010bf3>] (pcibios_enable_device+0xab/0xc0)
[<c0010bf3>] (pcibios_enable_device) from [<c017bb67>] (do_pci_enable_device+0x27/0x80)
[<c017bb67>] (do_pci_enable_device) from [<c017c727>] (pci_enable_device_flags+0x8f/0xc4)
[<c017c727>] (pci_enable_device_flags) from [<c017c67b>] (pci_enable_bridge+0x2b/0x48)
[<c017c67b>] (pci_enable_bridge) from [<c017c6e1>] (pci_enable_device_flags+0x49/0xc4)
[<c017c6e1>] (pci_enable_device_flags) from [<bf806aa1>] (igb_probe+0x1c/0xb80 [igb])
[<bf806aa1>] (igb_probe [igb]) from [<c017d1f5>] (pci_device_probe+0x45/0x6c)
[<c017d1f5>] (pci_device_probe) from [<c019bead>] (driver_probe_device+0xb5/0x174)
[<c019bead>] (driver_probe_device) from [<c019bfb7>] (__driver_attach+0x4b/0x4c)
[<c019bfb7>] (__driver_attach) from [<c019af43>] (bus_for_each_dev+0x27/0x48)
[<c019af43>] (bus_for_each_dev) from [<c019b967>] (bus_add_driver+0x87/0x128)
[<c019b967>] (bus_add_driver) from [<c019c379>] (driver_register+0x35/0x84)
[<c019c379>] (driver_register) from [<bf81e027>] (init_module+0x26/0x3f [igb])
[<bf81e027>] (init_module [igb]) from [<c0008809>] (do_one_initcall+0xa1/0xe4)
[<c0008809>] (do_one_initcall) from [<c00555e3>] (load_module+0x1067/0x1544)
[<c00555e3>] (load_module) from [<c0055b3f>] (SyS_init_module+0x7f/0x8c)
[<c0055b3f>] (SyS_init_module) from [<c000cc81>] (ret_fast_syscall+0x1/0x46)
---[ end trace 53f41ddadca51c5f ]---
PCI: enabling device 0000:02:00.0 (0140 -> 0142)
Unhandled fault: external abort on non-linefetch (0x1008) at 0xf0300018
Internal error: : 1008 [#1] SMP THUMB2
Modules linked in: igb(+) i2c_algo_bit
CPU: 0 PID: 1258 Comm: modprobe Tainted: G W 3.14.0-mvebu #18
task: c7534d00 ti: edb9c000 task.ti: edb9c000
PC is at igb_get_invariants_82575+0x75/0x894 [igb]
LR is at igb_probe+0x22a/0xb80 [igb]
pc : [<bf80fe26>] lr : [<bf806caf>] psr: 20000033
sp : edb9dd00 ip : bf811e51 fp : c7488898
r10: bf816950 r9 : 00000001 r8 : c7488440
r7 : ed9a7068 r6 : 00000001 r5 : 00000000 r4 : c7488898
r3 : f0300000 r2 : 00001524 r1 : bf819654 r0 : c7488898
Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA Thumb Segment user
Control: 50c53c7d Table: 075f406a DAC: 00000015
Process modprobe (pid: 1258, stack limit = 0xedb9c240)
Stack: (0xedb9dd00 to 0xedb9e000)
dd00: ed9a7068 c7488440 00000001 bf818c28 c7488898 ed9a7000 00000000 c7488000
dd20: ed9a7068 c7488440 00000001 bf816950 c7488898 bf806caf 7ffffffe ec96a870
dd40: ec96a900 c00bdd75 edaea900 edaea900 00000001 00000001 ec96a870 ec96a870
dd60: 00000000 ec96a900 ec96a870 c03f5560 edaea900 ed9a7068 bf818c64 ed9a7000
dd80: 00000000 bf818c30 00000001 edb9c000 00000000 c017d1f5 ed9a7068 c0701170
dda0: 00000000 bf818c64 bf81e001 c019bead 00000000 ed9a7068 bf818c64 ed9a709c
ddc0: 00000000 c019bfb7 00000000 bf818c64 c019bf6d c019af43 ed8a02dc edaebeb4
dde0: bf818c64 c75e5880 c065c5b8 c019b967 bf818cac bf818c64 bf815948 bf818c64
de00: bf815948 bf8196a4 00000001 c019c379 bf818c30 bf818c28 bf815948 bf81e027
de20: 00000000 00400100 bf8196b0 c0008809 edb9de30 edb9de30 ed8f4d00 ed9e2000
de40: ed8f4d00 ed9e2000 ef7d2590 ef7d2580 40000000 f02a2000 ef7d2580 00000001
de60: f02a2000 00000000 ecd6d800 c0671680 ef7d2590 0000001f edb6df80 f02a2000
de80: 00000000 00400100 bf8196b0 bf8196a4 00000001 ecd6d800 00000001 00000124
dea0: bf8196ec c00555e3 bf8196b0 00007fff c0053c85 f02c1000 f02c0fff 00000000
dec0: f02a2000 bf8196b0 0001a670 edb9c000 00000000 bf8196b0 edb9c000 00000000
dee0: 0001a090 c007cbe3 edb9c000 00000000 0000001f 00000000 00000000 00000000
df00: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
df20: 00000000 00000000 00000000 00000000 ffffffff 0001e400 0002e328 0001a670
df40: 00000080 c000ce24 edb9c000 00000000 0001a090 c0055b3f f02a2000 0001e400
df60: f02b83e4 f02b8239 f02ba824 00015800 000169b0 00000000 00000000 00000000
df80: 00000028 00000029 0000001e 00000000 00000017 00000000 0001a070 0001a6e8
dfa0: 0001a688 c000cc81 0001a070 0001a6e8 0002e328 0001e400 0001a670 00000000
dfc0: 0001a070 0001a6e8 0001a688 00000080 0001a670 0001a008 0001a090 0001a090
dfe0: 00019220 bef988f8 0000bbd3 b6f1e558 60000030 0002e328 00000000 00000000
[<bf80fe26>] (igb_get_invariants_82575 [igb]) from [<bf806caf>] (igb_probe+0x22a/0xb80 [igb])
[<bf806caf>] (igb_probe [igb]) from [<c017d1f5>] (pci_device_probe+0x45/0x6c)
[<c017d1f5>] (pci_device_probe) from [<c019bead>] (driver_probe_device+0xb5/0x174)
[<c019bead>] (driver_probe_device) from [<c019bfb7>] (__driver_attach+0x4b/0x4c)
[<c019bfb7>] (__driver_attach) from [<c019af43>] (bus_for_each_dev+0x27/0x48)
[<c019af43>] (bus_for_each_dev) from [<c019b967>] (bus_add_driver+0x87/0x128)
[<c019b967>] (bus_add_driver) from [<c019c379>] (driver_register+0x35/0x84)
[<c019c379>] (driver_register) from [<bf81e027>] (init_module+0x26/0x3f [igb])
[<bf81e027>] (init_module [igb]) from [<c0008809>] (do_one_initcall+0xa1/0xe4)
[<c0008809>] (do_one_initcall) from [<c00555e3>] (load_module+0x1067/0x1544)
[<c00555e3>] (load_module) from [<c0055b3f>] (SyS_init_module+0x7f/0x8c)
[<c0055b3f>] (SyS_init_module) from [<c000cc81>] (ret_fast_syscall+0x1/0x46)
Code: 33b9 f8c4 6300 6863 (f8d3) 8018
---[ end trace 53f41ddadca51c60 ]---
Kernel panic - not syncing: Fatal exception
CPU2: stopping
CPU: 2 PID: 0 Comm: swapper/2 Tainted: G D W 3.14.0-mvebu #18
[<c0011c39>] (unwind_backtrace) from [<c000f20b>] (show_stack+0xb/0xc)
[<c000f20b>] (show_stack) from [<c02b5d1b>] (dump_stack+0x4f/0x64)
[<c02b5d1b>] (dump_stack) from [<c00113b3>] (handle_IPI+0xcb/0x100)
[<c00113b3>] (handle_IPI) from [<c0008485>] (armada_370_xp_handle_irq+0x7d/0xc8)
[<c0008485>] (armada_370_xp_handle_irq) from [<c000f9fb>] (__irq_svc+0x3b/0x5c)
Exception stack(0xed87ffa8 to 0xed87fff0)
ffa0: ffffffed 2d990000 c064ea1c 00000c32 ed87e000 c064e4ac
ffc0: c064e448 c0671ce5 00000001 c0671ce5 c02bae78 00000000 a0000000 ed87fff0
ffe0: c000d74d c00416ec 600f0033 ffffffff
[<c000f9fb>] (__irq_svc) from [<c00416ec>] (cpu_startup_entry+0x44/0xd4)
[<c00416ec>] (cpu_startup_entry) from [<00008569>] (0x8569)
CPU1: stopping
CPU: 1 PID: 0 Comm: swapper/1 Tainted: G D W 3.14.0-mvebu #18
[<c0011c39>] (unwind_backtrace) from [<c000f20b>] (show_stack+0xb/0xc)
[<c000f20b>] (show_stack) from [<c02b5d1b>] (dump_stack+0x4f/0x64)
[<c02b5d1b>] (dump_stack) from [<c00113b3>] (handle_IPI+0xcb/0x100)
[<c00113b3>] (handle_IPI) from [<c0008485>] (armada_370_xp_handle_irq+0x7d/0xc8)
[<c0008485>] (armada_370_xp_handle_irq) from [<c000f9fb>] (__irq_svc+0x3b/0x5c)
Exception stack(0xed87dfa8 to 0xed87dff0)
dfa0: ffffffed 2d988000 c064ea1c 00001fa4 ed87c000 c064e4ac
dfc0: c064e448 c0671ce5 00000001 c0671ce5 c02bae78 00000000 a0000000 ed87dff0
dfe0: c000d74d c00416ec 600f0033 ffffffff
[<c000f9fb>] (__irq_svc) from [<c00416ec>] (cpu_startup_entry+0x44/0xd4)
[<c00416ec>] (cpu_startup_entry) from [<00008569>] (0x8569)
CPU3: stopping
CPU: 3 PID: 0 Comm: swapper/3 Tainted: G D W 3.14.0-mvebu #18
[<c0011c39>] (unwind_backtrace) from [<c000f20b>] (show_stack+0xb/0xc)
[<c000f20b>] (show_stack) from [<c02b5d1b>] (dump_stack+0x4f/0x64)
[<c02b5d1b>] (dump_stack) from [<c00113b3>] (handle_IPI+0xcb/0x100)
[<c00113b3>] (handle_IPI) from [<c0008485>] (armada_370_xp_handle_irq+0x7d/0xc8)
[<c0008485>] (armada_370_xp_handle_irq) from [<c000f9fb>] (__irq_svc+0x3b/0x5c)
Exception stack(0xed881fa8 to 0xed881ff0)
1fa0: ffffffed 2d998000 c064ea1c 000006fc ed880000 c064e4ac
1fc0: c064e448 c0671ce5 00000001 c0671ce5 c02bae78 00000000 a0000000 ed881ff0
1fe0: c000d74d c00416ec 60000033 ffffffff
[<c000f9fb>] (__irq_svc) from [<c00416ec>] (cpu_startup_entry+0x44/0xd4)
[<c00416ec>] (cpu_startup_entry) from [<00008569>] (0x8569)
Rebooting in 1 seconds..
BootROM 1.20
The cause is obvious, the two callers compute a wrong size, so the attached
patch fixes it. After that I still get the panic but for the proper reason :
root@xpgp:~# modprobe igb
igb: Intel(R) Gigabit Ethernet Network Driver - version 5.0.5-k
igb: Copyright (c) 2007-2013 Intel Corporation.
PCI: enabling device 0000:00:09.0 (0140 -> 0143)
mvebu_pcie_handle_iobase_change: base=e8010000 size=00001000
mvebu_pcie_handle_membase_change: base=e0000000 size=00300000
------------[ cut here ]------------
WARNING: CPU: 0 PID: 1242 at drivers/bus/mvebu-mbus.c:271 mvebu_mbus_setup_window+0xb3/0xe4()
Invalid MBus window size: 0x300000
Modules linked in: igb(+) i2c_algo_bit
CPU: 0 PID: 1242 Comm: modprobe Not tainted 3.14.0-mvebu #26
[<c0011c39>] (unwind_backtrace) from [<c000f20b>] (show_stack+0xb/0xc)
[<c000f20b>] (show_stack) from [<c02b5d4b>] (dump_stack+0x4f/0x64)
[<c02b5d4b>] (dump_stack) from [<c001a145>] (warn_slowpath_common+0x49/0x68)
[<c001a145>] (warn_slowpath_common) from [<c001a1bd>] (warn_slowpath_fmt+0x1d/0x28)
(...)
I still don't know why it ends up in a panic, but we die on the expected
test now.
Regards,
Willy
Dear Willy Tarreau,
On Wed, 9 Apr 2014 08:11:29 +0200, Willy Tarreau wrote:
> The WARN is correctly emitted for both igb ports here, but unfortunately
> despite EINVAL, I still get the panic. Also I find it surprizing that it
> reports sizes ending in ffff. I read the patch and it looks correct, so
> we possibly have something else wrong here.
Yes, the panic is expected: Jason's patch is not *fixing* anything,
it's just telling you *why* it's going to panic.
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
On Wed, Apr 09, 2014 at 09:12:34AM +0200, Thomas Petazzoni wrote:
> Dear Willy Tarreau,
>
> On Wed, 9 Apr 2014 08:11:29 +0200, Willy Tarreau wrote:
>
> > The WARN is correctly emitted for both igb ports here, but unfortunately
> > despite EINVAL, I still get the panic. Also I find it surprizing that it
> > reports sizes ending in ffff. I read the patch and it looks correct, so
> > we possibly have something else wrong here.
>
> Yes, the panic is expected: Jason's patch is not *fixing* anything,
> it's just telling you *why* it's going to panic.
I just thought that the EINVAL would prevent one from registering
the device, which would be more useful (if at all possible).
Willy
Dear Willy Tarreau,
On Wed, 9 Apr 2014 09:47:52 +0200, Willy Tarreau wrote:
> > Yes, the panic is expected: Jason's patch is not *fixing* anything,
> > it's just telling you *why* it's going to panic.
>
> I just thought that the EINVAL would prevent one from registering
> the device, which would be more useful (if at all possible).
Unfortunately, I don't think that's possible: the function that sets up
the MBus window is called by the emulated bridge code, i.e when the
Linux PCI core thinks it is doing a write to a bridge register. And I
don't think there is a way of "refusing" the write to let the Linux
PCI core know that it was not possible to set up the bridge BAR as it
was requested.
Maybe this is something that Jason can confirm/infirm. I remember
having a quick look at the core Linux PCI core to see if it was
somehow checking whether the bridge BAR has been properly configured,
but I think I concluded it was not the case, and it was just assuming
that write the memory base/limit in the bridge registers was
sufficient.
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
On Wed, Apr 09, 2014 at 09:53:50AM +0200, Thomas Petazzoni wrote:
> Dear Willy Tarreau,
>
> On Wed, 9 Apr 2014 09:47:52 +0200, Willy Tarreau wrote:
>
> > > Yes, the panic is expected: Jason's patch is not *fixing* anything,
> > > it's just telling you *why* it's going to panic.
> >
> > I just thought that the EINVAL would prevent one from registering
> > the device, which would be more useful (if at all possible).
>
> Unfortunately, I don't think that's possible: the function that sets up
> the MBus window is called by the emulated bridge code, i.e when the
> Linux PCI core thinks it is doing a write to a bridge register. And I
> don't think there is a way of "refusing" the write to let the Linux
> PCI core know that it was not possible to set up the bridge BAR as it
> was requested.
>
> Maybe this is something that Jason can confirm/infirm. I remember
> having a quick look at the core Linux PCI core to see if it was
> somehow checking whether the bridge BAR has been properly configured,
> but I think I concluded it was not the case, and it was just assuming
> that write the memory base/limit in the bridge registers was
> sufficient.
OK thanks for the detailed explanation!
Willy
On Wed, Apr 09, 2014 at 08:11:29AM +0200, Willy Tarreau wrote:
> OK I just got it by adding two printk() in pci-mvebu.c. Both functions
> mvebu_pcie_handle_iobase_change() and mvebu_pcie_handle_membase_change()
> do pass a size which is in fact a mask (size - 1) and not the real size.
> So the mbus is fed with an incorrect size which is off by one :
Yes, that is right. I tested my patch here and didn't see any problem,
but I realize now that the mbus code is bailing early due to this:
kernel: mvebu_mbus: cannot add window '4:e8', conflicts with another window
Which I've never got around to fixing.. (whole other story there)
Your patch looks fine, and it obviously needs to be sequenced before
mine. (Thomas/Jason C: how do you want to do this?)
Reviewed-By: Jason Gunthorpe <[email protected]>
> From de000611015c7490a07ced6e36bfffbfdd136832 Mon Sep 17 00:00:00 2001
> From: Willy Tarreau <[email protected]>
> Date: Wed, 9 Apr 2014 08:05:09 +0200
> Subject: pci: mvebu: fix off-by-one in the computed size of the mbus windows
>
> mvebu_pcie_handle_membase_change() and mvebu_pcie_handle_iobase_change()
> compute a window size which is in fact a mask. This size is fed to
> mvebu_mbus_add_window_by_id() which itself subtracts 1 to get the
> mask. So clearly the two functions above are wrong.
Mask isn't the right word, maybe:
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.
Regards,
Jason
On Wed, Apr 09, 2014 at 09:53:50AM +0200, Thomas Petazzoni wrote:
> Maybe this is something that Jason can confirm/infirm. I remember
> having a quick look at the core Linux PCI core to see if it was
> somehow checking whether the bridge BAR has been properly configured,
> but I think I concluded it was not the case, and it was just assuming
> that write the memory base/limit in the bridge registers was
> sufficient.
I think the best way to fail would be to return an error from
mvebu_pcie_align_resource - but we can't create the window in that
call.
Once things get to the BAR write stage it shouldn't fail unfortunately.
Jason
Hi Jason,
On Wed, Apr 09, 2014 at 10:20:40AM -0600, Jason Gunthorpe wrote:
> On Wed, Apr 09, 2014 at 08:11:29AM +0200, Willy Tarreau wrote:
>
> > OK I just got it by adding two printk() in pci-mvebu.c. Both functions
> > mvebu_pcie_handle_iobase_change() and mvebu_pcie_handle_membase_change()
> > do pass a size which is in fact a mask (size - 1) and not the real size.
> > So the mbus is fed with an incorrect size which is off by one :
>
> Yes, that is right. I tested my patch here and didn't see any problem,
> but I realize now that the mbus code is bailing early due to this:
>
> kernel: mvebu_mbus: cannot add window '4:e8', conflicts with another window
>
> Which I've never got around to fixing.. (whole other story there)
>
> Your patch looks fine, and it obviously needs to be sequenced before
> mine. (Thomas/Jason C: how do you want to do this?)
>
> Reviewed-By: Jason Gunthorpe <[email protected]>
OK, thank you. I've updated the attached patch with your better description.
Cheers,
Willy
Dear Jason Gunthorpe,
On Wed, 9 Apr 2014 10:20:40 -0600, Jason Gunthorpe wrote:
> > OK I just got it by adding two printk() in pci-mvebu.c. Both functions
> > mvebu_pcie_handle_iobase_change() and mvebu_pcie_handle_membase_change()
> > do pass a size which is in fact a mask (size - 1) and not the real size.
> > So the mbus is fed with an incorrect size which is off by one :
>
> Yes, that is right. I tested my patch here and didn't see any problem,
> but I realize now that the mbus code is bailing early due to this:
>
> kernel: mvebu_mbus: cannot add window '4:e8', conflicts with another window
>
> Which I've never got around to fixing.. (whole other story there)
>
> Your patch looks fine, and it obviously needs to be sequenced before
> mine. (Thomas/Jason C: how do you want to do this?)
What I can propose is that I accumulate in a branch all the patches
needed to solve the various PCIe/Mbus problems we've identified:
* Your patch adding warnings to the mvebu-mbus driver
* Willy's patch fixing the off-by-one on the size
* Neil's patch fixing the MSI teardown function
* My two patches fixing the rest of the MSI logic
* And patches to come for the link problem, and the cutting of
non-power-of-two BARs into power-of-two windows
This way, everybody will be able to test than in his specific
hardware situations, the patches are solving all the problems. Then I
can take care of formally submitting those patches to the relevant
maintainers, of course keeping the authorship as appropriate.
How does that sound?
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
On Thu, Apr 10, 2014 at 08:53:46AM +0200, Thomas Petazzoni wrote:
> Dear Jason Gunthorpe,
>
> On Wed, 9 Apr 2014 10:20:40 -0600, Jason Gunthorpe wrote:
>
> > > OK I just got it by adding two printk() in pci-mvebu.c. Both functions
> > > mvebu_pcie_handle_iobase_change() and mvebu_pcie_handle_membase_change()
> > > do pass a size which is in fact a mask (size - 1) and not the real size.
> > > So the mbus is fed with an incorrect size which is off by one :
> >
> > Yes, that is right. I tested my patch here and didn't see any problem,
> > but I realize now that the mbus code is bailing early due to this:
> >
> > kernel: mvebu_mbus: cannot add window '4:e8', conflicts with another window
> >
> > Which I've never got around to fixing.. (whole other story there)
> >
> > Your patch looks fine, and it obviously needs to be sequenced before
> > mine. (Thomas/Jason C: how do you want to do this?)
>
> What I can propose is that I accumulate in a branch all the patches
> needed to solve the various PCIe/Mbus problems we've identified:
>
> * Your patch adding warnings to the mvebu-mbus driver
> * Willy's patch fixing the off-by-one on the size
> * Neil's patch fixing the MSI teardown function
> * My two patches fixing the rest of the MSI logic
> * And patches to come for the link problem, and the cutting of
> non-power-of-two BARs into power-of-two windows
>
> This way, everybody will be able to test than in his specific
> hardware situations, the patches are solving all the problems. Then I
> can take care of formally submitting those patches to the relevant
> maintainers, of course keeping the authorship as appropriate.
>
> How does that sound?
Great! Please do.
thx,
Jason.