[PATCH] tg3: fix big endian MAC address collection failure

[PATCH] tg3: fix big endian MAC address collection failure

[James Bottomley]

[sorry for repost; wrong netdev address first time around]

We noticed on parisc that our broadcoms all swapped MAC addresses going
from 2.6.29 to 2.6.30-rc1:

Apr 11 07:48:24 ion kernel: eth0: Tigon3 [partno(BCM95700A6) rev 0105] (PCI:66MHz:64-bit) MAC address 00:30:6e:4b:15:59
Apr 13 07:34:34 ion kernel: eth0: Tigon3 [partno(BCM95700A6) rev 0105] (PCI:66MHz:64-bit) MAC address 00:00:59:15:4b:6e

The problem patch is:

commit 6d348f2c1e0bb1cf7a494b51fc921095ead3f6ae
Author: Matt Carlson <mcarlson@broadcom.com>
Date:   Wed Feb 25 14:25:52 2009 +0000

    tg3: Eliminate tg3_nvram_read_swab()

With the root cause being the use of memcpy to set the mac address:

   memcpy(&dev->dev_addr[0], ((char *)&hi) + 2, 2);
   memcpy(&dev->dev_addr[2], (char *)&lo, sizeof(lo));

This might work on little endian machines, but it can't on big endian
ones.  You have to use the original setting mechanism to be correct on
all architectures.

The attached patch fixes parisc.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>

---

diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index 6a736dd..7a837c4 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -12443,8 +12443,13 @@ static int __devinit tg3_get_device_address(struct tg3 *tp)
 		/* Next, try NVRAM. */
 		if (!tg3_nvram_read_be32(tp, mac_offset + 0, &hi) &&
 		    !tg3_nvram_read_be32(tp, mac_offset + 4, &lo)) {
-			memcpy(&dev->dev_addr[0], ((char *)&hi) + 2, 2);
-			memcpy(&dev->dev_addr[2], (char *)&lo, sizeof(lo));
+			dev->dev_addr[0] = ((hi >> 16) & 0xff);
+			dev->dev_addr[1] = ((hi >> 24) & 0xff);
+			dev->dev_addr[2] = ((lo >>  0) & 0xff);
+			dev->dev_addr[3] = ((lo >>  8) & 0xff);
+			dev->dev_addr[4] = ((lo >> 16) & 0xff);
+			dev->dev_addr[5] = ((lo >> 24) & 0xff);
+
 		}
 		/* Finally just fetch it out of the MAC control regs. */
 		else {

Re: [PATCH] tg3: fix big endian MAC address collection failure

[David Miller]

From: James Bottomley <James.Bottomley@HansenPartnership.com>
Date: Mon, 13 Apr 2009 15:29:54 +0000

> We noticed on parisc that our broadcoms all swapped MAC addresses going
> from 2.6.29 to 2.6.30-rc1:
> 
> Apr 11 07:48:24 ion kernel: eth0: Tigon3 [partno(BCM95700A6) rev 0105] (PCI:66MHz:64-bit) MAC address 00:30:6e:4b:15:59
> Apr 13 07:34:34 ion kernel: eth0: Tigon3 [partno(BCM95700A6) rev 0105] (PCI:66MHz:64-bit) MAC address 00:00:59:15:4b:6e
> 
> The problem patch is:
> 
> commit 6d348f2c1e0bb1cf7a494b51fc921095ead3f6ae
> Author: Matt Carlson <mcarlson@broadcom.com>
> Date:   Wed Feb 25 14:25:52 2009 +0000
> 
>     tg3: Eliminate tg3_nvram_read_swab()
> 
> With the root cause being the use of memcpy to set the mac address:
> 
>    memcpy(&dev->dev_addr[0], ((char *)&hi) + 2, 2);
>    memcpy(&dev->dev_addr[2], (char *)&lo, sizeof(lo));
> 
> This might work on little endian machines, but it can't on big endian
> ones.  You have to use the original setting mechanism to be correct on
> all architectures.
> 
> The attached patch fixes parisc.
> 
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>

I'm applying this, thanks a lot James!

Re: [PATCH] tg3: fix big endian MAC address collection failure

[James Bottomley]

On Mon, 2009-04-13 at 14:32 -0700, David Miller wrote:
> From: James Bottomley <James.Bottomley@HansenPartnership.com>
> Date: Mon, 13 Apr 2009 15:29:54 +0000
> 
> > We noticed on parisc that our broadcoms all swapped MAC addresses going
> > from 2.6.29 to 2.6.30-rc1:
> > 
> > Apr 11 07:48:24 ion kernel: eth0: Tigon3 [partno(BCM95700A6) rev 0105] (PCI:66MHz:64-bit) MAC address 00:30:6e:4b:15:59
> > Apr 13 07:34:34 ion kernel: eth0: Tigon3 [partno(BCM95700A6) rev 0105] (PCI:66MHz:64-bit) MAC address 00:00:59:15:4b:6e
> > 
> > The problem patch is:
> > 
> > commit 6d348f2c1e0bb1cf7a494b51fc921095ead3f6ae
> > Author: Matt Carlson <mcarlson@broadcom.com>
> > Date:   Wed Feb 25 14:25:52 2009 +0000
> > 
> >     tg3: Eliminate tg3_nvram_read_swab()
> > 
> > With the root cause being the use of memcpy to set the mac address:
> > 
> >    memcpy(&dev->dev_addr[0], ((char *)&hi) + 2, 2);
> >    memcpy(&dev->dev_addr[2], (char *)&lo, sizeof(lo));
> > 
> > This might work on little endian machines, but it can't on big endian
> > ones.  You have to use the original setting mechanism to be correct on
> > all architectures.
> > 
> > The attached patch fixes parisc.
> > 
> > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> 
> I'm applying this, thanks a lot James!

Actually, hang on ... I fat fingered the list address first time around
and we've been replying on the wrong list.

Michael apparently has the code working for sparc and we're all a bit
stumped why it doesn't work on parisc since they should follow identical
code paths.

This is the latest state of play:

---
On Mon, 2009-04-13 at 11:37 -0700, Matt Carlson wrote:
> But that is exactly what the code is doing.  tg3_nvram_read_be32() will
> return the data in bytestream format.  A memcpy() should be all that is
> needed to transport the data to a different memory location.

But not the one you've done.  cpu_to_be32 is a nop pass through on our
architecture, so tg3_nvram_read_be32 is equivalent to tg3_nvram_read on
our architecture (i.e. identical to the code that was doing the read in
2.6.29).  However, the memcpy is the wrong way around for us.  If you
look at an example, the original code said

dev_addr[0] = hi >> 16;
dev_addr[1] = hi >> 24

So MSB-1 and MSB.  However, on a BE machine these are at offset one and
zero from the start of the word.  The replacement memcopy is:

memcpy(&dev->dev_addr[0], ((char *)&hi) + 2, 2)

i.e. offset 3 and 4, which actually copies LSB-1 and LSB into there.
You can follow similar logic to show that the lo copy is wrong too.

Perhaps the fix is just to put the tg3_nvram_read() back as well as the
original by loads?
---

James

Re: [PATCH] tg3: fix big endian MAC address collection failure

[David Miller]

From: James Bottomley <James.Bottomley@HansenPartnership.com>
Date: Mon, 13 Apr 2009 21:42:31 +0000

> Actually, hang on ... I fat fingered the list address first time around
> and we've been replying on the wrong list.
> 
> Michael apparently has the code working for sparc and we're all a bit
> stumped why it doesn't work on parisc since they should follow identical
> code paths.
> 
> This is the latest state of play:

Too late, I already pushed out to my net-2.6 tree on kernel.org

You'll need to send me any fixups relative to this patch.

Thanks.

Re: [PATCH] tg3: fix big endian MAC address collection failure

[Michael Chan]

On Mon, 2009-04-13 at 14:42 -0700, James Bottomley wrote:

> ---
> On Mon, 2009-04-13 at 11:37 -0700, Matt Carlson wrote:
> > But that is exactly what the code is doing.  tg3_nvram_read_be32() will
> > return the data in bytestream format.  A memcpy() should be all that is
> > needed to transport the data to a different memory location.
> 
> But not the one you've done.  cpu_to_be32 is a nop pass through on our
> architecture, so tg3_nvram_read_be32 is equivalent to tg3_nvram_read on
> our architecture (i.e. identical to the code that was doing the read in
> 2.6.29).  However, the memcpy is the wrong way around for us.  If you
> look at an example, the original code said

The old tg3_nvram_read() had a swab32() after the readl().  The new
tg3_nvram_read() no longer has the swab32().  There were too many layers
of swapping in the old code and that's why Matt wanted to clean it up.

James, can do dump out the nvram content on the parisc?

ethtool -e eth0 length 0x90

Thanks.

> 
> dev_addr[0] = hi >> 16;
> dev_addr[1] = hi >> 24
> 
> So MSB-1 and MSB.  However, on a BE machine these are at offset one and
> zero from the start of the word.  The replacement memcopy is:
> 
> memcpy(&dev->dev_addr[0], ((char *)&hi) + 2, 2)
> 
> i.e. offset 3 and 4, which actually copies LSB-1 and LSB into there.
> You can follow similar logic to show that the lo copy is wrong too.
> 
> Perhaps the fix is just to put the tg3_nvram_read() back as well as the
> original by loads?
> ---
> 
> James
> 
> 
> 

Re: [PATCH] tg3: fix big endian MAC address collection failure

[James Bottomley]

On Mon, 2009-04-13 at 15:17 -0700, Michael Chan wrote:
> On Mon, 2009-04-13 at 14:42 -0700, James Bottomley wrote:
> 
> > ---
> > On Mon, 2009-04-13 at 11:37 -0700, Matt Carlson wrote:
> > > But that is exactly what the code is doing.  tg3_nvram_read_be32() will
> > > return the data in bytestream format.  A memcpy() should be all that is
> > > needed to transport the data to a different memory location.
> > 
> > But not the one you've done.  cpu_to_be32 is a nop pass through on our
> > architecture, so tg3_nvram_read_be32 is equivalent to tg3_nvram_read on
> > our architecture (i.e. identical to the code that was doing the read in
> > 2.6.29).  However, the memcpy is the wrong way around for us.  If you
> > look at an example, the original code said
> 
> The old tg3_nvram_read() had a swab32() after the readl().  The new
> tg3_nvram_read() no longer has the swab32().  There were too many layers
> of swapping in the old code and that's why Matt wanted to clean it up.
> 
> James, can do dump out the nvram content on the parisc?
> 
> ethtool -e eth0 length 0x90
> 
> Thanks.

Sure, ion's is

ion:~# ethtool -e eth0 length 0x90
Address         Data
----------      ----
0x00000000      0xaa
0x00000001      0x55
0x00000002      0x99
0x00000003      0x66
0x00000004      0x00
0x00000005      0x00
0x00000006      0x00
0x00000007      0x08
0x00000008      0x29
0x00000009      0x02
0x0000000a      0x00
0x0000000b      0x00
0x0000000c      0x00
0x0000000d      0x02
0x0000000e      0x00
0x0000000f      0x00
0x00000010      0xc5
0x00000011      0x3c
0x00000012      0x82
0x00000013      0x9b
0x00000014      0x00
0x00000015      0x00
0x00000016      0x00
0x00000017      0x00
0x00000018      0x00
0x00000019      0x00
0x0000001a      0x00
0x0000001b      0x00
0x0000001c      0x00
0x0000001d      0x00
0x0000001e      0x00
0x0000001f      0x00
0x00000020      0x00
0x00000021      0x00
0x00000022      0x00
0x00000023      0x00
0x00000024      0x00
0x00000025      0x00
0x00000026      0x00
0x00000027      0x00
0x00000028      0x00
0x00000029      0x00
0x0000002a      0x00
0x0000002b      0x00
0x0000002c      0x00
0x0000002d      0x00
0x0000002e      0x00
0x0000002f      0x00
0x00000030      0x00
0x00000031      0x00
0x00000032      0x00
0x00000033      0x00
0x00000034      0x00
0x00000035      0x00
0x00000036      0x00
0x00000037      0x00
0x00000038      0x00
0x00000039      0x00
0x0000003a      0x00
0x0000003b      0x00
0x0000003c      0x00
0x0000003d      0x00
0x0000003e      0x00
0x0000003f      0x00
0x00000040      0x00
0x00000041      0x00
0x00000042      0x00
0x00000043      0x00
0x00000044      0x00
0x00000045      0x00
0x00000046      0x00
0x00000047      0x00
0x00000048      0x00
0x00000049      0x00
0x0000004a      0x00
0x0000004b      0x00
0x0000004c      0x00
0x0000004d      0x00
0x0000004e      0x00
0x0000004f      0x00
0x00000050      0x00
0x00000051      0x00
0x00000052      0x00
0x00000053      0x00
0x00000054      0x00
0x00000055      0x00
0x00000056      0x00
0x00000057      0x00
0x00000058      0x00
0x00000059      0x00
0x0000005a      0x00
0x0000005b      0x00
0x0000005c      0x00
0x0000005d      0x00
0x0000005e      0x00
0x0000005f      0x00
0x00000060      0x00
0x00000061      0x00
0x00000062      0x00
0x00000063      0x00
0x00000064      0x00
0x00000065      0x00
0x00000066      0x00
0x00000067      0x00
0x00000068      0x00
0x00000069      0x00
0x0000006a      0x00
0x0000006b      0x00
0x0000006c      0x00
0x0000006d      0x00
0x0000006e      0x00
0x0000006f      0x00
0x00000070      0x00
0x00000071      0x00
0x00000072      0x00
0x00000073      0x00
0x00000074      0x8c
0x00000075      0x00
0x00000076      0x00
0x00000077      0x43
0x00000078      0x10
0x00000079      0x61
0x0000007a      0x20
0x0000007b      0x00
0x0000007c      0x30
0x0000007d      0x00
0x0000007e      0x00
0x0000007f      0x00
0x00000080      0x59
0x00000081      0x15
0x00000082      0x4b
0x00000083      0x6e
0x00000084      0x39
0x00000085      0x4d
0x00000086      0x43
0x00000087      0x42
0x00000088      0x30
0x00000089      0x30
0x0000008a      0x37
0x0000008b      0x35
0x0000008c      0x00
0x0000008d      0x00
0x0000008e      0x36
0x0000008f      0x41

James

Re: [PATCH] tg3: fix big endian MAC address collection failure

[Matt Carlson]

On Mon, Apr 13, 2009 at 03:32:14PM -0700, James Bottomley wrote:
> On Mon, 2009-04-13 at 15:17 -0700, Michael Chan wrote:
> > On Mon, 2009-04-13 at 14:42 -0700, James Bottomley wrote:
> > 
> > > ---
> > > On Mon, 2009-04-13 at 11:37 -0700, Matt Carlson wrote:
> > > > But that is exactly what the code is doing.  tg3_nvram_read_be32() will
> > > > return the data in bytestream format.  A memcpy() should be all that is
> > > > needed to transport the data to a different memory location.
> > > 
> > > But not the one you've done.  cpu_to_be32 is a nop pass through on our
> > > architecture, so tg3_nvram_read_be32 is equivalent to tg3_nvram_read on
> > > our architecture (i.e. identical to the code that was doing the read in
> > > 2.6.29).  However, the memcpy is the wrong way around for us.  If you
> > > look at an example, the original code said
> > 
> > The old tg3_nvram_read() had a swab32() after the readl().  The new
> > tg3_nvram_read() no longer has the swab32().  There were too many layers
> > of swapping in the old code and that's why Matt wanted to clean it up.
> > 
> > James, can do dump out the nvram content on the parisc?
> > 
> > ethtool -e eth0 length 0x90
> > 
> > Thanks.
> 
> Sure, ion's is
> 
> ion:~# ethtool -e eth0 length 0x90
> Address         Data
> ----------      ----
> 0x00000000      0xaa
> 0x00000001      0x55
> 0x00000002      0x99
> 0x00000003      0x66

Michael noticed that your NVRAM signature is byteswapped in NVRAM. (!)
That also explains why the driver is trying to obtain the MAC address
through NVRAM, rather than getting it from shared memory.  The device's
bootcode is not working correctly.

> 0x00000004      0x00
> 0x00000005      0x00
> 0x00000006      0x00
> 0x00000007      0x08
> 0x00000008      0x29
> 0x00000009      0x02
> 0x0000000a      0x00
> 0x0000000b      0x00
> 0x0000000c      0x00
> 0x0000000d      0x02
> 0x0000000e      0x00
> 0x0000000f      0x00
> 0x00000010      0xc5
> 0x00000011      0x3c
> 0x00000012      0x82
> 0x00000013      0x9b
> 0x00000014      0x00
> 0x00000015      0x00
> 0x00000016      0x00
> 0x00000017      0x00
> 0x00000018      0x00
> 0x00000019      0x00
> 0x0000001a      0x00
> 0x0000001b      0x00
> 0x0000001c      0x00
> 0x0000001d      0x00
> 0x0000001e      0x00
> 0x0000001f      0x00
> 0x00000020      0x00
> 0x00000021      0x00
> 0x00000022      0x00
> 0x00000023      0x00
> 0x00000024      0x00
> 0x00000025      0x00
> 0x00000026      0x00
> 0x00000027      0x00
> 0x00000028      0x00
> 0x00000029      0x00
> 0x0000002a      0x00
> 0x0000002b      0x00
> 0x0000002c      0x00
> 0x0000002d      0x00
> 0x0000002e      0x00
> 0x0000002f      0x00
> 0x00000030      0x00
> 0x00000031      0x00
> 0x00000032      0x00
> 0x00000033      0x00
> 0x00000034      0x00
> 0x00000035      0x00
> 0x00000036      0x00
> 0x00000037      0x00
> 0x00000038      0x00
> 0x00000039      0x00
> 0x0000003a      0x00
> 0x0000003b      0x00
> 0x0000003c      0x00
> 0x0000003d      0x00
> 0x0000003e      0x00
> 0x0000003f      0x00
> 0x00000040      0x00
> 0x00000041      0x00
> 0x00000042      0x00
> 0x00000043      0x00
> 0x00000044      0x00
> 0x00000045      0x00
> 0x00000046      0x00
> 0x00000047      0x00
> 0x00000048      0x00
> 0x00000049      0x00
> 0x0000004a      0x00
> 0x0000004b      0x00
> 0x0000004c      0x00
> 0x0000004d      0x00
> 0x0000004e      0x00
> 0x0000004f      0x00
> 0x00000050      0x00
> 0x00000051      0x00
> 0x00000052      0x00
> 0x00000053      0x00
> 0x00000054      0x00
> 0x00000055      0x00
> 0x00000056      0x00
> 0x00000057      0x00
> 0x00000058      0x00
> 0x00000059      0x00
> 0x0000005a      0x00
> 0x0000005b      0x00
> 0x0000005c      0x00
> 0x0000005d      0x00
> 0x0000005e      0x00
> 0x0000005f      0x00
> 0x00000060      0x00
> 0x00000061      0x00
> 0x00000062      0x00
> 0x00000063      0x00
> 0x00000064      0x00
> 0x00000065      0x00
> 0x00000066      0x00
> 0x00000067      0x00
> 0x00000068      0x00
> 0x00000069      0x00
> 0x0000006a      0x00
> 0x0000006b      0x00
> 0x0000006c      0x00
> 0x0000006d      0x00
> 0x0000006e      0x00
> 0x0000006f      0x00
> 0x00000070      0x00
> 0x00000071      0x00
> 0x00000072      0x00
> 0x00000073      0x00
> 0x00000074      0x8c
> 0x00000075      0x00
> 0x00000076      0x00
> 0x00000077      0x43
> 0x00000078      0x10
> 0x00000079      0x61
> 0x0000007a      0x20
> 0x0000007b      0x00
> 0x0000007c      0x30
> 0x0000007d      0x00
> 0x0000007e      0x00
> 0x0000007f      0x00
> 0x00000080      0x59
> 0x00000081      0x15
> 0x00000082      0x4b
> 0x00000083      0x6e

Yes.  And your MAC address in NVRAM is 00:00:59:15:4b:6e.  And again, HP
MAC addresses start with 00:30:6e, so the data is byteswapped in NVRAM.
(00:00:59 MAC addresses belong to "hellige gmbh").

> 0x00000084      0x39
> 0x00000085      0x4d
> 0x00000086      0x43
> 0x00000087      0x42
> 0x00000088      0x30
> 0x00000089      0x30
> 0x0000008a      0x37
> 0x0000008b      0x35
> 0x0000008c      0x00
> 0x0000008d      0x00
> 0x0000008e      0x36
> 0x0000008f      0x41
> 
> James
> 
> 
> 

Re: [PATCH] tg3: fix big endian MAC address collection failure

[David Miller]

From: "Matt Carlson" <mcarlson@broadcom.com>
Date: Mon, 13 Apr 2009 18:25:48 -0700

> Michael noticed that your NVRAM signature is byteswapped in NVRAM. (!)
> That also explains why the driver is trying to obtain the MAC address
> through NVRAM, rather than getting it from shared memory.  The device's
> bootcode is not working correctly.

If this problem is pervasive, which it seems it is since we
have at least two people hitting this problem, we'll need
to find a way to handle it without saying "update your
BIOS or system firmware"

Re: [PATCH] tg3: fix big endian MAC address collection failure

[Matt Carlson]

On Mon, Apr 13, 2009 at 06:40:35PM -0700, David Miller wrote:
> From: "Matt Carlson" <mcarlson@broadcom.com>
> Date: Mon, 13 Apr 2009 18:25:48 -0700
> 
> > Michael noticed that your NVRAM signature is byteswapped in NVRAM. (!)
> > That also explains why the driver is trying to obtain the MAC address
> > through NVRAM, rather than getting it from shared memory.  The device's
> > bootcode is not working correctly.
> 
> If this problem is pervasive, which it seems it is since we
> have at least two people hitting this problem, we'll need
> to find a way to handle it without saying "update your
> BIOS or system firmware"

Well, Robin's problem looks distinctly different than James'.  I believe
Robin's problem has to do with the legacy EEPROM access routines.  The
patchset may have overlooked something in that area.  I just need to
gather evidence to prove it.

I did run some tests this afternoon on a similar IA64 HP machine and
they ran fine.  Consequently, I'm optimistic that James' problem is local
to his machine.  If this turns out to be a wider problem, then I agree
we'll need to find an appropriate solution.

Re: [PATCH] tg3: fix big endian MAC address collection failure

[Kyle McMartin]

On Mon, Apr 13, 2009 at 07:00:15PM -0700, Matt Carlson wrote:
> I did run some tests this afternoon on a similar IA64 HP machine and
> they ran fine.  Consequently, I'm optimistic that James' problem is local
> to his machine.  If this turns out to be a wider problem, then I agree
> we'll need to find an appropriate solution.
> 

ia64 Linux is little endian...

regards, Kyle

Re: [PATCH] tg3: fix big endian MAC address collection failure

[David Miller]

From: "Matt Carlson" <mcarlson@broadcom.com>
Date: Mon, 13 Apr 2009 19:00:15 -0700

> I did run some tests this afternoon on a similar IA64 HP machine and
> they ran fine.

It can't be similar, James Bottomly is on HP PARISC (big endian) not
IA64 (which is little endian).

I'm still convinced this is a big-endian driver problem and it has
nothing to do with broken NVRAM or similar.

Re: [PATCH] tg3: fix big endian MAC address collection failure

[Michael Chan]

David Miller wrote:

> From: "Matt Carlson" <mcarlson@broadcom.com>
> Date: Mon, 13 Apr 2009 19:00:15 -0700
>
> > I did run some tests this afternoon on a similar IA64 HP machine and
> > they ran fine.
>
> It can't be similar, James Bottomly is on HP PARISC (big endian) not
> IA64 (which is little endian).

Matt brought up IA64 because Robin Holt reported what seemed like an
identical issue on SGI IA64 machine.

>
> I'm still convinced this is a big-endian driver problem and it has
> nothing to do with broken NVRAM or similar.
>
>

Agreed.  James' problem appears to be a big endian issue.  Everything
is endian-swapped in his NVRAM when he did the ethtool dump.

Re: [PATCH] tg3: fix big endian MAC address collection failure

[James Bottomley]

On Mon, 2009-04-13 at 18:25 -0700, Matt Carlson wrote:
> On Mon, Apr 13, 2009 at 03:32:14PM -0700, James Bottomley wrote:
> > On Mon, 2009-04-13 at 15:17 -0700, Michael Chan wrote:
> > > On Mon, 2009-04-13 at 14:42 -0700, James Bottomley wrote:
> > > 
> > > > ---
> > > > On Mon, 2009-04-13 at 11:37 -0700, Matt Carlson wrote:
> > > > > But that is exactly what the code is doing.  tg3_nvram_read_be32() will
> > > > > return the data in bytestream format.  A memcpy() should be all that is
> > > > > needed to transport the data to a different memory location.
> > > > 
> > > > But not the one you've done.  cpu_to_be32 is a nop pass through on our
> > > > architecture, so tg3_nvram_read_be32 is equivalent to tg3_nvram_read on
> > > > our architecture (i.e. identical to the code that was doing the read in
> > > > 2.6.29).  However, the memcpy is the wrong way around for us.  If you
> > > > look at an example, the original code said
> > > 
> > > The old tg3_nvram_read() had a swab32() after the readl().  The new
> > > tg3_nvram_read() no longer has the swab32().  There were too many layers
> > > of swapping in the old code and that's why Matt wanted to clean it up.
> > > 
> > > James, can do dump out the nvram content on the parisc?
> > > 
> > > ethtool -e eth0 length 0x90
> > > 
> > > Thanks.
> > 
> > Sure, ion's is
> > 
> > ion:~# ethtool -e eth0 length 0x90
> > Address         Data
> > ----------      ----
> > 0x00000000      0xaa
> > 0x00000001      0x55
> > 0x00000002      0x99
> > 0x00000003      0x66
> 
> Michael noticed that your NVRAM signature is byteswapped in NVRAM. (!)
> That also explains why the driver is trying to obtain the MAC address
> through NVRAM, rather than getting it from shared memory.  The device's
> bootcode is not working correctly.

Um, well, this is a parisc:  the device's boot code won't be working at
all (parisc doesn't have open firmware boot).  The values might be laid
down by the platform IODC, but usually for add in cards, they're the
default initialise values the card comes up with.

James

Re: [PATCH] tg3: fix big endian MAC address collection failure

[Michael Chan]

James Bottomley wrote:

> On Mon, 2009-04-13 at 18:25 -0700, Matt Carlson wrote:
> > On Mon, Apr 13, 2009 at 03:32:14PM -0700, James Bottomley wrote:
> > >
> > > ion:~# ethtool -e eth0 length 0x90
> > > Address         Data
> > > ----------      ----
> > > 0x00000000      0xaa
> > > 0x00000001      0x55
> > > 0x00000002      0x99
> > > 0x00000003      0x66
> >
> > Michael noticed that your NVRAM signature is byteswapped in
> NVRAM. (!)
> > That also explains why the driver is trying to obtain the
> MAC address
> > through NVRAM, rather than getting it from shared memory.
> The device's
> > bootcode is not working correctly.
>
> Um, well, this is a parisc:  the device's boot code won't be
> working at
> all (parisc doesn't have open firmware boot).  The values
> might be laid
> down by the platform IODC, but usually for add in cards, they're the
> default initialise values the card comes up with.
>

Matt was referring to MIPS code that runs inside the MIPS core inside
the TG3 chip.  The code is loaded from NVRAM and will start running
after chip reset.  But I don't agree with Matt that the swapped NVRAM
values were caused by bad MIPS code programmed in the NVRAM.

I agree with DaveM that this appears to be a driver big-endian problem
when reading the NVRAM.  Can you run the same ethtool -e on 2.6.29 to
confirm?  Thanks.

Re: [PATCH] tg3: fix big endian MAC address collection failure

[James Bottomley]

On Mon, 2009-04-13 at 21:11 -0700, Michael Chan wrote:
> James Bottomley wrote:
> 
> > On Mon, 2009-04-13 at 18:25 -0700, Matt Carlson wrote:
> > > On Mon, Apr 13, 2009 at 03:32:14PM -0700, James Bottomley wrote:
> > > >
> > > > ion:~# ethtool -e eth0 length 0x90
> > > > Address         Data
> > > > ----------      ----
> > > > 0x00000000      0xaa
> > > > 0x00000001      0x55
> > > > 0x00000002      0x99
> > > > 0x00000003      0x66
> > >
> > > Michael noticed that your NVRAM signature is byteswapped in
> > NVRAM. (!)
> > > That also explains why the driver is trying to obtain the
> > MAC address
> > > through NVRAM, rather than getting it from shared memory.
> > The device's
> > > bootcode is not working correctly.
> >
> > Um, well, this is a parisc:  the device's boot code won't be
> > working at
> > all (parisc doesn't have open firmware boot).  The values
> > might be laid
> > down by the platform IODC, but usually for add in cards, they're the
> > default initialise values the card comes up with.
> >
> 
> Matt was referring to MIPS code that runs inside the MIPS core inside
> the TG3 chip.  The code is loaded from NVRAM and will start running
> after chip reset.  But I don't agree with Matt that the swapped NVRAM
> values were caused by bad MIPS code programmed in the NVRAM.

Um, yes, me too ... if the boot process doesn't involve the platform,
there's no reason the values would get swapped (unless we're accessing
them wrongly).

> I agree with DaveM that this appears to be a driver big-endian problem
> when reading the NVRAM.  Can you run the same ethtool -e on 2.6.29 to
> confirm?  Thanks.

Sure (just had to rebuild a 2.6.29 kernel):

ion:~# ethtool -e eth0 length 0x90
Address         Data
----------      ----
0x00000000      0x66
0x00000001      0x99
0x00000002      0x55
0x00000003      0xaa
0x00000004      0x08
0x00000005      0x00
0x00000006      0x00
0x00000007      0x00
0x00000008      0x00
0x00000009      0x00
0x0000000a      0x02
0x0000000b      0x29
0x0000000c      0x00
0x0000000d      0x00
0x0000000e      0x02
0x0000000f      0x00
0x00000010      0x9b
0x00000011      0x82
0x00000012      0x3c
0x00000013      0xc5
0x00000014      0x00
0x00000015      0x00
0x00000016      0x00
0x00000017      0x00
0x00000018      0x00
0x00000019      0x00
0x0000001a      0x00
0x0000001b      0x00
0x0000001c      0x00
0x0000001d      0x00
0x0000001e      0x00
0x0000001f      0x00
0x00000020      0x00
0x00000021      0x00
0x00000022      0x00
0x00000023      0x00
0x00000024      0x00
0x00000025      0x00
0x00000026      0x00
0x00000027      0x00
0x00000028      0x00
0x00000029      0x00
0x0000002a      0x00
0x0000002b      0x00
0x0000002c      0x00
0x0000002d      0x00
0x0000002e      0x00
0x0000002f      0x00
0x00000030      0x00
0x00000031      0x00
0x00000032      0x00
0x00000033      0x00
0x00000034      0x00
0x00000035      0x00
0x00000036      0x00
0x00000037      0x00
0x00000038      0x00
0x00000039      0x00
0x0000003a      0x00
0x0000003b      0x00
0x0000003c      0x00
0x0000003d      0x00
0x0000003e      0x00
0x0000003f      0x00
0x00000040      0x00
0x00000041      0x00
0x00000042      0x00
0x00000043      0x00
0x00000044      0x00
0x00000045      0x00
0x00000046      0x00
0x00000047      0x00
0x00000048      0x00
0x00000049      0x00
0x0000004a      0x00
0x0000004b      0x00
0x0000004c      0x00
0x0000004d      0x00
0x0000004e      0x00
0x0000004f      0x00
0x00000050      0x00
0x00000051      0x00
0x00000052      0x00
0x00000053      0x00
0x00000054      0x00
0x00000055      0x00
0x00000056      0x00
0x00000057      0x00
0x00000058      0x00
0x00000059      0x00
0x0000005a      0x00
0x0000005b      0x00
0x0000005c      0x00
0x0000005d      0x00
0x0000005e      0x00
0x0000005f      0x00
0x00000060      0x00
0x00000061      0x00
0x00000062      0x00
0x00000063      0x00
0x00000064      0x00
0x00000065      0x00
0x00000066      0x00
0x00000067      0x00
0x00000068      0x00
0x00000069      0x00
0x0000006a      0x00
0x0000006b      0x00
0x0000006c      0x00
0x0000006d      0x00
0x0000006e      0x00
0x0000006f      0x00
0x00000070      0x00
0x00000071      0x00
0x00000072      0x00
0x00000073      0x00
0x00000074      0x43
0x00000075      0x00
0x00000076      0x00
0x00000077      0x8c
0x00000078      0x00
0x00000079      0x20
0x0000007a      0x61
0x0000007b      0x10
0x0000007c      0x00
0x0000007d      0x00
0x0000007e      0x00
0x0000007f      0x30
0x00000080      0x6e
0x00000081      0x4b
0x00000082      0x15
0x00000083      0x59
0x00000084      0x42
0x00000085      0x43
0x00000086      0x4d
0x00000087      0x39
0x00000088      0x35
0x00000089      0x37
0x0000008a      0x30
0x0000008b      0x30
0x0000008c      0x41
0x0000008d      0x36
0x0000008e      0x00
0x0000008f      0x00

So that's byteswapped from the 2.6.30 output ... confirming your theory,
I think.

James

Re: [PATCH] tg3: fix big endian MAC address collection failure

[Robin Holt]

Please don't let me distract from this conversation, but here are the
ethtool dumps on ia64 for the same BCM5701 adapter for both 2.6.30-rc1
and 2.6.27.y

When I dump BCM5704 adapters, the first four bytes match.

2.6.27.19
Address   	Data
----------	----
0x00000000	0x66
0x00000001	0x99
0x00000002	0x55
0x00000003	0xaa
0x00000004	0x08
0x00000005	0x00
0x00000006	0x00
0x00000007	0x00
0x00000008	0x00
0x00000009	0x00
0x0000000a	0x02
0x0000000b	0xa5
0x0000000c	0x00
0x0000000d	0x00
0x0000000e	0x02
0x0000000f	0x00
0x00000010	0x08
0x00000011	0xdf
0x00000012	0x2a
0x00000013	0xb1
0x00000014	0x00
0x00000015	0x01
0x00000016	0x00
0x00000017	0x00
0x00000018	0x00
0x00000019	0x00
0x0000001a	0x3c
0x0000001b	0x00
0x0000001c	0x00
0x0000001d	0x00
0x0000001e	0x10
0x0000001f	0x00
0x00000020	0x00
0x00000021	0x00
0x00000022	0x00
0x00000023	0x00
0x00000024	0x00
0x00000025	0x00
0x00000026	0x00
0x00000027	0x00
0x00000028	0x00
0x00000029	0x00
0x0000002a	0x00
0x0000002b	0x00
0x0000002c	0x00
0x0000002d	0x00
0x0000002e	0x00
0x0000002f	0x00
0x00000030	0x00
0x00000031	0x00
0x00000032	0x00
0x00000033	0x00
0x00000034	0x00
0x00000035	0x00
0x00000036	0x00
0x00000037	0x00
0x00000038	0x00
0x00000039	0x00
0x0000003a	0x00
0x0000003b	0x00
0x0000003c	0x00
0x0000003d	0x00
0x0000003e	0x00
0x0000003f	0x00
0x00000040	0x00
0x00000041	0x00
0x00000042	0x00
0x00000043	0x00
0x00000044	0x00
0x00000045	0x00
0x00000046	0x00
0x00000047	0x00
0x00000048	0x00
0x00000049	0x00
0x0000004a	0x00
0x0000004b	0x00
0x0000004c	0x00
0x0000004d	0x00
0x0000004e	0x00
0x0000004f	0x00
0x00000050	0x00
0x00000051	0x00
0x00000052	0x00
0x00000053	0x00
0x00000054	0x00
0x00000055	0x00
0x00000056	0x00
0x00000057	0x00
0x00000058	0x00
0x00000059	0x00
0x0000005a	0x00
0x0000005b	0x00
0x0000005c	0x00
0x0000005d	0x00
0x0000005e	0x00
0x0000005f	0x00
0x00000060	0x00
0x00000061	0x00
0x00000062	0x00
0x00000063	0x00
0x00000064	0x00
0x00000065	0x00
0x00000066	0x00
0x00000067	0x00
0x00000068	0x00
0x00000069	0x00
0x0000006a	0x00
0x0000006b	0x00
0x0000006c	0x00
0x0000006d	0x00
0x0000006e	0x00
0x0000006f	0x00
0x00000070	0x00
0x00000071	0x00
0x00000072	0x00
0x00000073	0x00
0x00000074	0x43
0x00000075	0x00
0x00000076	0x00
0x00000077	0x8c
0x00000078	0x00
0x00000079	0x00
0x0000007a	0x00
0x0000007b	0x01
0x0000007c	0x00
0x0000007d	0x00
0x0000007e	0x08
0x0000007f	0x00
0x00000080	0x69
0x00000081	0x13
0x00000082	0xe6
0x00000083	0x3c
0x00000084	0x33
0x00000085	0x43
0x00000086	0x39
0x00000087	0x39
0x00000088	0x36
0x00000089	0x42
0x0000008a	0x2d
0x0000008b	0x54
0x0000008c	0x00
0x0000008d	0x00
0x0000008e	0x00
0x0000008f	0x00


2.6.30-rc1
Address   	Data
----------	----
0x00000000	0xaa
0x00000001	0x55
0x00000002	0x99
0x00000003	0x66
0x00000004	0x00
0x00000005	0x00
0x00000006	0x00
0x00000007	0x08
0x00000008	0xa5
0x00000009	0x02
0x0000000a	0x00
0x0000000b	0x00
0x0000000c	0x00
0x0000000d	0x02
0x0000000e	0x00
0x0000000f	0x00
0x00000010	0xb1
0x00000011	0x2a
0x00000012	0xdf
0x00000013	0x08
0x00000014	0x00
0x00000015	0x00
0x00000016	0x01
0x00000017	0x00
0x00000018	0x00
0x00000019	0x3c
0x0000001a	0x00
0x0000001b	0x00
0x0000001c	0x00
0x0000001d	0x10
0x0000001e	0x00
0x0000001f	0x00
0x00000020	0x00
0x00000021	0x00
0x00000022	0x00
0x00000023	0x00
0x00000024	0x00
0x00000025	0x00
0x00000026	0x00
0x00000027	0x00
0x00000028	0x00
0x00000029	0x00
0x0000002a	0x00
0x0000002b	0x00
0x0000002c	0x00
0x0000002d	0x00
0x0000002e	0x00
0x0000002f	0x00
0x00000030	0x00
0x00000031	0x00
0x00000032	0x00
0x00000033	0x00
0x00000034	0x00
0x00000035	0x00
0x00000036	0x00
0x00000037	0x00
0x00000038	0x00
0x00000039	0x00
0x0000003a	0x00
0x0000003b	0x00
0x0000003c	0x00
0x0000003d	0x00
0x0000003e	0x00
0x0000003f	0x00
0x00000040	0x00
0x00000041	0x00
0x00000042	0x00
0x00000043	0x00
0x00000044	0x00
0x00000045	0x00
0x00000046	0x00
0x00000047	0x00
0x00000048	0x00
0x00000049	0x00
0x0000004a	0x00
0x0000004b	0x00
0x0000004c	0x00
0x0000004d	0x00
0x0000004e	0x00
0x0000004f	0x00
0x00000050	0x00
0x00000051	0x00
0x00000052	0x00
0x00000053	0x00
0x00000054	0x00
0x00000055	0x00
0x00000056	0x00
0x00000057	0x00
0x00000058	0x00
0x00000059	0x00
0x0000005a	0x00
0x0000005b	0x00
0x0000005c	0x00
0x0000005d	0x00
0x0000005e	0x00
0x0000005f	0x00
0x00000060	0x00
0x00000061	0x00
0x00000062	0x00
0x00000063	0x00
0x00000064	0x00
0x00000065	0x00
0x00000066	0x00
0x00000067	0x00
0x00000068	0x00
0x00000069	0x00
0x0000006a	0x00
0x0000006b	0x00
0x0000006c	0x00
0x0000006d	0x00
0x0000006e	0x00
0x0000006f	0x00
0x00000070	0x00
0x00000071	0x00
0x00000072	0x00
0x00000073	0x00
0x00000074	0x8c
0x00000075	0x00
0x00000076	0x00
0x00000077	0x43
0x00000078	0x01
0x00000079	0x00
0x0000007a	0x00
0x0000007b	0x00
0x0000007c	0x00
0x0000007d	0x08
0x0000007e	0x00
0x0000007f	0x00
0x00000080	0x3c
0x00000081	0xe6
0x00000082	0x13
0x00000083	0x69
0x00000084	0x39
0x00000085	0x39
0x00000086	0x43
0x00000087	0x33
0x00000088	0x54
0x00000089	0x2d
0x0000008a	0x42
0x0000008b	0x36
0x0000008c	0x00
0x0000008d	0x00
0x0000008e	0x00
0x0000008f	0x00

[PATCH] tg3: fix big endian MAC address collection failure

[James Bottomley]

We noticed on parisc that our broadcoms all swapped MAC addresses going
from 2.6.29 to 2.6.30-rc1:

Apr 11 07:48:24 ion kernel: eth0: Tigon3 [partno(BCM95700A6) rev 0105] (PCI:66MHz:64-bit) MAC address 00:30:6e:4b:15:59
Apr 13 07:34:34 ion kernel: eth0: Tigon3 [partno(BCM95700A6) rev 0105] (PCI:66MHz:64-bit) MAC address 00:00:59:15:4b:6e

The problem patch is:

commit 6d348f2c1e0bb1cf7a494b51fc921095ead3f6ae
Author: Matt Carlson <mcarlson@broadcom.com>
Date:   Wed Feb 25 14:25:52 2009 +0000

    tg3: Eliminate tg3_nvram_read_swab()

With the root cause being the use of memcpy to set the mac address:

   memcpy(&dev->dev_addr[0], ((char *)&hi) + 2, 2);
   memcpy(&dev->dev_addr[2], (char *)&lo, sizeof(lo));

This might work on little endian machines, but it can't on big endian
ones.  You have to use the original setting mechanism to be correct on
all architectures.

The attached patch fixes parisc.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>

---

diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index 6a736dd..7a837c4 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -12443,8 +12443,13 @@ static int __devinit tg3_get_device_address(struct tg3 *tp)
 		/* Next, try NVRAM. */
 		if (!tg3_nvram_read_be32(tp, mac_offset + 0, &hi) &&
 		    !tg3_nvram_read_be32(tp, mac_offset + 4, &lo)) {
-			memcpy(&dev->dev_addr[0], ((char *)&hi) + 2, 2);
-			memcpy(&dev->dev_addr[2], (char *)&lo, sizeof(lo));
+			dev->dev_addr[0] = ((hi >> 16) & 0xff);
+			dev->dev_addr[1] = ((hi >> 24) & 0xff);
+			dev->dev_addr[2] = ((lo >>  0) & 0xff);
+			dev->dev_addr[3] = ((lo >>  8) & 0xff);
+			dev->dev_addr[4] = ((lo >> 16) & 0xff);
+			dev->dev_addr[5] = ((lo >> 24) & 0xff);
+
 		}
 		/* Finally just fetch it out of the MAC control regs. */
 		else {

Re: [PATCH] tg3: fix big endian MAC address collection failure

[Matt Carlson]

Hi James.  I'd like to think about this some more before we apply it.  I
investigated this problem using a 5703 on a SPARC Ultra 10, and all my
assumptions checked out.  I don't dispute that you and Robin are having
a problem.  I just don't understand where the root cause of the problem
is yet.  Stay tuned.

On Mon, Apr 13, 2009 at 08:21:50AM -0700, James Bottomley wrote:
> We noticed on parisc that our broadcoms all swapped MAC addresses going
> from 2.6.29 to 2.6.30-rc1:
> 
> Apr 11 07:48:24 ion kernel: eth0: Tigon3 [partno(BCM95700A6) rev 0105] (PCI:66MHz:64-bit) MAC address 00:30:6e:4b:15:59
> Apr 13 07:34:34 ion kernel: eth0: Tigon3 [partno(BCM95700A6) rev 0105] (PCI:66MHz:64-bit) MAC address 00:00:59:15:4b:6e
> 
> The problem patch is:
> 
> commit 6d348f2c1e0bb1cf7a494b51fc921095ead3f6ae
> Author: Matt Carlson <mcarlson@broadcom.com>
> Date:   Wed Feb 25 14:25:52 2009 +0000
> 
>     tg3: Eliminate tg3_nvram_read_swab()
> 
> With the root cause being the use of memcpy to set the mac address:
> 
>    memcpy(&dev->dev_addr[0], ((char *)&hi) + 2, 2);
>    memcpy(&dev->dev_addr[2], (char *)&lo, sizeof(lo));
> 
> This might work on little endian machines, but it can't on big endian
> ones.  You have to use the original setting mechanism to be correct on
> all architectures.
> 
> The attached patch fixes parisc.
> 
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> 
> ---
> 
> diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
> index 6a736dd..7a837c4 100644
> --- a/drivers/net/tg3.c
> +++ b/drivers/net/tg3.c
> @@ -12443,8 +12443,13 @@ static int __devinit tg3_get_device_address(struct tg3 *tp)
>  		/* Next, try NVRAM. */
>  		if (!tg3_nvram_read_be32(tp, mac_offset + 0, &hi) &&
>  		    !tg3_nvram_read_be32(tp, mac_offset + 4, &lo)) {
> -			memcpy(&dev->dev_addr[0], ((char *)&hi) + 2, 2);
> -			memcpy(&dev->dev_addr[2], (char *)&lo, sizeof(lo));
> +			dev->dev_addr[0] = ((hi >> 16) & 0xff);
> +			dev->dev_addr[1] = ((hi >> 24) & 0xff);
> +			dev->dev_addr[2] = ((lo >>  0) & 0xff);
> +			dev->dev_addr[3] = ((lo >>  8) & 0xff);
> +			dev->dev_addr[4] = ((lo >> 16) & 0xff);
> +			dev->dev_addr[5] = ((lo >> 24) & 0xff);
> +
>  		}
>  		/* Finally just fetch it out of the MAC control regs. */
>  		else {
> 
> 
> 
> 

Re: [PATCH] tg3: fix big endian MAC address collection failure

[Kyle McMartin]

On Mon, Apr 13, 2009 at 11:00:52AM -0700, Matt Carlson wrote:
> Hi James.  I'd like to think about this some more before we apply it.  I
> investigated this problem using a 5703 on a SPARC Ultra 10, and all my
> assumptions checked out.  I don't dispute that you and Robin are having
> a problem.  I just don't understand where the root cause of the problem
> is yet.  Stay tuned.
> 

Did your card in the sparc have an openfirmware ROM? If so, it probably
used the ofw tree to obtain parameters instead of the card nvram...

regards, Kyle

Re: [PATCH] tg3: fix big endian MAC address collection failure

[Matt Carlson]

On Mon, Apr 13, 2009 at 11:04:26AM -0700, Kyle McMartin wrote:
> On Mon, Apr 13, 2009 at 11:00:52AM -0700, Matt Carlson wrote:
> > Hi James.  I'd like to think about this some more before we apply it.  I
> > investigated this problem using a 5703 on a SPARC Ultra 10, and all my
> > assumptions checked out.  I don't dispute that you and Robin are having
> > a problem.  I just don't understand where the root cause of the problem
> > is yet.  Stay tuned.
> > 
> 
> Did your card in the sparc have an openfirmware ROM? If so, it probably
> used the ofw tree to obtain parameters instead of the card nvram...

No, the test was run using a NIC.

Re: [PATCH] tg3: fix big endian MAC address collection failure

[James Bottomley]

On Mon, 2009-04-13 at 11:13 -0700, Matt Carlson wrote:
> On Mon, Apr 13, 2009 at 11:04:26AM -0700, Kyle McMartin wrote:
> > On Mon, Apr 13, 2009 at 11:00:52AM -0700, Matt Carlson wrote:
> > > Hi James.  I'd like to think about this some more before we apply it.  I
> > > investigated this problem using a 5703 on a SPARC Ultra 10, and all my
> > > assumptions checked out.  I don't dispute that you and Robin are having
> > > a problem.  I just don't understand where the root cause of the problem
> > > is yet.  Stay tuned.
> > > 
> > 
> > Did your card in the sparc have an openfirmware ROM? If so, it probably
> > used the ofw tree to obtain parameters instead of the card nvram...
> 
> No, the test was run using a NIC.

Possibly, then, the MAC comes from the MBOX, which looks correct?

tg3_get_device_address() has four mechanisms to get the mac address

     1. A sparc specific open firmware boot mac
     2. The card mac mbox
     3. the NVRAM (this is the failing one)
     4. a sparc specific default fallback if none of the above worked

The mbox route doesn't have a memcpy, only the NVRAM one.

James

Re: [PATCH] tg3: fix big endian MAC address collection failure

[Matt Carlson]

On Mon, Apr 13, 2009 at 11:18:37AM -0700, James Bottomley wrote:
> On Mon, 2009-04-13 at 11:13 -0700, Matt Carlson wrote:
> > On Mon, Apr 13, 2009 at 11:04:26AM -0700, Kyle McMartin wrote:
> > > On Mon, Apr 13, 2009 at 11:00:52AM -0700, Matt Carlson wrote:
> > > > Hi James.  I'd like to think about this some more before we apply it.  I
> > > > investigated this problem using a 5703 on a SPARC Ultra 10, and all my
> > > > assumptions checked out.  I don't dispute that you and Robin are having
> > > > a problem.  I just don't understand where the root cause of the problem
> > > > is yet.  Stay tuned.
> > > > 
> > > 
> > > Did your card in the sparc have an openfirmware ROM? If so, it probably
> > > used the ofw tree to obtain parameters instead of the card nvram...
> > 
> > No, the test was run using a NIC.
> 
> Possibly, then, the MAC comes from the MBOX, which looks correct?
> 
> tg3_get_device_address() has four mechanisms to get the mac address
> 
>      1. A sparc specific open firmware boot mac
>      2. The card mac mbox
>      3. the NVRAM (this is the failing one)
>      4. a sparc specific default fallback if none of the above worked
> 
> The mbox route doesn't have a memcpy, only the NVRAM one.
> 
> James

On this particular adapter, the MAC address would be obtained through
shared memory.  I had to modify the driver to force the code to obtain
the MAC address through NVRAM.  I printed both MAC addresses and they match.

Re: [PATCH] tg3: fix big endian MAC address collection failure

[James Bottomley]

On Mon, 2009-04-13 at 11:00 -0700, Matt Carlson wrote:
> Hi James.  I'd like to think about this some more before we apply it.  I
> investigated this problem using a 5703 on a SPARC Ultra 10, and all my
> assumptions checked out.

Um, I think you'll find the SPARC has a special routine for extracting
the MAC address from Open Firmware, so it doesn't exercise the NVRAM
path.

>   I don't dispute that you and Robin are having
> a problem.  I just don't understand where the root cause of the problem
> is yet.  Stay tuned.

The root cause is basically that if you get the MAC from NVRAM into two
native 32 bit words, you have to be careful about how you convert them
to a byte stream for the MAC address.  You can't use a memcpy from the
native number to the MAC because it's not endian invariant (it copies
the numbers the opposite ways around on big and little endian boxes).

James

Re: [PATCH] tg3: fix big endian MAC address collection failure

[Matt Carlson]

On Mon, Apr 13, 2009 at 11:15:16AM -0700, James Bottomley wrote:
> On Mon, 2009-04-13 at 11:00 -0700, Matt Carlson wrote:
> > Hi James.  I'd like to think about this some more before we apply it.  I
> > investigated this problem using a 5703 on a SPARC Ultra 10, and all my
> > assumptions checked out.
> 
> Um, I think you'll find the SPARC has a special routine for extracting
> the MAC address from Open Firmware, so it doesn't exercise the NVRAM
> path.

Yes, it does.  The driver moves beyond that point because the routine
fails on this system.  (It's really old and doesn't have a Broadcom LOM
on it.)

> >   I don't dispute that you and Robin are having
> > a problem.  I just don't understand where the root cause of the problem
> > is yet.  Stay tuned.
> 
> The root cause is basically that if you get the MAC from NVRAM into two
> native 32 bit words, you have to be careful about how you convert them
> to a byte stream for the MAC address.  You can't use a memcpy from the
> native number to the MAC because it's not endian invariant (it copies
> the numbers the opposite ways around on big and little endian boxes).
> 
> James

But that is exactly what the code is doing.  tg3_nvram_read_be32() will
return the data in bytestream format.  A memcpy() should be all that is
needed to transport the data to a different memory location.

Re: [PATCH] tg3: fix big endian MAC address collection failure

[James Bottomley]

On Mon, 2009-04-13 at 11:37 -0700, Matt Carlson wrote:
> But that is exactly what the code is doing.  tg3_nvram_read_be32() will
> return the data in bytestream format.  A memcpy() should be all that is
> needed to transport the data to a different memory location.

But not the one you've done.  cpu_to_be32 is a nop pass through on our
architecture, so tg3_nvram_read_be32 is equivalent to tg3_nvram_read on
our architecture (i.e. identical to the code that was doing the read in
2.6.29).  However, the memcpy is the wrong way around for us.  If you
look at an example, the original code said

dev_addr[0] = hi >> 16;
dev_addr[1] = hi >> 24

So MSB-1 and MSB.  However, on a BE machine these are at offset one and
zero from the start of the word.  The replacement memcopy is:

memcpy(&dev->dev_addr[0], ((char *)&hi) + 2, 2)

i.e. offset 3 and 4, which actually copies LSB-1 and LSB into there.
You can follow similar logic to show that the lo copy is wrong too.

Perhaps the fix is just to put the tg3_nvram_read() back as well as the
original by loads?

James

Re: [PATCH] tg3: fix big endian MAC address collection failure

[Matt Carlson]

Sorry for the delay.  I just ran the same experiment on an ia64 we have
in-house.  The results were the same.  The machine was a HP integrity
BL870C pre-production blade with dual 5704S LOMs.  More responses
inline.

On Mon, Apr 13, 2009 at 01:48:18PM -0700, James Bottomley wrote:
> On Mon, 2009-04-13 at 11:37 -0700, Matt Carlson wrote:
> > But that is exactly what the code is doing.  tg3_nvram_read_be32() will
> > return the data in bytestream format.  A memcpy() should be all that is
> > needed to transport the data to a different memory location.
> 
> But not the one you've done.  cpu_to_be32 is a nop pass through on our
> architecture, so tg3_nvram_read_be32 is equivalent to tg3_nvram_read on
> our architecture (i.e. identical to the code that was doing the read in
> 2.6.29).

tg3_nvram_read() returns a 32-bit word of NVRAM data, swapped according to
the byteswapping rules for all register accesses.  On big endian machines,
the data returned will be exactly as it would have been in NVRAM.  On
little endian machines, the data will be swapped when compared to a raw
read of the NVRAM.  To bring the data back to a bytestream format on LE
systems, a swap is needed.  After that, a memcpy should be all that is
required to extract the MAC address.

Code that operates on data in a bytestream format will necessarily look
different than code that operates on data as a 32-bit native endian
quantity.  The code changes can look a bit drastic and are easily
culpable at first glance.

> However, the memcpy is the wrong way around for us.

It shouldn't be, and that is the problem.  We are trying to determine
why the data isn't in the form we think it should be in.  The most
difficult part of this discussion is convincing ourselves that the end
result is the same, even though the path to get there changes in
significant ways.

The old code read the data from NVRAM, did a blind swap of the data and
then operated on that data as a 32-bit quantity.  The blind swap was
completely superfluous and only changed which byte the driver attempted
to get its data.  An important property of this version is that the
same code when working with machines of either endianness.

If you are with me this far, then hopefully you can see that, while the
two versions of the code appear different, they wind up with the same
result.  The change was made because the code became much more readable
when we describe the operations as a bytestream.

> If you look at an example, the original code said
> 
> dev_addr[0] = hi >> 16;
> dev_addr[1] = hi >> 24
> 
> So MSB-1 and MSB.  However, on a BE machine these are at offset one and
> zero from the start of the word.  The replacement memcopy is:
> 
> memcpy(&dev->dev_addr[0], ((char *)&hi) + 2, 2)
> 
> i.e. offset 3 and 4, which actually copies LSB-1 and LSB into there.
> You can follow similar logic to show that the lo copy is wrong too.

Yes.  The code will be different depending on whether you treat the data
as a native endian 32-bit word or as a 4 byte segment of a bytestream.

> Perhaps the fix is just to put the tg3_nvram_read() back as well as the
> original by loads?

The code is much more readable with the changes in place.  I'd rather
not back them out.  I'd like to find out why you are getting different
results than I am with my tests.

I just saw your NVRAM dump post.  Perhaps that'll shed some light on the
matter.

Re: [PATCH] tg3: fix big endian MAC address collection failure

[Grant Grundler]

On Mon, Apr 13, 2009 at 06:17:10PM -0700, Matt Carlson wrote:
> Sorry for the delay.  I just ran the same experiment on an ia64 we have
> in-house.  The results were the same.  The machine was a HP integrity
> BL870C pre-production blade with dual 5704S LOMs.  More responses
> inline.

Matt,
sorry - why test this on ia64?
ia64 is also little endian.
parisc is big endian.

> On Mon, Apr 13, 2009 at 01:48:18PM -0700, James Bottomley wrote:
> > On Mon, 2009-04-13 at 11:37 -0700, Matt Carlson wrote:
> > > But that is exactly what the code is doing.  tg3_nvram_read_be32() will
> > > return the data in bytestream format.  A memcpy() should be all that is
> > > needed to transport the data to a different memory location.
> > 
> > But not the one you've done.  cpu_to_be32 is a nop pass through on our
> > architecture, so tg3_nvram_read_be32 is equivalent to tg3_nvram_read on
> > our architecture (i.e. identical to the code that was doing the read in
> > 2.6.29).
> 
> tg3_nvram_read() returns a 32-bit word of NVRAM data, swapped according to
> the byteswapping rules for all register accesses.

Lets review that calling order I found in 2.6.29-rc8:
	tg3_nvram_read -> tr32 -> tg3_read32 -> readl -> swab

o The swab is actually in tg3_nvram_read.
o one should be able to get rid of indirect function call in tr32.

>  On big endian machines,
> the data returned will be exactly as it would have been in NVRAM.  On
> little endian machines, the data will be swapped when compared to a raw
> read of the NVRAM.

Is this referring to the returned value from readl() or from tg3_nvram_read()?
The readl() return value will be the same in both LE and BE architectures.
readl() adjusts for endianess.
__raw_readl() does not.

> To bring the data back to a bytestream format on LE
> systems, a swap is needed. 

For the above code path, if swap is needed on LE, it's also needed on BE.

> After that, a memcpy should be all that is
> required to extract the MAC address.
> 
> Code that operates on data in a bytestream format will necessarily look
> different than code that operates on data as a 32-bit native endian
> quantity.  The code changes can look a bit drastic and are easily
> culpable at first glance.

This is true for code paths that deal with DMA data. It's not true
for readl() or writel() since those are already taking care of
the endianess.

> > However, the memcpy is the wrong way around for us.
> 
> It shouldn't be, and that is the problem.  We are trying to determine
> why the data isn't in the form we think it should be in.  The most
> difficult part of this discussion is convincing ourselves that the end
> result is the same, even though the path to get there changes in
> significant ways.

It's not that hard. readl() is already compensating for endianess.

> The old code read the data from NVRAM, did a blind swap of the data and
> then operated on that data as a 32-bit quantity.  The blind swap was
> completely superfluous and only changed which byte the driver attempted
> to get its data.

The swap is not superfluous if one is using memcpy.

>  An important property of this version is that the
> same code when working with machines of either endianness.
> 
> If you are with me this far, then hopefully you can see that, while the
> two versions of the code appear different, they wind up with the same
> result.  The change was made because the code became much more readable
> when we describe the operations as a bytestream.

I think james already demonstrated that the result is not the same.

I'll stop here since most of the discussion is based on not knowing
that readl() takes care of endianess.

hth,
grant

> > If you look at an example, the original code said
> > 
> > dev_addr[0] = hi >> 16;
> > dev_addr[1] = hi >> 24
> > 
> > So MSB-1 and MSB.  However, on a BE machine these are at offset one and
> > zero from the start of the word.  The replacement memcopy is:
> > 
> > memcpy(&dev->dev_addr[0], ((char *)&hi) + 2, 2)
> > 
> > i.e. offset 3 and 4, which actually copies LSB-1 and LSB into there.
> > You can follow similar logic to show that the lo copy is wrong too.
> 
> Yes.  The code will be different depending on whether you treat the data
> as a native endian 32-bit word or as a 4 byte segment of a bytestream.
> 
> > Perhaps the fix is just to put the tg3_nvram_read() back as well as the
> > original by loads?
> 
> The code is much more readable with the changes in place.  I'd rather
> not back them out.  I'd like to find out why you are getting different
> results than I am with my tests.
> 
> I just saw your NVRAM dump post.  Perhaps that'll shed some light on the
> matter.
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-parisc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tg3: fix big endian MAC address collection failure

[Matt Carlson]

On Tue, Apr 14, 2009 at 08:39:11AM -0700, Grant Grundler wrote:
> On Mon, Apr 13, 2009 at 06:17:10PM -0700, Matt Carlson wrote:
> > Sorry for the delay.  I just ran the same experiment on an ia64 we have
> > in-house.  The results were the same.  The machine was a HP integrity
> > BL870C pre-production blade with dual 5704S LOMs.  More responses
> > inline.
> 
> Matt,
> sorry - why test this on ia64?
> ia64 is also little endian.
> parisc is big endian.

Like Michael said in another post, Robin Holt experienced a similar (the
same?) problem on an IA64.  We were thinking that a problem reproduction
on one machine might imply the same problem on the other.

> > On Mon, Apr 13, 2009 at 01:48:18PM -0700, James Bottomley wrote:
> > > On Mon, 2009-04-13 at 11:37 -0700, Matt Carlson wrote:
> > > > But that is exactly what the code is doing.  tg3_nvram_read_be32() will
> > > > return the data in bytestream format.  A memcpy() should be all that is
> > > > needed to transport the data to a different memory location.
> > > 
> > > But not the one you've done.  cpu_to_be32 is a nop pass through on our
> > > architecture, so tg3_nvram_read_be32 is equivalent to tg3_nvram_read on
> > > our architecture (i.e. identical to the code that was doing the read in
> > > 2.6.29).
> > 
> > tg3_nvram_read() returns a 32-bit word of NVRAM data, swapped according to
> > the byteswapping rules for all register accesses.
> 
> Lets review that calling order I found in 2.6.29-rc8:
> 	tg3_nvram_read -> tr32 -> tg3_read32 -> readl -> swab
> 
> o The swab is actually in tg3_nvram_read.

Right.

> o one should be able to get rid of indirect function call in tr32.

Well, there are situations where the read operation needs to be done
using a different method.  So that the reader could concentrate on the
higher-level algorthm, we hide the implementation details of the read
through a function pointer.

> >  On big endian machines,
> > the data returned will be exactly as it would have been in NVRAM.  On
> > little endian machines, the data will be swapped when compared to a raw
> > read of the NVRAM.
> 
> Is this referring to the returned value from readl() or from tg3_nvram_read()?

It is referring to the value from readl().

> The readl() return value will be the same in both LE and BE architectures.
> readl() adjusts for endianess.
> __raw_readl() does not.
>
> > To bring the data back to a bytestream format on LE
> > systems, a swap is needed. 
> 
> For the above code path, if swap is needed on LE, it's also needed on BE.

Well, the 32-bit _value_ will be the same on architectures of either endianness.
But our goal is to deal with the data as a bytestream.  Consequently a
swap is needed.

For example, reading offset 0x0 of NVRAM (for legacy NVRAM formats)
readl will always produce a value of 0x669955aa.  On LE machines, this 32-bit
value will be laid out as :

  i             i + 3
  |               |
  V               V
---------------------
| aa | 55 | 99 | 66 |
---------------------

On BE machines, the byte ordering is obviously reversed.  To do a
memcpy, the bytes must be in the BE format.

> > After that, a memcpy should be all that is
> > required to extract the MAC address.
> > 
> > Code that operates on data in a bytestream format will necessarily look
> > different than code that operates on data as a 32-bit native endian
> > quantity.  The code changes can look a bit drastic and are easily
> > culpable at first glance.
> 
> This is true for code paths that deal with DMA data. It's not true
> for readl() or writel() since those are already taking care of
> the endianess.

Again, think bytestream.

> > > However, the memcpy is the wrong way around for us.
> > 
> > It shouldn't be, and that is the problem.  We are trying to determine
> > why the data isn't in the form we think it should be in.  The most
> > difficult part of this discussion is convincing ourselves that the end
> > result is the same, even though the path to get there changes in
> > significant ways.
> 
> It's not that hard. readl() is already compensating for endianess.
> 
> > The old code read the data from NVRAM, did a blind swap of the data and
> > then operated on that data as a 32-bit quantity.  The blind swap was
> > completely superfluous and only changed which byte the driver attempted
> > to get its data.
> 
> The swap is not superfluous if one is using memcpy.
>
> >  An important property of this version is that the
> > same code when working with machines of either endianness.
> > 
> > If you are with me this far, then hopefully you can see that, while the
> > two versions of the code appear different, they wind up with the same
> > result.  The change was made because the code became much more readable
> > when we describe the operations as a bytestream.
> 
> I think james already demonstrated that the result is not the same.
> 
> I'll stop here since most of the discussion is based on not knowing
> that readl() takes care of endianess.

Yes.  I think the discussion will be resolved once we distinguish
between operating on the data as a value and operating on the data as a
bytestream.

Re: [PATCH] tg3: fix big endian MAC address collection failure

[Grant Grundler]

This is settled already I think...just following up to complete
the conversation. Was "Out of Office" this past week.

On Tue, Apr 14, 2009 at 12:02:32PM -0700, Matt Carlson wrote:
...
> > > tg3_nvram_read() returns a 32-bit word of NVRAM data, swapped according to
> > > the byteswapping rules for all register accesses.
> > 
> > Lets review that calling order I found in 2.6.29-rc8:
> > 	tg3_nvram_read -> tr32 -> tg3_read32 -> readl -> swab
> > 
> > o The swab is actually in tg3_nvram_read.
> 
> Right.
> 
> > o one should be able to get rid of indirect function call in tr32.
> 
> Well, there are situations where the read operation needs to be done
> using a different method.  So that the reader could concentrate on the
> higher-level algorthm, we hide the implementation details of the read
> through a function pointer.

I agree with "why" (problems demand it) but think "how" (implementation)
could be better. Indirect function calls are usually the next most
"expensive" operation after cache misses because they are not predictable
to the CPU (data dependency). I don't know as much about the compiler
behavior with respect to indirect function calls but expect the compiler
can't optimize well either.  So I usually prefer to only see indirect
function calls for "public" APIs and not internal functions.

So what are alternatives?
1) Embedding the choice in tr32() is the first obvious one. is use
   a switch statement and call the respective functions. The compiler
   can inline all the functions into one block since they are
   only being called from one place. Net cost is an extra branch.

2) Not as good as (1), but redefine tr32() to be a macro that is
   an indirect function call.  Maybe that's already happening via
   compiler inlining and I'm being obtuse. 

(1) would make code path explicit to HW/SW and work roughly the same
on all architectures.


> > >  On big endian machines,
> > > the data returned will be exactly as it would have been in NVRAM.  On
> > > little endian machines, the data will be swapped when compared to a raw
> > > read of the NVRAM.
> > 
> > Is this referring to the returned value from readl() or from tg3_nvram_read()?
> 
> It is referring to the value from readl().

Ok - than I hope it's clear the original assertion is not correct.

> 
> > The readl() return value will be the same in both LE and BE architectures.
> > readl() adjusts for endianess.
> > __raw_readl() does not.
> >
> > > To bring the data back to a bytestream format on LE
> > > systems, a swap is needed. 
> > 
> > For the above code path, if swap is needed on LE, it's also needed on BE.
> 
> Well, the 32-bit _value_ will be the same on architectures of either endianness.
> But our goal is to deal with the data as a bytestream.

Ok. This makes sense. But then maybe use __raw_read in this particular case?
Is it possible to read the bytes one at a time?

>  Consequently a > swap is needed.
> 
> For example, reading offset 0x0 of NVRAM (for legacy NVRAM formats)
> readl will always produce a value of 0x669955aa.  On LE machines, this 32-bit
> value will be laid out as :
> 
>   i             i + 3
>   |               |
>   V               V
> ---------------------
> | aa | 55 | 99 | 66 |
> ---------------------
> 
> On BE machines, the byte ordering is obviously reversed.  To do a
> memcpy, the bytes must be in the BE format.

memcpy() doesn't care which "order" bytes are in.
It treats the source/dest memory regions as byte streams as well.

> 
> > > After that, a memcpy should be all that is
> > > required to extract the MAC address.
> > > 
> > > Code that operates on data in a bytestream format will necessarily look
> > > different than code that operates on data as a 32-bit native endian
> > > quantity.  The code changes can look a bit drastic and are easily
> > > culpable at first glance.
> > 
> > This is true for code paths that deal with DMA data. It's not true
> > for readl() or writel() since those are already taking care of
> > the endianess.
> 
> Again, think bytestream.

The data is not a byte stream (yet) since we used readl() to retrieve
the value. Use cpu_to_le32() to convert what was originally an LE byte
stream back to a LE byte stream.

Using either __raw_read() or readb() would have avoided this confusion.


> > I'll stop here since most of the discussion is based on not knowing
> > that readl() takes care of endianess.
> 
> Yes.  I think the discussion will be resolved once we distinguish
> between operating on the data as a value and operating on the data as a
> bytestream.

Personally, it's easier for me compare "CPU register data" (aka "value")
vs "data in Memory" (which is inherently a byte stream). The CPU is the
one that is imposing an endianness to multi-byte data in memory.

thanks,
grant

Re: [PATCH] tg3: fix big endian MAC address collection failure

[Michael Chan]

Grant Grundler wrote:

> This is settled already I think...just following up to complete
> the conversation. Was "Out of Office" this past week.
>
> On Tue, Apr 14, 2009 at 12:02:32PM -0700, Matt Carlson wrote:
> ...
> > > > tg3_nvram_read() returns a 32-bit word of NVRAM data,
> swapped according to
> > > > the byteswapping rules for all register accesses.
> > >
> > > Lets review that calling order I found in 2.6.29-rc8:
> > >   tg3_nvram_read -> tr32 -> tg3_read32 -> readl -> swab
> > >
> > > o The swab is actually in tg3_nvram_read.
> >
> > Right.
> >
> > > o one should be able to get rid of indirect function call in tr32.
> >
> > Well, there are situations where the read operation needs to be done
> > using a different method.  So that the reader could
> concentrate on the
> > higher-level algorthm, we hide the implementation details
> of the read
> > through a function pointer.
>
> I agree with "why" (problems demand it) but think "how"
> (implementation)
> could be better. Indirect function calls are usually the next most
> "expensive" operation after cache misses because they are not
> predictable
> to the CPU (data dependency). I don't know as much about the compiler
> behavior with respect to indirect function calls but expect
> the compiler
> can't optimize well either.  So I usually prefer to only see indirect
> function calls for "public" APIs and not internal functions.

We've had a lengthy discussion on this topic with DaveM and jgarzik many
years ago.  Marc is down and I cannot find the archive.  It was agreed
that the function pointer approach is better than a cascade of if -
else if - else if approach.  The Branch target buffers in modern CPUs
are supposed to be able to predict these function pointer branches
if we are using the same method in the hot code path.

>
> So what are alternatives?
> 1) Embedding the choice in tr32() is the first obvious one. is use
>    a switch statement and call the respective functions. The compiler
>    can inline all the functions into one block since they are
>    only being called from one place. Net cost is an extra branch.
>
> 2) Not as good as (1), but redefine tr32() to be a macro that is
>    an indirect function call.  Maybe that's already happening via
>    compiler inlining and I'm being obtuse.
>
> (1) would make code path explicit to HW/SW and work roughly the same
> on all architectures.
>
>
> > > >  On big endian machines,
> > > > the data returned will be exactly as it would have been
> in NVRAM.  On
> > > > little endian machines, the data will be swapped when
> compared to a raw
> > > > read of the NVRAM.
> > >
> > > Is this referring to the returned value from readl() or
> from tg3_nvram_read()?
> >
> > It is referring to the value from readl().
>
> Ok - than I hope it's clear the original assertion is not correct.
>
> >
> > > The readl() return value will be the same in both LE and
> BE architectures.
> > > readl() adjusts for endianess.
> > > __raw_readl() does not.
> > >
> > > > To bring the data back to a bytestream format on LE
> > > > systems, a swap is needed.
> > >
> > > For the above code path, if swap is needed on LE, it's
> also needed on BE.
> >
> > Well, the 32-bit _value_ will be the same on architectures
> of either endianness.
> > But our goal is to deal with the data as a bytestream.
>
> Ok. This makes sense. But then maybe use __raw_read in this
> particular case?
> Is it possible to read the bytes one at a time?
>
> >  Consequently a > swap is needed.
> >
> > For example, reading offset 0x0 of NVRAM (for legacy NVRAM formats)
> > readl will always produce a value of 0x669955aa.  On LE
> machines, this 32-bit
> > value will be laid out as :
> >
> >   i             i + 3
> >   |               |
> >   V               V
> > ---------------------
> > | aa | 55 | 99 | 66 |
> > ---------------------
> >
> > On BE machines, the byte ordering is obviously reversed.  To do a
> > memcpy, the bytes must be in the BE format.
>
> memcpy() doesn't care which "order" bytes are in.
> It treats the source/dest memory regions as byte streams as well.

Yeah, I think the point is that the source data needs to be in the
desired byte stream order.

>
> >
> > > > After that, a memcpy should be all that is
> > > > required to extract the MAC address.
> > > >
> > > > Code that operates on data in a bytestream format will
> necessarily look
> > > > different than code that operates on data as a 32-bit
> native endian
> > > > quantity.  The code changes can look a bit drastic and
> are easily
> > > > culpable at first glance.
> > >
> > > This is true for code paths that deal with DMA data. It's not true
> > > for readl() or writel() since those are already taking care of
> > > the endianess.
> >
> > Again, think bytestream.
>
> The data is not a byte stream (yet) since we used readl() to retrieve
> the value. Use cpu_to_le32() to convert what was originally an LE byte
> stream back to a LE byte stream.
>
> Using either __raw_read() or readb() would have avoided this
> confusion.

It's a 32-bit so we cannot use readb().  I don't think the chip will decode
the byte enables.  I don't like to use __raw_read() because it does not have
the same memory barrier as regular readl().

>
>
> > > I'll stop here since most of the discussion is based on
> not knowing
> > > that readl() takes care of endianess.
> >
> > Yes.  I think the discussion will be resolved once we distinguish
> > between operating on the data as a value and operating on
> the data as a
> > bytestream.
>
> Personally, it's easier for me compare "CPU register data"
> (aka "value")
> vs "data in Memory" (which is inherently a byte stream). The
> CPU is the
> one that is imposing an endianness to multi-byte data in memory.

Not sure if I understand your statement.  The point is that we need
to provide a consistent view of the NVRAM data regardless of the
host machine's endianess.  So we present the data as an array of
bytes.

Thanks.

>
> thanks,
> grant
>
>

Re: [PATCH] tg3: fix big endian MAC address collection failure

[Grant Grundler]

On Sun, Apr 19, 2009 at 12:30:26AM -0700, Michael Chan wrote:
[ Deleted my comments about indirect function calls. ]
...
> We've had a lengthy discussion on this topic with DaveM and jgarzik many
> years ago.  Marc is down and I cannot find the archive.  It was agreed
> that the function pointer approach is better than a cascade of if -
> else if - else if approach.  The Branch target buffers in modern CPUs
> are supposed to be able to predict these function pointer branches
> if we are using the same method in the hot code path.

Ok - thanks. I can look for that.

...
> > The data is not a byte stream (yet) since we used readl() to retrieve
> > the value. Use cpu_to_le32() to convert what was originally an LE byte
> > stream back to a LE byte stream.
> >
> > Using either __raw_read() or readb() would have avoided this
> > confusion.
> 
> It's a 32-bit so we cannot use readb().  I don't think the chip will decode
> the byte enables.

Ok. Thanks for clarifying.

> I don't like to use __raw_read() because it does not have
> the same memory barrier as regular readl().

I don't think the memory barrier is necessary in the "get" case.
wmb() is probably needed in the "set" case.

I was thinking that avoiding several byte swaps would make the code
more maintainable and one wmb() could be explained with a comment.
Remember, we are having this conversation because the MAC address
was incorrectly ordered on BE machine. Seems that a "load from NVRAM"
and "Store to HostMem" preserves the byte stream order in the same
way memcpy would. And would be heck of a lot easier to understand.

thanks,
grant

Re: [PATCH] tg3: fix big endian MAC address collection failure

[Matt Carlson]

On Sun, Apr 19, 2009 at 03:32:02PM -0700, Grant Grundler wrote:
> > > The data is not a byte stream (yet) since we used readl() to retrieve
> > > the value. Use cpu_to_le32() to convert what was originally an LE byte
> > > stream back to a LE byte stream.
> > >
> > > Using either __raw_read() or readb() would have avoided this
> > > confusion.
> > 
> > It's a 32-bit so we cannot use readb().  I don't think the chip will decode
> > the byte enables.
> 
> Ok. Thanks for clarifying.
> 
> > I don't like to use __raw_read() because it does not have
> > the same memory barrier as regular readl().
> 
> I don't think the memory barrier is necessary in the "get" case.
> wmb() is probably needed in the "set" case.
> 
> I was thinking that avoiding several byte swaps would make the code
> more maintainable and one wmb() could be explained with a comment.
> Remember, we are having this conversation because the MAC address
> was incorrectly ordered on BE machine. Seems that a "load from NVRAM"
> and "Store to HostMem" preserves the byte stream order in the same
> way memcpy would. And would be heck of a lot easier to understand.

While we could reduce some of the byte swaps, I'm not sure this
problem will be any easier to grok for that effort.

The confusion in this case stems from the fact that the SEEPROM
interface deals with data in LE format.  This fact violates rule that
the chip always deals with its data in BE format.

We can move the discussion so that we are instead talking about values
as they appear on the bus, but then that just means we need to
highlight that this data comes across the bus in BE format, when every
other value (including the flash interface data) comes across in
LE format.  IOW, no matter where we choose to couch the discussion,
we will still have to somehow convey that the behaviors are backwards
of every other chip access.

The approach you are proposing here hides the problem in the behavioural
differences between __raw_read() and readl().  The problem with this is
that someone at some point in the future wants to make the same
conversion / optimization to the flash interface.  That person will then
be confronted with this exact same problem.

If we really want to make the code more maintainable, I might try
giving the EEPROM it's own "read as value" and "read as bytestream"
functions instead.  This would effectively hide the difference behind
an API.  The intentions would (still) be clear and we would avoid the
pointless byteswap in tg3_nvram_read_using_eeprom() and swapback in
tg3_nvram_read_be32() when attempting to read the data as a bytestream.
Swapping the readl() for __raw_read() then becomes an exercise in
optimization, and we could apply it equally to the flash interface.

The problem with this proposal is that we'd still have the ugly
swap32(be32_to_cpu(data)) wart in tg3_nvram_write_block_using_eeprom().
I'm not sure how I'd want to make that prettier.