Message-Id: <DCKQTNSCJD5Q.BKVVU59U0MU@bootlin.com>
Date: Fri, 05 Sep 2025 11:02:03 +0200
From: Théo Lebrun <theo.lebrun@...tlin.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: "Andrew Lunn" <andrew+netdev@...n.ch>, "David S. Miller"
 <davem@...emloft.net>, "Eric Dumazet" <edumazet@...gle.com>, "Jakub
 Kicinski" <kuba@...nel.org>, "Paolo Abeni" <pabeni@...hat.com>, "Rob
 Herring" <robh@...nel.org>, "Krzysztof Kozlowski" <krzk+dt@...nel.org>,
 "Conor Dooley" <conor+dt@...nel.org>, "Nicolas Ferre"
 <nicolas.ferre@...rochip.com>, "Claudiu Beznea" <claudiu.beznea@...on.dev>,
 "Geert Uytterhoeven" <geert@...ux-m68k.org>, "Harini Katakam"
 <harini.katakam@...inx.com>, "Richard Cochran" <richardcochran@...il.com>,
 <netdev@...r.kernel.org>, <devicetree@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>, "Thomas Petazzoni"
 <thomas.petazzoni@...tlin.com>, "Tawfik Bayouk"
 <tawfik.bayouk@...ileye.com>, "Sean Anderson" <sean.anderson@...ux.dev>
Subject: Re: [PATCH net v4 5/5] net: macb: avoid double endianness swap in
 macb_set_hwaddr()

Hello Russell,

On Wed Aug 20, 2025 at 5:25 PM CEST, Russell King (Oracle) wrote:
> On Wed, Aug 20, 2025 at 04:55:09PM +0200, Théo Lebrun wrote:
>> writel() does a CPU->LE conversion. Drop manual cpu_to_le*() calls.
>> 
>> On little-endian system:
>>  - cpu_to_le32() is a no-op (LE->LE),
>>  - writel() is a no-op (LE->LE),
>>  - dev_addr will therefore not be swapped and written as-is.
>> 
>> On big-endian system:
>>  - cpu_to_le32() is a swap (BE->LE),
>>  - writel() is a swap (BE->LE),
>>  - dev_addr will therefore be swapped twice and written as a BE value.
>
> I'm not convinced by this, I think you're missing something.
>
> writel() on a BE or LE system will give you bits 7:0 of the CPU value
> written to LE bit 7:0 of the register. It has to be this way, otherwise
> we would need to do endian conversions everwhere where we write simple
> numbers to device registers.
>
> Why?
>
> Remember that on a LE system with a 32-bit bus, a hex value of
> 0x76543210 at the CPU when written without conversion will appear
> as:
> 	0 on bus bits 0:3
> 	1 on bus bits 4:7
> 	...
> 	6 on bus bits 24:27
> 	7 on bus bits 28:31
>
> whereas on a BE system, this is reversed:
> 	6 on bus bits 0:3
> 	7 on bus bits 4:7
> 	...
> 	0 on bus bits 24:27
> 	1 on bus bits 28:31
>
> The specification is that writel() will write in LE format even on
> BE systems, so there is a need to do an endian conversion for BE
> systems.
>
> So, if a device expects bits 0:7 on the bus to be the first byte of
> the MAC address (high byte of the OUI) then this must be in CPU
> bits 0:7 as well.
>
>
> Now, assuming that a MAC address of AA:BB:CC:DD:EE:FF gets read as
> 0xDDCCBBAA by the first read on a LE machine, it will get read as
> 0xAABBCCDD on a BE machine.
>
> We can now see that combining these two, getting rid of the
> cpu_to_le32() is likely wrong.
>
> Therefore, I am not convinced this patch is actually correct.

Thanks for the above, in-detail, explanation. I agree with it all.
I've always have had a hard time wrapping my head around endianness
conversion.

Indeed the patch is wrong: the swap is required on BE platforms.
My gripe is more with the semantic of the current code:

   bottom = cpu_to_le32(*((u32 *)bp->dev->dev_addr));
   macb_or_gem_writel(bp, SA1B, bottom);
   top = cpu_to_le16(*((u16 *)(bp->dev->dev_addr + 4)));
   macb_or_gem_writel(bp, SA1T, top);

Notice how:
 - The type of the argument to cpu_to_le32(); pointer is to a CPU-endian
   value (u32) but in reality is to a BE32.
 - We apply cpu_to_le32() to get a swap but the semantic is wrong; input
   value is BE32 that we want to turn into CPU-endian.
 - Above two points apply to `u16 top` as well.
 - writel() are unrelated to the issue; they do the right thing by
   writing a CPU value into a LE device register.

Sparse is complaining about the second bulletpoint; it won't complain
about the first one because it trusts that you know what you are doing
with explicit casts.

   warning: incorrect type in assignment (different base types)
      expected unsigned int [usertype] bottom
      got restricted __le32 [usertype]

If we want to keep to the same structure, this does the exact same but
its semantic is more aligned to reality (to my eyes):

   bottom = le32_to_cpu(*((__le32 *)bp->dev->dev_addr));
   macb_or_gem_writel(bp, SA1B, bottom);
   top = le16_to_cpu(*((__le16 *)(bp->dev->dev_addr + 4)));
   macb_or_gem_writel(bp, SA1T, top);

Notice how:
 - Casts are fixed to signal proper types.
 - Use le32_to_cpu().

Sparse is happy and code has been tested on a BE platform.
Assembly generated is strictly identical.

However, I think we can do better. Second option:

   const unsigned char *addr = bp->dev->dev_addr;

   bottom = addr[0] << 0 | addr[1] << 8 | addr[2] << 16 | addr[3] << 24;
   top = addr[4] << 0 | addr[5] << 8;

This is a bit of a mouthful, what about this one?

   bottom = get_unaligned_le32(addr);
   top = get_unaligned_le16(addr + 4);

It is my preferred. I found those helpers reading more code that reads
the `unsigned char *dev_addr` field. Explicit and straight forward.

Can you confirm that last option fits well?

Thanks Russell,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Powered by blists - more mailing lists