[PATCH net v3 0/4] net: phy: as21xxx: simple series for trivial fixes

[PATCH net v3 0/4] net: phy: as21xxx: simple series for trivial fixes

[Christian Marangi]

This is a simple series to account some fixes for some corner
case for as21xxx

Changes v3:
- Rework to address some comments from sashiko

Changes v2:
- Add review tag

Christian Marangi (4):
  net: phy: as21xxx: handle stale link status during autoneg
  net: phy: as21xxx: fix read_status speed handling
  net: phy: as21xxx: force C45 OPs for AUTONEG
  net: phy: as21xxx: handle odd firmware on loading

 drivers/net/phy/as21xxx.c | 180 ++++++++++++++++++++++++++++----------
 1 file changed, 136 insertions(+), 44 deletions(-)

-- 
2.55.0

[PATCH net v3 1/4] net: phy: as21xxx: handle stale link status during autoneg

[Christian Marangi]

When autonegotiation is restarted, MDIO_STAT1_ANEGCOMPLETE may be
cleared before MDIO_STAT1_LSTATUS. In this case, the link status bit
may still indicate link up even though autonegotiation has not
completed.

Force the link down when autonegotiation is enabled but has not
completed, preventing as21xxx_read_status() from reporting a link up
with an unknown or stale speed.

Fixes: 830877d89edc ("net: phy: Add support for Aeonsemi AS21xxx PHYs")
Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/phy/as21xxx.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/phy/as21xxx.c b/drivers/net/phy/as21xxx.c
index d5738117eca6..0db82da8dbdf 100644
--- a/drivers/net/phy/as21xxx.c
+++ b/drivers/net/phy/as21xxx.c
@@ -658,6 +658,13 @@ static int as21xxx_read_link(struct phy_device *phydev, int *bmcr)
 		return status;
 
 	phydev->link = !!(status & MDIO_STAT1_LSTATUS);
+	phydev->autoneg_complete = !!(status & MDIO_AN_STAT1_COMPLETE);
+
+	/* Consider the case that autoneg was started and "aneg complete"
+	 * bit has been reset, but "link up" bit not yet.
+	 */
+	if (phydev->autoneg == AUTONEG_ENABLE && !phydev->autoneg_complete)
+		phydev->link = 0;
 
 	return 0;
 }
-- 
2.55.0

[PATCH net v3 2/4] net: phy: as21xxx: fix read_status speed handling

[Christian Marangi]

With further test with 2.5G NIC it was discovered that
phy_resolve_aneg_linkmode is not enough to detect speed higher that 1G
when autoneg is enabled.

Also in the switch case there is a typo where the speed mask is AND with
VEND1_SPEED_STATUS instead of the correct mask VEND1_SPEED_MASK.

Rework the read_status code to use the vendor register to read speed >1G.

While at it also address a problem in read link where autoneg_complete
wasn't cleared when AN was restarted.

Fixes: 830877d89edc ("net: phy: Add support for Aeonsemi AS21xxx PHYs")
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/phy/as21xxx.c | 100 ++++++++++++++++++++++----------------
 1 file changed, 58 insertions(+), 42 deletions(-)

diff --git a/drivers/net/phy/as21xxx.c b/drivers/net/phy/as21xxx.c
index 0db82da8dbdf..f5968eec11c8 100644
--- a/drivers/net/phy/as21xxx.c
+++ b/drivers/net/phy/as21xxx.c
@@ -650,6 +650,7 @@ static int as21xxx_read_link(struct phy_device *phydev, int *bmcr)
 	 */
 	if (*bmcr & BMCR_ANRESTART) {
 		phydev->link = 0;
+		phydev->autoneg_complete = 0;
 		return 0;
 	}
 
@@ -671,7 +672,7 @@ static int as21xxx_read_link(struct phy_device *phydev, int *bmcr)
 
 static int as21xxx_read_c22_lpa(struct phy_device *phydev)
 {
-	int lpagb;
+	int lpagb, lpa;
 
 	/* MII_STAT1000 are only filled in the mapped C22
 	 * in C45, use that to fill lpagb values and check.
@@ -698,13 +699,22 @@ static int as21xxx_read_c22_lpa(struct phy_device *phydev)
 	mii_stat1000_mod_linkmode_lpa_t(phydev->lp_advertising,
 					lpagb);
 
+	lpa = phy_read_mmd(phydev, MDIO_MMD_AN,
+			   AS21XXX_MDIO_AN_C22 + MII_LPA);
+	if (lpa < 0)
+		return lpa;
+
+	mii_lpa_mod_linkmode_lpa_t(phydev->lp_advertising, lpa);
+
 	return 0;
 }
 
 static int as21xxx_read_status(struct phy_device *phydev)
 {
 	int bmcr, old_link = phydev->link;
+	int speed;
 	int ret;
+	int val;
 
 	ret = as21xxx_read_link(phydev, &bmcr);
 	if (ret)
@@ -719,57 +729,63 @@ static int as21xxx_read_status(struct phy_device *phydev)
 	phydev->pause = 0;
 	phydev->asym_pause = 0;
 
+	if (!phydev->link)
+		return 0;
+
 	if (phydev->autoneg == AUTONEG_ENABLE) {
-		ret = genphy_c45_read_lpa(phydev);
-		if (ret)
-			return ret;
+		if (!phydev->autoneg_complete) {
+			mii_stat1000_mod_linkmode_lpa_t(phydev->lp_advertising,
+							0);
+			mii_lpa_mod_linkmode_lpa_t(phydev->lp_advertising, 0);
+			mii_10gbt_stat_mod_linkmode_lpa_t(phydev->lp_advertising, 0);
+			return 0;
+		}
 
 		ret = as21xxx_read_c22_lpa(phydev);
 		if (ret)
 			return ret;
 
-		phy_resolve_aneg_linkmode(phydev);
-	} else {
-		int speed;
+		val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_10GBT_STAT);
+		if (val < 0)
+			return val;
 
+		mii_10gbt_stat_mod_linkmode_lpa_t(phydev->lp_advertising, val);
+	} else {
 		linkmode_zero(phydev->lp_advertising);
+	}
 
-		speed = phy_read_mmd(phydev, MDIO_MMD_VEND1,
-				     VEND1_SPEED_STATUS);
-		if (speed < 0)
-			return speed;
+	if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) {
+		phy_resolve_aneg_linkmode(phydev);
+	} else if (phydev->autoneg == AUTONEG_DISABLE) {
+		ret = genphy_read_status_fixed(phydev);
+		if (ret < 0)
+			return ret;
+	}
 
-		switch (speed & VEND1_SPEED_STATUS) {
-		case VEND1_SPEED_10000:
-			phydev->speed = SPEED_10000;
-			phydev->duplex = DUPLEX_FULL;
-			break;
-		case VEND1_SPEED_5000:
-			phydev->speed = SPEED_5000;
-			phydev->duplex = DUPLEX_FULL;
-			break;
-		case VEND1_SPEED_2500:
-			phydev->speed = SPEED_2500;
-			phydev->duplex = DUPLEX_FULL;
-			break;
-		case VEND1_SPEED_1000:
-			phydev->speed = SPEED_1000;
-			if (bmcr & BMCR_FULLDPLX)
-				phydev->duplex = DUPLEX_FULL;
-			else
-				phydev->duplex = DUPLEX_HALF;
-			break;
-		case VEND1_SPEED_100:
-			phydev->speed = SPEED_100;
-			phydev->duplex = DUPLEX_FULL;
-			break;
-		case VEND1_SPEED_10:
-			phydev->speed = SPEED_10;
-			phydev->duplex = DUPLEX_FULL;
-			break;
-		default:
-			return -EINVAL;
-		}
+	speed = phy_read_mmd(phydev, MDIO_MMD_VEND1,
+			     VEND1_SPEED_STATUS);
+	if (speed < 0)
+		return speed;
+
+	switch (speed & VEND1_SPEED_MASK) {
+	case VEND1_SPEED_10000:
+		phydev->speed = SPEED_10000;
+		phydev->duplex = DUPLEX_FULL;
+		break;
+	case VEND1_SPEED_5000:
+		phydev->speed = SPEED_5000;
+		phydev->duplex = DUPLEX_FULL;
+		break;
+	case VEND1_SPEED_2500:
+		phydev->speed = SPEED_2500;
+		phydev->duplex = DUPLEX_FULL;
+		break;
+	case VEND1_SPEED_1000:
+	case VEND1_SPEED_100:
+	case VEND1_SPEED_10:
+		break;
+	default:
+		return -EINVAL;
 	}
 
 	return 0;
-- 
2.55.0

[PATCH net v3 3/4] net: phy: as21xxx: force C45 OPs for AUTONEG

[Christian Marangi]

With further testing with 2.5G NIC, it was discovered that the PHY
require the C45 OPs to configure and restart ANEG or speed higher than
1G doesn't function correctly.

To force C45 OPs with generic PHY function, clear the C22 bit from
devices_in_package bitmask and implement a custom .config_aneg.

Fixes: 830877d89edc ("net: phy: Add support for Aeonsemi AS21xxx PHYs")
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/phy/as21xxx.c | 67 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/drivers/net/phy/as21xxx.c b/drivers/net/phy/as21xxx.c
index f5968eec11c8..81e9827873bf 100644
--- a/drivers/net/phy/as21xxx.c
+++ b/drivers/net/phy/as21xxx.c
@@ -616,6 +616,13 @@ static int as21xxx_probe(struct phy_device *phydev)
 	if (ret)
 		return ret;
 
+	/* Even if PHY declare support for Clause 22 register,
+	 * Clause 45 register should be used for ANEG configuration
+	 * and restart. Clear the C22 bit for devices_in_package to
+	 * force C45 generic OPs in generic PHY ANGE OPs.
+	 */
+	phydev->c45_ids.devices_in_package &= ~BIT(0);
+
 	ret = aeon_ipc_sync_parity(phydev, priv);
 	if (ret)
 		return ret;
@@ -633,6 +640,56 @@ static int as21xxx_probe(struct phy_device *phydev)
 	return aeon_dpc_ra_enable(phydev);
 }
 
+static int as21xxx_config_aneg(struct phy_device *phydev)
+{
+	bool changed = false;
+	u16 ctl = 0;
+	int ret;
+
+	if (phydev->autoneg == AUTONEG_DISABLE)
+		return genphy_c45_pma_setup_forced(phydev);
+
+	ret = genphy_c45_an_config_aneg(phydev);
+	if (ret < 0)
+		return ret;
+	if (ret > 0)
+		changed = true;
+
+	switch (phydev->master_slave_set) {
+	case MASTER_SLAVE_CFG_MASTER_PREFERRED:
+		ctl |= CTL1000_PREFER_MASTER;
+		break;
+	case MASTER_SLAVE_CFG_SLAVE_PREFERRED:
+		break;
+	case MASTER_SLAVE_CFG_MASTER_FORCE:
+		ctl |= CTL1000_AS_MASTER;
+		fallthrough;
+	case MASTER_SLAVE_CFG_SLAVE_FORCE:
+		ctl |= CTL1000_ENABLE_MASTER;
+		break;
+	case MASTER_SLAVE_CFG_UNKNOWN:
+	case MASTER_SLAVE_CFG_UNSUPPORTED:
+		break;
+	default:
+		phydev_warn(phydev, "Unsupported Master/Slave mode\n");
+		return -EOPNOTSUPP;
+	}
+
+	ctl = linkmode_adv_to_mii_ctrl1000_t(phydev->advertising);
+
+	ret = phy_modify_mmd_changed(phydev, MDIO_MMD_AN,
+				     AS21XXX_MDIO_AN_C22 + MII_CTRL1000,
+				     ADVERTISE_1000FULL | ADVERTISE_1000HALF |
+				     CTL1000_ENABLE_MASTER | CTL1000_AS_MASTER |
+				     CTL1000_PREFER_MASTER, ctl);
+	if (ret < 0)
+		return ret;
+	if (ret > 0)
+		changed = true;
+
+	return genphy_c45_check_and_restart_aneg(phydev, changed);
+}
+
 static int as21xxx_read_link(struct phy_device *phydev, int *bmcr)
 {
 	int status;
@@ -982,6 +1039,7 @@ static struct phy_driver as21xxx_drivers[] = {
 		.name		= "Aeonsemi AS21011JB1",
 		.probe		= as21xxx_probe,
 		.match_phy_device = as21xxx_match_phy_device,
+		.config_aneg	= as21xxx_config_aneg,
 		.read_status	= as21xxx_read_status,
 		.led_brightness_set = as21xxx_led_brightness_set,
 		.led_hw_is_supported = as21xxx_led_hw_is_supported,
@@ -994,6 +1052,7 @@ static struct phy_driver as21xxx_drivers[] = {
 		.name		= "Aeonsemi AS21011PB1",
 		.probe		= as21xxx_probe,
 		.match_phy_device = as21xxx_match_phy_device,
+		.config_aneg	= as21xxx_config_aneg,
 		.read_status	= as21xxx_read_status,
 		.led_brightness_set = as21xxx_led_brightness_set,
 		.led_hw_is_supported = as21xxx_led_hw_is_supported,
@@ -1006,6 +1065,7 @@ static struct phy_driver as21xxx_drivers[] = {
 		.name		= "Aeonsemi AS21010PB1",
 		.probe		= as21xxx_probe,
 		.match_phy_device = as21xxx_match_phy_device,
+		.config_aneg	= as21xxx_config_aneg,
 		.read_status	= as21xxx_read_status,
 		.led_brightness_set = as21xxx_led_brightness_set,
 		.led_hw_is_supported = as21xxx_led_hw_is_supported,
@@ -1018,6 +1078,7 @@ static struct phy_driver as21xxx_drivers[] = {
 		.name		= "Aeonsemi AS21010JB1",
 		.probe		= as21xxx_probe,
 		.match_phy_device = as21xxx_match_phy_device,
+		.config_aneg	= as21xxx_config_aneg,
 		.read_status	= as21xxx_read_status,
 		.led_brightness_set = as21xxx_led_brightness_set,
 		.led_hw_is_supported = as21xxx_led_hw_is_supported,
@@ -1030,6 +1091,7 @@ static struct phy_driver as21xxx_drivers[] = {
 		.name		= "Aeonsemi AS21210PB1",
 		.probe		= as21xxx_probe,
 		.match_phy_device = as21xxx_match_phy_device,
+		.config_aneg	= as21xxx_config_aneg,
 		.read_status	= as21xxx_read_status,
 		.led_brightness_set = as21xxx_led_brightness_set,
 		.led_hw_is_supported = as21xxx_led_hw_is_supported,
@@ -1042,6 +1104,7 @@ static struct phy_driver as21xxx_drivers[] = {
 		.name		= "Aeonsemi AS21510JB1",
 		.probe		= as21xxx_probe,
 		.match_phy_device = as21xxx_match_phy_device,
+		.config_aneg	= as21xxx_config_aneg,
 		.read_status	= as21xxx_read_status,
 		.led_brightness_set = as21xxx_led_brightness_set,
 		.led_hw_is_supported = as21xxx_led_hw_is_supported,
@@ -1054,6 +1117,7 @@ static struct phy_driver as21xxx_drivers[] = {
 		.name		= "Aeonsemi AS21510PB1",
 		.probe		= as21xxx_probe,
 		.match_phy_device = as21xxx_match_phy_device,
+		.config_aneg	= as21xxx_config_aneg,
 		.read_status	= as21xxx_read_status,
 		.led_brightness_set = as21xxx_led_brightness_set,
 		.led_hw_is_supported = as21xxx_led_hw_is_supported,
@@ -1066,6 +1130,7 @@ static struct phy_driver as21xxx_drivers[] = {
 		.name		= "Aeonsemi AS21511JB1",
 		.probe		= as21xxx_probe,
 		.match_phy_device = as21xxx_match_phy_device,
+		.config_aneg	= as21xxx_config_aneg,
 		.read_status	= as21xxx_read_status,
 		.led_brightness_set = as21xxx_led_brightness_set,
 		.led_hw_is_supported = as21xxx_led_hw_is_supported,
@@ -1078,6 +1143,7 @@ static struct phy_driver as21xxx_drivers[] = {
 		.name		= "Aeonsemi AS21210JB1",
 		.probe		= as21xxx_probe,
 		.match_phy_device = as21xxx_match_phy_device,
+		.config_aneg	= as21xxx_config_aneg,
 		.read_status	= as21xxx_read_status,
 		.led_brightness_set = as21xxx_led_brightness_set,
 		.led_hw_is_supported = as21xxx_led_hw_is_supported,
@@ -1090,6 +1156,7 @@ static struct phy_driver as21xxx_drivers[] = {
 		.name		= "Aeonsemi AS21511PB1",
 		.probe		= as21xxx_probe,
 		.match_phy_device = as21xxx_match_phy_device,
+		.config_aneg	= as21xxx_config_aneg,
 		.read_status	= as21xxx_read_status,
 		.led_brightness_set = as21xxx_led_brightness_set,
 		.led_hw_is_supported = as21xxx_led_hw_is_supported,
-- 
2.55.0

[PATCH net v3 4/4] net: phy: as21xxx: handle odd firmware on loading

[Christian Marangi]

Even if firmware for AS21xxx should be always u16 aligned, we can't assume
this will be the same forever. Better address this by simply validate the
index before reading out of bound from the data buffer.

A zero padded buffer will be passed to the AS21xxx mailbox in such case.

Fixes: 830877d89edc ("net: phy: Add support for Aeonsemi AS21xxx PHYs")
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/phy/as21xxx.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/as21xxx.c b/drivers/net/phy/as21xxx.c
index 81e9827873bf..655127a656c4 100644
--- a/drivers/net/phy/as21xxx.c
+++ b/drivers/net/phy/as21xxx.c
@@ -323,9 +323,11 @@ static int aeon_firmware_boot(struct phy_device *phydev, const u8 *data,
 		return -EINVAL;
 	}
 
-	/* Firmware is always aligned to u16 */
+	/* Firmware should be u16 aligned, zero-pad a trailing odd byte */
 	for (i = 0; i < size; i += 2) {
-		val = data[i + 1] << 8 | data[i];
+		val = data[i];
+		if (i + 1 < size)
+			val |= data[i + 1] << 8;
 
 		ret = phy_write_mmd(phydev, MDIO_MMD_VEND1,
 				    VEND1_GLB_REG_MDIO_INDIRECT_LOAD, val);
-- 
2.55.0

Re: [PATCH net v3 1/4] net: phy: as21xxx: handle stale link status during autoneg

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message for "net: phy: as21xxx: handle stale link
  status during autoneg" says "MDIO_STAT1_ANEGCOMPLETE may be cleared…

--

net: phy: as21xxx: handle stale link status during autoneg

The patch makes as21xxx_read_link() record whether autonegotiation has
completed. If autoneg is enabled but not yet complete, it forces
phydev->link down, so a stale link up bit cannot be reported with an
unknown speed.

> When autonegotiation is restarted, MDIO_STAT1_ANEGCOMPLETE may be
> cleared before MDIO_STAT1_LSTATUS. In this case, the link status bit
> may still indicate link up even though autonegotiation has not
> completed.

[Severity: Low]
This isn't a bug, but MDIO_STAT1_ANEGCOMPLETE doesn't seem to exist
anywhere in the tree.

The code in as21xxx_read_link() tests a different macro:

    phydev->autoneg_complete = !!(status & MDIO_AN_STAT1_COMPLETE);

That macro is defined in include/uapi/linux/mdio.h:

    #define MDIO_AN_STAT1_COMPLETE		BMSR_ANEGCOMPLETE

Could the commit message say MDIO_AN_STAT1_COMPLETE instead, so it matches
the code and the name can be found with grep?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261028073024.25175-1-ansuelsmth%40gmail.com

Re: [PATCH net v3 2/4] net: phy: as21xxx: fix read_status speed handling

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 6 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 5 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] With AUTONEG_DISABLE, as21xxx_read_status() no longer takes
  speed/duplex from the PHY.
- [Medium] In autoneg mode, as21xxx_read_status() now reads
  VEND1_SPEED_STATUS on every link-up, which is the PHY's own report of…
- [Medium] The new early `if (!phydev->link) return 0;` in
  as21xxx_read_status() (drivers/net/phy/as21xxx.c:732-733) makes the…
- [Medium] In forced mode (AUTONEG_DISABLE), phydev->speed/duplex hold
  the user's configuration.
- [Medium] net: phy: as21xxx: missed pause and <=1G speed in
  as21xxx_read_status()
- [Low] VEND1_SPEED_STATUS is now read on every autoneg link-up as well
  as in forced mode.

--

net: phy: as21xxx: fix read_status speed handling

This patch reworks as21xxx_read_status() to use the VEND1_SPEED_STATUS
vendor register for speeds above 1G. Speeds of 1G and below come from
phy_resolve_aneg_linkmode() in autoneg mode and from
genphy_read_status_fixed() in forced mode. It also changes the switch mask
to VEND1_SPEED_MASK, reads MII_LPA from the mapped C22 space, and clears
autoneg_complete when AN is restarted.

> diff --git a/drivers/net/phy/as21xxx.c b/drivers/net/phy/as21xxx.c
> index 0db82da8dbdf3..f5968eec11c85 100644
> --- a/drivers/net/phy/as21xxx.c
> +++ b/drivers/net/phy/as21xxx.c

[ ... ]

> @@ -719,57 +729,63 @@ static int as21xxx_read_status(struct phy_device *phydev)
>  	phydev->pause = 0;
>  	phydev->asym_pause = 0;
>  
> +	if (!phydev->link)
> +		return 0;
> +

[Severity: Medium]
With AUTONEG_DISABLE, phydev->speed and phydev->duplex hold the forced
configuration. Just above this, as21xxx_read_status() does:

	phydev->speed = SPEED_UNKNOWN;
	phydev->duplex = DUPLEX_UNKNOWN;

The "why bother the PHY" shortcut only applies to AUTONEG_ENABLE.

In forced mode with the link down, does this new early return mean nothing
puts those values back? genphy_read_status() re-reads the fixed settings
whatever the link state is.

On the next _phy_start_aneg(), for example after ip link set down/up or
resume, phy_sanitize_settings() calls phy_caps_lookup() with
SPEED_UNKNOWN/DUPLEX_UNKNOWN. Nothing matches, so it falls back to:

	if (!match && !exact)
		match = last;

That is the slowest supported mode, which can be 10/half.

Would the PHY then be reprogrammed at that rate instead of the speed the
user forced?

The later patch in this series, "net: phy: as21xxx: force C45 OPs for
AUTONEG", makes as21xxx_config_aneg() call genphy_c45_pma_setup_forced().
That function returns -EINVAL for anything other than DUPLEX_FULL. With
that patch, could the PHY end up in PHY_ERROR on every restart until it is
reconfigured with ethtool?

>  	if (phydev->autoneg == AUTONEG_ENABLE) {
> -		ret = genphy_c45_read_lpa(phydev);
> -		if (ret)
> -			return ret;
> +		if (!phydev->autoneg_complete) {
> +			mii_stat1000_mod_linkmode_lpa_t(phydev->lp_advertising,
> +							0);
> +			mii_lpa_mod_linkmode_lpa_t(phydev->lp_advertising, 0);
> +			mii_10gbt_stat_mod_linkmode_lpa_t(phydev->lp_advertising, 0);
> +			return 0;
> +		}

[Severity: Medium]
Is this block reachable? as21xxx_read_link() sets link to 0 on
BMCR_ANRESTART. It also does:

	if (phydev->autoneg == AUTONEG_ENABLE && !phydev->autoneg_complete)
		phydev->link = 0;

After the new !phydev->link early return, autoneg_complete is therefore
always 1 at this point. For the same reason, the autoneg_complete test in
the phy_resolve_aneg_linkmode() condition below is redundant.

Does this mean lp_advertising is no longer cleared when the link drops or
AN restarts? Before this patch, genphy_c45_read_lpa() cleared the partner
bits when AN was not complete, and the forced branch always ran
linkmode_zero().

phylib zeroes lp_advertising only in PHY_HALTED, and
phy_ethtool_ksettings_get() copies it out unconditionally. Would ethtool
then keep showing the old link partner's advertised modes after the cable
is unplugged?

[ ... ]

> +	if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) {
> +		phy_resolve_aneg_linkmode(phydev);
> +	} else if (phydev->autoneg == AUTONEG_DISABLE) {
> +		ret = genphy_read_status_fixed(phydev);
> +		if (ret < 0)
> +			return ret;
> +	}

[Severity: Medium]
Before this patch, genphy_c45_read_lpa() set phydev->pause and
phydev->asym_pause directly from LPA_PAUSE_CAP and LPA_PAUSE_ASYM.
With that call gone, pause is only resolved by phy_resolve_aneg_pause()
inside phy_resolve_aneg_linkmode(). That helper only updates pause
when phydev->duplex is already DUPLEX_FULL at the time it runs.

phy_resolve_aneg_linkmode() only sets duplex when
phy_caps_lookup_by_linkmode() finds a match in lp_advertising &
advertising. The commit message suggests the >1G partner bits are not
usable on this PHY. If that is so, and there is no common <=1G
full-duplex mode (for example when only 2.5G/5G/10G is advertised),
duplex is still DUPLEX_UNKNOWN here.

The VEND1_SPEED_STATUS switch below then sets DUPLEX_FULL for the
2500/5000/10000 codes. Nothing resolves pause again after that.

Wouldn't that leave pause and asym_pause at 0 on a full-duplex >1G
link even when the partner advertised pause? The MAC would then come
up without flow control.

The same ordering causes the downshift problem with the 1000/100/10
cases below. If the mode resolved here is above the rate VEND1
reports, phydev->speed keeps the value taken from the advertisement
rather than the actual link rate.

Could speed and duplex be taken from VEND1 for every code first, with
phy_resolve_aneg_pause() called after the switch?

[Severity: Medium]
Can genphy_read_status_fixed() be trusted on this PHY? It does a plain C22
read:

	int bmcr = phy_read(phydev, MII_BMCR);

It then takes speed from BMCR_SPEED1000/BMCR_SPEED100 and duplex from
BMCR_FULLDPLX. as21xxx_read_link() deliberately avoids that register:

	/* Normal C22 BMCR report inconsistent data, use
	 * the mapped C22 in C45 to have more consistent link info.
	 */

The mapped bmcr value it returns is no longer used anywhere in
as21xxx_read_status(). The VEND1_SPEED_1000/100/10 cases below only
break, so for forced 10/100/1000 the reported values come from the native
BMCR.

With the later patch in this series, "net: phy: as21xxx: force C45 OPs for
AUTONEG", forced mode is set up only through genphy_c45_pma_setup_forced()
(PMA/PMD CTRL1/CTRL2, full duplex only). The native BMCR is then never
written.

Could this report DUPLEX_HALF, or a speed that differs from what VEND1
reports?

There is also the case of an MDIO bus with no C22 read callback. There,
__mdiobus_read() returns -EOPNOTSUPP, which would now leave read_status
and put the PHY in PHY_ERROR. The old forced path made no C22 access.

[ ... ]

> +	speed = phy_read_mmd(phydev, MDIO_MMD_VEND1,
> +			     VEND1_SPEED_STATUS);
> +	if (speed < 0)
> +		return speed;
> +
> +	switch (speed & VEND1_SPEED_MASK) {
> +	case VEND1_SPEED_10000:
> +		phydev->speed = SPEED_10000;
> +		phydev->duplex = DUPLEX_FULL;
> +		break;

[ ... ]

> +	case VEND1_SPEED_1000:
> +	case VEND1_SPEED_100:
> +	case VEND1_SPEED_10:
> +		break;

[Severity: Medium]
In autoneg mode the hardware-reported rate is now read on every link-up.
For 1000/100/10, though, it is discarded, and phydev->speed/duplex keep
whatever phy_resolve_aneg_linkmode() set: the highest mode both sides
advertise.

The commit message says phy_resolve_aneg_linkmode() is "not enough" for
this PHY, but it stays the only source at 1G and below. Two cases look
like problems.

Suppose lp_advertising contains a mode above 1G (MDIO_AN_10GBT_STAT is
now read), but the link trains at 1G or 100M, for example after a firmware
downshift. Would read_status report 2.5G while VEND1 says 1G?

If the common advertisement has no match, would the link be reported up
with SPEED_UNKNOWN/DUPLEX_UNKNOWN even though VEND1 holds a valid code?

Would it make sense to set speed and duplex from VEND1 for all six codes?

> +	default:
> +		return -EINVAL;
>  	}

[Severity: Low]
Before this patch, VEND1_SPEED_STATUS was read only in forced mode. It is
now read on every autoneg link-up as well. Any value outside the six known
codes lands here. _phy_state_machine() turns the -EINVAL into
phy_error_precise(), which leaves the PHY in PHY_ERROR.

Can this firmware-maintained register hold a transient or unlisted value
at link-up? If so, would it be safer to keep the resolved speed than to
fail read_status?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261028073024.25175-1-ansuelsmth%40gmail.com

Re: [PATCH net v3 3/4] net: phy: as21xxx: force C45 OPs for AUTONEG

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 3 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] as21xxx_config_aneg() (drivers/net/phy/as21xxx.c) never
  applies the requested master/slave role, and it clears any existing…
- [Medium] The commit message says the fix is only about configuring and
  restarting ANEG over C45.
- [Medium] net: phy: as21xxx: C45 forced setup in as21xxx_config_aneg()
  mismatches C22 read

--

net: phy: as21xxx: force C45 OPs for AUTONEG

This commit clears the C22 bit from devices_in_package in as21xxx_probe()
so the generic PHY helpers use C45 accesses. It also adds a custom
as21xxx_config_aneg() that configures and restarts autonegotiation through
the C45 MMD registers, and sets it as .config_aneg for all AS21xxx variants.

> diff --git a/drivers/net/phy/as21xxx.c b/drivers/net/phy/as21xxx.c
> index f5968eec11c85..81e9827873bf2 100644
> --- a/drivers/net/phy/as21xxx.c
> +++ b/drivers/net/phy/as21xxx.c

[ ... ]

> @@ -633,6 +640,56 @@ static int as21xxx_probe(struct phy_device *phydev)
>  	return aeon_dpc_ra_enable(phydev);
>  }
>  
> +static int as21xxx_config_aneg(struct phy_device *phydev)
> +{
> +	bool changed = false;
> +	u16 ctl = 0;
> +	int ret;
> +
> +	if (phydev->autoneg == AUTONEG_DISABLE)
> +		return genphy_c45_pma_setup_forced(phydev);

[Severity: Medium]
With autoneg off, this now programs the forced speed through the C45
PMA/PMD registers (MDIO_CTRL1 and MDIO_CTRL2 in MDIO_MMD_PMAPMD) and
turns AN off in MDIO_MMD_AN. Nothing in the driver writes the C22
MII_BMCR any more. Doesn't the read side still assume the C22 setup?

as21xxx_read_status() handles AUTONEG_DISABLE by calling
genphy_read_status_fixed(). That does a plain phy_read() of MII_BMCR
and derives phydev->speed and phydev->duplex from BMCR_SPEED1000,
BMCR_SPEED100 and BMCR_FULLDPLX. The VEND1_SPEED_STATUS switch after
it only overrides the 2.5G/5G/10G cases. For 10/100/1000 the reported
speed and duplex come straight from that BMCR read.

Since config_aneg no longer writes BMCR, those bits keep whatever value
was left there before, unless the PHY mirrors the PMA control into the
C22 BMCR. as21xxx_read_link() already says the normal C22 BMCR reports
inconsistent data on this PHY, which is why it reads the mapped copy at
AS21XXX_MDIO_AN_C22 + MII_BMCR. After something like
"ethtool -s <dev> speed 100 duplex full autoneg off", wouldn't
userspace see a stale speed or duplex that doesn't match what was
programmed?

On an MDIO bus that only implements C45 accesses, phy_read() of
MII_BMCR returns -EOPNOTSUPP. as21xxx_read_status() would then fail for
every forced speed, even though the forced configuration itself was
done entirely over C45.

Should the AUTONEG_DISABLE branch in as21xxx_read_status() use
genphy_c45_read_pma() instead, so the read path matches what
as21xxx_config_aneg() now programs? Or does the hardware keep the C22
BMCR in sync with the PMA control registers?

[Severity: Medium]
The commit message only talks about using C45 to configure and restart
ANEG. Does this also change what happens when autoneg is disabled?

Before this patch, phy_config_aneg()->genphy_config_aneg() handled forced
mode in __genphy_config_aneg():

drivers/net/phy/phy_device.c:__genphy_config_aneg() {
    ...
	} else if (phydev->speed < SPEED_1000) {
		return genphy_setup_forced(phydev);
	} else {
		linkmode_zero(fixed_advert);

		c = phy_caps_lookup(phydev->speed, phydev->duplex,
				    phydev->supported, true);
    ...
}

So a forced speed of 1000 or more was emulated through autonegotiation
with a single advertised mode. Forced 10/100 half duplex was accepted by
genphy_setup_forced().

A forced 1000 Mb/s link now writes the PMA speed/type and then turns AN
off through genphy_c45_an_disable_aneg(). 1000BASE-T needs AN to resolve
master/slave. Could something like "ethtool -s <dev> speed 1000 duplex
full autoneg off" stop getting a link, when it worked before?

genphy_c45_pma_setup_forced() also rejects half duplex:

drivers/net/phy/phy-c45.c:genphy_c45_pma_setup_forced() {
	/* Half duplex is not supported */
	if (phydev->duplex != DUPLEX_FULL)
		return -EINVAL;
    ...
}

phy_ethtool_ksettings_set() accepts DUPLEX_HALF with autoneg off, and the
PHY lists 10/100 half in phydev->supported. For a request like "speed 100
duplex half autoneg off", the error would propagate like this:

phy_state_machine()
  _phy_start_aneg()
    phy_config_aneg()
      as21xxx_config_aneg()
        genphy_c45_pma_setup_forced()   /* returns -EINVAL */
  phy_error_precise()
    WARN(1, ...)
    phy_process_error()

Would this trigger a WARN and leave the PHY in PHY_ERROR until the
interface is restarted?

> +
> +	ret = genphy_c45_an_config_aneg(phydev);
> +	if (ret < 0)
> +		return ret;
> +	if (ret > 0)
> +		changed = true;
> +
> +	switch (phydev->master_slave_set) {
> +	case MASTER_SLAVE_CFG_MASTER_PREFERRED:
> +		ctl |= CTL1000_PREFER_MASTER;
> +		break;
> +	case MASTER_SLAVE_CFG_SLAVE_PREFERRED:
> +		break;
> +	case MASTER_SLAVE_CFG_MASTER_FORCE:
> +		ctl |= CTL1000_AS_MASTER;
> +		fallthrough;
> +	case MASTER_SLAVE_CFG_SLAVE_FORCE:
> +		ctl |= CTL1000_ENABLE_MASTER;
> +		break;
> +	case MASTER_SLAVE_CFG_UNKNOWN:
> +	case MASTER_SLAVE_CFG_UNSUPPORTED:
> +		break;
> +	default:
> +		phydev_warn(phydev, "Unsupported Master/Slave mode\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	ctl = linkmode_adv_to_mii_ctrl1000_t(phydev->advertising);

[Severity: Medium]
Doesn't this assignment throw away the master/slave bits the switch above
just set in ctl? Was |= intended here?

linkmode_adv_to_mii_ctrl1000_t() only returns ADVERTISE_1000HALF and
ADVERTISE_1000FULL. The phy_modify_mmd_changed() below has
CTL1000_ENABLE_MASTER, CTL1000_AS_MASTER and CTL1000_PREFER_MASTER in its
mask. That means the role bits in the mapped CTRL1000 register always
get written as 0.

A role can be requested through the DT timing-role property:
of_set_phy_timing_role() runs from phy_probe() and sets
phydev->master_slave_set, for example to MASTER_SLAVE_CFG_MASTER_FORCE.
With this code, the requested role would be ignored and cleared on every
config_aneg.

There is a second problem that remains even with |=.
MASTER_SLAVE_CFG_UNKNOWN and MASTER_SLAVE_CFG_UNSUPPORTED also break into
the same masked write. The generic code being replaced returns early in
those cases:

drivers/net/phy/phy_device.c:genphy_setup_master_slave() {
    ...
	case MASTER_SLAVE_CFG_UNKNOWN:
	case MASTER_SLAVE_CFG_UNSUPPORTED:
		return 0;
    ...
}

Could this wipe a role set by straps or firmware every time autoneg is
configured?

As a side effect, as21xxx_read_c22_lpa() can no longer reach this branch,
since it reads CTL1000_ENABLE_MASTER back from the same register:

	if (adv & CTL1000_ENABLE_MASTER)
		phydev_err(phydev, "Master/Slave resolution failed, maybe conflicting manual settings?\n");

The commit message doesn't mention any master/slave change. Is this
behaviour intended?

> +
> +	ret = phy_modify_mmd_changed(phydev, MDIO_MMD_AN,
> +				     AS21XXX_MDIO_AN_C22 + MII_CTRL1000,
> +				     ADVERTISE_1000FULL | ADVERTISE_1000HALF |
> +				     CTL1000_ENABLE_MASTER | CTL1000_AS_MASTER |
> +				     CTL1000_PREFER_MASTER, ctl);
> +	if (ret < 0)
> +		return ret;
> +	if (ret > 0)
> +		changed = true;
> +
> +	return genphy_c45_check_and_restart_aneg(phydev, changed);
> +}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261028073024.25175-1-ansuelsmth%40gmail.com