linux-kernel - Re: [net-next RFC PATCH v3 3/4] 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: <20250327092418.78f55466@fedora-2.home>
Date: Thu, 27 Mar 2025 09:24:18 +0100
From: Maxime Chevallier <maxime.chevallier@...tlin.com>
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>, Eric Woudstra <ericwouds@...il.com>, Daniel Golle
 <daniel@...rotopia.org>, netdev@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [net-next RFC PATCH v3 3/4] net: phy: Add support for Aeonsemi
 AS21xxx PHYs

Hi Christian,

On Thu, 27 Mar 2025 00:35:03 +0100
Christian Marangi <ansuelsmth@...il.com> wrote:

> Add support for Aeonsemi AS21xxx 10G C45 PHYs. These PHYs integrate
> an IPC to setup some configuration and require special handling to
> sync with the parity bit. The parity bit is a way the IPC use to
> follow correct order of command sent.
> 
> Supported PHYs AS21011JB1, AS21011PB1, AS21010JB1, AS21010PB1,
> AS21511JB1, AS21511PB1, AS21510JB1, AS21510PB1, AS21210JB1,
> AS21210PB1 that all register with the PHY ID 0x7500 0x7510
> before the firmware is loaded.
> 
> They all support up to 5 LEDs with various HW mode supported.
> 
> While implementing it was found some strange coincidence with using the
> same logic for implementing C22 in MMD regs in Broadcom PHYs.
> 
> For reference here the AS21xxx PHY name logic:
> 
> AS21x1xxB1
>     ^ ^^
>     | |J: Supports SyncE/PTP
>     | |P: No SyncE/PTP support
>     | 1: Supports 2nd Serdes
>     | 2: Not 2nd Serdes support
>     0: 10G, 5G, 2.5G
>     5: 5G, 2.5G
>     2: 2.5G
> 
> Signed-off-by: Christian Marangi <ansuelsmth@...il.com>

 [...]

I know this is only RFC, but I have some questions

> +static int as21xxx_match_phy_device(struct phy_device *phydev,
> +				    const struct phy_driver *phydrv)
> +{
> +	struct as21xxx_priv *priv;
> +	u32 phy_id;
> +	int ret;
> +
> +	/* Skip PHY that are not AS21xxx or already have firmware loaded */
> +	if (phydev->c45_ids.device_ids[MDIO_MMD_PCS] != PHY_ID_AS21XXX)
> +		return phydev->phy_id == phydrv->phy_id;
> +
> +	/* Read PHY ID to handle firmware just loaded */
> +	ret = phy_read_mmd(phydev, MDIO_MMD_PCS, MII_PHYSID1);
> +	if (ret < 0)
> +		return ret;
> +	phy_id = ret << 16;
> +
> +	ret = phy_read_mmd(phydev, MDIO_MMD_PCS, MII_PHYSID2);
> +	if (ret < 0)
> +		return ret;	
> +	phy_id |= ret;
> +
> +	/* With PHY ID not the generic AS21xxx one assume
> +	 * the firmware just loaded
> +	 */
> +	if (phy_id != PHY_ID_AS21XXX)
> +		return phy_id == phydrv->phy_id;
> +
> +	/* Allocate temp priv and load the firmware */
> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	mutex_init(&priv->ipc_lock);
> +
> +	ret = aeon_firmware_load(phydev);
> +	if (ret)
> +		return ret;

Here, and below, you leak priv by returning early.

> +
> +	ret = aeon_ipc_sync_parity(phydev, priv);
> +	if (ret)
> +		return ret;
> +
> +	/* Enable PTP clk if not already Enabled */
> +	ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, VEND1_PTP_CLK,
> +			       VEND1_PTP_CLK_EN);
> +	if (ret)
> +		return ret;
> +
> +	ret = aeon_dpc_ra_enable(phydev, priv);
> +	if (ret)
> +		return ret;

Does all of the above with sync_parity, PTP clock cfg and so on needs
to be done in this first pass of the matching process for this PHY ?

From what I got from the discussions, the only important bit is to load
the FW to get the correct PHY id ?

> +	mutex_destroy(&priv->ipc_lock);
> +	kfree(priv);
> +
> +	/* Return not maching anyway as PHY ID will change after
> +	 * firmware is loaded.
> +	 */
> +	return 0;
> +}

Maxime