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

[Christian Marangi <ansuelsmth@gmail.com>]

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?

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?

> > diff --git a/drivers/net/phy/aquantia/aquantia_firmware.c b/drivers/net/phy/aquantia/aquantia_firmware.c
> > index c5f292b1c4c8..bd093633d0cf 100644
> > --- a/drivers/net/phy/aquantia/aquantia_firmware.c
> > +++ b/drivers/net/phy/aquantia/aquantia_firmware.c
> > @@ -93,9 +93,9 @@ static int aqr_fw_load_memory(struct phy_device *phydev, u32 addr,
> >  	u16 crc = 0, up_crc;
> >  	size_t pos;
> >  
> > -	/* PHY expect addr in LE */
> > -	addr = (__force u32)cpu_to_le32(addr);
> > -
> > +	/* PHY expect addr in LE. Hardcoded addr in defines are
> > +	 * already in this format.
> > +	 */
> >  	phy_write_mmd(phydev, MDIO_MMD_VEND1,
> >  		      VEND1_GLOBAL_MAILBOX_INTERFACE1,
> >  		      VEND1_GLOBAL_MAILBOX_INTERFACE1_CRC_RESET);
> > @@ -128,7 +128,7 @@ static int aqr_fw_load_memory(struct phy_device *phydev, u32 addr,
> >  		 * We convert word to big-endian as PHY is BE and mailbox will
> >  		 * return a BE CRC.
> >  		 */
> > -		word = (__force u32)cpu_to_be32(word);
> > +		word = __swab32(word);
> >  		crc = crc_ccitt_false(crc, (u8 *)&word, sizeof(word));
> 
> Again, I think you need to be careful with the endianness here again.
> From what I understand here, it seems the CRC needs to be generated by
> looking at the byte at ptr[3] first, then ptr[2], ptr[1] and finally
> ptr[0] ?
> 
> If that is the case, the problem is using __swab32() on LE will do the
> job for you, but on BE machines, it will be wrong.
> 
> 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.

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)

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).

-- 
	Ansuel