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

[Michael Buesch <mb@bu3sch.de>]

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.