[patch 09/11] b44: fix eeprom endianess issue

[patch 09/11] b44: fix eeprom endianess issue

[akpm]

From: Michael Buesch <mb@bu3sch.de>

This fixes eeprom read on big-endian architectures.

Signed-off-by: Michael Buesch <mb@bu3sch.de>
Cc: Jeff Garzik <jeff@garzik.org>
Signed-off-by: Andrew Morton <akpm@osdl.org>
---

 drivers/net/b44.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff -puN drivers/net/b44.c~b44-fix-eeprom-endianess-issue drivers/net/b44.c
--- a/drivers/net/b44.c~b44-fix-eeprom-endianess-issue
+++ a/drivers/net/b44.c
@@ -2055,7 +2055,7 @@ static int b44_read_eeprom(struct b44 *b
 	u16 *ptr = (u16 *) data;
 
 	for (i = 0; i < 128; i += 2)
-		ptr[i / 2] = readw(bp->regs + 4096 + i);
+		ptr[i / 2] = cpu_to_le16(readw(bp->regs + 4096 + i));
 
 	return 0;
 }
_

Re: [patch 09/11] b44: fix eeprom endianess issue

[Jeff Garzik]

akpm@osdl.org wrote:
> From: Michael Buesch <mb@bu3sch.de>
> 
> This fixes eeprom read on big-endian architectures.
> 
> Signed-off-by: Michael Buesch <mb@bu3sch.de>
> Cc: Jeff Garzik <jeff@garzik.org>
> Signed-off-by: Andrew Morton <akpm@osdl.org>
> ---
> 
>  drivers/net/b44.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff -puN drivers/net/b44.c~b44-fix-eeprom-endianess-issue drivers/net/b44.c
> --- a/drivers/net/b44.c~b44-fix-eeprom-endianess-issue
> +++ a/drivers/net/b44.c
> @@ -2055,7 +2055,7 @@ static int b44_read_eeprom(struct b44 *b
>  	u16 *ptr = (u16 *) data;
>  
>  	for (i = 0; i < 128; i += 2)
> -		ptr[i / 2] = readw(bp->regs + 4096 + i);
> +		ptr[i / 2] = cpu_to_le16(readw(bp->regs + 4096 + i));

Seems highly silly to me:  readw() already swaps on big endian, so 
you're just swapping again.  And then...  read the call of 
b44_read_eeprom():  bp->dev->dev_addr[] assignment is such that it swaps 
YET AGAIN.

Seems like a better solution would be to remove the manual swap from the 
caller of b44_read_eeprom().

	Jeff

Re: [patch 09/11] b44: fix eeprom endianess issue

[Michael Buesch]

On Tuesday 26 September 2006 01:59, Jeff Garzik wrote:
> akpm@osdl.org wrote:
> > From: Michael Buesch <mb@bu3sch.de>
> > 
> > This fixes eeprom read on big-endian architectures.
> > 
> > Signed-off-by: Michael Buesch <mb@bu3sch.de>
> > Cc: Jeff Garzik <jeff@garzik.org>
> > Signed-off-by: Andrew Morton <akpm@osdl.org>
> > ---
> > 
> >  drivers/net/b44.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff -puN drivers/net/b44.c~b44-fix-eeprom-endianess-issue drivers/net/b44.c
> > --- a/drivers/net/b44.c~b44-fix-eeprom-endianess-issue
> > +++ a/drivers/net/b44.c
> > @@ -2055,7 +2055,7 @@ static int b44_read_eeprom(struct b44 *b
> >  	u16 *ptr = (u16 *) data;
> >  
> >  	for (i = 0; i < 128; i += 2)
> > -		ptr[i / 2] = readw(bp->regs + 4096 + i);
> > +		ptr[i / 2] = cpu_to_le16(readw(bp->regs + 4096 + i));
> 
> Seems highly silly to me:  readw() already swaps on big endian, so 
> you're just swapping again.  And then...  read the call of 
> b44_read_eeprom():  bp->dev->dev_addr[] assignment is such that it swaps 
> YET AGAIN.
> 
> Seems like a better solution would be to remove the manual swap from the 
> caller of b44_read_eeprom().

I already explained this to you, but I will repeat me.
readw returns the data in CPU order.
With cpu_to_le16 we convert it to little endian, because
"ptr" is a pointer to a _byte_ arrray. See the cast above.
A byte array is little endian.
Should I go into deeper detail, or did you get it? ;)

The dev_addr assignment _intentionally_ swaps yet again, because
the values are stored in bigendian format in the SPROM.

Removing the swap for dev_addr would definately the worse and wrong
solution.

-- 
Greetings Michael.

Re: [patch 09/11] b44: fix eeprom endianess issue

[Jeff Garzik]

Michael Buesch wrote:
> On Tuesday 26 September 2006 01:59, Jeff Garzik wrote:
>> akpm@osdl.org wrote:
>>> From: Michael Buesch <mb@bu3sch.de>
>>>
>>> This fixes eeprom read on big-endian architectures.
>>>
>>> Signed-off-by: Michael Buesch <mb@bu3sch.de>
>>> Cc: Jeff Garzik <jeff@garzik.org>
>>> Signed-off-by: Andrew Morton <akpm@osdl.org>
>>> ---
>>>
>>>  drivers/net/b44.c |    2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff -puN drivers/net/b44.c~b44-fix-eeprom-endianess-issue drivers/net/b44.c
>>> --- a/drivers/net/b44.c~b44-fix-eeprom-endianess-issue
>>> +++ a/drivers/net/b44.c
>>> @@ -2055,7 +2055,7 @@ static int b44_read_eeprom(struct b44 *b
>>>  	u16 *ptr = (u16 *) data;
>>>  
>>>  	for (i = 0; i < 128; i += 2)
>>> -		ptr[i / 2] = readw(bp->regs + 4096 + i);
>>> +		ptr[i / 2] = cpu_to_le16(readw(bp->regs + 4096 + i));
>> Seems highly silly to me:  readw() already swaps on big endian, so 
>> you're just swapping again.  And then...  read the call of 
>> b44_read_eeprom():  bp->dev->dev_addr[] assignment is such that it swaps 
>> YET AGAIN.
>>
>> Seems like a better solution would be to remove the manual swap from the 
>> caller of b44_read_eeprom().
> 
> I already explained this to you, but I will repeat me.
> readw returns the data in CPU order.
> With cpu_to_le16 we convert it to little endian, because
> "ptr" is a pointer to a _byte_ arrray. See the cast above.
> A byte array is little endian.
> Should I go into deeper detail, or did you get it? ;)
> 
> The dev_addr assignment _intentionally_ swaps yet again, because
> the values are stored in bigendian format in the SPROM.
> 
> Removing the swap for dev_addr would definately the worse and wrong
> solution.

AFAICS it's an obviously-correct end result, with far fewer swaps to boot.

	Jeff

Re: [patch 09/11] b44: fix eeprom endianess issue

[Michael Buesch]

On Tuesday 26 September 2006 17:59, Jeff Garzik wrote:
> Michael Buesch wrote:
> > On Tuesday 26 September 2006 01:59, Jeff Garzik wrote:
> >> akpm@osdl.org wrote:
> >>> From: Michael Buesch <mb@bu3sch.de>
> >>>
> >>> This fixes eeprom read on big-endian architectures.
> >>>
> >>> Signed-off-by: Michael Buesch <mb@bu3sch.de>
> >>> Cc: Jeff Garzik <jeff@garzik.org>
> >>> Signed-off-by: Andrew Morton <akpm@osdl.org>
> >>> ---
> >>>
> >>>  drivers/net/b44.c |    2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff -puN drivers/net/b44.c~b44-fix-eeprom-endianess-issue drivers/net/b44.c
> >>> --- a/drivers/net/b44.c~b44-fix-eeprom-endianess-issue
> >>> +++ a/drivers/net/b44.c
> >>> @@ -2055,7 +2055,7 @@ static int b44_read_eeprom(struct b44 *b
> >>>  	u16 *ptr = (u16 *) data;
> >>>  
> >>>  	for (i = 0; i < 128; i += 2)
> >>> -		ptr[i / 2] = readw(bp->regs + 4096 + i);
> >>> +		ptr[i / 2] = cpu_to_le16(readw(bp->regs + 4096 + i));
> >> Seems highly silly to me:  readw() already swaps on big endian, so 
> >> you're just swapping again.  And then...  read the call of 
> >> b44_read_eeprom():  bp->dev->dev_addr[] assignment is such that it swaps 
> >> YET AGAIN.
> >>
> >> Seems like a better solution would be to remove the manual swap from the 
> >> caller of b44_read_eeprom().
> > 
> > I already explained this to you, but I will repeat me.
> > readw returns the data in CPU order.
> > With cpu_to_le16 we convert it to little endian, because
> > "ptr" is a pointer to a _byte_ arrray. See the cast above.
> > A byte array is little endian.
> > Should I go into deeper detail, or did you get it? ;)
> > 
> > The dev_addr assignment _intentionally_ swaps yet again, because
> > the values are stored in bigendian format in the SPROM.
> > 
> > Removing the swap for dev_addr would definately the worse and wrong
> > solution.
> 
> AFAICS it's an obviously-correct end result, with far fewer swaps to boot.

No it isn't. There are lots of other parameters in the ssb SPROM.
And most of them are _not_ in bigendian.
I think we also read the PHYport or something in b44.
See bcm43xx or the ssb module for the rest of the values.

Returning a SPROM bytearray as BABABABA is just plain wrong.
It must always be ABABABAB, because we expect this. This patch
encures ABABAB order on every platform. The interpret function
must not know on which platform we are, as it's just interpreting
the _byte_ array (byte array, without any endian semantics).

This _fixes_ a bug (In the correct way, so that future bugs will
not appear)

-- 
Greetings Michael.

Re: [patch 09/11] b44: fix eeprom endianess issue

[Jeff Garzik]

Michael Buesch wrote:
> No it isn't. There are lots of other parameters in the ssb SPROM.
> And most of them are _not_ in bigendian.
> I think we also read the PHYport or something in b44.
> See bcm43xx or the ssb module for the rest of the values.
> 
> Returning a SPROM bytearray as BABABABA is just plain wrong.
> It must always be ABABABAB, because we expect this. This patch
> encures ABABAB order on every platform. The interpret function
> must not know on which platform we are, as it's just interpreting
> the _byte_ array (byte array, without any endian semantics).
> 
> This _fixes_ a bug (In the correct way, so that future bugs will
> not appear)


It's amusing to call something incorrect, when my suggested solution 
will interpret the correct values from b44_get_invariants() simply by 
changing the b44_get_invariants() code from reading BABABABA to ABABABAB.

	Jeff

Re: [patch 09/11] b44: fix eeprom endianess issue

[Michael Buesch]

On Tuesday 26 September 2006 18:08, Jeff Garzik wrote:
> Michael Buesch wrote:
> > No it isn't. There are lots of other parameters in the ssb SPROM.
> > And most of them are _not_ in bigendian.
> > I think we also read the PHYport or something in b44.
> > See bcm43xx or the ssb module for the rest of the values.
> > 
> > Returning a SPROM bytearray as BABABABA is just plain wrong.
> > It must always be ABABABAB, because we expect this. This patch
> > encures ABABAB order on every platform. The interpret function
> > must not know on which platform we are, as it's just interpreting
> > the _byte_ array (byte array, without any endian semantics).
> > 
> > This _fixes_ a bug (In the correct way, so that future bugs will
> > not appear)
> 
> 
> It's amusing to call something incorrect, when my suggested solution 
> will interpret the correct values from b44_get_invariants() simply by 
> changing the b44_get_invariants() code from reading BABABABA to ABABABAB.

Jeff, I officially do not care about b44 any longer now.
I am not going to waste my time now.
If you don't want this patch, stay with current buggy code. It's OK
to me.

Maybe the following line of code from get_invariants explains best
why my fix is the correct one:


	bp->phy_addr = eeprom[90] & 0x1f;

If you don't swap to LE in b44_read_eeprom this _won't_ lead
to the expected result of getting byte 90 of the SPROM (on BE archs).

Wanna have code like this?

#ifdef BIG_ENDIAN
	bp->phy_addr = eeprom[91] & 0x1f;
#else
	bp->phy_addr = eeprom[90] & 0x1f;
#endif


Or maybe

	bp->phy_addr = ((u16 *)eeprom)[90/2] & 0x1f;

I don't really think so.

-- 
Greetings Michael.

Re: [patch 09/11] b44: fix eeprom endianess issue

[Jeff Garzik]

Michael Buesch wrote:
> #ifdef BIG_ENDIAN
> 	bp->phy_addr = eeprom[91] & 0x1f;
> #else
> 	bp->phy_addr = eeprom[90] & 0x1f;
> #endif

False.  It's already fixed endian.  It's just not the endian you like.

	Jeff

Re: [patch 09/11] b44: fix eeprom endianess issue

[Michael Buesch]

On Tuesday 26 September 2006 19:00, Jeff Garzik wrote:
> Michael Buesch wrote:
> > #ifdef BIG_ENDIAN
> > 	bp->phy_addr = eeprom[91] & 0x1f;
> > #else
> > 	bp->phy_addr = eeprom[90] & 0x1f;
> > #endif
> 
> False.  It's already fixed endian.  It's just not the endian you like.

Jeff, come on...
Read Johannes' mail.
readX returns CPU endian data.

-- 
Greetings Michael.

Re: [patch 09/11] b44: fix eeprom endianess issue

[Johannes Berg]

On Tue, 2006-09-26 at 13:00 -0400, Jeff Garzik wrote:

> False.  It's already fixed endian.  It's just not the endian you like.

No, that's where you're wrong. It isn't fixed endian. It's CPU endian,
and the thing overlays a u8 array over CPU endian, hence it's not fixed.

johannes

Re: [patch 09/11] b44: fix eeprom endianess issue

[Johannes Berg]

On Mon, 2006-09-25 at 19:59 -0400, Jeff Garzik wrote:

> Seems like a better solution would be to remove the manual swap from the 
> caller of b44_read_eeprom().

No.

You're not looking at the problem from the right angle.

The thing is that Broadcom decided to store the EEPROM data as an array
of u16s:
  u16 eeprom_data_as_stored[64];

In there, you have, for example, the mac address in 3 u16s at offsets
39-41:
  | offset | ... | 39     | 40     | 41     | ...
  | data   | ... | MAC 01 | MAC 23 | MAC 45 | ...

which together forms MAC 012345 (6 bytes)

Broadcom further assumes that things are in little endian, so it's
actually necessary to byteswap the MAC bytes 01, 23, 45 which I have
omitted from above pictures because it's an array of u16 so endianness
doesn't matter as long as you store it to the mac in big endian.


Now come along the current code which didn't understand this and
overlayed the eeprom_data_as_stored with a new array:
    u8 eeprom[128];

That's fine on little endian machines because each u16 will be
represented in exactly the same way, and it all works out.

However, because of Broadcom always thinking "little endian u16" *and*
the current code using "u8" there's a mismatch on big endian machines
which Michael fixed up by making the u8 "overlay" the driver is using
see "little endian u16". Insofar, it is the correct fix because the
current code wants to use a u8 overlay over an array that was intended
to be a "little endian u16" array.

Now, arguably the correct fix would be to make the b44_read_eeprom
routine read an array of u16 as stored, and then use byte shifting
everywhere to fix up for the fact that Broadcom stores things into the
high/low byte of a u16 (yes, they always use a u16 layout even if the
data they want to store is just a u8 or less, then they stuff it into a
u16 and store it, they don't store byte-wise).

Now, making that fix would result in quite some ugly code along the
lines of:
        bp->dev->dev_addr[0] = eeprom[39] >> 8;
        bp->dev->dev_addr[1] = eeprom[39] & 0xFF;
        bp->dev->dev_addr[2] = eeprom[40] >> 8;
        bp->dev->dev_addr[3] = eeprom[40] & 0xFF;
etc.

Similar for the other parameters (which you seem to be forgetting by
advocating the byte swap here, but they too are stored into u16s!)

The current code obviously didn't want to do that so they overlaid the
u16 array with a u8 array to read out the bytes directly. This results
in the manual-seeming byte-swapping for some parameters of course.

Now, Michaels fix works differently instead, it properly reads out the
sprom data so that the overlaid u8 array gets what it expects, a view
into a "little endian u16" array. You really have to think of it as a
view. This is obviously what the original authors intended to do but
they were unaware of the endianness complications with that approach.

johannes

Re: [patch 09/11] b44: fix eeprom endianess issue

[Francois Romieu]

Johannes Berg <johannes@sipsolutions.net> :
[...]
> Now, arguably the correct fix would be to make the b44_read_eeprom
> routine read an array of u16 as stored, and then use byte shifting
> everywhere to fix up for the fact that Broadcom stores things into the

Yes.

> high/low byte of a u16 (yes, they always use a u16 layout even if the
> data they want to store is just a u8 or less, then they stuff it into a
> u16 and store it, they don't store byte-wise).
>
> Now, making that fix would result in quite some ugly code along the
> lines of:
>         bp->dev->dev_addr[0] = eeprom[39] >> 8;
>         bp->dev->dev_addr[1] = eeprom[39] & 0xFF;
>         bp->dev->dev_addr[2] = eeprom[40] >> 8;
>         bp->dev->dev_addr[3] = eeprom[40] & 0xFF;
> etc.

1 - see drivers/net/tg3.c. The drivers/net code does not swaps register
    a lot: one can find some cpu_to_{l/b}e or {l/b}e_to_cpu which go
    along read/write but they do not look like the norm.
2 - The code is extracting an u8 from an u16. Nothing shocking (ok, the
    capitalized 0xFF _is_ shocking :o) ).
3 - The data in the eeprom contains u16 and it has a known endianness.
    It can be annotated as such (__be16 eeprom[]). I do not know how to
    do the same with an u8 [].

-- 
Ueimor

Re: [patch 09/11] b44: fix eeprom endianess issue

[Michael Buesch]

On Tuesday 26 September 2006 19:49, Francois Romieu wrote:
> Johannes Berg <johannes@sipsolutions.net> :
> [...]
> > Now, arguably the correct fix would be to make the b44_read_eeprom
> > routine read an array of u16 as stored, and then use byte shifting
> > everywhere to fix up for the fact that Broadcom stores things into the
> 
> Yes.

That is damn unnecessary. Why change semantics and not _fix_ _current_
semantics? (Espacially, if fixing current semantics is a _lot_ easier).

> > high/low byte of a u16 (yes, they always use a u16 layout even if the
> > data they want to store is just a u8 or less, then they stuff it into a
> > u16 and store it, they don't store byte-wise).
> >
> > Now, making that fix would result in quite some ugly code along the
> > lines of:
> >         bp->dev->dev_addr[0] = eeprom[39] >> 8;
> >         bp->dev->dev_addr[1] = eeprom[39] & 0xFF;
> >         bp->dev->dev_addr[2] = eeprom[40] >> 8;
> >         bp->dev->dev_addr[3] = eeprom[40] & 0xFF;
> > etc.
> 
> 1 - see drivers/net/tg3.c. The drivers/net code does not swaps register
>     a lot: one can find some cpu_to_{l/b}e or {l/b}e_to_cpu which go
>     along read/write but they do not look like the norm.

Well, so maybe tg3 is broken, too.
But that does not matter now. We are talking about b44.

> 2 - The code is extracting an u8 from an u16. Nothing shocking (ok, the
>     capitalized 0xFF _is_ shocking :o) ).

o_O No comment, really.

> 3 - The data in the eeprom contains u16 and it has a known endianness.
>     It can be annotated as such (__be16 eeprom[]). I do not know how to
>     do the same with an u8 [].

No. The eeprom is mostly not be16. The eeprom is mostly little
endian, except the mac address and some wireless specific bits
I forgot.
But more important: This does _NOT_ matter here, as that's not
related to the acutual bug in b44.

The bug is:
Reading u16 values with readw, casting them into an u8 array
and accessing this u8 array as an u8 (byte) array.
The correct fix is to swap the CPU-ordering value returned
by readw into little endian, as the u8 array is little
endian.
This compiles to nothing on little endian hardware (so it
does not change b44 code on LE hardware), but _fixes_
code on BE hardware.

-- 
Greetings Michael.

Re: [patch 09/11] b44: fix eeprom endianess issue

[Francois Romieu]

Michael Buesch <mb@bu3sch.de> :
[...]
> > 3 - The data in the eeprom contains u16 and it has a known endianness.
> >     It can be annotated as such (__be16 eeprom[]). I do not know how to
> >     do the same with an u8 [].
> 
> No. The eeprom is mostly not be16. The eeprom is mostly little
> endian, except the mac address and some wireless specific bits
> I forgot.

Sorry, s/__be16/__le16/

-- 
Ueimor