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

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

[Christian Marangi]

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.

Use get_unaligned_le32 instead of get_unaligned for FW data word load to
correctly convert data in the correct order to follow system endian.

Also drop the cpu_to_be32 for CRC calculation as it's wrong and use
get_unaligned_be32 instead. The word is taken from firmware and is
always LE, the mailbox will emit a BE CRC from BE word hence the
word needs to be swapped on u8 to u32 cast on LE system.
This is needed as crc_ccitt_false will recast u32 to u8 and read order
changes between BE and LE system. By using get_unaligned_be32, word is
swapped only when needed resulting in the correct CRC calculated.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202311210414.sEJZjlcD-lkp@intel.com/
Fixes: e93984ebc1c8 ("net: phy: aquantia: add firmware load support")
Tested-by: Robert Marko <robimarko@gmail.com> # ipq8072 LE device
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
Changes v2:
- Add further explaination in commit description
- Fix wrong CRC conversion and swap only when needed

 drivers/net/phy/aquantia/aquantia_firmware.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/aquantia/aquantia_firmware.c b/drivers/net/phy/aquantia/aquantia_firmware.c
index c5f292b1c4c8..c12e8a3acb77 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);
@@ -113,7 +113,7 @@ static int aqr_fw_load_memory(struct phy_device *phydev, u32 addr,
 		u32 word;
 
 		/* FW data is always stored in little-endian */
-		word = get_unaligned((const u32 *)(data + pos));
+		word = get_unaligned_le32((const u32 *)(data + pos));
 
 		phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_MAILBOX_INTERFACE5,
 			      VEND1_GLOBAL_MAILBOX_INTERFACE5_MSW_DATA(word));
@@ -125,10 +125,10 @@ static int aqr_fw_load_memory(struct phy_device *phydev, u32 addr,
 			      VEND1_GLOBAL_MAILBOX_INTERFACE1_WRITE);
 
 		/* calculate CRC as we load data to the mailbox.
-		 * We convert word to big-endian as PHY is BE and mailbox will
+		 * We read word as big-endian as PHY is BE and mailbox will
 		 * return a BE CRC.
 		 */
-		word = (__force u32)cpu_to_be32(word);
+		word = get_unaligned_be32((const u32 *)(data + pos));
 		crc = crc_ccitt_false(crc, (u8 *)&word, sizeof(word));
 	}
 
-- 
2.40.1

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

[Russell King (Oracle)]

On Mon, Nov 27, 2023 at 01:29:24AM +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.
> 
> Use get_unaligned_le32 instead of get_unaligned for FW data word load to
> correctly convert data in the correct order to follow system endian.
> 
> Also drop the cpu_to_be32 for CRC calculation as it's wrong and use
> get_unaligned_be32 instead. The word is taken from firmware and is
> always LE, the mailbox will emit a BE CRC from BE word hence the
> word needs to be swapped on u8 to u32 cast on LE system.
> This is needed as crc_ccitt_false will recast u32 to u8 and read order
> changes between BE and LE system. By using get_unaligned_be32, word is
> swapped only when needed resulting in the correct CRC calculated.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202311210414.sEJZjlcD-lkp@intel.com/
> Fixes: e93984ebc1c8 ("net: phy: aquantia: add firmware load support")
> Tested-by: Robert Marko <robimarko@gmail.com> # ipq8072 LE device
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
> Changes v2:
> - Add further explaination in commit description
> - Fix wrong CRC conversion and swap only when needed
> 
>  drivers/net/phy/aquantia/aquantia_firmware.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/phy/aquantia/aquantia_firmware.c b/drivers/net/phy/aquantia/aquantia_firmware.c
> index c5f292b1c4c8..c12e8a3acb77 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.
> +	 */

Please fix this comment. No, the address is not in LE. You program the
address into a register in the PHY. Bit 0 of the register is bit 0 of
the data field on the MDIO bus, and bit 0 of the value passed to
phy_write_mmd() which is in CPU endian. Bit 15 of the register is bit
15 of the data field on the MDIO bus, which is bit 15 of the value
passed to phy_write_mmd().

So the talk of "LE" here is meaningless. Please stop over-complicating
this.

>  	phy_write_mmd(phydev, MDIO_MMD_VEND1,
>  		      VEND1_GLOBAL_MAILBOX_INTERFACE1,
>  		      VEND1_GLOBAL_MAILBOX_INTERFACE1_CRC_RESET);
> @@ -113,7 +113,7 @@ static int aqr_fw_load_memory(struct phy_device *phydev, u32 addr,
>  		u32 word;
>  
>  		/* FW data is always stored in little-endian */
> -		word = get_unaligned((const u32 *)(data + pos));
> +		word = get_unaligned_le32((const u32 *)(data + pos));

This comment is appropriate, and get_unaligned_le32() is correct.

>  
>  		phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_MAILBOX_INTERFACE5,
>  			      VEND1_GLOBAL_MAILBOX_INTERFACE5_MSW_DATA(word));
> @@ -125,10 +125,10 @@ static int aqr_fw_load_memory(struct phy_device *phydev, u32 addr,
>  			      VEND1_GLOBAL_MAILBOX_INTERFACE1_WRITE);
>  
>  		/* calculate CRC as we load data to the mailbox.
> -		 * We convert word to big-endian as PHY is BE and mailbox will
> +		 * We read word as big-endian as PHY is BE and mailbox will
>  		 * return a BE CRC.

Is that true (about returning a BE CRC) ?

>  		 */
> -		word = (__force u32)cpu_to_be32(word);
> +		word = get_unaligned_be32((const u32 *)(data + pos));
>  		crc = crc_ccitt_false(crc, (u8 *)&word, sizeof(word));
>  	}

And you _still_ haven't taken on board my issue with this, sigh. No,
this will be subject to variation in result from CPU endianness.

For the sake of your education on the first point, do this:

	u16 le_crc = 0, be_crc = 0, up_crc;

...
		le_crc = crc_ccitt_false(le_crc, (u8 *)&word, sizeof(word));
		word = get_unaligned_be32((const u32 *)(data + pos));
		be_crc = crc_ccitt_false(be_crc, (u8 *)&word, sizeof(word));
	}

	printk("le_crc=0x%04x be_crc=0x%04x\n", le_crc, be_crc);

What you will find is that the two CRCs are _totally_ different - not
just different in endian, but different in value as well. The endianness
of the input data does not simply change the endianness of the resulting
CRC, it will change the value.

So, returning a "BE CRC" has no bearing on whether the data passed to
the CRC function needs to be in big endian order or not.

On the second point, as I have stated numerous times, and it seems I'm
not getting anywhere with:


	u32 word;
	int i;
	u8 *p;

	word = 0x01020304;

	p = (u8 *)&word;

	for (i = 0; i < sizeof(word); i++)
		printf("p[%d] = %02x\n", i, p[i]);

on little endian machines will print:

p[0] = 0x01
p[1] = 0x02
p[2] = 0x03
p[3] = 0x04

but on big endian machines will print:

p[0] = 0x04
p[1] = 0x03
p[2] = 0x02
p[3] = 0x01

The order in which you provide the bytes to a CRC function will change
the CRC value. So, the endianness of the machine/CPU your code will be
running on will change the resulting CRC.

Your code is buggy.

I've given you solutions to it, but you are very resistant to these
suggestions.

I know endianness can be difficult to those who don't understand it,
but I've described it numerous times, complete with code examples to
show how things change - including for the above issue. It seems I'm
demonstrably waiting my time because it's having very little effect.

Therefore, quite simply, NAK to this patch.

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

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

[Christian Marangi]

On Mon, Nov 27, 2023 at 10:02:51AM +0000, Russell King (Oracle) wrote:
> On Mon, Nov 27, 2023 at 01:29:24AM +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.
> > 
> > Use get_unaligned_le32 instead of get_unaligned for FW data word load to
> > correctly convert data in the correct order to follow system endian.
> > 
> > Also drop the cpu_to_be32 for CRC calculation as it's wrong and use
> > get_unaligned_be32 instead. The word is taken from firmware and is
> > always LE, the mailbox will emit a BE CRC from BE word hence the
> > word needs to be swapped on u8 to u32 cast on LE system.
> > This is needed as crc_ccitt_false will recast u32 to u8 and read order
> > changes between BE and LE system. By using get_unaligned_be32, word is
> > swapped only when needed resulting in the correct CRC calculated.
> > 
> > Reported-by: kernel test robot <lkp@intel.com>
> > Closes: https://lore.kernel.org/oe-kbuild-all/202311210414.sEJZjlcD-lkp@intel.com/
> > Fixes: e93984ebc1c8 ("net: phy: aquantia: add firmware load support")
> > Tested-by: Robert Marko <robimarko@gmail.com> # ipq8072 LE device
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> > Changes v2:
> > - Add further explaination in commit description
> > - Fix wrong CRC conversion and swap only when needed
> > 
> >  drivers/net/phy/aquantia/aquantia_firmware.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/net/phy/aquantia/aquantia_firmware.c b/drivers/net/phy/aquantia/aquantia_firmware.c
> > index c5f292b1c4c8..c12e8a3acb77 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.
> > +	 */
> 
> Please fix this comment. No, the address is not in LE. You program the
> address into a register in the PHY. Bit 0 of the register is bit 0 of
> the data field on the MDIO bus, and bit 0 of the value passed to
> phy_write_mmd() which is in CPU endian. Bit 15 of the register is bit
> 15 of the data field on the MDIO bus, which is bit 15 of the value
> passed to phy_write_mmd().
> 
> So the talk of "LE" here is meaningless. Please stop over-complicating
> this.
>

Will drop sorry.

> >  	phy_write_mmd(phydev, MDIO_MMD_VEND1,
> >  		      VEND1_GLOBAL_MAILBOX_INTERFACE1,
> >  		      VEND1_GLOBAL_MAILBOX_INTERFACE1_CRC_RESET);
> > @@ -113,7 +113,7 @@ static int aqr_fw_load_memory(struct phy_device *phydev, u32 addr,
> >  		u32 word;
> >  
> >  		/* FW data is always stored in little-endian */
> > -		word = get_unaligned((const u32 *)(data + pos));
> > +		word = get_unaligned_le32((const u32 *)(data + pos));
> 
> This comment is appropriate, and get_unaligned_le32() is correct.
> 
> >  
> >  		phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_MAILBOX_INTERFACE5,
> >  			      VEND1_GLOBAL_MAILBOX_INTERFACE5_MSW_DATA(word));
> > @@ -125,10 +125,10 @@ static int aqr_fw_load_memory(struct phy_device *phydev, u32 addr,
> >  			      VEND1_GLOBAL_MAILBOX_INTERFACE1_WRITE);
> >  
> >  		/* calculate CRC as we load data to the mailbox.
> > -		 * We convert word to big-endian as PHY is BE and mailbox will
> > +		 * We read word as big-endian as PHY is BE and mailbox will
> >  		 * return a BE CRC.
> 
> Is that true (about returning a BE CRC) ?
> 

More info at the bottom.

> >  		 */
> > -		word = (__force u32)cpu_to_be32(word);
> > +		word = get_unaligned_be32((const u32 *)(data + pos));
> >  		crc = crc_ccitt_false(crc, (u8 *)&word, sizeof(word));
> >  	}
> 
> And you _still_ haven't taken on board my issue with this, sigh. No,
> this will be subject to variation in result from CPU endianness.
> 
> For the sake of your education on the first point, do this:
> 
> 	u16 le_crc = 0, be_crc = 0, up_crc;
> 
> ...
> 		le_crc = crc_ccitt_false(le_crc, (u8 *)&word, sizeof(word));
> 		word = get_unaligned_be32((const u32 *)(data + pos));
> 		be_crc = crc_ccitt_false(be_crc, (u8 *)&word, sizeof(word));
> 	}
> 
> 	printk("le_crc=0x%04x be_crc=0x%04x\n", le_crc, be_crc);
> 
> What you will find is that the two CRCs are _totally_ different - not
> just different in endian, but different in value as well. The endianness
> of the input data does not simply change the endianness of the resulting
> CRC, it will change the value.
> 
> So, returning a "BE CRC" has no bearing on whether the data passed to
> the CRC function needs to be in big endian order or not.
> 
> On the second point, as I have stated numerous times, and it seems I'm
> not getting anywhere with:
> 
> 
> 	u32 word;
> 	int i;
> 	u8 *p;
> 
> 	word = 0x01020304;
> 
> 	p = (u8 *)&word;
> 
> 	for (i = 0; i < sizeof(word); i++)
> 		printf("p[%d] = %02x\n", i, p[i]);
> 
> on little endian machines will print:
> 
> p[0] = 0x01
> p[1] = 0x02
> p[2] = 0x03
> p[3] = 0x04
> 
> but on big endian machines will print:
> 
> p[0] = 0x04
> p[1] = 0x03
> p[2] = 0x02
> p[3] = 0x01
> 
> The order in which you provide the bytes to a CRC function will change
> the CRC value. So, the endianness of the machine/CPU your code will be
> running on will change the resulting CRC.
> 
> Your code is buggy.
> 
> I've given you solutions to it, but you are very resistant to these
> suggestions.
> 
> I know endianness can be difficult to those who don't understand it,
> but I've described it numerous times, complete with code examples to
> show how things change - including for the above issue. It seems I'm
> demonstrably waiting my time because it's having very little effect.
> 
> Therefore, quite simply, NAK to this patch.
>

Sorry for getting you annoyed, wasn't my intention at all. Also wasn't
reistantant but more of trying to find a solution without falling back
to the raw shift, trying to keep them out of the function code. Anyway
on the the explaination on how this majestic thing works.

We verified and it's almost everything correct, just badly worded I
guess...(not referring to your explaination but what is currently
implemented and written in the driver comments)

The thing works this way:
1. Things are loaded to mailbox with the data regs. (LE order expected)
2. Internally the word is converted to BE
3. CRC is calculated in the just converted data (CRC calculated on BE
   word)
4. CRC is provided via the mailbox CRC regs in LE order.

We verified this by loading a word and comparing what was the CRC
returned and we observed it's bit match the CRC of the word with the bytes
swapped.

Given this sorted out and having a clear idea of what happens
internally.

Yes your suggested way of an u8 struct it's the only way to make the crc
function to work given the cast...

An alternative might be:
		crc = crc_ccitt_false_byte(crc, data + pos + 3);
		crc = crc_ccitt_false_byte(crc, data + pos + 2);
		crc = crc_ccitt_false_byte(crc, data + pos + 1);
		crc = crc_ccitt_false_byte(crc, data + pos);

but I think this is unreadable.

Also the CRC returned from the mailbox CRC has to be converted with
le16_to_cpu since it's LE and won't match on BE system. Am I wrong?

-- 
	Ansuel

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

[Russell King (Oracle)]

On Tue, Nov 28, 2023 at 01:44:39PM +0100, Christian Marangi wrote:
> Also the CRC returned from the mailbox CRC has to be converted with
> le16_to_cpu since it's LE and won't match on BE system. Am I wrong?

I think you are. As I've said before, everything transferred over the
MDIO bus is totally independent of the CPU endianness. Bit 0 in the
registers on the PHY will appear in bit 0 of the CPU register, and
bit 15 in the registers on the PHY will appear in bit 15 of the CPU
register.

If this weren't the case, then if we access the BMCR register, and
the BMCR contains 0x1140 (indicating AN is enabled, full duplex,
1000Mbps) then if these were CPU endian-dependent, we'd end up reading
0x4011, and that will break phylib _and_ user applications (these
register definitions are exported to userspace.)

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