linux-kernel - Re: [net-next PATCH 1/2] net: phy: Add support for new Aeonsemi 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: <67e1692c.050a0220.2b4ad0.c073@mx.google.com>
Date: Mon, 24 Mar 2025 15:16:08 +0100
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>, netdev@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [net-next PATCH 1/2] net: phy: Add support for new Aeonsemi PHYs

On Mon, Mar 24, 2025 at 03:03:51PM +0100, Andrew Lunn wrote:
> > Supported PHYs AS21011JB1, AS21011PB1, AS21010JB1, AS21010PB1,
> > AS21511JB1, AS21511PB1, AS21510JB1, AS21510PB1, AS21210JB1,
> > AS21210PB1 that all register with the PHY ID 0x7500 0x7500
> > before the firmware is loaded.
> 
> Does the value change after the firmware is loaded? Is the same
> firmware used for all variants?
>

Yes It does... Can PHY subsystem react on this? Like probe for the
generic one and then use the OPs for the real PHY ID?

> > +++ b/drivers/net/phy/Kconfig
> > @@ -121,6 +121,18 @@ config AMCC_QT2025_PHY
> >  
> >  source "drivers/net/phy/aquantia/Kconfig"
> >  
> > +config AS21XXX_PHY
> > +	tristate "Aeonsemi AS21xxx PHYs"
> 
> The sorting is based on the tristate value, so that when you look at
> 'make menuconfig' the menu is in alphabetical order. So this goes
> before aquantia.
> 

Tought it was only alphabetical, will move.

> > +/* 5 LED at step of 0x20
> > + * FE: Fast-Ethernet (100)
> > + * GE: Gigabit-Ethernet (1000)
> > + * NG: New-Generation (2500/5000/10000)
> > + * (Lovely ChatGPT managed to translate meaning of NG)
> 
> It might be a reference to NBase-T Gigabit.
> 

A better LED table was provided for this confirming ChatGPT assumption.
Will update the LED pattern as there were some mistake for activity
blink.

Also confirmed no way to make the LED only blink... It's sad cause the
blink time can be configured...

> Please add a comment somewhere about how locking works for IPCs. As
> far as i see, the current locking scheme is that IPCs are only called
> from probe, so no locking is actually required. But:
> 
> > +#define IPC_CMD_NG_TESTMODE		0x1b /* Set NG test mode and tone */
> > +#define IPC_CMD_TEMP_MON		0x15 /* Temperature monitoring function */
> > +#define IPC_CMD_SET_LED			0x23 /* Set led */
> 
> suggests IPCs might in the future be needed outside of probe, and then
> a different locking scheme might be needed, particularly for
> temperature monitoring.
> 

Indeed I will push temperature monitor in a followup patch so yes I will
add locking just for preparation of the additional feature.

> > +static int as21xxx_get_features(struct phy_device *phydev)
> > +{
> > +	int ret;
> > +
> > +	ret = genphy_read_abilities(phydev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* AS21xxx supports 100M/1G/2.5G/5G/10G speed. */
> > +	linkmode_clear_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT,
> > +			   phydev->supported);
> > +	linkmode_clear_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT,
> > +			   phydev->supported);
> > +	linkmode_clear_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT,
> > +			   phydev->supported);
> 
> Does this mean the registers genphy_read_abilities() reads are broken
> and report link modes it does not actually support?
> 
> > +	linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT,
> > +			 phydev->supported);
> > +	linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
> > +			 phydev->supported);
> 
> and it is also not reporting modes it does actually support? Is
> genphy_read_abilities() actually doing anything useful? Some more
> comments would be good here.
> 
> > +	linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
> > +			 phydev->supported);
> > +	linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
> > +			 phydev->supported);
> > +	linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT,
> > +			 phydev->supported);
> 
> Does this mean genphy_c45_pma_read_abilities() also returns the wrong
> values?
> 

Honestly I followed what the SDK driver did for the get_feature. I will
check the register making sure correct bits are reported.

Looking at the get_status It might be conflicting as they map C22 in C45
vendor regs.

> > +static int as21xxx_read_link(struct phy_device *phydev, int *bmcr)
> > +{
> > +	int status;
> > +
> > +	*bmcr = phy_read_mmd(phydev, MDIO_MMD_AN,
> > +			     MDIO_AN_C22 + MII_BMCR);
> > +	if (*bmcr < 0)
> > +		return *bmcr;
> > +
> > +	/* Autoneg is being started, therefore disregard current
> > +	 * link status and report link as down.
> > +	 */
> > +	if (*bmcr & BMCR_ANRESTART) {
> > +		phydev->link = 0;
> > +		return 0;
> > +	}
> > +
> > +	status = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
> 
> No MDIO_AN_C22 + here? Maybe add a comment about which C22 registers
> are mapped into C45 space.
>

Nope, not a typo... They took the vendor route for some register but for
BMSR they used the expected one.

Just to make it clear, using the AN_C22 wasn't a choice... the C45 BMCR
reports inconsistent data compared to the vendor C22 one.

-- 
	Ansuel