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

[Simon Horman]

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.

...

> +/* 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.

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.

> +		crc = crc_ccitt_false(crc, (u8 *)&word, sizeof(word));
> +	}

...

pw-bot: changes-requested