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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <67fe40cb.050a0220.130904.f08d@mx.google.com>
Date: Tue, 15 Apr 2025 13:19:34 +0200
From: Christian Marangi <ansuelsmth@...il.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Russell King <linux@...linux.org.uk>,
	Florian Fainelli <florian.fainelli@...adcom.com>,
	Broadcom internal kernel review list <bcm-kernel-feedback-list@...adcom.com>,
	Marek Behún <kabel@...nel.org>,
	Andrei Botila <andrei.botila@....nxp.com>,
	Sabrina Dubroca <sd@...asysnail.net>,
	Eric Woudstra <ericwouds@...il.com>,
	Daniel Golle <daniel@...rotopia.org>, netdev@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.or
Subject: Re: [net-next PATCH v7 5/6] net: phy: Add support for Aeonsemi
 AS21xxx PHYs

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