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

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

[Neil Greatorex]

Hi all,

I've recently acquired a mini-PCIe Intel dual Gigabit Ethernet card
and was hoping to add it to my Globalscale Mirabox (based on the
Marvell Armada 370 SoC).

I have built the kernel from
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux based on
v3.14.0-rc8, with CONFIG_PCI_MVEBU=y and CONFIG_IGB=m.

When the system boots, it doesn't detect the PCIe card. Here's the
output of lspci:

mirabox ~ # lspci
00:01.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01)
00:02.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01)
02:00.0 USB controller: Fresco Logic FL1009 USB 3.0 Host Controller (rev 02)

Output from dmesg after boot: https://gist.github.com/ngreatorex/9769713
Output of "lspci -vvv" after boot: https://gist.github.com/ngreatorex/9769732

Now for the strange part - if I do an "echo 1 > /sys/bus/pci/rescan"
it detects the network card, but I get an oops. It doesn't work,
presumably due to the oops, but it shows up in lspci:

mirabox ~ # lspci
00:01.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01)
00:02.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01)
01:00.0 Ethernet controller: Intel Corporation I350 Gigabit Network
Connection (rev 01)
01:00.1 Ethernet controller: Intel Corporation I350 Gigabit Network
Connection (rev 01)
02:00.0 USB controller: Fresco Logic FL1009 USB 3.0 Host Controller (rev ff)
03:00.0 USB controller: Fresco Logic FL1009 USB 3.0 Host Controller (rev 02)

Output from dmesg including the oops: https://gist.github.com/ngreatorex/9769438
Output of "lspci -vvv" now: https://gist.github.com/ngreatorex/9769327

I'm afraid that I have zero knowledge of PCI so I'm not sure where to
start with debugging this. I would appreciate it if someone could give
me some pointers? If you want any extra debugging information then
just ask...

Cheers,
Neil

PS. I suspect the problem is with the pci-mvebu driver rather than the
igb driver, so I haven't sent to the igb maintainers...

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

[Thomas Petazzoni]

Dear Neil Greatorex,

On Tue, 25 Mar 2014 20:07:59 +0000, Neil Greatorex wrote:

> I've recently acquired a mini-PCIe Intel dual Gigabit Ethernet card
> and was hoping to add it to my Globalscale Mirabox (based on the
> Marvell Armada 370 SoC).
> 
> I have built the kernel from
> git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux based on
> v3.14.0-rc8, with CONFIG_PCI_MVEBU=y and CONFIG_IGB=m.
> 
> When the system boots, it doesn't detect the PCIe card. Here's the
> output of lspci:
> 
> mirabox ~ # lspci
> 00:01.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01)
> 00:02.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01)
> 02:00.0 USB controller: Fresco Logic FL1009 USB 3.0 Host Controller (rev 02)
> 
> Output from dmesg after boot: https://gist.github.com/ngreatorex/9769713
> Output of "lspci -vvv" after boot: https://gist.github.com/ngreatorex/9769732
> 
> Now for the strange part - if I do an "echo 1 > /sys/bus/pci/rescan"
> it detects the network card, but I get an oops. It doesn't work,
> presumably due to the oops, but it shows up in lspci:
> 
> mirabox ~ # lspci
> 00:01.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01)
> 00:02.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01)
> 01:00.0 Ethernet controller: Intel Corporation I350 Gigabit Network
> Connection (rev 01)
> 01:00.1 Ethernet controller: Intel Corporation I350 Gigabit Network
> Connection (rev 01)
> 02:00.0 USB controller: Fresco Logic FL1009 USB 3.0 Host Controller (rev ff)
> 03:00.0 USB controller: Fresco Logic FL1009 USB 3.0 Host Controller (rev 02)
> 
> Output from dmesg including the oops: https://gist.github.com/ngreatorex/9769438
> Output of "lspci -vvv" now: https://gist.github.com/ngreatorex/9769327
> 
> I'm afraid that I have zero knowledge of PCI so I'm not sure where to
> start with debugging this. I would appreciate it if someone could give
> me some pointers? If you want any extra debugging information then
> just ask...

Willy Tarreau has also reported issues with igb cards on Marvell
platforms using the pci-mvebu driver. I think in his case the card was
detected even at boot time, but he had similar MSI-X issues later on.
Willy gave me one of these cards last month, but I haven't had the time
to look into the problem yet.

I'll try to find some time to debug this in the near future, at which
point I'll most likely get back to you with some patches to test things.

Thanks for the report!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

[Jason Gunthorpe]

On Tue, Mar 25, 2014 at 08:07:59PM +0000, Neil Greatorex wrote:

> mirabox ~ # lspci
> 00:01.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01)
> 00:02.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01)
> 02:00.0 USB controller: Fresco Logic FL1009 USB 3.0 Host Controller (rev 02)

A starting guess is that something to do with the reset sequence is
not long enough. PCI requires a certain time in-reset before a device
is required to respond to configuration... Depending on what your
bootloader does Linux might be the first thing to enable the link. Add
a 1 second sleep to the mvebu driver before the bus scan and see if
that helps?

> Output from dmesg after boot: https://gist.github.com/ngreatorex/9769713
> Output of "lspci -vvv" after boot: https://gist.github.com/ngreatorex/9769732
> 
> Now for the strange part - if I do an "echo 1 > /sys/bus/pci/rescan"
> it detects the network card, but I get an oops. It doesn't work,
> presumably due to the oops, but it shows up in lspci:
> 
> mirabox ~ # lspci
> 00:01.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01)
> 00:02.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01)
> 01:00.0 Ethernet controller: Intel Corporation I350 Gigabit Network
> Connection (rev 01)
> 01:00.1 Ethernet controller: Intel Corporation I350 Gigabit Network
> Connection (rev 01)

> 02:00.0 USB controller: Fresco Logic FL1009 USB 3.0 Host Controller (rev ff)
> 03:00.0 USB controller: Fresco Logic FL1009 USB 3.0 Host Controller (rev 02) (prog-if 30 [XHCI])
 ^^^^^^^^^^^

That is *really* weird looking - plus this:

02:00.0 USB controller: Fresco Logic FL1009 USB 3.0 Host Controller (rev ff) (prog-if ff)
        !!! Unknown header type 7f
        Kernel driver in use: xhci_hcd

Something really wonky happend here during the rescan. It looks like
for some reason the USB controller was seen on the wrong port for a
short time!? It would be awesome to track this down..

> I'm afraid that I have zero knowledge of PCI so I'm not sure where to
> start with debugging this. I would appreciate it if someone could give
> me some pointers? If you want any extra debugging information then
> just ask...

Hopefully Thomas can comment on the MSI oops.. But it might very well
be related to the xhci controller going insane.

Jason

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

[Thomas Petazzoni]

Jason, Neil,

On Tue, 25 Mar 2014 14:22:49 -0600, Jason Gunthorpe wrote:

> > mirabox ~ # lspci
> > 00:01.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01)
> > 00:02.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01)
> > 02:00.0 USB controller: Fresco Logic FL1009 USB 3.0 Host Controller (rev 02)
> 
> A starting guess is that something to do with the reset sequence is
> not long enough. PCI requires a certain time in-reset before a device
> is required to respond to configuration... Depending on what your
> bootloader does Linux might be the first thing to enable the link. Add
> a 1 second sleep to the mvebu driver before the bus scan and see if
> that helps?
> 
> > Output from dmesg after boot: https://gist.github.com/ngreatorex/9769713
> > Output of "lspci -vvv" after boot: https://gist.github.com/ngreatorex/9769732
> > 
> > Now for the strange part - if I do an "echo 1 > /sys/bus/pci/rescan"
> > it detects the network card, but I get an oops. It doesn't work,
> > presumably due to the oops, but it shows up in lspci:
> > 
> > mirabox ~ # lspci
> > 00:01.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01)
> > 00:02.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01)
> > 01:00.0 Ethernet controller: Intel Corporation I350 Gigabit Network
> > Connection (rev 01)
> > 01:00.1 Ethernet controller: Intel Corporation I350 Gigabit Network
> > Connection (rev 01)
> 
> > 02:00.0 USB controller: Fresco Logic FL1009 USB 3.0 Host Controller (rev ff)
> > 03:00.0 USB controller: Fresco Logic FL1009 USB 3.0 Host Controller (rev 02) (prog-if 30 [XHCI])
>  ^^^^^^^^^^^
> 
> That is *really* weird looking - plus this:
> 
> 02:00.0 USB controller: Fresco Logic FL1009 USB 3.0 Host Controller (rev ff) (prog-if ff)
>         !!! Unknown header type 7f
>         Kernel driver in use: xhci_hcd
> 
> Something really wonky happend here during the rescan. It looks like
> for some reason the USB controller was seen on the wrong port for a
> short time!? It would be awesome to track this down..

We have recently identified a weird thing on Armada 38x, maybe previous
revisions are affected as well. We haven't fully understood the problem
yet, but the following patch allows PCIe devices to be detected in all
situations on Armada 38x. It seems like setting the local device number
requires "some time" to be taken into account.

Neil, could you test the below patch:

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index 13478ec..350b115 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -983,6 +983,7 @@ static int mvebu_pcie_probe(struct platform_device *pdev)
                }
 
                mvebu_pcie_set_local_dev_nr(port, 1);
+               udelay(100);
 
                port->dn = child;
                spin_lock_init(&port->conf_lock);

It will clearly not solve the MSI-X panic, but maybe the PCIe device
detection problem at boot time. It would be nice if you could check
this, and also see if the weird Fresco double instance disappears.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

[Willy Tarreau]

Hi,

On Tue, Mar 25, 2014 at 09:20:10PM +0100, Thomas Petazzoni wrote:
> Willy Tarreau has also reported issues with igb cards on Marvell
> platforms using the pci-mvebu driver. I think in his case the card was
> detected even at boot time, but he had similar MSI-X issues later on.

To be precise on this, originally the card was not detected at all,
neither by the kernel nor by U-Boot. But one of your patches to
renumber the PCI end points made it appear and work well. Later,
the MSI-X issue surfaced after the following patch was merged :

   36dd1f3 PCI: mvebu: Disable prefetchable memory support in PCI-to-PCI bridge

Reverting it solves the issue but as we discussed, doing this is
just a workaround as the patch is fine. I tracked down the oops
to be caused by something related to the NIC failing to register
an MSI-X interrupt for both ports but one only, trying to rollback
to do MSI only for both ports and there failing on this part which
dereferences the pointer returned in *value that's zero while it
used to be -1 before the patch. That's all I know, I added maybe
200 printks all around but don't know the PCI code and did not have
enough time to track it down further nor to make a detailed bug
report. I don't even know whether it's what the NIC driver does,
what the PCI stack does or what the MVEBU PCI driver does which is
at fault, or even any combination of them.

In the mean time if you want to test your NIC, try reverting the
patch above, but keep in mind that without this patch, other NICs
were reported to fail in some conditions (realtek IIRC).

Regards,
Willy

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

[Jason Gunthorpe]

On Tue, Mar 25, 2014 at 09:36:38PM +0100, Thomas Petazzoni wrote:

> We have recently identified a weird thing on Armada 38x, maybe previous
> revisions are affected as well. We haven't fully understood the problem
> yet, but the following patch allows PCIe devices to be detected in all
> situations on Armada 38x. It seems like setting the local device number
> requires "some time" to be taken into account.

What was the value of mvebu_pcie_link_up around this point?

PHY training would be the most likely time sensitive thing to muck up
discovery..

Jason

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

[Thomas Petazzoni]

Dear Jason Gunthorpe,

On Tue, 25 Mar 2014 15:12:40 -0600, Jason Gunthorpe wrote:
> On Tue, Mar 25, 2014 at 09:36:38PM +0100, Thomas Petazzoni wrote:
> 
> > We have recently identified a weird thing on Armada 38x, maybe previous
> > revisions are affected as well. We haven't fully understood the problem
> > yet, but the following patch allows PCIe devices to be detected in all
> > situations on Armada 38x. It seems like setting the local device number
> > requires "some time" to be taken into account.
> 
> What was the value of mvebu_pcie_link_up around this point?
> 
> PHY training would be the most likely time sensitive thing to muck up
> discovery..

The link is up, but the PCIe device is not visible under the right
bus/device number until we call mvebu_pcie_set_local_dev_nr(), and wait
about 50-100 microseconds. By "not visible", I mean reading the
vendor/product ID from the PCI configuration space returns
0xffff:0xffff, while after waiting 50-100 us, the right value is read.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

[Neil Greatorex]

Thomas et al,

On Tue, Mar 25, 2014 at 8:36 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Jason, Neil,
>
> On Tue, 25 Mar 2014 14:22:49 -0600, Jason Gunthorpe wrote:
>
>> > mirabox ~ # lspci
>> > 00:01.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01)
>> > 00:02.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01)
>> > 02:00.0 USB controller: Fresco Logic FL1009 USB 3.0 Host Controller (rev 02)
>>
>> A starting guess is that something to do with the reset sequence is
>> not long enough. PCI requires a certain time in-reset before a device
>> is required to respond to configuration... Depending on what your
>> bootloader does Linux might be the first thing to enable the link. Add
>> a 1 second sleep to the mvebu driver before the bus scan and see if
>> that helps?
>>

<snip>

> We have recently identified a weird thing on Armada 38x, maybe previous
> revisions are affected as well. We haven't fully understood the problem
> yet, but the following patch allows PCIe devices to be detected in all
> situations on Armada 38x. It seems like setting the local device number
> requires "some time" to be taken into account.
>
> Neil, could you test the below patch:
>

<patch snipped>

I applied the patch as given with udelay(100) and it made no difference.

I then replaced it with mdelay(1000) out of interest, and it succeeded
in detecting the card at boot. It then proceeded exactly as described
by Willy in his later e-mail (it successfully registers an MSI-X
interrupt for one port, and then proceeds to implode).

Here's the console output for this boot:
https://gist.github.com/ngreatorex/9771902
Here's the "lspci -vvv" after the oops:
https://gist.github.com/ngreatorex/9771943

I then blacklisted igb to prevent modprobe loading the module, and it
boots fine, with the card detected in lspci. Here's the "lspci -vvv"
for that situation: https://gist.github.com/ngreatorex/9772195

With the module blacklisted, I then tried rescanning the bus, and
there were no changes - the Fresco controller didn't go haywire and
show up twice.

Do you want me to experiment with different delays and see where the
detection starts to fail?

>
> It will clearly not solve the MSI-X panic, but maybe the PCIe device
> detection problem at boot time. It would be nice if you could check
> this, and also see if the weird Fresco double instance disappears.
>
> Thanks!
>
> Thomas
> --
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com

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

[Jason Gunthorpe]

On Tue, Mar 25, 2014 at 10:03:29PM +0000, Neil Greatorex wrote:

> I then replaced it with mdelay(1000) out of interest, and it succeeded
> in detecting the card at boot. It then proceeded exactly as described
> by Willy in his later e-mail (it successfully registers an MSI-X
> interrupt for one port, and then proceeds to implode).

Seem to confirm not enough time after reset..

Try this (untested) debugging patch:

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index b8f2fc9..74e2e20 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -999,6 +999,34 @@ static int mvebu_pcie_probe(struct platform_device *pdev)
 
 		mvebu_pcie_set_local_dev_nr(port, 1);
 
+		/* Wait for the link to come up */
+		if (!mvebu_pcie_link_up(port)) {
+			unsigned int I;
+
+			dev_info(&pdev->dev, "Waiting for link up\n");
+			for (I = 0; I != 100; I++) {
+				udelay(100);
+				if (mvebu_pcie_link_up(port))
+					break;
+			}
+			dev_info(&pdev->dev, "Link is %u\n",
+				 mvebu_pcie_link_up(port));
+		}
+
+		/* Turn on CRS Software Visibility */
+		mvebu_writel(port, mvebu_readl(port, PCIE_CTRL_OFF) | BIT(31),
+			     PCIE_CTRL_OFF);
+
+		/* Read the vendor ID from the connected device */
+		mvebu_writel(port, PCIE_CONF_ADDR(1, 0, 0), PCIE_CONF_ADDR_OFF);
+		dev_info(&pdev->dev, "Vendor ID is %x\n",
+			 mvebu_readl(port, PCIE_CONF_DATA_OFF));
+		msleep(1000);
+		mvebu_writel(port, PCIE_CONF_ADDR(1, 0, 0), PCIE_CONF_ADDR_OFF);
+		dev_info(&pdev->dev, "Try 2: Vendor ID is %x\n",
+			 mvebu_readl(port, PCIE_CONF_DATA_OFF));
+
+
 		port->dn = child;
 		spin_lock_init(&port->conf_lock);
 		mvebu_sw_pci_bridge_init(port);

Also, are you using the "reset-gpios" DT property? If so, ensure the
delay is at least 100 ms.

There is still an oddball problem:

00:01.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01) (prog-if 00 [Normal decode])
	Bus: primary=00, secondary=01, subordinate=02, sec-latency=0

I think the subordinate bus should be 01 - not sure what is going on
there... Maybe related to the rescan problem..

Jason

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

[Jason Gunthorpe]

On Tue, Mar 25, 2014 at 04:24:04PM -0600, Jason Gunthorpe wrote:
> On Tue, Mar 25, 2014 at 10:03:29PM +0000, Neil Greatorex wrote:
> 
> > I then replaced it with mdelay(1000) out of interest, and it succeeded
> > in detecting the card at boot. It then proceeded exactly as described
> > by Willy in his later e-mail (it successfully registers an MSI-X
> > interrupt for one port, and then proceeds to implode).
> 
> Seem to confirm not enough time after reset..
> 
> Try this (untested) debugging patch:

Sorry, it has a mistake, that is not how you detect CRS on this chip..

CRS might be something the driver has to emulate as well.....

This is better.. CRS will be bit 19 in the ICR

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index b8f2fc9..658a33f 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -999,6 +999,36 @@ static int mvebu_pcie_probe(struct platform_device *pdev)
 
 		mvebu_pcie_set_local_dev_nr(port, 1);
 
+		/* Wait for the link to come up */
+		if (!mvebu_pcie_link_up(port)) {
+			unsigned int I;
+
+			dev_info(&pdev->dev, "Waiting for link up\n");
+			for (I = 0; I != 100; I++) {
+				udelay(100);
+				if (mvebu_pcie_link_up(port))
+					break;
+			}
+			dev_info(&pdev->dev, "Link is %u\n",
+				 mvebu_pcie_link_up(port));
+		}
+
+		/* Clear and report the ICR */
+		mvebu_writel(port, 0, 0x1900);
+		dev_info(&pdev->dev, "ICR is %x\n", mvebu_readl(port, 0x1900));
+
+		/* Read the vendor ID from the connected device */
+		mvebu_writel(port, PCIE_CONF_ADDR(1, 0, 0), PCIE_CONF_ADDR_OFF);
+		dev_info(&pdev->dev, "Vendor ID is %x\n",
+			 mvebu_readl(port, PCIE_CONF_DATA_OFF));
+		dev_info(&pdev->dev, "ICR is %x\n", mvebu_readl(port, 0x1900));
+		msleep(1000);
+		mvebu_writel(port, PCIE_CONF_ADDR(1, 0, 0), PCIE_CONF_ADDR_OFF);
+		dev_info(&pdev->dev, "Try 2: Vendor ID is %x\n",
+			 mvebu_readl(port, PCIE_CONF_DATA_OFF));
+		dev_info(&pdev->dev, "ICR is %x\n", mvebu_readl(port, 0x1900));
+
+
 		port->dn = child;
 		spin_lock_init(&port->conf_lock);
 		mvebu_sw_pci_bridge_init(port);

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

[Neil Greatorex]

Jason,

On Tue, Mar 25, 2014 at 10:35 PM, Jason Gunthorpe
<jgunthorpe@obsidianresearch.com> wrote:
> On Tue, Mar 25, 2014 at 04:24:04PM -0600, Jason Gunthorpe wrote:
>> On Tue, Mar 25, 2014 at 10:03:29PM +0000, Neil Greatorex wrote:
>>
>> > I then replaced it with mdelay(1000) out of interest, and it succeeded
>> > in detecting the card at boot. It then proceeded exactly as described
>> > by Willy in his later e-mail (it successfully registers an MSI-X
>> > interrupt for one port, and then proceeds to implode).
>>
>> Seem to confirm not enough time after reset..
>>
>> Try this (untested) debugging patch:
>
> Sorry, it has a mistake, that is not how you detect CRS on this chip..
>
> CRS might be something the driver has to emulate as well.....
>
> This is better.. CRS will be bit 19 in the ICR
>

<patch snipped>

I ran your patch. The dmesg output is available at
https://gist.github.com/ngreatorex/9790164

I'm afraid the output doesn't mean much to me :-)

Cheers,
Neil

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

[Jason Gunthorpe]

On Wed, Mar 26, 2014 at 07:31:12PM +0000, Neil Greatorex wrote:
> > Sorry, it has a mistake, that is not how you detect CRS on this chip..
> >
> > CRS might be something the driver has to emulate as well.....
> >
> > This is better.. CRS will be bit 19 in the ICR

> I ran your patch. The dmesg output is available at
> https://gist.github.com/ngreatorex/9790164
> 
> I'm afraid the output doesn't mean much to me :-)

This is the relevant bit:

[    0.136040] mvebu-pcie pcie-controller.3: ICR is 0
[    0.161162] mvebu-pcie pcie-controller.3: Vendor ID is ffffffff
[    0.161170] mvebu-pcie pcie-controller.3: ICR is 800200
[    1.170071] mvebu-pcie pcie-controller.3: Try 2: Vendor ID is ffffffff
[    1.170083] mvebu-pcie pcie-controller.3: ICR is 808200 

#1 The ICR indicates PexLinkFail and NFErrDet after the first vendor
   read. I think PexLinkFail is very surprising, documentation says it
   means there was an error on the link and it re-trained. Perhaps it
   is a side effect of #2..

#2 It looks like I botched the format of the configuration since the NIC
   didn't respond, I think I see why.

#3 After sleeping the PEX detected a UR or CA TLP, which is not
   necessarily surprising, considering #2..

So, we need a result that actually reads the vendor id..

Here is a revision, I suspect the error with the debugging was not
calling mvebu_pcie_set_local_bus_nr, so a type 1 transaction was being
produced. I am looking for the 2nd Vendor ID to return 80861521.

Also, are you using the "reset-gpios" DT property?

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index b8f2fc9..6da4983 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -999,6 +999,38 @@ static int mvebu_pcie_probe(struct platform_device *pdev)
 
 		mvebu_pcie_set_local_dev_nr(port, 1);
 
+		/* Wait for the link to come up */
+		if (!mvebu_pcie_link_up(port)) {
+			unsigned int I;
+
+			dev_info(&pdev->dev, "Waiting for link up\n");
+			for (I = 0; I != 100; I++) {
+				udelay(100);
+				if (mvebu_pcie_link_up(port))
+					break;
+			}
+			dev_info(&pdev->dev, "Link is %u\n",
+				 mvebu_pcie_link_up(port));
+		}
+
+		/* Clear and report the ICR */
+		mvebu_writel(port, 0, 0x1900);
+		dev_info(&pdev->dev, "ICR is %x\n", mvebu_readl(port, 0x1900));
+
+		/* Read the vendor ID from the connected device */
+		mvebu_pcie_set_local_bus_nr(port, 1);
+		mvebu_writel(port, PCIE_CONF_ADDR(1, 0, 0), PCIE_CONF_ADDR_OFF);
+		dev_info(&pdev->dev, "Vendor ID is %x\n",
+			 mvebu_readl(port, PCIE_CONF_DATA_OFF));
+		dev_info(&pdev->dev, "ICR is %x\n", mvebu_readl(port, 0x1900));
+		mvebu_writel(port, 0, 0x1900);
+		msleep(1000);
+		mvebu_writel(port, PCIE_CONF_ADDR(1, 0, 0), PCIE_CONF_ADDR_OFF);
+		dev_info(&pdev->dev, "Try 2: Vendor ID is %x\n",
+			 mvebu_readl(port, PCIE_CONF_DATA_OFF));
+		dev_info(&pdev->dev, "ICR is %x\n", mvebu_readl(port, 0x1900));
+
+
 		port->dn = child;
 		spin_lock_init(&port->conf_lock);
 		mvebu_sw_pci_bridge_init(port);

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

[Neil Greatorex]

Jason,

On Wed, Mar 26, 2014 at 8:12 PM, Jason Gunthorpe
<jgunthorpe@obsidianresearch.com> wrote:
>
> So, we need a result that actually reads the vendor id..
>
> Here is a revision, I suspect the error with the debugging was not
> calling mvebu_pcie_set_local_bus_nr, so a type 1 transaction was being
> produced. I am looking for the 2nd Vendor ID to return 80861521.
>

Thanks. Here's the relevant output with that patch:

[    0.135772] mvebu-pcie pcie-controller.3: ICR is 0
[    0.160889] mvebu-pcie pcie-controller.3: Vendor ID is ffffffff
[    0.160897] mvebu-pcie pcie-controller.3: ICR is 800200
[    1.170215] mvebu-pcie pcie-controller.3: Try 2: Vendor ID is 15218086
[    1.170228] mvebu-pcie pcie-controller.3: ICR is 0
[    1.170290] mvebu-pcie pcie-controller.3: ICR is 0
[    1.170299] mvebu-pcie pcie-controller.3: Vendor ID is 10091b73
[    1.170306] mvebu-pcie pcie-controller.3: ICR is 0
[    2.180207] mvebu-pcie pcie-controller.3: Try 2: Vendor ID is 10091b73
[    2.180215] mvebu-pcie pcie-controller.3: ICR is 0

Success! For reference, the full dmesg is at https://gist.github.com/9792618

> Also, are you using the "reset-gpios" DT property?
>

I'm appending arch/arm/boot/dts/armada-370-mirabox.dtb to the kernel
image as the bootloader on the Mirabox is ancient and doesn't support
DTs. As far as I can see, there's no reset-gpios property anywhere in
that DT.

With regards to the bootloader, in case the information helps, it prints out:

PEX 0: Root Complex Interface, Detected Link X1
PEX 1: Root Complex Interface, Detected Link X1

As I am booting from the SD card, which is internally connected via
USB, the boot loader is obviously initialising the XHCI controller.
I'm guessing this is why it is detected immediately without needing
the delay.

Cheers,
Neil

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

[Jason Gunthorpe]

On Wed, Mar 26, 2014 at 08:34:19PM +0000, Neil Greatorex wrote:
> Thanks. Here's the relevant output with that patch:
> 
> [    0.135772] mvebu-pcie pcie-controller.3: ICR is 0
> [    0.160889] mvebu-pcie pcie-controller.3: Vendor ID is ffffffff
> [    0.160897] mvebu-pcie pcie-controller.3: ICR is 800200
> [    1.170215] mvebu-pcie pcie-controller.3: Try 2: Vendor ID is 15218086
> [    1.170228] mvebu-pcie pcie-controller.3: ICR is 0

Okay, this looks better..

Thomas: Can you verify the decoding of the ICR register (offset
0x1900)?

My Kirkwood manual says 0x800200 is 'Non-Fatal Error Detected' and 'Link
Failure Indication' - the latter seems very strange.

Could there be a doc error or change in the 370 version?

I checked on my board here with the link down and I get:

mvebu-pcie pex.1: Link is 0
mvebu-pcie pex.1: ICR is 0
mvebu-pcie pex.1: Vendor ID is ffffffff
mvebu-pcie pex.1: ICR is 201

Which makes sense - NF Error + Tx while in Link down Error.

In any event, lets try this.

>From 859a60617e050c51dc6bb83b2ed745d38a029b0d Mon Sep 17 00:00:00 2001
From: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Date: Wed, 26 Mar 2014 15:38:44 -0600
Subject: [PATCH] PCI: mvebu - Repeat the ID read if there is a CRS reply

Some PCI-E peers take a long time before they will respond to config
transactions. In this case the spec says they should return a
CRS status in the configuration read completion and the host should
retry.

mvebu docs say it sets a bit in the interrupt cause register when
CRS is received. In-circuit testing with a 8086:1521 NIC suggests
this might not be true, so we also monitor the non-fatal error bit,
which does get set.

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
---
 drivers/pci/host/pci-mvebu.c | 50 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 46 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index b8f2fc9..789cdb2 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -49,6 +49,10 @@
 	 PCIE_CONF_FUNC(PCI_FUNC(devfn)) | PCIE_CONF_REG(where) | \
 	 PCIE_CONF_ADDR_EN)
 #define PCIE_CONF_DATA_OFF	0x18fc
+#define PCIE_ICR		0x1900
+#define  PCIE_ICR_TX_IN_DOWN		BIT(0)
+#define  PCIE_ICR_NFERR_DET		BIT(9)
+#define  PCIE_ICR_CRS			BIT(19)
 #define PCIE_MASK_OFF		0x1910
 #define  PCIE_MASK_ENABLE_INTS          0x0f000000
 #define PCIE_CTRL_OFF		0x1a00
@@ -256,10 +260,44 @@ static int mvebu_pcie_hw_rd_conf(struct mvebu_pcie_port *port,
 				 struct pci_bus *bus,
 				 u32 devfn, int where, int size, u32 *val)
 {
-	mvebu_writel(port, PCIE_CONF_ADDR(bus->number, devfn, where),
-		     PCIE_CONF_ADDR_OFF);
-
-	*val = mvebu_readl(port, PCIE_CONF_DATA_OFF);
+	unsigned int tries = 0;
+
+	while (1) {
+		if (where == 0)
+			mvebu_writel(port, ~(PCIE_ICR_TX_IN_DOWN |
+					     PCIE_ICR_NFERR_DET | PCIE_ICR_CRS),
+				     PCIE_ICR);
+
+		mvebu_writel(port, PCIE_CONF_ADDR(bus->number, devfn, where),
+			     PCIE_CONF_ADDR_OFF);
+
+		*val = mvebu_readl(port, PCIE_CONF_DATA_OFF);
+
+		if (where == 0) {
+			u32 icr = mvebu_readl(port, PCIE_ICR);
+			if (icr & PCIE_ICR_TX_IN_DOWN)
+				goto err_out;
+
+			/*
+			 * Implement Configuration Request Retry. If the
+			 * configuration requst for the ID fails with a CRS or
+			 * Non-Fatal status we try again for 100 ms. NFERR_DET
+			 * is checked too, because CRS doesn't seem
+			 * reliable */
+			if (icr & (PCIE_ICR_NFERR_DET | PCIE_ICR_CRS)) {
+				if (tries >= 100)
+					goto err_out;
+				mdelay(1);
+				tries++;
+				continue;
+			}
+		}
+		break;
+	}
+	if (tries != 0)
+		dev_info(&port->pcie->pdev->dev,
+			 "Port %u repeated ID read %u times\n", port->port,
+			 tries);
 
 	if (size == 1)
 		*val = (*val >> (8 * (where & 3))) & 0xff;
@@ -267,6 +305,10 @@ static int mvebu_pcie_hw_rd_conf(struct mvebu_pcie_port *port,
 		*val = (*val >> (8 * (where & 3))) & 0xffff;
 
 	return PCIBIOS_SUCCESSFUL;
+
+err_out:
+	*val = 0xffffffff;
+	return PCIBIOS_DEVICE_NOT_FOUND;
 }
 
 static int mvebu_pcie_hw_wr_conf(struct mvebu_pcie_port *port,
-- 
1.8.1.2

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

[Thomas Petazzoni]

Dear Jason Gunthorpe,

On Wed, 26 Mar 2014 15:42:59 -0600, Jason Gunthorpe wrote:
> On Wed, Mar 26, 2014 at 08:34:19PM +0000, Neil Greatorex wrote:
> > Thanks. Here's the relevant output with that patch:
> > 
> > [    0.135772] mvebu-pcie pcie-controller.3: ICR is 0
> > [    0.160889] mvebu-pcie pcie-controller.3: Vendor ID is ffffffff
> > [    0.160897] mvebu-pcie pcie-controller.3: ICR is 800200
> > [    1.170215] mvebu-pcie pcie-controller.3: Try 2: Vendor ID is 15218086
> > [    1.170228] mvebu-pcie pcie-controller.3: ICR is 0
> 
> Okay, this looks better..
> 
> Thomas: Can you verify the decoding of the ICR register (offset
> 0x1900)?
> 
> My Kirkwood manual says 0x800200 is 'Non-Fatal Error Detected' and 'Link
> Failure Indication' - the latter seems very strange.

Decoding is:

 bit 0, TxReqInDIdownErr
 bit 1, MDis
 bit 2, reserved
 bit 3, ErrWrToReg
 bit 4, HitDfltWinErr
 bit 5, reserved
 bit 6, RxRamParErr
 bit 7, TxRamParErr
 bit 8, CorErrDet
 bit 9, NFErrDet
 bit 10, FErrDet
 bit 11, DstateChange
 bit 12, BIST
 bit 13, reserved
 bit 14, FlowCtrlProtocol
 bit 15, RcvUrCaErr
 bit 16, RcvErrFatal
 bit 17, RcvErrNonFatal
 bit 18, RcvErrCor
 bit 19, RcvCRS
 bit 20, PexSlvHotReset
 bit 21, PexSlvDisLink
 bit 22, PexSlvLb
 bit 23, PexLinkFail
 bit 24, RcvIntA
 bit 25, RcvIntB
 bit 26, RcvIntC
 bit 27, RcvIntD
 bit 28, RcvPmPme
 bit 29, RcvTurnOff
 bit 30, reserved
 bit 31, RcvMsi

So 800200 = bit 23 and bit 9, which means:

 * bit 9, NFErrDet, Non fatal error detected
 * bit 23, PexLinkFail

So it's the same as Kirkwood, at least for these bits. 

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

[Neil Greatorex]

Jason,

On Wed, Mar 26, 2014 at 9:42 PM, Jason Gunthorpe 
<jgunthorpe@obsidianresearch.com> wrote:
> On Wed, Mar 26, 2014 at 08:34:19PM +0000, Neil Greatorex wrote:
>> Thanks. Here's the relevant output with that patch:
>>
>> [    0.135772] mvebu-pcie pcie-controller.3: ICR is 0
>> [    0.160889] mvebu-pcie pcie-controller.3: Vendor ID is ffffffff
>> [    0.160897] mvebu-pcie pcie-controller.3: ICR is 800200
>> [    1.170215] mvebu-pcie pcie-controller.3: Try 2: Vendor ID is 
15218086
>> [    1.170228] mvebu-pcie pcie-controller.3: ICR is 0
>
> Okay, this looks better..
>
<snip>
>
> I checked on my board here with the link down and I get:
>
> mvebu-pcie pex.1: Link is 0
> mvebu-pcie pex.1: ICR is 0
> mvebu-pcie pex.1: Vendor ID is ffffffff
> mvebu-pcie pex.1: ICR is 201
>
> Which makes sense - NF Error + Tx while in Link down Error.
>
> In any event, lets try this.
>

I ran with this patch applied (but none of the previous ones you sent - 
was that correct?) but the new dev_info line doesn't fire. I also no 
longer get the ethernet card detected at boot, and get the weird dual XHCI 
controller after a rescan.

mirabox ~ # dmesg | grep "ID read"
mirabox ~ # lspci
00:01.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01)
00:02.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01)
02:00.0 USB controller: Fresco Logic FL1009 USB 3.0 Host Controller (rev 02)
mirabox ~ # echo 1 > /sys/bus/pci/rescan
mirabox ~ # lspci
00:01.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01)
00:02.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01)
01:00.0 Ethernet controller: Intel Corporation I350 Gigabit Network Connection (rev 01)
01:00.1 Ethernet controller: Intel Corporation I350 Gigabit Network Connection (rev 01)
02:00.0 USB controller: Fresco Logic FL1009 USB 3.0 Host Controller (rev ff)
03:00.0 USB controller: Fresco Logic FL1009 USB 3.0 Host Controller (rev 02)
mirabox ~ # dmesg | grep "ID read"
mirabox ~ #

Full dmesg showing boot and rescan at https://gist.github.com/9796043

I then added an extra dev_info to print the ICR just after you read it in 
the loop, and get this:

[    0.137047] pci_bus 0000:01: scanning bus
[    0.161098] mvebu-pcie pcie-controller.3: ICR is 808200
[    0.162104] mvebu-pcie pcie-controller.3: ICR is 808201
[    0.162191] pci_bus 0000:01: fixups for bus

So it seems that the first time we have NFErrDet and PexLinkFail, and on 
the second time through the loop we have NFErrDet, PexLinkFail and 
TxReqInDIDownErr, so it then errors out of the loop.

Full dmesg for this boot is at https://gist.github.com/9796442

Then, I added back in the 1 second delay just after the call to 
mvebu_pcie_set_local_dev_nr and the card was detected again with the 
following:

[    2.133299] pci_bus 0000:01: scanning bus
[    2.133313] mvebu-pcie pcie-controller.3: ICR is 808000
[    2.133351] pci 0000:01:00.0: [8086:1521] type 00 class 0x020000
[    2.133379] pci 0000:01:00.0: reg 0x10: [mem 0x00000000-0x0007ffff]
[    2.133405] pci 0000:01:00.0: reg 0x18: [io  0x0000-0x001f]
[    2.133422] pci 0000:01:00.0: reg 0x1c: [mem 0x00000000-0x00003fff]
[    2.133456] pci 0000:01:00.0: reg 0x30: [mem 0x00000000-0x0007ffff pref]
[    2.133473] pci 0000:01:00.0: calling pci_fixup_ide_bases+0x0/0x3c
[    2.133589] pci 0000:01:00.0: PME# supported from D0 D3hot D3cold
[    2.133601] pci 0000:01:00.0: PME# disabled
[    2.133658] pci 0000:01:00.0: reg 0x184: [mem 0x00000000-0x00003fff]
[    2.133692] pci 0000:01:00.0: reg 0x190: [mem 0x00000000-0x00003fff]
[    2.133945] mvebu-pcie pcie-controller.3: ICR is 808000
[    2.133987] pci 0000:01:00.1: [8086:1521] type 00 class 0x020000
[    2.134014] pci 0000:01:00.1: reg 0x10: [mem 0x00000000-0x0007ffff]
[    2.134040] pci 0000:01:00.1: reg 0x18: [io  0x0000-0x001f]
[    2.134057] pci 0000:01:00.1: reg 0x1c: [mem 0x00000000-0x00003fff]
[    2.134091] pci 0000:01:00.1: reg 0x30: [mem 0x00000000-0x0007ffff pref]
[    2.134106] pci 0000:01:00.1: calling pci_fixup_ide_bases+0x0/0x3c
[    2.134215] pci 0000:01:00.1: PME# supported from D0 D3hot D3cold
[    2.134226] pci 0000:01:00.1: PME# disabled
[    2.134281] pci 0000:01:00.1: reg 0x184: [mem 0x00000000-0x00003fff]
[    2.134316] pci 0000:01:00.1: reg 0x190: [mem 0x00000000-0x00003fff]
[    2.134560] mvebu-pcie pcie-controller.3: ICR is 808000
[    2.134571] mvebu-pcie pcie-controller.3: ICR is 808000
[    2.134581] mvebu-pcie pcie-controller.3: ICR is 808000
[    2.134590] mvebu-pcie pcie-controller.3: ICR is 808000
[    2.134599] mvebu-pcie pcie-controller.3: ICR is 808000
[    2.134607] mvebu-pcie pcie-controller.3: ICR is 808000
[    2.134633] pci_bus 0000:01: fixups for bus

mirabox ~ # lspci
[   71.400126] mvebu-pcie pcie-controller.3: ICR is 0
[   71.407226] mvebu-pcie pcie-controller.3: ICR is 808000
[   71.412559] mvebu-pcie pcie-controller.3: ICR is 808000
00:01.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01)
00:02.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01)
01:00.0 Ethernet controller: Intel Corporation I350 Gigabit Network Connection (rev 01)
01:00.1 Ethernet controller: Intel Corporation I350 Gigabit Network Connection (rev 01)
03:00.0 USB controller: Fresco Logic FL1009 USB 3.0 Host Controller (rev 02)

Full dmesg for this boot is at: https://gist.github.com/9796851

For clarity, the patch that I've applied on top of your last patch is at 
the end of this e-mail...

Cheers,
Neil

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index 75d2a73..46f72f54 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -273,6 +273,8 @@ static int mvebu_pcie_hw_rd_conf(struct 
mvebu_pcie_port *port,

  		if (where == 0) {
  			u32 icr = mvebu_readl(port, PCIE_ICR);
+			dev_info(&port->pcie->pdev->dev,
+				 "ICR is %x\n", icr);
  			if (icr & PCIE_ICR_TX_IN_DOWN)
  				goto err_out;

@@ -1018,6 +1020,7 @@ static int mvebu_pcie_probe(struct platform_device 
*pdev)
  		}

  		mvebu_pcie_set_local_dev_nr(port, 1);
+		mdelay(1000);

  		port->dn = child;
  		spin_lock_init(&port->conf_lock);

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

[Jason Gunthorpe]

On Thu, Mar 27, 2014 at 12:29:32AM +0000, Neil Greatorex wrote:

> I then added an extra dev_info to print the ICR just after you read
> it in the loop, and get this:
> 
> [    0.137047] pci_bus 0000:01: scanning bus
> [    0.161098] mvebu-pcie pcie-controller.3: ICR is 808200
> [    0.162104] mvebu-pcie pcie-controller.3: ICR is 808201
> [    0.162191] pci_bus 0000:01: fixups for bus
 
> So it seems that the first time we have NFErrDet and PexLinkFail,
> and on the second time through the loop we have NFErrDet,
> PexLinkFail and TxReqInDIDownErr, so it then errors out of the loop.

Interesting, so that confirms that the PexLinkFail is real. So
something is triggering the link reset, either the Marvell PEX core is
doing it (but not telling us why) or the NIC is doing it (and that
would probably be non-compliant).

Looks like you need the sleep, but I'm not really sure how you'd
implement it in a generic way, and I'm puzzled why the time from the
bootloader starting the PEX to the kernel starting isn't sufficient
(is it really short?).

Maybe it is a similar problem to what Thomas figured out needed a
sleep, it would be interesting to see if the ICR has similar
information on Thomas's case too...

Try moving the 1 second sleep around:
 - In the boot loader after starting the PEX, but before loading the
   kernel
 - In the kernel, at the mvebu board setup function
 - In the kernel at the start of the mvebu pci driver probe
 - At various places in the mvebu pci driver between start of probe
   and the after the device id is iset

Basically - binary search to find where adding the sleep works, to try
and detmine exactly what code is starting the time clock.

Jason

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

[Neil Greatorex]

Jason, Thomas,

With the help you've given, I think I've identified what the problem is. 
It is basically what Thomas suggested the problem was (that he'd seen with 
the Armada 38x). The call to mvebu_pcie_set_local_dev_nr causes the PCIe 
link to reset. In my tests with this Intel card, it takes ~25ms for the 
link to go down, and then another ~13ms for it to come back up.

I have put together a patch below that I would be interested to get your 
opinions on and also for you to test against any cards you may have. I 
only have the one mini PCIe card so it's not a great test!

With my card, it is now detected correctly, and there are no issues when 
rescanning the PCI bus.

Cheers,
Neil


>From a0ca1552e737c18b5651fe1f27ade76f512ca191 Mon Sep 17 00:00:00 2001
From: Neil Greatorex <neil@fatboyfat.co.uk>
Date: Fri, 28 Mar 2014 00:35:04 +0000
Subject: [PATCH] PCI: mvebu: Wait for PCIe link to reset after setting
  device number

After setting the local device nr, the PCIe link may go down briefly. If 
we don't wait for the link to come back up we may miss that device when we 
scan the bus (assuming it is not present).

This patch waits up to 40ms for the link to go down, and if it did go down,
waits up to 100ms for it to come back up. If the link was down to begin with,
it will not wait at all.

Signed-off-by: Neil Greatorex <neil@fatboyfat.co.uk>
---
  drivers/pci/host/pci-mvebu.c | 57 
++++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 57 insertions(+)

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index 0e79665..afd0dce 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -891,6 +891,7 @@ static int mvebu_pcie_probe(struct platform_device 
*pdev)
  	for_each_child_of_node(pdev->dev.of_node, child) {
  		struct mvebu_pcie_port *port = &pcie->ports[i];
  		enum of_gpio_flags flags;
+		u32 link_present, retry_count = 0;

  		if (!of_device_is_available(child))
  			continue;
@@ -975,8 +976,64 @@ static int mvebu_pcie_probe(struct platform_device 
*pdev)
  			continue;
  		}

+		/* After setting the local dev number, the PCI-E link may go
+		 * down for a period of time. If we don't wait for it to come
+		 * back up, then we risk missing enumerating that device when
+		 * we scan the bus. However, if the link was down to begin with,
+		 * there's no point waiting, so check now.
+		 */
+		link_present = mvebu_pcie_link_up(port);
+
  		mvebu_pcie_set_local_dev_nr(port, 1);

+		if (link_present) {
+			/* In testing, the link took ~25ms to go down, and
+			 * another ~15ms to come back up, so wait ~40ms for the
+			 * link to go down, and then up to ~100ms for it to come
+			 * back up. If the link doesn't go down after ~40ms, then
+			 * it probably won't go down at all, so carry on.
+			 */
+			if (mvebu_pcie_link_up(port)) {
+				while (retry_count < 40) {
+					if (!mvebu_pcie_link_up(port))
+						break;
+					retry_count++;
+					mdelay(1);
+				}
+
+				if (retry_count == 40) {
+					dev_dbg(&pdev->dev,
+						"PCIe%d.%d: after setting dev nr, link stayed up\n",
+						port->port, port->lane);
+				} else {
+					dev_info(&pdev->dev,
+						 "PCIe%d.%d: after setting dev nr, link went down after %d polls\n",
+						 port->port, port->lane, retry_count);
+				}
+			}
+
+			retry_count = 0;
+
+			if (!mvebu_pcie_link_up(port)) {
+				while (retry_count < 100) {
+					if (mvebu_pcie_link_up(port))
+						break;
+					retry_count++;
+					mdelay(1);
+				}
+
+				if (retry_count == 100) {
+					dev_info(&pdev->dev,
+						"PCIe%d.%d: after setting dev nr, link failed to come back up (timeout)\n",
+						port->port, port->lane);
+				} else {
+					dev_info(&pdev->dev,
+						 "PCIe%d.%d: after setting dev nr, link came back up after %d polls ",
+						 port->port, port->lane, retry_count);
+				}
+			}
+		}
+
  		port->dn = child;
  		spin_lock_init(&port->conf_lock);
  		mvebu_sw_pci_bridge_init(port);
-- 
1.8.3.2

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

[Jason Gunthorpe]

On Fri, Mar 28, 2014 at 01:03:55AM +0000, Neil Greatorex wrote:

> With the help you've given, I think I've identified what the problem
> is. It is basically what Thomas suggested the problem was (that he'd
> seen with the Armada 38x). The call to mvebu_pcie_set_local_dev_nr
> causes the PCIe link to reset. In my tests with this Intel card, it
> takes ~25ms for the link to go down, and then another ~13ms for it
> to come back up.

Wow, fascinating - not only does the link go down, but it takes 25ms,
and it seemed based on the patch I sent you, that the first config op
triggers this reset immediately..

If I have time I'll look in the spec, maybe the devnumber is encoded
in low level flow control packets or something that makes sense of
this..

What does your kernel boot log look like? Does the USB PEX link behave
the same?? Presumably it happens faster?

A different option would be to force the link down, then change the
dev number, then bring it back up. At least that way the process is
under the control of the driver and we are not waiting for the
mysterious 25 ms. Or maybe change the dev number, then trigger a
link reset...

> I have put together a patch below that I would be interested to get
> your opinions on and also for you to test against any cards you may
> have. I only have the one mini PCIe card so it's not a great test!

Unfortunately my systems here never boot with the PEX up so I cannot
easily verify..

One suggestion about the patch I have is to move everything you added
into the mvebu_pcie_set_local_dev_nr function and bail if the
devnumber is already correct. That way the bootloader could set it
properly and avoid this.

Regards,
Jason

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

[Neil Greatorex]

Thomas,

After managing to get the card detected on my mirabox, I have been looking 
into getting the thing actually working. With the information from Willy 
and my own investigations I knew there was some issue in the setting up of 
the MSIs for the card. Through adding a copious amount of printk()s I was 
able to determine that the initialisation for the igb driver allocates, 
frees and re-allocates 3 MSIs for the card. I noticed that in doing this 
there was a problem whereby any call to free an MSI was trying to free 
MSI#0. I was able to track this down to the fact that the mapping for the 
IRQ was being disposed before the MSI was actually freed. The below patch 
fixes this problem.

With this patch, I can get one port on the card working. With both ports 
enabled, I still get an OOPS, so some further work is needed.

I would appreciate it if you could test this patch and let me know if you 
find any problems.

Cheers,
Neil

>From 50aa11018059704229dd43ca1016defdda04f90c Mon Sep 17 00:00:00 2001
From: Neil Greatorex <neil@fatboyfat.co.uk>
Date: Fri, 4 Apr 2014 13:47:09 +0100
Subject: [PATCH] irqchip: armada-370-xp: Fix releasing of MSIs

This patch moves the call to irq_dispose_mapping() to after the call to
armada_370_xp_free_msi(). Without this patch, the armada_370_xp_free_msi
function would always free MSI#0, no matter what was passed to it.

Signed-off-by: <neil@fatboyfat.co.uk>
---
  drivers/irqchip/irq-armada-370-xp.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
index 5409564..f5e129e 100644
--- a/drivers/irqchip/irq-armada-370-xp.c
+++ b/drivers/irqchip/irq-armada-370-xp.c
@@ -157,8 +157,8 @@ static void armada_370_xp_teardown_msi_irq(struct msi_chip *chip,
  					   unsigned int irq)
  {
  	struct irq_data *d = irq_get_irq_data(irq);
-	irq_dispose_mapping(irq);
  	armada_370_xp_free_msi(d->hwirq);
+	irq_dispose_mapping(irq);
  }

  static struct irq_chip armada_370_xp_msi_irq_chip = {
-- 
1.8.3.2

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

[Willy Tarreau]

Hi Neil,

On Fri, Apr 04, 2014 at 02:19:44PM +0100, Neil Greatorex wrote:
> Thomas,
> 
> After managing to get the card detected on my mirabox, I have been looking 
> into getting the thing actually working. With the information from Willy 
> and my own investigations I knew there was some issue in the setting up of 
> the MSIs for the card. Through adding a copious amount of printk()s I was 
> able to determine that the initialisation for the igb driver allocates, 
> frees and re-allocates 3 MSIs for the card.

That sequence matches the memories I have about it indeed.

> I noticed that in doing this 
> there was a problem whereby any call to free an MSI was trying to free 
> MSI#0. I was able to track this down to the fact that the mapping for the 
> IRQ was being disposed before the MSI was actually freed. The below patch 
> fixes this problem.
> 
> With this patch, I can get one port on the card working. With both ports 
> enabled, I still get an OOPS, so some further work is needed.
> 
> I would appreciate it if you could test this patch and let me know if you 
> find any problems.

I don't know how to disable one port on my card (except by fiddling directly
with its eeprom but I don't want to do this anymore, there's a high risk of
bricking it). So I got the same panic as before with the two ports enabled.
However I tried to revert the prefetchable memory patch that makes the NIC
work for me and I noticed that your fix indeed improves things. After the
oops, I no longer get :

---[ end trace f6c1769398756b02 ]---
trying to free unused MSI#0
trying to free unused MSI#0
trying to free unused MSI#0
trying to free unused MSI#0
trying to free unused MSI#0
trying to free unused MSI#0

as I used to see with the revert alone. So yes I think it's a nice step forward!

Note that this was 3.13.9 on the XP-GP board (quad-core XP).

Feel free to add my Tested-by if you like.

Cheers,
Willy

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

[Thomas Petazzoni]

Dear Neil Greatorex,

On Fri, 4 Apr 2014 14:19:44 +0100 (BST), Neil Greatorex wrote:

> From 50aa11018059704229dd43ca1016defdda04f90c Mon Sep 17 00:00:00 2001
> From: Neil Greatorex <neil@fatboyfat.co.uk>
> Date: Fri, 4 Apr 2014 13:47:09 +0100
> Subject: [PATCH] irqchip: armada-370-xp: Fix releasing of MSIs
> 
> This patch moves the call to irq_dispose_mapping() to after the call to
> armada_370_xp_free_msi(). Without this patch, the armada_370_xp_free_msi
> function would always free MSI#0, no matter what was passed to it.
> 
> Signed-off-by: <neil@fatboyfat.co.uk>
> ---
>   drivers/irqchip/irq-armada-370-xp.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
> index 5409564..f5e129e 100644
> --- a/drivers/irqchip/irq-armada-370-xp.c
> +++ b/drivers/irqchip/irq-armada-370-xp.c
> @@ -157,8 +157,8 @@ static void armada_370_xp_teardown_msi_irq(struct msi_chip *chip,
>   					   unsigned int irq)
>   {
>   	struct irq_data *d = irq_get_irq_data(irq);
> -	irq_dispose_mapping(irq);
>   	armada_370_xp_free_msi(d->hwirq);
> +	irq_dispose_mapping(irq);
>   }
> 
>   static struct irq_chip armada_370_xp_msi_irq_chip = {

I want to give it some test, but as it is, I'd prefer to have the
irq_dispose_mapping() done before, and use a local variable to store
d->hwirq to that armada_370_xp_free_msi() can be called after
irq_dispose_mapping(). This is to ensure the sequence is symmetrical
with the MSI setup sequence.

Also, can you detail how you tested with just one port?

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

[Willy Tarreau]

Hi Thomas,

On Sat, Apr 05, 2014 at 07:34:35PM +0200, Thomas Petazzoni wrote:
> Also, can you detail how you tested with just one port?

OK I just found this ugly method, I'm not sure it's the best one but it
does the job :-) It simply refuses to configure any controller but the
first one on the chip.

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 46d31a4..88bf1d5 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -2208,6 +2208,9 @@ static int igb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 		return -EINVAL;
 	}
 
+	if (PCI_FUNC(pdev->devfn) != 0)
+		return -ENODEV;
+
 	err = pci_enable_device_mem(pdev);
 	if (err)
 		return err;

Best regards,
Willy

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

[Neil Greatorex]

Willy, Thomas,  

I have the igb module blacklisted so that it doesn?t get loaded during boot. Then I simply did:

# echo 1 > /sys/bus/pci/devices/0000\:01\:00.1/remove
# modprobe igb  

and it only fires up the first port. I have tried the reverse (and removed 0000:01:00.0 instead) but that gives the oops.

Cheers,
Neil


On Saturday, 5 April 2014 at 7:04pm, Willy Tarreau wrote:

> Hi Thomas,
>  
> On Sat, Apr 05, 2014 at 07:34:35PM +0200, Thomas Petazzoni wrote:
> > Also, can you detail how you tested with just one port?
>  
>  
>  
> OK I just found this ugly method, I'm not sure it's the best one but it
> does the job :-) It simply refuses to configure any controller but the
> first one on the chip.
>  
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index 46d31a4..88bf1d5 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -2208,6 +2208,9 @@ static int igb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> return -EINVAL;
> }
>  
> + if (PCI_FUNC(pdev->devfn) != 0)
> + return -ENODEV;
> +
> err = pci_enable_device_mem(pdev);
> if (err)
> return err;
>  
> Best regards,
> Willy

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

[Neil Greatorex]

Thomas, 


On Saturday, 5 April 2014 at 6:34pm, Thomas Petazzoni wrote:

> Dear Neil Greatorex,
> 
> On Fri, 4 Apr 2014 14:19:44 +0100 (BST), Neil Greatorex wrote:
> 
> > From 50aa11018059704229dd43ca1016defdda04f90c Mon Sep 17 00:00:00 2001
> > From: Neil Greatorex <neil at fatboyfat.co.uk (mailto:neil@fatboyfat.co.uk)>
> > Date: Fri, 4 Apr 2014 13:47:09 +0100
> > Subject: [PATCH] irqchip: armada-370-xp: Fix releasing of MSIs
> > 
> > This patch moves the call to irq_dispose_mapping() to after the call to
> > armada_370_xp_free_msi(). Without this patch, the armada_370_xp_free_msi
> > function would always free MSI#0, no matter what was passed to it.
> > 
> > Signed-off-by: <neil at fatboyfat.co.uk (mailto:neil@fatboyfat.co.uk)>
> > ---
> > drivers/irqchip/irq-armada-370-xp.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
> > index 5409564..f5e129e 100644
> > --- a/drivers/irqchip/irq-armada-370-xp.c
> > +++ b/drivers/irqchip/irq-armada-370-xp.c
> > @@ -157,8 +157,8 @@ static void armada_370_xp_teardown_msi_irq(struct msi_chip *chip,
> > unsigned int irq)
> > {
> > struct irq_data *d = irq_get_irq_data(irq);
> > - irq_dispose_mapping(irq);
> > armada_370_xp_free_msi(d->hwirq);
> > + irq_dispose_mapping(irq);
> > }
> > 
> > static struct irq_chip armada_370_xp_msi_irq_chip = {
> 
> I want to give it some test, but as it is, I'd prefer to have the
> irq_dispose_mapping() done before, and use a local variable to store
> d->hwirq to that armada_370_xp_free_msi() can be called after
> irq_dispose_mapping(). This is to ensure the sequence is symmetrical
> with the MSI setup sequence.

I will redo the patch with a local variable tomorrow and resend it. 
 
Cheers, 
Neil

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

[Willy Tarreau]

On Sat, Apr 05, 2014 at 07:55:03PM +0100, Neil Greatorex wrote:
> Willy, Thomas,  
> 
> I have the igb module blacklisted so that it doesn???t get loaded during
> boot. Then I simply did:
> 
> # echo 1 > /sys/bus/pci/devices/0000\:01\:00.1/remove
> # modprobe igb  

Hehe feeling stupid now :-)
Thanks for the trick, that's really useful.

> and it only fires up the first port. I have tried the reverse (and removed
> 0000:01:00.0 instead) but that gives the oops.

Quite interesting indeed.

Thanks,
Willy

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

[Neil Greatorex]

Thomas,

On Sat, 5 Apr 2014, Neil Greatorex wrote:

> I will redo the patch with a local variable tomorrow and resend it.
>

As promised, here is the updated patch. As before, I would appreciate 
comments and/or Tested-bys...

Cheers,
Neil

>From e5698a4ae6b21c7e78538e16d293123903abbb40 Mon Sep 17 00:00:00 2001
From: Neil Greatorex <neil@fatboyfat.co.uk>
Date: Sun, 6 Apr 2014 16:10:43 +0100
Subject: [PATCH] irqchip: armada-370-xp: Fix releasing of MSIs

Store the value of d->hwirq in a local variable as the real value is wiped out
by calling irq_dispose_mapping. Without this patch, the armada_370_xp_free_msi
function would always free MSI#0, no matter what was passed to it.

Signed-off-by: Neil Greatorex <neil@fatboyfat.co.uk>
---
  drivers/irqchip/irq-armada-370-xp.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
index 5409564..916fae2 100644
--- a/drivers/irqchip/irq-armada-370-xp.c
+++ b/drivers/irqchip/irq-armada-370-xp.c
@@ -157,8 +157,10 @@ static void armada_370_xp_teardown_msi_irq(struct msi_chip *chip,
  					   unsigned int irq)
  {
  	struct irq_data *d = irq_get_irq_data(irq);
+	unsigned long hwirq = d->hwirq;
+
  	irq_dispose_mapping(irq);
-	armada_370_xp_free_msi(d->hwirq);
+	armada_370_xp_free_msi(hwirq);
  }

  static struct irq_chip armada_370_xp_msi_irq_chip = {
-- 
1.8.3.2

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

[Willy Tarreau]

Hi Neil,

On Sun, Apr 06, 2014 at 04:34:08PM +0100, Neil Greatorex wrote:
> Thomas,
> 
> On Sat, 5 Apr 2014, Neil Greatorex wrote:
> 
> >I will redo the patch with a local variable tomorrow and resend it.
> >
> 
> As promised, here is the updated patch. As before, I would appreciate 
> comments and/or Tested-bys...

OK still works on XP-GP.

   Tested-by: Willy Tarreau <w@1wt.eu>

Willy

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

[Willy Tarreau]

Hi Neil,

On Fri, Apr 04, 2014 at 02:19:44PM +0100, Neil Greatorex wrote:
> With this patch, I can get one port on the card working. With both ports 
> enabled, I still get an OOPS, so some further work is needed.

Concerning this point, here's an update on my side. I found where the crash
happens, but I don't exactly understand why, I suspect that an area is not
correctly mapped :

root at xpgp:~# insmod /tmp/igb.ko 
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)
PCI: enabling device 0000:02:00.0 (0140 -> 0142)
request_region(pdev=edb21000, 00000049)
hw_addr = pci_iomap(pdev=edb21000, 0, 0) = f0300000
mem_start=e0000000 mem_end=e007ffff
hw_addr = f0300000
hw_addr=f0300000
igb 0000:02:00.0: added PHC on eth4
igb 0000:02:00.0: Intel(R) Gigabit Ethernet Network Connection
igb 0000:02:00.0: eth4: (PCIe:5.0Gb/s:Width x1) 00:30:18:a6:6c:6a
igb 0000:02:00.0: eth4: PBA No: FFFFFF-0FF
igb 0000:02:00.0: Using MSI-X interrupts. 4 rx queue(s), 4 tx queue(s)
PCI: enabling device 0000:02:00.1 (0140 -> 0142)
request_region(pdev=ed99d800, 00000049)
hw_addr = pci_iomap(pdev=ed99d800, 0, 0) = f0400000
mem_start=e0100000 mem_end=e017ffff
hw_addr = f0400000
hw_addr=f0400000
Unhandled fault: external abort on non-linefetch (0x1008) at 0xf0400018
Internal error: : 1008 [#1] SMP THUMB2

in e1000_82575.o:igb_get_invariants_82575() :
    123c:       f884 33b4       strb.w  r3, [r4, #948]
    1240:       f884 33b9       strb.w  r3, [r4, #953]
    1244:       f8c4 6300       str.w   r6, [r4, #768]
    1248:       6863            ldr     r3, [r4, #4]
=>  124a:       f8d3 8018       ldr.w   r8, [r3, #24]

in e1000_82575.c:igb_get_invariants_82575() :
        hw->phy.media_type = e1000_media_type_copper;
        dev_spec->sgmii_active = false;
        dev_spec->module_plugged = false;

here=>  ctrl_ext = rd32(E1000_CTRL_EXT);

according to e1000_hw.h:

  #define E1000_CTRL_EXT 0x00018      /* Extended Device Control - RW */

according to e1000_regs.h:

  #define rd32(reg) (readl(hw->hw_addr + reg))

===> ctrl_ext = readl((hw->hw_addr = 0xf0400000) + (reg = 0x18))

As you can see, the sequence is exactly the same for both ports. The
first one has no problem performing the readl(), but the second one
cannot. Both of them got the memory address returned by a call to
pci_iomap(dev, 0, 0). I could verify that the pci_resource_len() for
both is 524288 (0x80000).

The last "hwaddr=f0400000" is printed just before calling rd32() and
shows that it was still OK there. Since the resource flags are 0x40200,
we only have IORESOURCE_MEM so pci_iomap() calls ioremap_nocache().

Thus I suspect something is not behaving correctly in the code which
configures the emulated bridge and/or the memory areas, resulting in
the second port not being correctly mapped, thus causing the crash.

But that's the deepest I can go unfortunately, I got lost after that.

Regards,
Willy

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

[Thomas Petazzoni]

Dear Willy Tarreau,

On Sun, 6 Apr 2014 20:58:33 +0200, Willy Tarreau wrote:

> On Fri, Apr 04, 2014 at 02:19:44PM +0100, Neil Greatorex wrote:
> > With this patch, I can get one port on the card working. With both ports 
> > enabled, I still get an OOPS, so some further work is needed.
> 
> Concerning this point, here's an update on my side. I found where the crash
> happens, but I don't exactly understand why, I suspect that an area is not
> correctly mapped :
> 
> root at xpgp:~# insmod /tmp/igb.ko 
> 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)
> PCI: enabling device 0000:02:00.0 (0140 -> 0142)
> request_region(pdev=edb21000, 00000049)
> hw_addr = pci_iomap(pdev=edb21000, 0, 0) = f0300000
> mem_start=e0000000 mem_end=e007ffff
> hw_addr = f0300000
> hw_addr=f0300000
> igb 0000:02:00.0: added PHC on eth4
> igb 0000:02:00.0: Intel(R) Gigabit Ethernet Network Connection
> igb 0000:02:00.0: eth4: (PCIe:5.0Gb/s:Width x1) 00:30:18:a6:6c:6a
> igb 0000:02:00.0: eth4: PBA No: FFFFFF-0FF
> igb 0000:02:00.0: Using MSI-X interrupts. 4 rx queue(s), 4 tx queue(s)
> PCI: enabling device 0000:02:00.1 (0140 -> 0142)
> request_region(pdev=ed99d800, 00000049)
> hw_addr = pci_iomap(pdev=ed99d800, 0, 0) = f0400000
> mem_start=e0100000 mem_end=e017ffff
> hw_addr = f0400000
> hw_addr=f0400000
> Unhandled fault: external abort on non-linefetch (0x1008) at 0xf0400018
> Internal error: : 1008 [#1] SMP THUMB2
> 
> in e1000_82575.o:igb_get_invariants_82575() :
>     123c:       f884 33b4       strb.w  r3, [r4, #948]
>     1240:       f884 33b9       strb.w  r3, [r4, #953]
>     1244:       f8c4 6300       str.w   r6, [r4, #768]
>     1248:       6863            ldr     r3, [r4, #4]
> =>  124a:       f8d3 8018       ldr.w   r8, [r3, #24]
> 
> in e1000_82575.c:igb_get_invariants_82575() :
>         hw->phy.media_type = e1000_media_type_copper;
>         dev_spec->sgmii_active = false;
>         dev_spec->module_plugged = false;
> 
> here=>  ctrl_ext = rd32(E1000_CTRL_EXT);
> 
> according to e1000_hw.h:
> 
>   #define E1000_CTRL_EXT 0x00018      /* Extended Device Control - RW */
> 
> according to e1000_regs.h:
> 
>   #define rd32(reg) (readl(hw->hw_addr + reg))
> 
> ===> ctrl_ext = readl((hw->hw_addr = 0xf0400000) + (reg = 0x18))
> 
> As you can see, the sequence is exactly the same for both ports. The
> first one has no problem performing the readl(), but the second one
> cannot. Both of them got the memory address returned by a call to
> pci_iomap(dev, 0, 0). I could verify that the pci_resource_len() for
> both is 524288 (0x80000).
> 
> The last "hwaddr=f0400000" is printed just before calling rd32() and
> shows that it was still OK there. Since the resource flags are 0x40200,
> we only have IORESOURCE_MEM so pci_iomap() calls ioremap_nocache().
> 
> Thus I suspect something is not behaving correctly in the code which
> configures the emulated bridge and/or the memory areas, resulting in
> the second port not being correctly mapped, thus causing the crash.
> 
> But that's the deepest I can go unfortunately, I got lost after that.

Thanks a lot again for all this investigation. I'm hoping to be able to
look at this PCIe issue this week.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

[Neil Greatorex]

Willy,

On Sun, 6 Apr 2014, Willy Tarreau wrote:

> As you can see, the sequence is exactly the same for both ports. The
> first one has no problem performing the readl(), but the second one
> cannot. Both of them got the memory address returned by a call to
> pci_iomap(dev, 0, 0). I could verify that the pci_resource_len() for
> both is 524288 (0x80000).
>
> The last "hwaddr=f0400000" is printed just before calling rd32() and
> shows that it was still OK there. Since the resource flags are 0x40200,
> we only have IORESOURCE_MEM so pci_iomap() calls ioremap_nocache().
>

Good work - I've been doing similar things myself! I can confirm that I 
see exactly the same thing with similar printks:

First port:
[ 1809.452878] igb 0000:01:00.0: enabling bus mastering
[ 1809.453098] igb 0000:01:00.0 (unregistered net_device): hw_addr is 
f1000000, start=e0000000, len=80000, flags=40200
[ 1809.453109] igb 0000:01:00.0 (unregistered net_device): About to read 
from offset 18
[ 1809.453120] igb 0000:01:00.0 (unregistered net_device): Read from 18 
returned 1400c0

Second port:
[ 1809.459445] igb 0000:01:00.1: enabling bus mastering
[ 1809.459563] igb 0000:01:00.1 (unregistered net_device): hw_addr is
f1100000, start=e0100000, len=80000, flags=40200
[ 1809.459573] igb 0000:01:00.1 (unregistered net_device): About to read
from offset 18
[ 1809.459581] Unhandled fault: external abort on non-linefetch (0x1008) 
at 0xf1100018

In the output above, the start= part shows the physical address and 
hw_addr shows the mapped address.

The physical addresses match those given in the lspci -vvv output (see 
https://gist.github.com/ngreatorex/9772195). I don't know enough about 
PCIe, the SoC *or* the Intel card to know if these addresses look correct 
or even sane! I did wonder if there was some issue due to the fact that 
the resources for 01:00.0 and 01:00.1 overlap, but I would guess(!?) that 
it's common in hardware that presents multiple devices.

It is perhaps noteworthy that this is the first access to the hardware for 
the 2nd port - i.e. there are no successful accesses, other than to enable 
the hardware, which AFAICT is simply accessing registers on the PCIe 
controller.

> Thus I suspect something is not behaving correctly in the code which
> configures the emulated bridge and/or the memory areas, resulting in
> the second port not being correctly mapped, thus causing the crash.
>
> But that's the deepest I can go unfortunately, I got lost after that.
>

Same here. I've tried playing around with a few things but not discovered 
anything even close to useful. Hopefully Thomas will be able to debug 
further when he gets the time.

> Regards,
> Willy
>
>

Cheers,
Neil

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

[Willy Tarreau]

On Sun, Apr 06, 2014 at 10:57:40PM +0100, Neil Greatorex wrote:
> Good work - I've been doing similar things myself! I can confirm that I 
> see exactly the same thing with similar printks:

Great! A reproduceable bug is always half-resolved :-)

> The physical addresses match those given in the lspci -vvv output

same here.

> (see 
> https://gist.github.com/ngreatorex/9772195). I don't know enough about 
> PCIe, the SoC *or* the Intel card to know if these addresses look correct 
> or even sane! I did wonder if there was some issue due to the fact that 
> the resources for 01:00.0 and 01:00.1 overlap, but I would guess(!?) that 
> it's common in hardware that presents multiple devices.

My understanding and old memories tell me that's OK.

> It is perhaps noteworthy that this is the first access to the hardware for 
> the 2nd port - i.e. there are no successful accesses, other than to enable 
> the hardware, which AFAICT is simply accessing registers on the PCIe 
> controller.

Indeed, I forgot to mention that but you're right, enabling only the
second function leads to an immediate panic as well.

> I've tried playing around with a few things but not discovered 
> anything even close to useful. Hopefully Thomas will be able to debug 
> further when he gets the time.

Yeah, let's hope we have not opened a can of worms!

Willy

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

[Thomas Petazzoni]

Dear Neil Greatorex,

On Sun, 6 Apr 2014 22:57:40 +0100 (BST), Neil Greatorex wrote:

> Second port:
> [ 1809.459445] igb 0000:01:00.1: enabling bus mastering
> [ 1809.459563] igb 0000:01:00.1 (unregistered net_device): hw_addr is
> f1100000, start=e0100000, len=80000, flags=40200
> [ 1809.459573] igb 0000:01:00.1 (unregistered net_device): About to read
> from offset 18
> [ 1809.459581] Unhandled fault: external abort on non-linefetch (0x1008) 
> at 0xf1100018

This address is a virtual address, so there's not much we can do with
it, as it is.

> The physical addresses match those given in the lspci -vvv output (see 
> https://gist.github.com/ngreatorex/9772195). I don't know enough about 
> PCIe, the SoC *or* the Intel card to know if these addresses look correct 
> or even sane! I did wonder if there was some issue due to the fact that 
> the resources for 01:00.0 and 01:00.1 overlap, but I would guess(!?) that 
> it's common in hardware that presents multiple devices.
> 
> It is perhaps noteworthy that this is the first access to the hardware for 
> the 2nd port - i.e. there are no successful accesses, other than to enable 
> the hardware, which AFAICT is simply accessing registers on the PCIe 
> controller.

It really looks like the MBus window has not been sized correctly, or
there is a missing MBus window. Probably a deficiency in the PCI bridge
emulation layer in pci-mvebu.

Tomorrow, I have a bit of work on AHCI on Armada 38x, but I'm hoping to
get to this after that.

Thanks again for all the investigation. All your research is clearly
going to make the debugging a *lot* easier.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

[Neil Greatorex]

Thomas,

On Sun, Apr 6, 2014 at 11:16 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Dear Neil Greatorex,
>
> On Sun, 6 Apr 2014 22:57:40 +0100 (BST), Neil Greatorex wrote:
>
>> Second port:
>> [ 1809.459445] igb 0000:01:00.1: enabling bus mastering
>> [ 1809.459563] igb 0000:01:00.1 (unregistered net_device): hw_addr is
>> f1100000, start=e0100000, len=80000, flags=40200
>> [ 1809.459573] igb 0000:01:00.1 (unregistered net_device): About to read
>> from offset 18
>> [ 1809.459581] Unhandled fault: external abort on non-linefetch (0x1008)
>> at 0xf1100018
>
> This address is a virtual address, so there's not much we can do with
> it, as it is.
>

The address in the fault message is, but the physical address can be
worked out from the 2 lines above. "start=e0100000" and "offset 18".
Adding those together we get the physical address 0xe100018.

>> The physical addresses match those given in the lspci -vvv output (see
>> https://gist.github.com/ngreatorex/9772195). I don't know enough about
>> PCIe, the SoC *or* the Intel card to know if these addresses look correct
>> or even sane! I did wonder if there was some issue due to the fact that
>> the resources for 01:00.0 and 01:00.1 overlap, but I would guess(!?) that
>> it's common in hardware that presents multiple devices.
>>
>> It is perhaps noteworthy that this is the first access to the hardware for
>> the 2nd port - i.e. there are no successful accesses, other than to enable
>> the hardware, which AFAICT is simply accessing registers on the PCIe
>> controller.
>
> It really looks like the MBus window has not been sized correctly, or
> there is a missing MBus window. Probably a deficiency in the PCI bridge
> emulation layer in pci-mvebu.
>

OK well that gives me another direction to point my debugging
attempts. If I discover anything interesting I'll let you know.

> Tomorrow, I have a bit of work on AHCI on Armada 38x, but I'm hoping to
> get to this after that.

Excellent news.

>
> Thanks again for all the investigation. All your research is clearly
> going to make the debugging a *lot* easier.

No problem. I'm always interested in trying to learn more about the
hardware I have and how it all works. Of course it would be much
easier if Marvell would publicly release the datasheet but we can't
have everything!

Cheers,
Neil

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

[Jason Gunthorpe]

On Sun, Apr 06, 2014 at 10:57:40PM +0100, Neil Greatorex wrote:
> Willy,
> 
> On Sun, 6 Apr 2014, Willy Tarreau wrote:
> 
> >As you can see, the sequence is exactly the same for both ports. The
> >first one has no problem performing the readl(), but the second one
> >cannot. Both of them got the memory address returned by a call to
> >pci_iomap(dev, 0, 0). I could verify that the pci_resource_len() for
> >both is 524288 (0x80000).
> >
> >The last "hwaddr=f0400000" is printed just before calling rd32() and
> >shows that it was still OK there. Since the resource flags are 0x40200,
> >we only have IORESOURCE_MEM so pci_iomap() calls ioremap_nocache().
> >
> 
> Good work - I've been doing similar things myself! I can confirm
> that I see exactly the same thing with similar printks:
> 
> First port:
> [ 1809.452878] igb 0000:01:00.0: enabling bus mastering
> [ 1809.453098] igb 0000:01:00.0 (unregistered net_device): hw_addr
> is f1000000, start=e0000000, len=80000, flags=40200
> [ 1809.453109] igb 0000:01:00.0 (unregistered net_device): About to
> read from offset 18
> [ 1809.453120] igb 0000:01:00.0 (unregistered net_device): Read from
> 18 returned 1400c0
> 
> Second port:
> [ 1809.459445] igb 0000:01:00.1: enabling bus mastering
> [ 1809.459563] igb 0000:01:00.1 (unregistered net_device): hw_addr is
> f1100000, start=e0100000, len=80000, flags=40200
> [ 1809.459573] igb 0000:01:00.1 (unregistered net_device): About to read
> from offset 18
> [ 1809.459581] Unhandled fault: external abort on non-linefetch
> (0x1008) at 0xf1100018
> 
> In the output above, the start= part shows the physical address and
> hw_addr shows the mapped address.

This is very similar to what Matthew Minter
<matthew_minter@xyratex.com> is seeing on Hot Plug with AHCI. (See
'Armada XP (mvebu) PCIe memory (BAR/window) re-allocation' thread)

That probably says it is somehow mbus related - dumping the mbus
registers when the fault happens should clarify that point. The size
would a good place to check first.

> The physical addresses match those given in the lspci -vvv output
> (see https://gist.github.com/ngreatorex/9772195). I don't know
> enough about PCIe, the SoC *or* the Intel card to know if these
> addresses look correct or even sane! I did wonder if there was some
> issue due to the fact that the resources for 01:00.0 and 01:00.1
> overlap, but I would guess(!?) that it's common in hardware that
> presents multiple devices.

Which overlap?

To be very clear, PCI BARs, should never overlap.

The bridge windows should fully contain downstream bars:

00:01.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01) (prog-if 00 [Normal decode])
	Bus: primary=00, secondary=01, subordinate=02, sec-latency=0
	Memory behind bridge: e0000000-e02fffff
01:00.0 Ethernet controller: Intel Corporation I350 Gigabit Network Connection (rev 01)
	Region 0: Memory at e0000000 (32-bit, non-prefetchable) [disabled] [size=512K]
01:00.1 Ethernet controller: Intel Corporation I350 Gigabit Network Connection (rev 01)
	Region 0: Memory at e0100000 (32-bit, non-prefetchable) [disabled] [size=512K]

Looks good to me.

HOWEVER, looking now very closely:

00:01.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01) (prog-if 00 [Normal decode])
   Memory behind bridge: e0000000-e02fffff
00:02.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01) (prog-if 00 [Normal decode])
   Memory behind bridge: e0300000-e03fffff

This is certainly wrong, MBUS requires special alignment and sizing.
0x300000 is not a size which is a power of two, and the next window
starts right after.

We need to see the first bridge use e0000000-e03fffff

Just to confirm, what does something like the below say for you guys?

diff --git a/drivers/bus/mvebu-mbus.c b/drivers/bus/mvebu-mbus.c
index 2ac754e..dfeb0dd 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,8 @@ 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));
+
        ctrl = ((size - 1) & WIN_CTRL_SIZE_MASK) |
                (attr << WIN_CTRL_ATTR_SHIFT)    |
                (target << WIN_CTRL_TGT_SHIFT)   |


Regards,
Jason

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

[Neil Greatorex]

Jason, Thomas,

On Mon, Apr 7, 2014 at 6:41 PM, Jason Gunthorpe
<jgunthorpe@obsidianresearch.com> wrote:
>>
>> First port:
>> [ 1809.452878] igb 0000:01:00.0: enabling bus mastering
>> [ 1809.453098] igb 0000:01:00.0 (unregistered net_device): hw_addr
>> is f1000000, start=e0000000, len=80000, flags=40200
>> [ 1809.453109] igb 0000:01:00.0 (unregistered net_device): About to
>> read from offset 18
>> [ 1809.453120] igb 0000:01:00.0 (unregistered net_device): Read from
>> 18 returned 1400c0
>>
>> Second port:
>> [ 1809.459445] igb 0000:01:00.1: enabling bus mastering
>> [ 1809.459563] igb 0000:01:00.1 (unregistered net_device): hw_addr is
>> f1100000, start=e0100000, len=80000, flags=40200
>> [ 1809.459573] igb 0000:01:00.1 (unregistered net_device): About to read
>> from offset 18
>> [ 1809.459581] Unhandled fault: external abort on non-linefetch
>> (0x1008) at 0xf1100018
>>
>> In the output above, the start= part shows the physical address and
>> hw_addr shows the mapped address.
>
> This is very similar to what Matthew Minter
> <matthew_minter@xyratex.com> is seeing on Hot Plug with AHCI. (See
> 'Armada XP (mvebu) PCIe memory (BAR/window) re-allocation' thread)
>
> That probably says it is somehow mbus related - dumping the mbus
> registers when the fault happens should clarify that point. The size
> would a good place to check first.
>
>> The physical addresses match those given in the lspci -vvv output
>> (see https://gist.github.com/ngreatorex/9772195). I don't know
>> enough about PCIe, the SoC *or* the Intel card to know if these
>> addresses look correct or even sane! I did wonder if there was some
>> issue due to the fact that the resources for 01:00.0 and 01:00.1
>> overlap, but I would guess(!?) that it's common in hardware that
>> presents multiple devices.
>
> Which overlap?
>
> To be very clear, PCI BARs, should never overlap.
>

I realise that overlap was probably the wrong word. I meant that the
resources for 01:00.0 and 01:00.1 are not contiguous but are mixed
together. If you sort by address you get:

e0000000-e007ffff : 0000:01:00.0
e0080000-e00fffff : 0000:01:00.0
e0100000-e017ffff : 0000:01:00.1
e0180000-e01fffff : 0000:01:00.1
e0200000-e0203fff : 0000:01:00.0
e0204000-e0223fff : 0000:01:00.0
e0224000-e0243fff : 0000:01:00.0
e0244000-e0247fff : 0000:01:00.1
e0248000-e0267fff : 0000:01:00.1
e0268000-e0287fff : 0000:01:00.1

> The bridge windows should fully contain downstream bars:
>
> 00:01.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01) (prog-if 00 [Normal decode])
>         Bus: primary=00, secondary=01, subordinate=02, sec-latency=0
>         Memory behind bridge: e0000000-e02fffff
> 01:00.0 Ethernet controller: Intel Corporation I350 Gigabit Network Connection (rev 01)
>         Region 0: Memory at e0000000 (32-bit, non-prefetchable) [disabled] [size=512K]
> 01:00.1 Ethernet controller: Intel Corporation I350 Gigabit Network Connection (rev 01)
>         Region 0: Memory at e0100000 (32-bit, non-prefetchable) [disabled] [size=512K]
>
> Looks good to me.
>
> HOWEVER, looking now very closely:
>
> 00:01.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01) (prog-if 00 [Normal decode])
>    Memory behind bridge: e0000000-e02fffff
> 00:02.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01) (prog-if 00 [Normal decode])
>    Memory behind bridge: e0300000-e03fffff
>
> This is certainly wrong, MBUS requires special alignment and sizing.
> 0x300000 is not a size which is a power of two, and the next window
> starts right after.
>

Interesting. Does the PCI code provide a way to specify that the sizes
much be a power of 2? I don't fully understand the implications but
would it be possible to assign just one MBUS window for the whole of
the PCIe memory instead?

> We need to see the first bridge use e0000000-e03fffff
>
> Just to confirm, what does something like the below say for you guys?

See https://gist.github.com/ngreatorex/10025253 for the dmesg output.
I have also included the contents of
/sys/kernel/debug/mvebu-mbus/devices both before and after the
modprobe / oops. As you can see I get a total of 3 WARNINGs - one at
boot for the xHCI controller, and two when inserting igb.ko. Note that
this time I did this with both ports enabled.

Cheers,
Neil

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

[Jason Gunthorpe]

On Mon, Apr 07, 2014 at 08:41:52PM +0100, Neil Greatorex wrote:
> > To be very clear, PCI BARs, should never overlap.
> 
> I realise that overlap was probably the wrong word. I meant that the
> resources for 01:00.0 and 01:00.1 are not contiguous but are mixed
> together. If you sort by address you get:
> 
> e0000000-e007ffff : 0000:01:00.0
> e0080000-e00fffff : 0000:01:00.0
> e0100000-e017ffff : 0000:01:00.1
> e0180000-e01fffff : 0000:01:00.1
> e0200000-e0203fff : 0000:01:00.0
> e0204000-e0223fff : 0000:01:00.0
> e0224000-e0243fff : 0000:01:00.0
> e0244000-e0247fff : 0000:01:00.1
> e0248000-e0267fff : 0000:01:00.1
> e0268000-e0287fff : 0000:01:00.1

Yes, that is fine. It is just a quirk of the allocator.
They are all within the downstream bridge window. 
 
> > HOWEVER, looking now very closely:
> >
> > 00:01.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01) (prog-if 00 [Normal decode])
> >    Memory behind bridge: e0000000-e02fffff
> > 00:02.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01) (prog-if 00 [Normal decode])
> >    Memory behind bridge: e0300000-e03fffff
> >
> > This is certainly wrong, MBUS requires special alignment and sizing.
> > 0x300000 is not a size which is a power of two, and the next window
> > starts right after.

> Interesting. Does the PCI code provide a way to specify that the sizes
> much be a power of 2? I don't fully understand the implications but
> would it be possible to assign just one MBUS window for the whole of
> the PCIe memory instead?

I know this has come up before, but I don't recall where things
settled out.. mvebu_pcie_align_resource is the function to look at,
the size at that point needs to be rounded up to a power of two and
communicated back to the caller.

Alternately, I belive Thomas once discussed using multiple mbus
windows for these non-aligned requests.

> > Just to confirm, what does something like the below say for you guys?
> 
> See https://gist.github.com/ngreatorex/10025253 for the dmesg output.
> I have also included the contents of
> /sys/kernel/debug/mvebu-mbus/devices both before and after the
> modprobe / oops. As you can see I get a total of 3 WARNINGs - one at
> boot for the xHCI controller, and two when inserting igb.ko. Note that
> this time I did this with both ports enabled.

Yep, that is certainly the root problem - most likely for
everyone. This also explains why Will saw success when he reverted
that unrelated patch. That change altered the allocation pattern of
the BARs and it just so happened to make things fall out properly.

We should also fix mvebu_mbus_read_window, so debugfs reports the
actual HW function. Instead of this:
        *size = (ctrlreg | ~WIN_CTRL_SIZE_MASK) + 1;

Something with ffs is required, perhaps (untested):
        *size = 2 << ffs(~(ctrlreg | ~WIN_CTRL_SIZE_MASK));

Thomas: Do you think the WARN_ON should head into mainline?

Jason

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

[Neil Greatorex]

Jason, Thomas, Willy,

On Mon, 7 Apr 2014, Jason Gunthorpe wrote:

>>> HOWEVER, looking now very closely:
>>>
>>> 00:01.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01) (prog-if 00 [Normal decode])
>>>    Memory behind bridge: e0000000-e02fffff
>>> 00:02.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01) (prog-if 00 [Normal decode])
>>>    Memory behind bridge: e0300000-e03fffff
>>>
>>> This is certainly wrong, MBUS requires special alignment and sizing.
>>> 0x300000 is not a size which is a power of two, and the next window
>>> starts right after.
>
>> Interesting. Does the PCI code provide a way to specify that the sizes
>> much be a power of 2? I don't fully understand the implications but
>> would it be possible to assign just one MBUS window for the whole of
>> the PCIe memory instead?
>
> I know this has come up before, but I don't recall where things
> settled out.. mvebu_pcie_align_resource is the function to look at,
> the size at that point needs to be rounded up to a power of two and
> communicated back to the caller.
>
> Alternately, I belive Thomas once discussed using multiple mbus
> windows for these non-aligned requests.
>
>>> Just to confirm, what does something like the below say for you guys?
>>
>> See https://gist.github.com/ngreatorex/10025253 for the dmesg output.
>> I have also included the contents of
>> /sys/kernel/debug/mvebu-mbus/devices both before and after the
>> modprobe / oops. As you can see I get a total of 3 WARNINGs - one at
>> boot for the xHCI controller, and two when inserting igb.ko. Note that
>> this time I did this with both ports enabled.
>
> Yep, that is certainly the root problem - most likely for
> everyone. This also explains why Will saw success when he reverted
> that unrelated patch. That change altered the allocation pattern of
> the BARs and it just so happened to make things fall out properly.
>

I have finally managed to get the card working on both ports! Of course, 
to do so I have added some nice kludges to the code that now need to be 
implemented properly, but it is verification of what the problem is and 
how to fix it!

I have included the patch at the end of this e-mail. It probably won't 
apply cleanly for you as I have other dev_dbg calls in pci-mvebu.c.

What I did was to alter mvebu_pcie_align_resource to make the bridge 
memory resource aligned to 4M. This had the effect that the 2nd bridge to 
the xHCI controller was bumped to address 0xe0400000 instead of 
0xe0300000. I then also made it so that when we request the MBUS window to 
be set up we ensure that the size is a power of 2. This has the effect of 
creating the windows and addresses how we want them:

Relevant part of lspci -vvv:

00:01.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01) 
(prog-if 00 [Normal decode])
         Memory behind bridge: e0000000-e02fffff

00:02.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01) 
(prog-if 00 [Normal decode])
         Memory behind bridge: e0400000-e04fffff

cat /sys/kernel/debug/mvebu-mbus/devices:

[00] 00000000e8010000 - 00000000e8020000 : 0004:00e0 (remap 
0000000000010000)
[01] disabled
[02] disabled
[03] disabled
[04] disabled
[05] disabled
[06] disabled
[07] disabled
[08] 00000000fff00000 - 0000000100000000 : 0001:00e0
[09] 00000000e0400000 - 00000000e0500000 : 0008:00e8
[10] 00000000e0000000 - 00000000e0400000 : 0004:00e8
[11] disabled
[12] disabled
[13] disabled
[14] disabled
[15] disabled
[16] disabled
[17] disabled
[18] disabled
[19] disabled

Now, over to the experts to implement this properly :-)

Thanks to Jason, Thomas and Willy for your help with tracking this down.

Cheers,
Neil


>From e68870135cd54609d0421746ccdc1cb58ebbd4dd Mon Sep 17 00:00:00 2001
From: Neil Greatorex <neil@fatboyfat.co.uk>
Date: Mon, 7 Apr 2014 22:33:22 +0100
Subject: [PATCH] pci: host: mvebu - Test fix for Mirabox PCI issues

Ensure that bridge MBUS window is aligned at 4M to bump up 2nd bridge to address
e0400000 instead of e0300000. Also ensure that when we request the MBUS window,
it's size is a power of 2.
---
  drivers/pci/host/pci-mvebu.c | 5 ++++-
  1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index afd0dce..5e0144c 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -364,6 +364,9 @@ static void mvebu_pcie_handle_membase_change(struct mvebu_pcie_port *port)
  		(((port->bridge.memlimit & 0xFFF0) << 16) | 0xFFFFF) -
  		port->memwin_base;

+	if (!is_power_of_2(port->memwin_size))
+		port->memwin_size = 1 << fls(port->memwin_size);
+
  	mvebu_mbus_add_window_by_id(port->mem_target, port->mem_attr,
  				    port->memwin_base, port->memwin_size);
  }
@@ -728,7 +731,7 @@ static resource_size_t mvebu_pcie_align_resource(struct pci_dev *dev,
  	if (res->flags & IORESOURCE_IO)
  		return round_up(start, max_t(resource_size_t, SZ_64K, size));
  	else if (res->flags & IORESOURCE_MEM)
-		return round_up(start, max_t(resource_size_t, SZ_1M, size));
+		return round_up(start, max_t(resource_size_t, SZ_4M, size));
  	else
  		return start;
  }
-- 
1.8.3.2

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

[Willy Tarreau]

Hi Neil,

On Mon, Apr 07, 2014 at 10:58:36PM +0100, Neil Greatorex wrote:
> I have finally managed to get the card working on both ports! Of course, 
> to do so I have added some nice kludges to the code that now need to be 
> implemented properly, but it is verification of what the problem is and 
> how to fix it!
> 
> I have included the patch at the end of this e-mail. It probably won't 
> apply cleanly for you as I have other dev_dbg calls in pci-mvebu.c.
> 
> What I did was to alter mvebu_pcie_align_resource to make the bridge 
> memory resource aligned to 4M. This had the effect that the 2nd bridge to 
> the xHCI controller was bumped to address 0xe0400000 instead of 
> 0xe0300000. I then also made it so that when we request the MBUS window to 
> be set up we ensure that the size is a power of 2. This has the effect of 
> creating the windows and addresses how we want them:
> 
> Relevant part of lspci -vvv:
> 
> 00:01.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01) 
> (prog-if 00 [Normal decode])
>         Memory behind bridge: e0000000-e02fffff
> 
> 00:02.0 PCI bridge: Marvell Technology Group Ltd. Device 6710 (rev 01) 
> (prog-if 00 [Normal decode])
>         Memory behind bridge: e0400000-e04fffff
> 
> cat /sys/kernel/debug/mvebu-mbus/devices:
> 
> [00] 00000000e8010000 - 00000000e8020000 : 0004:00e0 (remap 
> 0000000000010000)
> [01] disabled
> [02] disabled
> [03] disabled
> [04] disabled
> [05] disabled
> [06] disabled
> [07] disabled
> [08] 00000000fff00000 - 0000000100000000 : 0001:00e0
> [09] 00000000e0400000 - 00000000e0500000 : 0008:00e8
> [10] 00000000e0000000 - 00000000e0400000 : 0004:00e8
> [11] disabled
> [12] disabled
> [13] disabled
> [14] disabled
> [15] disabled
> [16] disabled
> [17] disabled
> [18] disabled
> [19] disabled
> 
> Now, over to the experts to implement this properly :-)
> 
> Thanks to Jason, Thomas and Willy for your help with tracking this down.

Well, on the XPGP board, it made some progress, but now I'm getting
another crash related to IRQs again when both ports are enabled (note
that I do have your other MSI fix). However, enabling only the second
port works now, so I guess it's just an IRQ assignment issue which is
killing it.

Here's what the bus looks like with your patch :

root at xpgp:~# lspci -tvnn
-[0000:00]-+-01.0-[01]--
           +-09.0-[02]--+-00.0  Intel Corporation Device [8086:1521]
           |            \-00.1  Intel Corporation Device [8086:1521]
           \-0a.0-[03]--

root at xpgp:~# lspci -vvv | egrep -i '(^0|memory)'
00:01.0 PCI bridge: Marvell Technology Group Ltd. Device 7846 (rev 02) (prog-if 00 [Normal decode])
        Memory behind bridge: fff00000-000fffff
        Prefetchable memory behind bridge: 00000000-000fffff
00:09.0 PCI bridge: Marvell Technology Group Ltd. Device 7846 (rev 02) (prog-if 00 [Normal decode])
        Memory behind bridge: e0000000-e02fffff
        Prefetchable memory behind bridge: 00000000-000fffff
00:0a.0 PCI bridge: Marvell Technology Group Ltd. Device 7846 (rev 02) (prog-if 00 [Normal decode])
        Memory behind bridge: fff00000-000fffff
        Prefetchable memory behind bridge: 00000000-000fffff
02:00.0 Ethernet controller: Intel Corporation Device 1521 (rev 01)
        Region 0: Memory at e0000000 (32-bit, non-prefetchable) [disabled] [size=512K]
        Region 3: Memory at e0200000 (32-bit, non-prefetchable) [disabled] [size=16K]
02:00.1 Ethernet controller: Intel Corporation Device 1521 (rev 01)
        Region 0: Memory at e0100000 (32-bit, non-prefetchable) [disabled] [size=512K]
        Region 3: Memory at e0204000 (32-bit, non-prefetchable) [disabled] [size=16K]

I don't know if it's normal to see bridges 00:01.0 and 00:0a.0 overlap
their areas or not. Maybe it's just because they're not configured.
The second bridge seems to correctly cover the IGB's regions though.
Also noteworthy, I get the exact same output when leaving SZ_1M instead
of SZ_4M in your patch. Thus I think that the real part of the fix is this
one :

	if (!is_power_of_2(port->memwin_size))
		port->memwin_size = 1 << fls(port->memwin_size);

BTW, this could be simplified this way (which also happens to be more
readable) which I could verify also works :

	port->memwin_size = roundup_pow_of_two(port->memwin_size);
	
Concerning the panic with the two ports enabled, I suspect that it's again
an issue related to the way IRQs are registered and rolled back in case of
error.

Before the patch :
PCI: enabling device 0000:02:00.1 (0140 -> 0142)
Unhandled fault: external abort on non-linefetch (0x1008) at 0xf0400018
Internal error: : 1008 [#1] SMP THUMB2
Modules linked in: igb(+) i2c_algo_bit
CPU: 1 PID: 1250 Comm: modprobe Not tainted 3.14.0-mvebu #6
task: c74b0e40 ti: c751c000 task.ti: c751c000
PC is at igb_get_invariants_82575+0x75/0x894 [igb]
LR is at igb_probe+0x22a/0xb80 [igb]
...

After the patch :
PCI: enabling device 0000:02:00.1 (0140 -> 0142)
------------[ cut here ]------------
WARNING: CPU: 0 PID: 1266 at kernel/irq/irqdomain.c:277 irq_domain_associate+0xb9/0x110()
error: hwirq 0xffffffe4 is too large for armada_370_xp_msi_irq
Modules linked in: igb(+) i2c_algo_bit
CPU: 0 PID: 1266 Comm: modprobe Not tainted 3.14.0-mvebu #4
[<c0011c39>] (unwind_backtrace) from [<c000f20b>] (show_stack+0xb/0xc)
[<c000f20b>] (show_stack) from [<c02b5cbb>] (dump_stack+0x4f/0x64)
[<c02b5cbb>] (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 [<c0044e81>] (irq_domain_associate+0xb9/0x110)
[<c0044e81>] (irq_domain_associate) from [<c0044f1d>] (irq_create_mapping+0x45/0xa0)
[<c0044f1d>] (irq_create_mapping) from [<c016fd2d>] (armada_370_xp_setup_msi_irq+0x35/0x80)
[<c016fd2d>] (armada_370_xp_setup_msi_irq) from [<c0185243>] (arch_setup_msi_irq+0x17/0x2c)
[<c0185243>] (arch_setup_msi_irq) from [<c018530d>] (arch_setup_msi_irqs+0x39/0x4c)
[<c018530d>] (arch_setup_msi_irqs) from [<c01858bd>] (pci_enable_msix+0x195/0x2b0)
[<c01858bd>] (pci_enable_msix) from [<bf80617b>] (igb_msix_other+0x8de/0xb44 [igb])
[<bf80617b>] (igb_msix_other [igb]) from [<bf806dff>] (igb_probe+0x37a/0xb80 [igb])
[<bf806dff>] (igb_probe [igb]) from [<c017d185>] (pci_device_probe+0x45/0x6c)
...
Unable to handle kernel NULL pointer dereference at virtual address 00000024
pgd = ed9a0000
[00000024] *pgd=074b3831, *pte=00000000, *ppte=00000000
Internal error: Oops: 17 [#1] SMP THUMB2
Modules linked in: igb(+) i2c_algo_bit
CPU: 0 PID: 1266 Comm: modprobe Tainted: G        W    3.14.0-mvebu #4
task: ed97aec0 ti: c75be000 task.ti: c75be000
PC is at igb_set_mac+0x5d/0x164 [igb]
LR is at igb_set_mac+0xaa/0x164 [igb]
pc : [<bf80489e>]    lr : [<bf8048eb>]    psr: 200f0033
sp : c75bfce8  ip : 00000000  fp : ec938898
r10: bf816950  r9 : 00000001  r8 : ec938440
r7 : edadc868  r6 : 00000008  r5 : ec938440  r4 : 00000006
r3 : 00000000  r2 : 80000000  r1 : ec93845c  r0 : ec938440
Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA Thumb  Segment user
Control: 50c53c7d  Table: 2d9a006a  DAC: 00000015
Process modprobe (pid: 1266, stack limit = 0xc75be240)
...
[<bf80489e>] (igb_set_mac [igb]) from [<bf8048eb>] (igb_set_mac+0xaa/0x164 [igb])
[<bf8048eb>] (igb_set_mac [igb]) from [<bf806183>] (igb_msix_other+0x8e6/0xb44 [igb])
[<bf806183>] (igb_msix_other [igb]) from [<bf806dff>] (igb_probe+0x37a/0xb80 [igb])
[<bf806dff>] (igb_probe [igb]) from [<c017d185>] (pci_device_probe+0x45/0x6c)

So we had :

     igb_probe()
         igb_msix_other()
             pci_enable_msix()  => Warning
             igb_set_mac()      => Panic

Cheers,
Willy

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

[Willy Tarreau]

Oh and BTW, it also fixed another issue I had loading myri10ge on this
board :

Before the patch :

root at xpgp:~# lspci -vvnn |egrep -i '^0|memory'
00:01.0 PCI bridge [0604]: Marvell Technology Group Ltd. Device [11ab:7846] (rev 02) (prog-if 00 [Normal decode])
        Prefetchable memory behind bridge: 00000000-000fffff
00:09.0 PCI bridge [0604]: Marvell Technology Group Ltd. Device [11ab:7846] (rev 02) (prog-if 00 [Normal decode])
        Prefetchable memory behind bridge: 00000000-000fffff
00:0a.0 PCI bridge [0604]: Marvell Technology Group Ltd. Device [11ab:7846] (rev 02) (prog-if 00 [Normal decode])
        Memory behind bridge: e0000000-e17fffff
        Prefetchable memory behind bridge: 00000000-000fffff
03:00.0 Ethernet controller [0200]: MYRICOM Inc. Myri-10G Dual-Protocol NIC [14c1:0008]
        Region 0: Memory at e0000000 (64-bit, prefetchable) [size=16M]
        Region 2: Memory at e1000000 (64-bit, non-prefetchable) [size=1M]

root at xpgp:~# modprobe myri10ge
myri10ge: Version 1.5.3-1.534
PCI: enabling device 0000:00:0a.0 (0140 -> 0143)
myri10ge 0000:03:00.0: PCIE x4 Link
myri10ge 0000:03:00.0: Direct firmware load failed with error -2
myri10ge 0000:03:00.0: Falling back to user helper
myri10ge 0000:03:00.0: Unable to load myri10ge_eth_z8e.dat firmware image via hotplug
myri10ge 0000:03:00.0: hotplug firmware loading failed
myri10ge 0000:03:00.0: Successfully adopted running firmware
myri10ge 0000:03:00.0: Using firmware currently running on NIC.  For optimal
myri10ge 0000:03:00.0: performance consider loading optimized firmware
myri10ge 0000:03:00.0: via hotplug
myri10ge 0000:03:00.0: dummy rdma enable failed
myri10ge 0000:03:00.0: command 44 timed out, result = -1
myri10ge 0000:03:00.0: command 12 timed out, result = -1
myri10ge 0000:03:00.0: failed MXGEFW_CMD_GET_RX_RING_SIZE
myri10ge 0000:03:00.0: failed to load firmware
myri10ge 0000:03:00.0: myri10ge_probe() failed: MAC=00:60:dd:47:63:e1, SN=320225

After the patch, the lspci output is *exactly* the same but it works
much better :

root at xpgp:~# lspci -vvnn |egrep -i '^0|memory'
00:01.0 PCI bridge [0604]: Marvell Technology Group Ltd. Device [11ab:7846] (rev 02) (prog-if 00 [Normal decode])
        Prefetchable memory behind bridge: 00000000-000fffff
00:09.0 PCI bridge [0604]: Marvell Technology Group Ltd. Device [11ab:7846] (rev 02) (prog-if 00 [Normal decode])
        Prefetchable memory behind bridge: 00000000-000fffff
00:0a.0 PCI bridge [0604]: Marvell Technology Group Ltd. Device [11ab:7846] (rev 02) (prog-if 00 [Normal decode])
        Memory behind bridge: e0000000-e17fffff
        Prefetchable memory behind bridge: 00000000-000fffff
03:00.0 Ethernet controller [0200]: MYRICOM Inc. Myri-10G Dual-Protocol NIC [14c1:0008]
        Region 0: Memory at e0000000 (64-bit, prefetchable) [size=16M]
        Region 2: Memory at e1000000 (64-bit, non-prefetchable) [size=1M]

root at xpgp:~# modprobe myri10ge
myri10ge: Version 1.5.3-1.534
PCI: enabling device 0000:00:0a.0 (0140 -> 0143)
myri10ge 0000:03:00.0: PCIE x1 Link
myri10ge 0000:03:00.0: Direct firmware load failed with error -2
myri10ge 0000:03:00.0: Falling back to user helper
myri10ge 0000:03:00.0: Unable to load myri10ge_eth_z8e.dat firmware image via hotplug
myri10ge 0000:03:00.0: hotplug firmware loading failed
myri10ge 0000:03:00.0: Successfully adopted running firmware
myri10ge 0000:03:00.0: Using firmware currently running on NIC.  For optimal
myri10ge 0000:03:00.0: performance consider loading optimized firmware
myri10ge 0000:03:00.0: via hotplug
myri10ge 0000:03:00.0: MSI IRQ 112, tx bndry 2048, fw adopted, WC Disabled
root at xpgp:~#

Thus I guess that the timeout I was seeing above during the modprobe was
caused by the incorrect mbus window.

So +1 on this part of the patch here :

        port->memwin_size = roundup_pow_of_two(port->memwin_size);

Cheers,
Willy

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

[Matthew Minter]

Just to confirm, this patch is stopping my kernel panic also, I am
still suffering from another minor issue however this does seem to be
the crux of my problem also. I can however happily say that I regard
my hotplug issue closed.

Many thanks guys

On 8 April 2014 07:40, Willy Tarreau <w@1wt.eu> wrote:
> Oh and BTW, it also fixed another issue I had loading myri10ge on this
> board :
>
> Before the patch :
>
> root at xpgp:~# lspci -vvnn |egrep -i '^0|memory'
> 00:01.0 PCI bridge [0604]: Marvell Technology Group Ltd. Device [11ab:7846] (rev 02) (prog-if 00 [Normal decode])
>         Prefetchable memory behind bridge: 00000000-000fffff
> 00:09.0 PCI bridge [0604]: Marvell Technology Group Ltd. Device [11ab:7846] (rev 02) (prog-if 00 [Normal decode])
>         Prefetchable memory behind bridge: 00000000-000fffff
> 00:0a.0 PCI bridge [0604]: Marvell Technology Group Ltd. Device [11ab:7846] (rev 02) (prog-if 00 [Normal decode])
>         Memory behind bridge: e0000000-e17fffff
>         Prefetchable memory behind bridge: 00000000-000fffff
> 03:00.0 Ethernet controller [0200]: MYRICOM Inc. Myri-10G Dual-Protocol NIC [14c1:0008]
>         Region 0: Memory at e0000000 (64-bit, prefetchable) [size=16M]
>         Region 2: Memory at e1000000 (64-bit, non-prefetchable) [size=1M]
>
> root at xpgp:~# modprobe myri10ge
> myri10ge: Version 1.5.3-1.534
> PCI: enabling device 0000:00:0a.0 (0140 -> 0143)
> myri10ge 0000:03:00.0: PCIE x4 Link
> myri10ge 0000:03:00.0: Direct firmware load failed with error -2
> myri10ge 0000:03:00.0: Falling back to user helper
> myri10ge 0000:03:00.0: Unable to load myri10ge_eth_z8e.dat firmware image via hotplug
> myri10ge 0000:03:00.0: hotplug firmware loading failed
> myri10ge 0000:03:00.0: Successfully adopted running firmware
> myri10ge 0000:03:00.0: Using firmware currently running on NIC.  For optimal
> myri10ge 0000:03:00.0: performance consider loading optimized firmware
> myri10ge 0000:03:00.0: via hotplug
> myri10ge 0000:03:00.0: dummy rdma enable failed
> myri10ge 0000:03:00.0: command 44 timed out, result = -1
> myri10ge 0000:03:00.0: command 12 timed out, result = -1
> myri10ge 0000:03:00.0: failed MXGEFW_CMD_GET_RX_RING_SIZE
> myri10ge 0000:03:00.0: failed to load firmware
> myri10ge 0000:03:00.0: myri10ge_probe() failed: MAC=00:60:dd:47:63:e1, SN=320225
>
> After the patch, the lspci output is *exactly* the same but it works
> much better :
>
> root at xpgp:~# lspci -vvnn |egrep -i '^0|memory'
> 00:01.0 PCI bridge [0604]: Marvell Technology Group Ltd. Device [11ab:7846] (rev 02) (prog-if 00 [Normal decode])
>         Prefetchable memory behind bridge: 00000000-000fffff
> 00:09.0 PCI bridge [0604]: Marvell Technology Group Ltd. Device [11ab:7846] (rev 02) (prog-if 00 [Normal decode])
>         Prefetchable memory behind bridge: 00000000-000fffff
> 00:0a.0 PCI bridge [0604]: Marvell Technology Group Ltd. Device [11ab:7846] (rev 02) (prog-if 00 [Normal decode])
>         Memory behind bridge: e0000000-e17fffff
>         Prefetchable memory behind bridge: 00000000-000fffff
> 03:00.0 Ethernet controller [0200]: MYRICOM Inc. Myri-10G Dual-Protocol NIC [14c1:0008]
>         Region 0: Memory at e0000000 (64-bit, prefetchable) [size=16M]
>         Region 2: Memory at e1000000 (64-bit, non-prefetchable) [size=1M]
>
> root at xpgp:~# modprobe myri10ge
> myri10ge: Version 1.5.3-1.534
> PCI: enabling device 0000:00:0a.0 (0140 -> 0143)
> myri10ge 0000:03:00.0: PCIE x1 Link
> myri10ge 0000:03:00.0: Direct firmware load failed with error -2
> myri10ge 0000:03:00.0: Falling back to user helper
> myri10ge 0000:03:00.0: Unable to load myri10ge_eth_z8e.dat firmware image via hotplug
> myri10ge 0000:03:00.0: hotplug firmware loading failed
> myri10ge 0000:03:00.0: Successfully adopted running firmware
> myri10ge 0000:03:00.0: Using firmware currently running on NIC.  For optimal
> myri10ge 0000:03:00.0: performance consider loading optimized firmware
> myri10ge 0000:03:00.0: via hotplug
> myri10ge 0000:03:00.0: MSI IRQ 112, tx bndry 2048, fw adopted, WC Disabled
> root at xpgp:~#
>
> Thus I guess that the timeout I was seeing above during the modprobe was
> caused by the incorrect mbus window.
>
> So +1 on this part of the patch here :
>
>         port->memwin_size = roundup_pow_of_two(port->memwin_size);
>
> Cheers,
> Willy
>

-- 


------------------------------
For additional information including the registered office and the treatment of Xyratex confidential information please visit www.xyratex.com

------------------------------

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

[Matthew Minter]

To add to my previous point, I would consider releasing these patches
as soon as possible as it seems that it can easily cause panics on a
range of boards. Failing that perhaps it is at least worth releasing a
hotfix which will cause PCI probing to fail should the window not be a
power of two? Thus it will fail early with a warning instead of
potentially causing a kernel crash.

Best regards,
Matt

On 8 April 2014 11:53, Matthew Minter <matthew_minter@xyratex.com> wrote:
> Just to confirm, this patch is stopping my kernel panic also, I am
> still suffering from another minor issue however this does seem to be
> the crux of my problem also. I can however happily say that I regard
> my hotplug issue closed.
>
> Many thanks guys
>
> On 8 April 2014 07:40, Willy Tarreau <w@1wt.eu> wrote:
>> Oh and BTW, it also fixed another issue I had loading myri10ge on this
>> board :
>>
>> Before the patch :
>>
>> root at xpgp:~# lspci -vvnn |egrep -i '^0|memory'
>> 00:01.0 PCI bridge [0604]: Marvell Technology Group Ltd. Device [11ab:7846] (rev 02) (prog-if 00 [Normal decode])
>>         Prefetchable memory behind bridge: 00000000-000fffff
>> 00:09.0 PCI bridge [0604]: Marvell Technology Group Ltd. Device [11ab:7846] (rev 02) (prog-if 00 [Normal decode])
>>         Prefetchable memory behind bridge: 00000000-000fffff
>> 00:0a.0 PCI bridge [0604]: Marvell Technology Group Ltd. Device [11ab:7846] (rev 02) (prog-if 00 [Normal decode])
>>         Memory behind bridge: e0000000-e17fffff
>>         Prefetchable memory behind bridge: 00000000-000fffff
>> 03:00.0 Ethernet controller [0200]: MYRICOM Inc. Myri-10G Dual-Protocol NIC [14c1:0008]
>>         Region 0: Memory at e0000000 (64-bit, prefetchable) [size=16M]
>>         Region 2: Memory at e1000000 (64-bit, non-prefetchable) [size=1M]
>>
>> root at xpgp:~# modprobe myri10ge
>> myri10ge: Version 1.5.3-1.534
>> PCI: enabling device 0000:00:0a.0 (0140 -> 0143)
>> myri10ge 0000:03:00.0: PCIE x4 Link
>> myri10ge 0000:03:00.0: Direct firmware load failed with error -2
>> myri10ge 0000:03:00.0: Falling back to user helper
>> myri10ge 0000:03:00.0: Unable to load myri10ge_eth_z8e.dat firmware image via hotplug
>> myri10ge 0000:03:00.0: hotplug firmware loading failed
>> myri10ge 0000:03:00.0: Successfully adopted running firmware
>> myri10ge 0000:03:00.0: Using firmware currently running on NIC.  For optimal
>> myri10ge 0000:03:00.0: performance consider loading optimized firmware
>> myri10ge 0000:03:00.0: via hotplug
>> myri10ge 0000:03:00.0: dummy rdma enable failed
>> myri10ge 0000:03:00.0: command 44 timed out, result = -1
>> myri10ge 0000:03:00.0: command 12 timed out, result = -1
>> myri10ge 0000:03:00.0: failed MXGEFW_CMD_GET_RX_RING_SIZE
>> myri10ge 0000:03:00.0: failed to load firmware
>> myri10ge 0000:03:00.0: myri10ge_probe() failed: MAC=00:60:dd:47:63:e1, SN=320225
>>
>> After the patch, the lspci output is *exactly* the same but it works
>> much better :
>>
>> root at xpgp:~# lspci -vvnn |egrep -i '^0|memory'
>> 00:01.0 PCI bridge [0604]: Marvell Technology Group Ltd. Device [11ab:7846] (rev 02) (prog-if 00 [Normal decode])
>>         Prefetchable memory behind bridge: 00000000-000fffff
>> 00:09.0 PCI bridge [0604]: Marvell Technology Group Ltd. Device [11ab:7846] (rev 02) (prog-if 00 [Normal decode])
>>         Prefetchable memory behind bridge: 00000000-000fffff
>> 00:0a.0 PCI bridge [0604]: Marvell Technology Group Ltd. Device [11ab:7846] (rev 02) (prog-if 00 [Normal decode])
>>         Memory behind bridge: e0000000-e17fffff
>>         Prefetchable memory behind bridge: 00000000-000fffff
>> 03:00.0 Ethernet controller [0200]: MYRICOM Inc. Myri-10G Dual-Protocol NIC [14c1:0008]
>>         Region 0: Memory at e0000000 (64-bit, prefetchable) [size=16M]
>>         Region 2: Memory at e1000000 (64-bit, non-prefetchable) [size=1M]
>>
>> root at xpgp:~# modprobe myri10ge
>> myri10ge: Version 1.5.3-1.534
>> PCI: enabling device 0000:00:0a.0 (0140 -> 0143)
>> myri10ge 0000:03:00.0: PCIE x1 Link
>> myri10ge 0000:03:00.0: Direct firmware load failed with error -2
>> myri10ge 0000:03:00.0: Falling back to user helper
>> myri10ge 0000:03:00.0: Unable to load myri10ge_eth_z8e.dat firmware image via hotplug
>> myri10ge 0000:03:00.0: hotplug firmware loading failed
>> myri10ge 0000:03:00.0: Successfully adopted running firmware
>> myri10ge 0000:03:00.0: Using firmware currently running on NIC.  For optimal
>> myri10ge 0000:03:00.0: performance consider loading optimized firmware
>> myri10ge 0000:03:00.0: via hotplug
>> myri10ge 0000:03:00.0: MSI IRQ 112, tx bndry 2048, fw adopted, WC Disabled
>> root at xpgp:~#
>>
>> Thus I guess that the timeout I was seeing above during the modprobe was
>> caused by the incorrect mbus window.
>>
>> So +1 on this part of the patch here :
>>
>>         port->memwin_size = roundup_pow_of_two(port->memwin_size);
>>
>> Cheers,
>> Willy
>>

-- 


------------------------------
For additional information including the registered office and the treatment of Xyratex confidential information please visit www.xyratex.com

------------------------------

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

[Willy Tarreau]

Matthew,

On Tue, Apr 08, 2014 at 01:31:48PM +0100, Matthew Minter wrote:
> To add to my previous point, I would consider releasing these patches
> as soon as possible as it seems that it can easily cause panics on a
> range of boards. Failing that perhaps it is at least worth releasing a
> hotfix which will cause PCI probing to fail should the window not be a
> power of two? Thus it will fail early with a warning instead of
> potentially causing a kernel crash.

Have you tested the whole patch or just the one enforcing the power of 2 ?
It would be interesting to know if the one with SZ_4M is needed for you or
not (I guess not).

I suspect that just this single-liner will work as well, as it does for me.

Thanks,
Willy


diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index 0e79665..552ab73 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -363,6 +363,7 @@ static void mvebu_pcie_handle_membase_change(struct mvebu_pcie_port *port)
 	port->memwin_size  =
 		(((port->bridge.memlimit & 0xFFF0) << 16) | 0xFFFFF) -
 		port->memwin_base;
+	port->memwin_size = roundup_pow_of_two(port->memwin_size);
 
 	mvebu_mbus_add_window_by_id(port->mem_target, port->mem_attr,
 				    port->memwin_base, port->memwin_size);

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

[Thomas Petazzoni]

Willy, Neil, Matthew,

On Tue, 8 Apr 2014 14:36:32 +0200, Willy Tarreau wrote:

> On Tue, Apr 08, 2014 at 01:31:48PM +0100, Matthew Minter wrote:
> > To add to my previous point, I would consider releasing these patches
> > as soon as possible as it seems that it can easily cause panics on a
> > range of boards. Failing that perhaps it is at least worth releasing a
> > hotfix which will cause PCI probing to fail should the window not be a
> > power of two? Thus it will fail early with a warning instead of
> > potentially causing a kernel crash.
> 
> Have you tested the whole patch or just the one enforcing the power of 2 ?
> It would be interesting to know if the one with SZ_4M is needed for you or
> not (I guess not).
> 
> I suspect that just this single-liner will work as well, as it does for me.
> 
> Thanks,
> Willy
> 
> 
> diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
> index 0e79665..552ab73 100644
> --- a/drivers/pci/host/pci-mvebu.c
> +++ b/drivers/pci/host/pci-mvebu.c
> @@ -363,6 +363,7 @@ static void mvebu_pcie_handle_membase_change(struct mvebu_pcie_port *port)
>  	port->memwin_size  =
>  		(((port->bridge.memlimit & 0xFFF0) << 16) | 0xFFFFF) -
>  		port->memwin_base;
> +	port->memwin_size = roundup_pow_of_two(port->memwin_size);
>  
>  	mvebu_mbus_add_window_by_id(port->mem_target, port->mem_attr,
>  				    port->memwin_base, port->memwin_size);

Thanks to all of you for the investigation. So if I summarize your
findings, we have two patches needed to fix the problems of everybody:

 1) The bug fix for the MSI teardown function in irq-armada-370-xp.c.
    This one is easy, I'll test it right now, and give my formal
    Acked-by soon.

 2) The problem of non-power of 2 sized windows. This one is more
    complicated, as we cannot simply round up the size of the windows
    inside the pci-mvebu.c driver: the Linux PCI core is not aware of
    this rounding, and might therefore allocate a BAR for the next
    device at an address that overlaps the previous window we have
    enlarged to match the power of two size requirement. This is
    something that was already discussed with the PCI maintainers, but
    the discussion needs to be revived I guess.

Am I correct, or are other patches needed, or are remaining problems
not solved by these two fixes?

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

[Matthew Minter]

That is indeed correct. Those two patches correct the behaviour fully
on my board. I am by no means an expert on this, but could problem 2
be fixed using resource_alignment?  I am a little fuzzy about how
calls to it work but I know the sysfs entry can force the alignment/
size of PCI resources so presumably the kernel already has this
capability? My board is unfortunately unable to diagnose any possible
issues regarding possibly overlapping device windows as it physically
only allows 1 PCIe device at once.

Again, many thanks with your help, that has indeed fixed it for me.

On 8 April 2014 15:43, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Willy, Neil, Matthew,
>
> On Tue, 8 Apr 2014 14:36:32 +0200, Willy Tarreau wrote:
>
>> On Tue, Apr 08, 2014 at 01:31:48PM +0100, Matthew Minter wrote:
>> > To add to my previous point, I would consider releasing these patches
>> > as soon as possible as it seems that it can easily cause panics on a
>> > range of boards. Failing that perhaps it is at least worth releasing a
>> > hotfix which will cause PCI probing to fail should the window not be a
>> > power of two? Thus it will fail early with a warning instead of
>> > potentially causing a kernel crash.
>>
>> Have you tested the whole patch or just the one enforcing the power of 2 ?
>> It would be interesting to know if the one with SZ_4M is needed for you or
>> not (I guess not).
>>
>> I suspect that just this single-liner will work as well, as it does for me.
>>
>> Thanks,
>> Willy
>>
>>
>> diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
>> index 0e79665..552ab73 100644
>> --- a/drivers/pci/host/pci-mvebu.c
>> +++ b/drivers/pci/host/pci-mvebu.c
>> @@ -363,6 +363,7 @@ static void mvebu_pcie_handle_membase_change(struct mvebu_pcie_port *port)
>>       port->memwin_size  =
>>               (((port->bridge.memlimit & 0xFFF0) << 16) | 0xFFFFF) -
>>               port->memwin_base;
>> +     port->memwin_size = roundup_pow_of_two(port->memwin_size);
>>
>>       mvebu_mbus_add_window_by_id(port->mem_target, port->mem_attr,
>>                                   port->memwin_base, port->memwin_size);
>
> Thanks to all of you for the investigation. So if I summarize your
> findings, we have two patches needed to fix the problems of everybody:
>
>  1) The bug fix for the MSI teardown function in irq-armada-370-xp.c.
>     This one is easy, I'll test it right now, and give my formal
>     Acked-by soon.
>
>  2) The problem of non-power of 2 sized windows. This one is more
>     complicated, as we cannot simply round up the size of the windows
>     inside the pci-mvebu.c driver: the Linux PCI core is not aware of
>     this rounding, and might therefore allocate a BAR for the next
>     device at an address that overlaps the previous window we have
>     enlarged to match the power of two size requirement. This is
>     something that was already discussed with the PCI maintainers, but
>     the discussion needs to be revived I guess.
>
> Am I correct, or are other patches needed, or are remaining problems
> not solved by these two fixes?
>
> Thomas
> --
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com

-- 


------------------------------
For additional information including the registered office and the treatment of Xyratex confidential information please visit www.xyratex.com

------------------------------

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

[Willy Tarreau]

Hi Thomas,

On Tue, Apr 08, 2014 at 04:43:17PM +0200, Thomas Petazzoni wrote:
> Willy, Neil, Matthew,
> 
> On Tue, 8 Apr 2014 14:36:32 +0200, Willy Tarreau wrote:
> 
> > On Tue, Apr 08, 2014 at 01:31:48PM +0100, Matthew Minter wrote:
> > > To add to my previous point, I would consider releasing these patches
> > > as soon as possible as it seems that it can easily cause panics on a
> > > range of boards. Failing that perhaps it is at least worth releasing a
> > > hotfix which will cause PCI probing to fail should the window not be a
> > > power of two? Thus it will fail early with a warning instead of
> > > potentially causing a kernel crash.
> > 
> > Have you tested the whole patch or just the one enforcing the power of 2 ?
> > It would be interesting to know if the one with SZ_4M is needed for you or
> > not (I guess not).
> > 
> > I suspect that just this single-liner will work as well, as it does for me.
> > 
> > Thanks,
> > Willy
> > 
> > 
> > diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
> > index 0e79665..552ab73 100644
> > --- a/drivers/pci/host/pci-mvebu.c
> > +++ b/drivers/pci/host/pci-mvebu.c
> > @@ -363,6 +363,7 @@ static void mvebu_pcie_handle_membase_change(struct mvebu_pcie_port *port)
> >  	port->memwin_size  =
> >  		(((port->bridge.memlimit & 0xFFF0) << 16) | 0xFFFFF) -
> >  		port->memwin_base;
> > +	port->memwin_size = roundup_pow_of_two(port->memwin_size);
> >  
> >  	mvebu_mbus_add_window_by_id(port->mem_target, port->mem_attr,
> >  				    port->memwin_base, port->memwin_size);
> 
> Thanks to all of you for the investigation. So if I summarize your
> findings, we have two patches needed to fix the problems of everybody:
> 
>  1) The bug fix for the MSI teardown function in irq-armada-370-xp.c.
>     This one is easy, I'll test it right now, and give my formal
>     Acked-by soon.
> 
>  2) The problem of non-power of 2 sized windows. This one is more
>     complicated, as we cannot simply round up the size of the windows
>     inside the pci-mvebu.c driver: the Linux PCI core is not aware of
>     this rounding, and might therefore allocate a BAR for the next
>     device at an address that overlaps the previous window we have
>     enlarged to match the power of two size requirement. This is
>     something that was already discussed with the PCI maintainers, but
>     the discussion needs to be revived I guess.
> 
> Am I correct, or are other patches needed, or are remaining problems
> not solved by these two fixes?

There's still the older issue I got with trying to load a 2-port based
igb card on the xp-gp, the code says that hwirq 0xffffffe4 is invalid
and cannot be enabled and after that it panics. I recall having debugged
this one a few months ago, and finding something like ffffffe4 == -ENOSPC
which was returned by one of the inner functions. Let's put that aside
for a moment though, but I would appreciate it if you find the time to
try your igb NIC on your XPGP in 3.14 to see if you get the same result.

Thanks,
Willy

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

[Thomas Petazzoni]

Hello all,

On Sun, 6 Apr 2014 16:34:08 +0100 (BST), Neil Greatorex wrote:

> From e5698a4ae6b21c7e78538e16d293123903abbb40 Mon Sep 17 00:00:00 2001
> From: Neil Greatorex <neil@fatboyfat.co.uk>
> Date: Sun, 6 Apr 2014 16:10:43 +0100
> Subject: [PATCH] irqchip: armada-370-xp: Fix releasing of MSIs
> 
> Store the value of d->hwirq in a local variable as the real value is wiped out
> by calling irq_dispose_mapping. Without this patch, the armada_370_xp_free_msi
> function would always free MSI#0, no matter what was passed to it.
> 
> Signed-off-by: Neil Greatorex <neil@fatboyfat.co.uk>
> ---
>   drivers/irqchip/irq-armada-370-xp.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
> index 5409564..916fae2 100644
> --- a/drivers/irqchip/irq-armada-370-xp.c
> +++ b/drivers/irqchip/irq-armada-370-xp.c
> @@ -157,8 +157,10 @@ static void armada_370_xp_teardown_msi_irq(struct msi_chip *chip,
>   					   unsigned int irq)
>   {
>   	struct irq_data *d = irq_get_irq_data(irq);
> +	unsigned long hwirq = d->hwirq;
> +
>   	irq_dispose_mapping(irq);
> -	armada_370_xp_free_msi(d->hwirq);
> +	armada_370_xp_free_msi(hwirq);
>   }
> 
>   static struct irq_chip armada_370_xp_msi_irq_chip = {

Unfortunately here your patch is not sufficient to solve the problem
apparently. I've fixed another problem where the return value of
armada_370_xp_alloc_msi() (which is signed) is casted into an unsigned
irq_hw_number_t in armada_370_xp_setup_msi_irq(), but I'm still seeing
many MSIs allocated, then some freed, and finally a kernel panic.

 * Log: https://gist.github.com/tpetazzoni/10140012

 * Diff against v3.14: https://gist.github.com/tpetazzoni/10140121

Ideas? If some of you are interested in discussing this live, I'm on
#mvlinux on Freenode.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

[Thomas Petazzoni]

Dear Willy Tarreau,

On Tue, 8 Apr 2014 16:53:44 +0200, Willy Tarreau wrote:

> > Am I correct, or are other patches needed, or are remaining problems
> > not solved by these two fixes?
> 
> There's still the older issue I got with trying to load a 2-port based
> igb card on the xp-gp, the code says that hwirq 0xffffffe4 is invalid
> and cannot be enabled and after that it panics. I recall having debugged
> this one a few months ago, and finding something like ffffffe4 == -ENOSPC
> which was returned by one of the inner functions. Let's put that aside
> for a moment though, but I would appreciate it if you find the time to
> try your igb NIC on your XPGP in 3.14 to see if you get the same result.

See my second e-mail: this is due to improperly casting a signed value
into a unsigned value.

I currently have the igb NIC in my Armada XP DB, and I've reported a
panic :-)

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

[Thomas Petazzoni]

Hello all,

On Tue, 8 Apr 2014 17:13:09 +0200, Thomas Petazzoni wrote:

> Unfortunately here your patch is not sufficient to solve the problem
> apparently. I've fixed another problem where the return value of
> armada_370_xp_alloc_msi() (which is signed) is casted into an unsigned
> irq_hw_number_t in armada_370_xp_setup_msi_irq(), but I'm still seeing
> many MSIs allocated, then some freed, and finally a kernel panic.
> 
>  * Log: https://gist.github.com/tpetazzoni/10140012
> 
>  * Diff against v3.14: https://gist.github.com/tpetazzoni/10140121
> 
> Ideas? If some of you are interested in discussing this live, I'm on
> #mvlinux on Freenode.

Please find attached the 3 patches I'm currently using on the
irq-armada-370-xp. They make the situation a bit better (the igb driver
no longer believes we support MSI-X), but it still crashes in the igb
driver (see https://gist.github.com/tpetazzoni/10144886).

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-irqchip-armada-370-xp-fix-invalid-cast-of-signed-val.patch
Type: text/x-patch
Size: 1375 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140408/eff616ce/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-irqchip-armada-370-xp-implement-the-check_device-msi.patch
Type: text/x-patch
Size: 1698 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140408/eff616ce/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-irqchip-armada-370-xp-Fix-releasing-of-MSIs.patch
Type: text/x-patch
Size: 1271 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140408/eff616ce/attachment-0002.bin>

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

[Willy Tarreau]

On Tue, Apr 08, 2014 at 05:13:09PM +0200, Thomas Petazzoni wrote:
> Unfortunately here your patch is not sufficient to solve the problem
> apparently. I've fixed another problem where the return value of
> armada_370_xp_alloc_msi() (which is signed) is casted into an unsigned
> irq_hw_number_t in armada_370_xp_setup_msi_irq(), but I'm still seeing
> many MSIs allocated, then some freed, and finally a kernel panic.

I thought we already fixed that one months ago ?

Willy

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

[Thomas Petazzoni]

Hello all,

On Tue, 8 Apr 2014 17:40:34 +0200, Thomas Petazzoni wrote:
> Hello all,
> 
> On Tue, 8 Apr 2014 17:13:09 +0200, Thomas Petazzoni wrote:
> 
> > Unfortunately here your patch is not sufficient to solve the problem
> > apparently. I've fixed another problem where the return value of
> > armada_370_xp_alloc_msi() (which is signed) is casted into an unsigned
> > irq_hw_number_t in armada_370_xp_setup_msi_irq(), but I'm still seeing
> > many MSIs allocated, then some freed, and finally a kernel panic.
> > 
> >  * Log: https://gist.github.com/tpetazzoni/10140012
> > 
> >  * Diff against v3.14: https://gist.github.com/tpetazzoni/10140121
> > 
> > Ideas? If some of you are interested in discussing this live, I'm on
> > #mvlinux on Freenode.
> 
> Please find attached the 3 patches I'm currently using on the
> irq-armada-370-xp. They make the situation a bit better (the igb driver
> no longer believes we support MSI-X), but it still crashes in the igb
> driver (see https://gist.github.com/tpetazzoni/10144886).

Ok, this panic, and another one that comes after, are fixed in mainline
by:

  http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/net/ethernet/intel/igb/igb_main.c?id=cb06d102327eadcd1bdc480bfd9f8876251d1007
  http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/net/ethernet/intel/igb/igb_main.c?id=b709323d2477614823a38c2f2a9a206e087e28fc

With the 3 patches I've sent in my previous e-mail, and those two ones,
I'm able to use the two ports of the igb NIC on my Armada XP DB board
on v3.14. I can ping remote machines, using either of the two
interfaces of the igb NIC that Willy gave me.

lspci output:

01:00.0 Ethernet controller: Intel Corporation 82575EB Gigabit Network Connection (rev 02)
	Subsystem: Intel Corporation 82575EB Gigabit Network Connection
	Flags: bus master, fast devsel, latency 0, IRQ 118
	Memory at e0800000 (32-bit, non-prefetchable) [size=128K]
	Memory at e0000000 (32-bit, non-prefetchable) [size=2M]
	I/O ports at 10000 [disabled] [size=32]
	Memory at e0840000 (32-bit, non-prefetchable) [size=16K]
	[virtual] Expansion ROM at e0200000 [disabled] [size=2M]
	Capabilities: [40] Power Management version 2
	Capabilities: [50] MSI: Enable+ Count=1/1 Maskable- 64bit+
	Capabilities: [60] MSI-X: Enable- Count=10 Masked-
	Capabilities: [a0] Express Endpoint, MSI 00
	Capabilities: [100] Advanced Error Reporting
	Capabilities: [140] Device Serial Number 00-08-60-ff-ff-00-53-96
	Kernel driver in use: igb

01:00.1 Ethernet controller: Intel Corporation 82575EB Gigabit Network Connection (rev 02)
	Subsystem: Intel Corporation 82575EB Gigabit Network Connection
	Flags: bus master, fast devsel, latency 0, IRQ 119
	Memory at e0820000 (32-bit, non-prefetchable) [size=128K]
	Memory at e0400000 (32-bit, non-prefetchable) [size=2M]
	I/O ports at 10020 [disabled] [size=32]
	Memory at e0844000 (32-bit, non-prefetchable) [size=16K]
	[virtual] Expansion ROM at e0600000 [disabled] [size=2M]
	Capabilities: [40] Power Management version 2
	Capabilities: [50] MSI: Enable+ Count=1/1 Maskable- 64bit+
	Capabilities: [60] MSI-X: Enable- Count=10 Masked-
	Capabilities: [a0] Express Endpoint, MSI 00
	Capabilities: [100] Advanced Error Reporting
	Capabilities: [140] Device Serial Number 00-08-60-ff-ff-00-53-96
	Kernel driver in use: igb

mvebu-mbus/devices output:

# cat /sys/kernel/debug/mvebu-mbus/devices 
[00] 00000000e8010000 - 00000000e8020000 : 0004:00e0 (remap 0000000000010000)
[01] disabled
[02] disabled
[03] disabled
[04] disabled
[05] disabled
[06] disabled
[07] disabled
[08] 00000000fff00000 - 0000000100000000 : 0001:001d
[09] 00000000f0000000 - 00000000f1000000 : 0001:002f
[10] 00000000e0000000 - 00000000e0900000 : 0004:00e8
[11] disabled
[12] disabled
[13] disabled
[14] disabled
[15] disabled
[16] disabled
[17] disabled
[18] disabled
[19] disabled

I'm not sure why I don't have the non-power-of-two problem for the MBus
windows.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

[Thomas Petazzoni]

Dear Willy Tarreau,

On Tue, 8 Apr 2014 17:53:45 +0200, Willy Tarreau wrote:
> On Tue, Apr 08, 2014 at 05:13:09PM +0200, Thomas Petazzoni wrote:
> > Unfortunately here your patch is not sufficient to solve the problem
> > apparently. I've fixed another problem where the return value of
> > armada_370_xp_alloc_msi() (which is signed) is casted into an unsigned
> > irq_hw_number_t in armada_370_xp_setup_msi_irq(), but I'm still seeing
> > many MSIs allocated, then some freed, and finally a kernel panic.
> 
> I thought we already fixed that one months ago ?

Well, I don't see it fixed in mainline:

 http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/irqchip/irq-armada-370-xp.c#n130

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

[Matthew Minter]

Hi Thomas,

I have found on the GP dev board largely the problem does not show
unless hot plugging devices are added. It is difficult for me to say
why but my guess is that the layout of the hardware on that board
causes it to work by chance. However in short I can confirm I have
never had the problem with the dev board except by connecting more
than 3 devices by using a PCI bridge.

On 8 April 2014 16:55, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Hello all,
>
> On Tue, 8 Apr 2014 17:40:34 +0200, Thomas Petazzoni wrote:
>> Hello all,
>>
>> On Tue, 8 Apr 2014 17:13:09 +0200, Thomas Petazzoni wrote:
>>
>> > Unfortunately here your patch is not sufficient to solve the problem
>> > apparently. I've fixed another problem where the return value of
>> > armada_370_xp_alloc_msi() (which is signed) is casted into an unsigned
>> > irq_hw_number_t in armada_370_xp_setup_msi_irq(), but I'm still seeing
>> > many MSIs allocated, then some freed, and finally a kernel panic.
>> >
>> >  * Log: https://gist.github.com/tpetazzoni/10140012
>> >
>> >  * Diff against v3.14: https://gist.github.com/tpetazzoni/10140121
>> >
>> > Ideas? If some of you are interested in discussing this live, I'm on
>> > #mvlinux on Freenode.
>>
>> Please find attached the 3 patches I'm currently using on the
>> irq-armada-370-xp. They make the situation a bit better (the igb driver
>> no longer believes we support MSI-X), but it still crashes in the igb
>> driver (see https://gist.github.com/tpetazzoni/10144886).
>
> Ok, this panic, and another one that comes after, are fixed in mainline
> by:
>
>   http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/net/ethernet/intel/igb/igb_main.c?id=cb06d102327eadcd1bdc480bfd9f8876251d1007
>   http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/net/ethernet/intel/igb/igb_main.c?id=b709323d2477614823a38c2f2a9a206e087e28fc
>
> With the 3 patches I've sent in my previous e-mail, and those two ones,
> I'm able to use the two ports of the igb NIC on my Armada XP DB board
> on v3.14. I can ping remote machines, using either of the two
> interfaces of the igb NIC that Willy gave me.
>
> lspci output:
>
> 01:00.0 Ethernet controller: Intel Corporation 82575EB Gigabit Network Connection (rev 02)
>         Subsystem: Intel Corporation 82575EB Gigabit Network Connection
>         Flags: bus master, fast devsel, latency 0, IRQ 118
>         Memory at e0800000 (32-bit, non-prefetchable) [size=128K]
>         Memory at e0000000 (32-bit, non-prefetchable) [size=2M]
>         I/O ports at 10000 [disabled] [size=32]
>         Memory at e0840000 (32-bit, non-prefetchable) [size=16K]
>         [virtual] Expansion ROM at e0200000 [disabled] [size=2M]
>         Capabilities: [40] Power Management version 2
>         Capabilities: [50] MSI: Enable+ Count=1/1 Maskable- 64bit+
>         Capabilities: [60] MSI-X: Enable- Count=10 Masked-
>         Capabilities: [a0] Express Endpoint, MSI 00
>         Capabilities: [100] Advanced Error Reporting
>         Capabilities: [140] Device Serial Number 00-08-60-ff-ff-00-53-96
>         Kernel driver in use: igb
>
> 01:00.1 Ethernet controller: Intel Corporation 82575EB Gigabit Network Connection (rev 02)
>         Subsystem: Intel Corporation 82575EB Gigabit Network Connection
>         Flags: bus master, fast devsel, latency 0, IRQ 119
>         Memory at e0820000 (32-bit, non-prefetchable) [size=128K]
>         Memory at e0400000 (32-bit, non-prefetchable) [size=2M]
>         I/O ports at 10020 [disabled] [size=32]
>         Memory at e0844000 (32-bit, non-prefetchable) [size=16K]
>         [virtual] Expansion ROM at e0600000 [disabled] [size=2M]
>         Capabilities: [40] Power Management version 2
>         Capabilities: [50] MSI: Enable+ Count=1/1 Maskable- 64bit+
>         Capabilities: [60] MSI-X: Enable- Count=10 Masked-
>         Capabilities: [a0] Express Endpoint, MSI 00
>         Capabilities: [100] Advanced Error Reporting
>         Capabilities: [140] Device Serial Number 00-08-60-ff-ff-00-53-96
>         Kernel driver in use: igb
>
> mvebu-mbus/devices output:
>
> # cat /sys/kernel/debug/mvebu-mbus/devices
> [00] 00000000e8010000 - 00000000e8020000 : 0004:00e0 (remap 0000000000010000)
> [01] disabled
> [02] disabled
> [03] disabled
> [04] disabled
> [05] disabled
> [06] disabled
> [07] disabled
> [08] 00000000fff00000 - 0000000100000000 : 0001:001d
> [09] 00000000f0000000 - 00000000f1000000 : 0001:002f
> [10] 00000000e0000000 - 00000000e0900000 : 0004:00e8
> [11] disabled
> [12] disabled
> [13] disabled
> [14] disabled
> [15] disabled
> [16] disabled
> [17] disabled
> [18] disabled
> [19] disabled
>
> I'm not sure why I don't have the non-power-of-two problem for the MBus
> windows.
>
> Thomas
> --
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com

-- 


------------------------------
For additional information including the registered office and the treatment of Xyratex confidential information please visit www.xyratex.com

------------------------------

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

[Willy Tarreau]

On Tue, Apr 08, 2014 at 06:00:32PM +0200, Thomas Petazzoni wrote:
> Dear Willy Tarreau,
> 
> On Tue, 8 Apr 2014 17:53:45 +0200, Willy Tarreau wrote:
> > On Tue, Apr 08, 2014 at 05:13:09PM +0200, Thomas Petazzoni wrote:
> > > Unfortunately here your patch is not sufficient to solve the problem
> > > apparently. I've fixed another problem where the return value of
> > > armada_370_xp_alloc_msi() (which is signed) is casted into an unsigned
> > > irq_hw_number_t in armada_370_xp_setup_msi_irq(), but I'm still seeing
> > > many MSIs allocated, then some freed, and finally a kernel panic.
> > 
> > I thought we already fixed that one months ago ?
> 
> Well, I don't see it fixed in mainline:
> 
>  http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/irqchip/irq-armada-370-xp.c#n130

No, I just found, I sent it to you on 23/dec/2013 as part of a private
conversation during my long debugging session on the PCIe regressions,
and since one of the patches was incorrect (revert of the mask), the
other one was lost in the noise. No problem anyway, the most important
thing is that everything is now fixed :-)

Cheers,
Willy

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

[Jason Gunthorpe]

On Tue, Apr 08, 2014 at 05:55:45PM +0200, Thomas Petazzoni 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. 

It looks like you are lucky, for your system the undefined behavior
creates a window that hits all the BARs, but the others were not so
lucky.

What do you think about this:

>From 235b0bf637242a50ec45c8766d18a942bff601cf Mon Sep 17 00:00:00 2001
From: Jason Gunthorpe <jgunthorpe@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
- 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'

All together this makes the recent problems with silent failure
of PCI very obvious, noisy and debuggable.

Signed-off-by: Jason Gunthorpe <jgunthorpe@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);
+
 	ctrl = ((size - 1) & WIN_CTRL_SIZE_MASK) |
 		(attr << WIN_CTRL_ATTR_SHIFT)    |
 		(target << WIN_CTRL_TGT_SHIFT)   |
-- 
1.8.1.2

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

[Willy Tarreau]

Hi Jason,

On Tue, Apr 08, 2014 at 11:14:17AM -0600, Jason Gunthorpe wrote:
> 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

So if I understand it right, for example when my first NIC registers
e0000000-e02fffff, then we'll truncate it down to e0000000-e01fffff,
won't that cause any issue, for example if the NIC needs to access
data beyond that limit ?

BTW I can try your patch with the myricom NIC which started to work
when rounding up, I'll quickly see if that fixes the issue as well,
but I'm now a little bit confused.

Regards,
Willy

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

[Willy Tarreau]

On Tue, Apr 08, 2014 at 01:31:48PM +0100, Matthew Minter wrote:
> To add to my previous point, I would consider releasing these patches
> as soon as possible as it seems that it can easily cause panics on a
> range of boards.

On the other hand, it's not the random user who stuffs PCIe cards on
such boards PCIe slots, so in general there's a long testing period.
We identified the igb issue in mid-december without getting completely
down on it, so we remained 3 months without any other single report
about this. Thus I think that the only persons really interested in
having these patches are in this thread, so the emergency is not that
high :-)

Cheers,
Willy

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@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@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

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

[Jason Gunthorpe]

On Tue, Apr 08, 2014 at 07:53:24PM +0200, Willy Tarreau wrote:
> Hi Jason,
> 
> On Tue, Apr 08, 2014 at 11:14:17AM -0600, Jason Gunthorpe wrote:
> > 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
> 
> So if I understand it right, for example when my first NIC registers
> e0000000-e02fffff, then we'll truncate it down to e0000000-e01fffff,
> won't that cause any issue, for example if the NIC needs to access
> data beyond that limit ?

The mbus windows are assigned to the bridge, not to the NIC bars:

00:0a.0 PCI bridge [0604]: Marvell Technology Group Ltd. Device [11ab:7846] (rev 02) (prog-if 00 [Normal decode])
        Memory behind bridge: e0000000-e17fffff

So the above is not OK, 17fffff is not a power of two. The patch
causes the mbus to WARN_ON and then round down so that the hardware is
at least in a defined configuration.

> BTW I can try your patch with the myricom NIC which started to work
> when rounding up, I'll quickly see if that fixes the issue as well,
> but I'm now a little bit confused.

This patch won't fix anything, it just fixes the mbus driver to always
program the hardware with valid values, even if the upper layer
requests something invalid. Basically, we spent a few weeks tracking
this behavior down, the patch would ensure that time doesn't get spent
again.

The goal now is to avoid the WARN_ON in the patch from firing.

For a proper fix, something like this to create multiple aligned
windows is a simple option (untested, need the inverse on the
de-register, loop should probably live in mbus code):

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index 789cdb2..7312c82 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -382,6 +382,9 @@ static void mvebu_pcie_handle_iobase_change(struct mvebu_pcie_port *port)
 
 static void mvebu_pcie_handle_membase_change(struct mvebu_pcie_port *port)
 {
+       phys_addr_t base;
+       unsigned int mapped_size;
+
        /* Are the new membase/memlimit values invalid? */
        if (port->bridge.memlimit < port->bridge.membase ||
            !(port->bridge.command & PCI_COMMAND_MEMORY)) {
@@ -408,8 +411,16 @@ static void mvebu_pcie_handle_membase_change(struct mvebu_pcie_port *port)
                (((port->bridge.memlimit & 0xFFF0) << 16) | 0xFFFFF) -
                port->memwin_base;
 
-       mvebu_mbus_add_window_by_id(port->mem_target, port->mem_attr,
-                                   port->memwin_base, port->memwin_size);
+       base = port->memwin_base;
+       mapped_size = 0;
+       while (mapped_size < port->memwin_size) {
+               unsigned int size =
+                   rounddown_pow_of_two(port->memwin_size - mapped_size);
+               mvebu_mbus_add_window_by_id(port->mem_target, port->mem_attr,
+                                           base, size);
+               base += size;
+               mapped_size += size;
+       }
 }
 

Jason

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

[Thomas Petazzoni]

Dear Jason Gunthorpe,

On Tue, 8 Apr 2014 12:08:28 -0600, Jason Gunthorpe wrote:

> > BTW I can try your patch with the myricom NIC which started to work
> > when rounding up, I'll quickly see if that fixes the issue as well,
> > but I'm now a little bit confused.
> 
> This patch won't fix anything, it just fixes the mbus driver to always
> program the hardware with valid values, even if the upper layer
> requests something invalid. Basically, we spent a few weeks tracking
> this behavior down, the patch would ensure that time doesn't get spent
> again.

Agreed.

> The goal now is to avoid the WARN_ON in the patch from firing.
> 
> For a proper fix, something like this to create multiple aligned
> windows is a simple option (untested, need the inverse on the
> de-register, loop should probably live in mbus code):
> 
> diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
> index 789cdb2..7312c82 100644
> --- a/drivers/pci/host/pci-mvebu.c
> +++ b/drivers/pci/host/pci-mvebu.c
> @@ -382,6 +382,9 @@ static void mvebu_pcie_handle_iobase_change(struct mvebu_pcie_port *port)
>  
>  static void mvebu_pcie_handle_membase_change(struct mvebu_pcie_port *port)
>  {
> +       phys_addr_t base;
> +       unsigned int mapped_size;
> +
>         /* Are the new membase/memlimit values invalid? */
>         if (port->bridge.memlimit < port->bridge.membase ||
>             !(port->bridge.command & PCI_COMMAND_MEMORY)) {
> @@ -408,8 +411,16 @@ static void mvebu_pcie_handle_membase_change(struct mvebu_pcie_port *port)
>                 (((port->bridge.memlimit & 0xFFF0) << 16) | 0xFFFFF) -
>                 port->memwin_base;
>  
> -       mvebu_mbus_add_window_by_id(port->mem_target, port->mem_attr,
> -                                   port->memwin_base, port->memwin_size);
> +       base = port->memwin_base;
> +       mapped_size = 0;
> +       while (mapped_size < port->memwin_size) {
> +               unsigned int size =
> +                   rounddown_pow_of_two(port->memwin_size - mapped_size);
> +               mvebu_mbus_add_window_by_id(port->mem_target, port->mem_attr,
> +                                           base, size);
> +               base += size;
> +               mapped_size += size;
> +       }
>  }

The problem I have with this approach is the consumption of windows. We
have a limited number of them, and this approach could easily consume
quite a few windows for one single bridge BAR, depending on its size.
Like a bridge BAR of 120 MB would consume four windows (one 64 MB, one
32 MB, one 16 MB and one 8 MB).

Instead, I would really like to see the PCI core being told about this
constraint, so that it can oversize the bridge BAR (to 128 MB in the
above example of a 120 MB bridge BAR). Of course, it would be better if
the PCI core would not blindly apply this logic: if there is a 129 MB
bridge BAR, we'd prefer to have two windows (one 128 MB and one 1 MB),
so as to not loose too much physical address space.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

[Jason Gunthorpe]

On Tue, Apr 08, 2014 at 08:01:40PM +0200, Thomas Petazzoni wrote:

> > 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.

If we knew the algorithm in the HW it would probably make sense :)

> > - 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 agree, it should not happen if everything is correct, but the
apparently correct debugfs output obscured the root cause of the
problem..

> 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!" : "");

That sounds good
 
> > +	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.

Yes, agree, sounds good

> 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 :-)

Right, but Will pointed out it took 3 months to get to the root cause,
so it might save that time in future :)

I'll revise/resend the patch.

Jason

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

[Thomas Petazzoni]

Dear Jason Gunthorpe,

On Tue, 8 Apr 2014 12:22:30 -0600, Jason Gunthorpe wrote:

> > 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.
> 
> If we knew the algorithm in the HW it would probably make sense :)

Indeed :)

> > 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 agree, it should not happen if everything is correct, but the
> apparently correct debugfs output obscured the root cause of the
> problem..

Absolutely.

> > 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 :-)
> 
> Right, but Will pointed out it took 3 months to get to the root cause,
> so it might save that time in future :)

Yes, I completely agree, and I'm totally in favor of making things more
robust in mvebu-mbus.

Actually, should we simply return an error if something tries to set up
a window with an invalid size? But maybe it's a bit too strong for the
moment, until the power of two size problem gets resolved on the PCI
side.

But the other day, I also had funky issues with another window, and it
turned out to be caused by a non-power-of-two window as well...

> I'll revise/resend the patch.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

[Jason Gunthorpe]

On Tue, Apr 08, 2014 at 08:15:47PM +0200, Thomas Petazzoni wrote:

> The problem I have with this approach is the consumption of windows. We
> have a limited number of them, and this approach could easily consume
> quite a few windows for one single bridge BAR, depending on its size.
> Like a bridge BAR of 120 MB would consume four windows (one 64 MB, one
> 32 MB, one 16 MB and one 8 MB).

I agree, but on the other hand, the three people hitting this problem
have lots and lots of free windows. This won't work for someone using
a large number of PEX ports, but it does work right now, and it is
small enough to go into -stable..
 
> Instead, I would really like to see the PCI core being told about this
> constraint, so that it can oversize the bridge BAR (to 128 MB in the
> above example of a 120 MB bridge BAR). Of course, it would be better if
> the PCI core would not blindly apply this logic: if there is a 129 MB
> bridge BAR, we'd prefer to have two windows (one 128 MB and one 1 MB),
> so as to not loose too much physical address space.

Right, it would be great if the alignment function could alter the
size as well - but as you say, once we get there you will still want
to allocate more than 1 mbus window per bridge window, so the code
will still be required ..

Also, as you pointed out, the base alignment is also important to
consider, so this needs fixing too: 

	if (res->flags & IORESOURCE_IO)
		return round_up(start, max_t(resource_size_t, SZ_64K, size));
	else if (res->flags & IORESOURCE_MEM)
		return round_up(start, max_t(resource_size_t, SZ_1M, size));

What does round_up do when size is not a power of 2? Not the right
thing, I think. Size should be rounded first..

And the rough loop should consider the base alignment when
constraining the size as well...

Jason

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

[Willy Tarreau]

On Tue, Apr 08, 2014 at 12:08:28PM -0600, Jason Gunthorpe wrote:
> > So if I understand it right, for example when my first NIC registers
> > e0000000-e02fffff, then we'll truncate it down to e0000000-e01fffff,
> > won't that cause any issue, for example if the NIC needs to access
> > data beyond that limit ?
> 
> The mbus windows are assigned to the bridge, not to the NIC bars:
> 
> 00:0a.0 PCI bridge [0604]: Marvell Technology Group Ltd. Device [11ab:7846] (rev 02) (prog-if 00 [Normal decode])
>         Memory behind bridge: e0000000-e17fffff
> 
> So the above is not OK, 17fffff is not a power of two. The patch
> causes the mbus to WARN_ON and then round down so that the hardware is
> at least in a defined configuration.
>
> > BTW I can try your patch with the myricom NIC which started to work
> > when rounding up, I'll quickly see if that fixes the issue as well,
> > but I'm now a little bit confused.
> 
> This patch won't fix anything, it just fixes the mbus driver to always
> program the hardware with valid values, even if the upper layer
> requests something invalid.

OK that's what I understood, but I mean why rounding down instead of up
in order to correctly cover the window the device expects ? And we all
found that rounding up fixed our respective devices (3 igb NICs, one
myri10ge NIC, one SATA controller I believe).

By rounding up you'd have had 1ffffff instead, which at least covers the
window the device expects.

It's just this specific point of the reason for rounding down versus up
that concerns me.

Thanks,
Willy

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

[Jason Gunthorpe]

On Tue, Apr 08, 2014 at 09:15:14PM +0200, Willy Tarreau wrote:

> OK that's what I understood, but I mean why rounding down instead of up
> in order to correctly cover the window the device expects ? And we all
> found that rounding up fixed our respective devices (3 igb NICs, one
> myri10ge NIC, one SATA controller I believe).
> 
> By rounding up you'd have had 1ffffff instead, which at least covers the
> window the device expects.

It is also an error to configure the mbus to have overlaping windows
and the code checks for overlaps with the base/size given. If we round
up then it might create an overlap.

You guys were OK with the round up in the PCI code only because your
systems have a single used PEX so the rounding didn't create an
overlaping situation..

The generic code should either round down, or compeltely bail, as
Thomas suggested.

Fundementally we shouldn't get to the WARN_ON, so what happens after
is just an attempt to salvage something from the situation.

Jason

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

[Matthew Minter]

This is just a guess but perhaps the best we could do would be to round up but also add logic to fail completely if adding a PCI device would cause a window to overlap?

 This would mean situations will be fixed where we can without design changes and situations that cannot be fixed will fail with clear errors?

On 8 Apr 2014, at 20:21, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote:

> On Tue, Apr 08, 2014 at 09:15:14PM +0200, Willy Tarreau wrote:
> 
>> OK that's what I understood, but I mean why rounding down instead of up
>> in order to correctly cover the window the device expects ? And we all
>> found that rounding up fixed our respective devices (3 igb NICs, one
>> myri10ge NIC, one SATA controller I believe).
>> 
>> By rounding up you'd have had 1ffffff instead, which at least covers the
>> window the device expects.
> 
> It is also an error to configure the mbus to have overlaping windows
> and the code checks for overlaps with the base/size given. If we round
> up then it might create an overlap.
> 
> You guys were OK with the round up in the PCI code only because your
> systems have a single used PEX so the rounding didn't create an
> overlaping situation..
> 
> The generic code should either round down, or compeltely bail, as
> Thomas suggested.
> 
> Fundementally we shouldn't get to the WARN_ON, so what happens after
> is just an attempt to salvage something from the situation.
> 
> Jason

-- 


------------------------------
For additional information including the registered office and the treatment of Xyratex confidential information please visit www.xyratex.com

------------------------------

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

[Neil Greatorex]

On Tue, 8 Apr 2014, Jason Gunthorpe wrote:
> On Tue, Apr 08, 2014 at 09:15:14PM +0200, Willy Tarreau wrote:
>
>> OK that's what I understood, but I mean why rounding down instead of up
>> in order to correctly cover the window the device expects ? And we all
>> found that rounding up fixed our respective devices (3 igb NICs, one
>> myri10ge NIC, one SATA controller I believe).
>>
>> By rounding up you'd have had 1ffffff instead, which at least covers the
>> window the device expects.
>
> It is also an error to configure the mbus to have overlaping windows
> and the code checks for overlaps with the base/size given. If we round
> up then it might create an overlap.

If I use only the rounding up part of my patch and not the 4M alignment 
part then this is the exact scenario I get:

00:01.0 PCI bridge [0604]: Marvell Technology Group Ltd. Device 
[11ab:6710] (rev 01) (prog-if 00 [Normal decode])
 	Memory behind bridge: e0000000-e02fffff
 	Prefetchable memory behind bridge: 00000000-000fffff
00:02.0 PCI bridge [0604]: Marvell Technology Group Ltd. Device 
[11ab:6710] (rev 01) (prog-if 00 [Normal decode])
 	Memory behind bridge: e0300000-e03fffff
 	Prefetchable memory behind bridge: 00000000-000fffff

The window for bridge 00:01.0 gets rounded up to cover e0000000-e03fffff 
and the attempt to add the window for bridge 00:02.0 fails.

> You guys were OK with the round up in the PCI code only because your
> systems have a single used PEX so the rounding didn't create an
> overlaping situation..

In my case it worked because I changed the alignment to 4M to prevent the 
overlap.

> The generic code should either round down, or compeltely bail, as
> Thomas suggested.
>
> Fundementally we shouldn't get to the WARN_ON, so what happens after
> is just an attempt to salvage something from the situation.

Agreed.

Cheers,
Neil

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

[Willy Tarreau]

Hi Jason,

On Tue, Apr 08, 2014 at 01:21:41PM -0600, Jason Gunthorpe wrote:
> On Tue, Apr 08, 2014 at 09:15:14PM +0200, Willy Tarreau wrote:
> 
> > OK that's what I understood, but I mean why rounding down instead of up
> > in order to correctly cover the window the device expects ? And we all
> > found that rounding up fixed our respective devices (3 igb NICs, one
> > myri10ge NIC, one SATA controller I believe).
> > 
> > By rounding up you'd have had 1ffffff instead, which at least covers the
> > window the device expects.
> 
> It is also an error to configure the mbus to have overlaping windows
> and the code checks for overlaps with the base/size given. If we round
> up then it might create an overlap.

OK, Thomas just explained this to me over the phone, it makes sense now.
In fact, I think that a mix between your patch to create multiple windows
and something Thomas wanted to do (ie allocate larger and inform the PCI
core) would be a good solution by preferring to round up small windows
(low waste) and create multiple windows where the waste is too large
(eg: address space divided by the max number of windows).

Thank you for your explanation.

Willy

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

[Thomas Petazzoni]

Dear Matthew Minter,

On Tue, 8 Apr 2014 21:17:47 +0100, Matthew Minter wrote:

> This is just a guess but perhaps the best we could do would be to round up but also add logic to fail completely if adding a PCI device would cause a window to overlap?

Rounding up does not work. Let me try to explain why.

The PCI core enumerates all the BARs into your device and decides that
it should be mapped from 0xe0000000 to 0xe0900000 (9 MB). The PCI core
writes those values into the BAR of the emulated bridge in the
pci-mvebu driver. The pci-mvebu driver captures this write, and figures
out that 9 MB is not good for a MBus window. So it rounds it up to 16
MB, from 0xe0000000 to 0xe1000000, and configures the MBus window. Your
device will work perfectly fine.

Now, the PCI core goes on and enumerate the next device. This device
needs 1 MB of memory space. From the PCI core point of view, only
0xe0000000 to 0xe0900000 is occupied, so for the next device, it may
well decide to use 0xe0900000 -> 0xe0a00000, write this to the BAR of
the emulated bridge, which will lead the emulated bridge to try to
create a window for this area... which isn't possible as it overlaps
the previous window.

Conclusion: rounding up the size of the BARs cannot be done without the
cooperation of the PCI core.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com