Re: [PATCH net v4 5/5] net: macb: avoid double endianness swap in macb_set_hwaddr()

["Russell King (Oracle)" <linux@armlinux.org.uk>]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!