Re: [net-next PATCH v7 5/6] net: phy: Add support for Aeonsemi AS21xxx PHYs

[Christian Marangi <ansuelsmth@gmail.com>]

On Tue, Apr 15, 2025 at 03:14:13AM +0200, Andrew Lunn wrote:
> > +#define AEON_MAX_LDES			5
> 
> AEON_MAX_LEDS?
> 
> > +#define AEON_IPC_DELAY			10000
> > +#define AEON_IPC_TIMEOUT		(AEON_IPC_DELAY * 100)
> > +#define AEON_IPC_DATA_MAX		(8 * sizeof(u16))
> 
> > +
> 
> > +static int aeon_ipc_rcv_msg(struct phy_device *phydev,
> > +			    u16 ret_sts, u16 *data)
> > +{
> 
> It would be good to add a comment here about what the return value
> means. I'm having to work hard to figure out if it is bytes, or number
> of u16s.
> 
> > +	struct as21xxx_priv *priv = phydev->priv;
> > +	unsigned int size;
> > +	int ret;
> > +	int i;
> > +
> > +	if ((ret_sts & AEON_IPC_STS_STATUS) == AEON_IPC_STS_STATUS_ERROR)
> > +		return -EINVAL;
> > +
> > +	/* Prevent IPC from stack smashing the kernel */
> > +	size = FIELD_GET(AEON_IPC_STS_SIZE, ret_sts);
> > +	if (size > AEON_IPC_DATA_MAX)
> > +		return -EINVAL;
> 
> This suggests size is bytes, and can be upto 16?
> 
> > +
> > +	mutex_lock(&priv->ipc_lock);
> > +
> > +	for (i = 0; i < DIV_ROUND_UP(size, sizeof(u16)); i++) {
> > +		ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, VEND1_IPC_DATA(i));
> > +		if (ret < 0) {
> > +			size = ret;
> > +			goto out;
> > +		}
> > +
> > +		data[i] = ret;
> 
> and this is looping up to 8 times reading words.
> 
> > +static int aeon_ipc_get_fw_version(struct phy_device *phydev)
> > +{
> > +	char fw_version[AEON_IPC_DATA_MAX + 1];
> > +	u16 ret_data[8], data[1];
> 
> I think there should be a #define for this 8. It is pretty fundamental
> to these message transfers.
> 
> > +	u16 ret_sts;
> > +	int ret;
> > +
> > +	data[0] = IPC_INFO_VERSION;
> 
> Not a normal question i would ask for MDIO, but are there any
> endianness issues here? Since everything is in u16, the base size for
> MDIO, i doubt there is.
>

In theory not, anything comes to mine that might be problematic? 

> > +	ret = aeon_ipc_send_msg(phydev, IPC_CMD_INFO, data,
> > +				sizeof(data), &ret_sts);
> > +	if (ret)
> > +		return ret;
> 
> > +	ret = aeon_ipc_rcv_msg(phydev, ret_sts, ret_data);
> > +	if (ret < 0)
> > +		return ret;
> > +
> 
> but ret is in bytes, not words, so we start getting into odd
> situations. Have you seen the firmware return an add number of bytes
> in its message? If it does, is it clear which part of the last word
> should be used.
> 

ret is size from IPC STATUS that return the transmitted data in bytes.
I use u16 to follow the fact that there are 8 rw IPC data register.

The returned firmware value is 1.8.2 for example and that is 5 bytes.

We allocate 17 bytes for the firmware string and we pass a buffer of 8
u16 (16 bytes)

Am I missing something?

> > +
> > +	/* Make sure FW version is NULL terminated */
> > +	memcpy(fw_version, ret_data, ret);
> > +	fw_version[ret] = '\0';
> 
> 
> Given that ret is bytes, this works, despite ret_data being words.
> 
> 	Andrew

-- 
	Ansuel