Intel I350 mini-PCIe card (igb) on Mirabox (mvebu / Armada 370)

[Thomas Petazzoni]

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 <jgunthorpe at obsidianresearch.com>
> 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 <jgunthorpe at obsidianresearch.com>
> ---
>  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 <linux/of.h>
>  #include <linux/of_address.h>
>  #include <linux/debugfs.h>
> +#include <linux/log2.h>
>  
>  /*
>   * 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