Re: [net-next PATCH] net: phy: aquantia: drop wrong endianness conversion for addr and CRC

[Russell King (Oracle)]

On Wed, Nov 22, 2023 at 06:53:39PM +0100, Christian Marangi wrote:
> On Wed, Nov 22, 2023 at 05:24:33PM +0000, Russell King (Oracle) wrote:
> > On Wed, Nov 22, 2023 at 06:08:13PM +0100, Christian Marangi wrote:
> > > On further testing on BE target with kernel test robot, it was notice
> > > that the endianness conversion for addr and CRC in fw_load_memory was
> > > wrong and actually not needed. Values in define doesn't get converted
> > > and are passed as is and hardcoded values are already in what the PHY
> > > require, that is LE.
> > > 
> > > Also drop the cpu_to_be32 for CRC calculation as it's wrong and use
> > > _swab32 instead, the word is taked from firmware and is always LE, the
> > 
> >                                taken
> > 
> > > mailbox will emit a BE CRC hence the word needs to be always swapped and
> > > the endianness of the host needs to be ignored.
> > 
> > I'm not convinced. If the firmware is a bytestream (as most "files" are)
> > then for val = get_unaligned((u32 *)ptr), where ptr is an array of u8:
> > 
> > ptr[0]	ptr[1]	ptr[2]	ptr[3]	val on LE	val on BE
> > 0x01	0x02	0x03	0x04	0x04030201	0x01020304
> > 
> > So, endianness matters here, and I think as Jakub already suggested, you
> > need to use get_unaligned_le32().
> >
> 
> So they DO get converted to the HOST endian on reading the firmware from
> an nvmem cell or a filesystem?

I don't like "converted". It's *not* a conversion. It's a fundamental
property of accessing memory using different sizes of access.

As I attempted to explain above, if you have a file, and byte 0
contains 0xAA, byte 1 of the file contains 0xBB, byte 2 contains
0xCC, and byte 3 contains 0xDD, then if you read that file byte by
byte, you will get 0xAA, then 0xBB, then 0xCC and then 0xDD.

If you map that file into memory, e.g. in userspace, using mmap(),
or allocating memory and reading four bytes into memory, and access
it using bytes, then at offset 0, you will find 0xAA, offset 1 will
be 0xBB, etc.

The problems with endianness start when you move away from byte
access.

If you use 16-bit accessors, then, a little endian machine is defined
that a 16-bit load from memory will result in the first byte being put
into the LSB of the 16-bit value, and the second byte will be put into
the MSB of the 16-bit value. So that would be 0xBBAA. However, on a big
endian machine, a 16-bit load will result in the first byte being put
into the MSB of the 16-bit value, and the second byte will be put into
the LSB of that value - meaning the 16-bit value will be 0xAABB.

The second 16-bit value uses the next two bytes, and the order at which
these two bytes are placed into the 16-bit value reflects the same as
the first two bytes. So LE will be 0xDDCC and BE would be 0xCCDD.

The same "swapping" happens with 32-bit, but of course instead of just
two bytes, it covers four bytes. On LE, a 32-bit access will give
0xDDCCBBAA. On BE, that will be 0xAABBCCDD.

Again, this is not to do with any kind of "conversion" happening in
software. It's a property of how the memory subsystem inside the CPU
works.

> Again this is really dumping raw data from the read file directly to the
> mailbox. Unless phy_write does some conversion internally, but in that
> case how does it know what endian is the PHY internally?

phy_write() does *no* conversion. The MDIO bus defines that a 16-bit
register value will be transferred, and the MDIO bus specifies that
bit 15 will be sent first, followed by subsequent bits down to bit 0.

The access to the hardware to make this happen is required to ensure
that the value passed to phy_write() and read using phy_read() will
reflect this. So, if one does this:

	val = phy_read(phydev, 0);

	for (i = 15; i >= 0; i--)
		printk("%u", !!(val & BIT(i)));

	printk("\n");

This will give you the stream of bits in the _order_ that they appeared
on the MDIO bus when phy_read() accessed. Doing the same with a value
to be written will produce the bits in the same value that they will
be placed on the MDIO bus.

So, this means that if the BMCR contains 0x1234 in the PHY, phy_read()
will return 0x1234. Passing 0x1234 into phy_write() will write 0x1234
in that register. The host endian is entirely irrelevant here.

> > I would make this explicit:
> > 
> > 		u8 crc_data[4];
> > 
> > 		...
> > 
> > 		/* CRC is calculated using BE order */
> > 		crc_data[0] = word >> 24;
> > 		crc_data[1] = word >> 16;
> > 		crc_data[2] = word >> 8;
> > 		crc_data[3] = word;
> > 
> > 		crc = crc_ccitt_false(crc, crc_data, sizeof(crc_data));
> > 
> > which will be (a) completely unambiguous, and (b) completely
> > independent of the host endianness.
> 
> But isn't this exactly what is done with ___constant_swab32 ?
> __swab32 should not change if the HOST is BE or LE.

Let try again to make this clear. If one has this code:

		u32 word = 0x01020304;
		u8 *ptr;
		int i;

		ptr = (u8 *)&word;

		for (i = 0; i < 4; i++)
			printk(" %02x", ptr[i]);
		printk("\n");

Then, on a:
- LE machine, this will print " 04 03 02 01"
- BE machine, this will print " 01 02 03 04"

Now, if you look at the definition of crc_ccitt_false(), it is
defined to do:

        while (len--)
                crc = crc_ccitt_false_byte(crc, *buffer++);

So, on a LE machine, this will feed the above bytes in the order of
0x04, 0x03, 0x02, 0x01 in a LE machine, and 0x01, 0x02, 0x03, 0x04
on a BE machine.

> The real question is if word is converted. (by either the read API on
> reading the FW or by phy_write on writing the thing to mailbox) (the
> test are done on a LE HOST)

There are no conversions - where a conversion I define as something
that the software explicitly has to do rather than what the underlying
machine hardware does.

> Our theory is that mailbox takes LE and internally converts to BE (as
> the PHY is BE) but the CRC reg calculates the CRC out of the converted
> data aka it does calculates the CRC from the BE data (converted
> internally).

I think the talk about the endian-ness of the PHY is entirely
unhelpful and is probably adding to confusion. The endian-ness of the
PHY is *not* exposed to the host because the MDIO interface to the PHY
is defined in terms of 16-bit register quantities, and bit 0 of the
register will be bit 0 on the host irrespective of host endian.

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