Re: [net-next RFC PATCH v6 3/4] net: phy: aquantia: add firmware load support

[Christian Marangi]

On Fri, Nov 10, 2023 at 07:57:02PM +0000, Simon Horman wrote:
> On Thu, Nov 09, 2023 at 01:32:52PM +0100, Christian Marangi wrote:
> > From: Robert Marko <robimarko@xxxxxxxxx>
> > 
> > Aquantia PHY-s require firmware to be loaded before they start operating.
> > It can be automatically loaded in case when there is a SPI-NOR connected
> > to Aquantia PHY-s or can be loaded from the host via MDIO.
> > 
> > This patch adds support for loading the firmware via MDIO as in most cases
> > there is no SPI-NOR being used to save on cost.
> > Firmware loading code itself is ported from mainline U-boot with cleanups.
> > 
> > The firmware has mixed values both in big and little endian.
> > PHY core itself is big-endian but it expects values to be in little-endian.
> > The firmware is little-endian but CRC-16 value for it is stored at the end
> > of firmware in big-endian.
> > 
> > It seems the PHY does the conversion internally from firmware that is
> > little-endian to the PHY that is big-endian on using the mailbox
> > but mailbox returns a big-endian CRC-16 to verify the written data
> > integrity.
> > 
> > Co-developed-by: Christian Marangi <ansuelsmth@xxxxxxxxx>
> > Signed-off-by: Robert Marko <robimarko@xxxxxxxxx>
> > Signed-off-by: Christian Marangi <ansuelsmth@xxxxxxxxx>
> 
> Hi Christian and Robert,
> 
> thanks for your patch-set.
> 
> I spotted some minor endien issues which I have highlighted below.
> 
> ...
>

Hi Simon,

thanks for the check!

> > +/* load data into the phy's memory */
> > +static int aqr_fw_load_memory(struct phy_device *phydev, u32 addr,
> > +			      const u8 *data, size_t len)
> > +{
> > +	u16 crc = 0, up_crc;
> > +	size_t pos;
> > +
> > +	/* PHY expect addr in LE */
> > +	addr = cpu_to_le32(addr);
> 
> The type of addr is host byte-order,
> but here it is assigned a little-endian value.
> 
> Flagged by Sparse.
> 
> > +
> > +	phy_write_mmd(phydev, MDIO_MMD_VEND1,
> > +		      VEND1_GLOBAL_MAILBOX_INTERFACE1,
> > +		      VEND1_GLOBAL_MAILBOX_INTERFACE1_CRC_RESET);
> > +	phy_write_mmd(phydev, MDIO_MMD_VEND1,
> > +		      VEND1_GLOBAL_MAILBOX_INTERFACE3,
> > +		      VEND1_GLOBAL_MAILBOX_INTERFACE3_MSW_ADDR(addr));
> 
> VEND1_GLOBAL_MAILBOX_INTERFACE3_MSW_ADDR() performs a bit-shift on addr,
> and applies a mask which is in host-byte order.
> But, as highlighted above, addr is a little-endian value.
> This does not seem right.
>

It's really just some magic to split the addr and swap if we are not
in little-endian. The passed addr are defined here in the code and are
hardcoded, they doesn't come from the firmware. What I can do is just
recast __le32 to u32 again with __force to mute the warning...

Resulting in this snippet:

	__le32 addr;
	size_t pos;

	/* PHY expect addr in LE */
	addr = cpu_to_le32(load_addr);

	phy_write_mmd(phydev, MDIO_MMD_VEND1,
		      VEND1_GLOBAL_MAILBOX_INTERFACE1,
		      VEND1_GLOBAL_MAILBOX_INTERFACE1_CRC_RESET);
	phy_write_mmd(phydev, MDIO_MMD_VEND1,
		      VEND1_GLOBAL_MAILBOX_INTERFACE3,
		      VEND1_GLOBAL_MAILBOX_INTERFACE3_MSW_ADDR((__force u32)addr));
	phy_write_mmd(phydev, MDIO_MMD_VEND1,
		      VEND1_GLOBAL_MAILBOX_INTERFACE4,
		      VEND1_GLOBAL_MAILBOX_INTERFACE4_LSW_ADDR((__force u32)addr));

Also things needs to be casted to u16 anyway as phy_write_mmd expect a
u16. And as you said FILED_PREP will use int (from the define) so I
wonder if a more clean way would be just addr = (__force u32)cpu_to_le32(load_addr)
resulting in a simple bswap32 if we are in big-endian.

Would love some feedback about this.

> This is all hidden by a cast in VEND1_GLOBAL_MAILBOX_INTERFACE3_MSW_ADDR()
> This seems dangerous to me.
> 
> 
> > +	phy_write_mmd(phydev, MDIO_MMD_VEND1,
> > +		      VEND1_GLOBAL_MAILBOX_INTERFACE4,
> > +		      VEND1_GLOBAL_MAILBOX_INTERFACE4_LSW_ADDR(addr));
> 
> There seem to be similar issues with the use of addr here.
> 
> > +
> > +	/* We assume and enforce the size to be word aligned.
> > +	 * If a firmware that is not word aligned is found, please report upstream.
> > +	 */
> > +	for (pos = 0; pos < len; pos += sizeof(u32)) {
> > +		u32 word = get_unaligned((const u32 *)(data + pos));
> > +
> > +		phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_MAILBOX_INTERFACE5,
> > +			      VEND1_GLOBAL_MAILBOX_INTERFACE5_MSW_DATA(word));
> > +		phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_MAILBOX_INTERFACE6,
> > +			      VEND1_GLOBAL_MAILBOX_INTERFACE6_LSW_DATA(word));
> > +
> > +		phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_MAILBOX_INTERFACE1,
> > +			      VEND1_GLOBAL_MAILBOX_INTERFACE1_EXECUTE |
> > +			      VEND1_GLOBAL_MAILBOX_INTERFACE1_WRITE);
> > +
> > +		/* calculate CRC as we load data to the mailbox.
> > +		 * We convert word to big-endiang as PHY is BE and mailbox will
> > +		 * return a BE CRC.
> > +		 */
> > +		word = cpu_to_be32(word);
> 
> Similarly here, Sparse flags that a little-endian value is assigned to a
> host byte-order variable.
> 

Same here, I'm solving by declaring a new __be32 variable but in
crc_ccitt_false the thing needs to be casted anyway, doesn't that makes
the check useless?

> > +		crc = crc_ccitt_false(crc, (u8 *)&word, sizeof(word));
> > +	}
> 
> ...
> 
> pw-bot: changes-requested

-- 
	Ansuel