From mboxrd@z Thu Jan 1 00:00:00 1970 From: thomas.petazzoni@free-electrons.com (Thomas Petazzoni) Date: Tue, 8 Apr 2014 20:01:40 +0200 Subject: Intel I350 mini-PCIe card (igb) on Mirabox (mvebu / Armada 370) In-Reply-To: <20140408171417.GB27776@obsidianresearch.com> References: <20140327044054.GA22681@obsidianresearch.com> <20140405193435.50d8dd81@skate> <54BB31A2B04145E8908E0183FAB6B61B@fatboyfat.co.uk> <20140408171309.09bbf968@skate> <20140408174034.79df403e@skate> <20140408175545.1b4d55a5@skate> <20140408171417.GB27776@obsidianresearch.com> Message-ID: <20140408200140.5dcc2ad7@skate> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Dear Jason Gunthorpe, On Tue, 8 Apr 2014 11:14:17 -0600, Jason Gunthorpe wrote: > > mvebu-mbus/devices output: > > > > # cat /sys/kernel/debug/mvebu-mbus/devices > > [10] 00000000e0000000 - 00000000e0900000 : 0004:00e8 > > > I'm not sure why I don't have the non-power-of-two problem for the MBus > > windows. > > Well, you do, 0x900000 is not aligned. It converts to size=0b10001111, > which is undefined behavior for mbus. Ah correct. Though I'm still puzzled as to why the undefined behavior works for me, and not for Willy, who I believe has the same NIC as me. > What do you think about this: > > From 235b0bf637242a50ec45c8766d18a942bff601cf Mon Sep 17 00:00:00 2001 > From: Jason Gunthorpe > Date: Tue, 8 Apr 2014 11:12:41 -0600 > Subject: [PATCH] bus: mvebu-mbus: Avoid setting an undefined window size > > The mbus hardware requires a power of two size, 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. > > - WARN_ON to make this invalid usage really obvious > - Round down to the nearest power of two so we only stuff a valid > size into the HW I perfectly fine with those two. > - When reading interpret undefined values in a conservative way, > the value is assumed to be the lowest power of two. This avoids > the debugfs output showing a value that looks 'correct' But I am not sure with this one. Since now you're anyway rounding down sizes, how is it possible to get a non-power-of-two size in the registers? I would probably prefer to have mvebu_devs_debug_show() do something like: seq_printf(seq, "[%02d] %016llx - %016llx : %04x:%04x%s", win, (unsigned long long)wbase, (unsigned long long)(wbase + wsize), wtarget, wattr, (!is_power_of_2(wsize)) ? " non-pow2 undefined behavior!" : ""); or something like that. This way at least the function does not "hide" the fact that the configured value is invalid. See one more comment below. > All together this makes the recent problems with silent failure > of PCI very obvious, noisy and debuggable. > > Signed-off-by: Jason Gunthorpe > --- > drivers/bus/mvebu-mbus.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/bus/mvebu-mbus.c b/drivers/bus/mvebu-mbus.c > index 2ac754e..d26f63c 100644 > --- a/drivers/bus/mvebu-mbus.c > +++ b/drivers/bus/mvebu-mbus.c > @@ -56,6 +56,7 @@ > #include > #include > #include > +#include > > /* > * DDR target is the same on all platforms. > @@ -147,7 +148,7 @@ static void mvebu_mbus_read_window(struct mvebu_mbus_state *mbus, > *enabled = 1; > *base = ((u64)basereg & WIN_BASE_HIGH) << 32; > *base |= (basereg & WIN_BASE_LOW); > - *size = (ctrlreg | ~WIN_CTRL_SIZE_MASK) + 1; > + *size = 1 << (ffs(~(ctrlreg | ~WIN_CTRL_SIZE_MASK)) - 1); > > if (target) > *target = (ctrlreg & WIN_CTRL_TGT_MASK) >> WIN_CTRL_TGT_SHIFT; > @@ -266,6 +267,9 @@ static int mvebu_mbus_setup_window(struct mvebu_mbus_state *mbus, > mbus->soc->win_cfg_offset(win); > u32 ctrl, remap_addr; > > + WARN_ON(!is_power_of_2(size)); > + size = rounddown_pow_of_two(size); Maybe something like: if (!is_power_of_2(size)) { WARN(true, "Invalid MBus window size: 0x%x, rounding down to 0x%x\n", size, rounddown_pow_of_two(size)); size = rounddown_pow_of_two(size); } And while we're adding checks, why not also verify that the base address is a multiple of the window size? I think it's the other requirement. That being said, this warning doesn't really solve the problem that the PCI core may allocate BARs whose size cannot be represented through MBus windows :-) Thomas -- Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com