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: <8760a101-4536-474f-a0db-5b88ed4c0ec2@lunn.ch>
Date: Tue, 15 Apr 2025 03:14:13 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Christian Marangi <ansuelsmth@...il.com>
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

> +#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.

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

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