[PATCH net v2 1/4] net: phy: as21xxx: handle corner case with link and autoneg complete

[PATCH net v2 1/4] net: phy: as21xxx: handle corner case with link and autoneg complete

[Christian Marangi]

Add missing case in custom read_link, when autoneg is started, autoneg
complete bit is reset but link is still not up.

Fixes: 830877d89edc ("net: phy: Add support for Aeonsemi AS21xxx PHYs")
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
Changes v2:
- Add Review tag

 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 v2 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 always read the speed from the vendor
register and parse the generic bit only for the pause frame.

Fixes: 830877d89edc ("net: phy: Add support for Aeonsemi AS21xxx PHYs")
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
Changes v2:
- Handle half duplex on 10 and 100mbps
- Handle >1G advertise

 drivers/net/phy/as21xxx.c | 104 +++++++++++++++++++++++---------------
 1 file changed, 64 insertions(+), 40 deletions(-)

diff --git a/drivers/net/phy/as21xxx.c b/drivers/net/phy/as21xxx.c
index 0db82da8dbdf..519b78d3579d 100644
--- a/drivers/net/phy/as21xxx.c
+++ b/drivers/net/phy/as21xxx.c
@@ -671,7 +671,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 +698,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)
@@ -720,58 +729,73 @@ static int as21xxx_read_status(struct phy_device *phydev)
 	phydev->asym_pause = 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;
-
-		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;
+	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:
+		phydev->speed = SPEED_1000;
+		if (bmcr & BMCR_FULLDPLX)
 			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;
+		else
+			phydev->duplex = DUPLEX_HALF;
+		break;
+	case VEND1_SPEED_100:
+		phydev->speed = SPEED_100;
+		if (bmcr & BMCR_FULLDPLX)
 			phydev->duplex = DUPLEX_FULL;
-			break;
-		case VEND1_SPEED_10:
-			phydev->speed = SPEED_10;
+		else
+			phydev->duplex = DUPLEX_HALF;
+		break;
+	case VEND1_SPEED_10:
+		phydev->speed = SPEED_10;
+		if (bmcr & BMCR_FULLDPLX)
 			phydev->duplex = DUPLEX_FULL;
-			break;
-		default:
-			return -EINVAL;
-		}
+		else
+			phydev->duplex = DUPLEX_HALF;
+		break;
+	default:
+		return -EINVAL;
 	}
 
+	if (phydev->autoneg == AUTONEG_ENABLE)
+		phy_resolve_aneg_pause(phydev);
+
 	return 0;
 }
 
-- 
2.55.0

[PATCH net v2 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>
---
Changes v2:
- Implement custom config_aneg

 drivers/net/phy/as21xxx.c | 43 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/drivers/net/phy/as21xxx.c b/drivers/net/phy/as21xxx.c
index 519b78d3579d..e2988cb10c04 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,32 @@ 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;
+	int ret;
+	u16 adv;
+
+	ret = genphy_c45_an_config_aneg(phydev);
+	if (ret < 0)
+		return ret;
+	if (ret > 0)
+		changed = true;
+
+	adv = 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, adv);
+	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;
@@ -990,6 +1023,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,
@@ -1002,6 +1036,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,
@@ -1014,6 +1049,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,
@@ -1026,6 +1062,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,
@@ -1038,6 +1075,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,
@@ -1050,6 +1088,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,
@@ -1062,6 +1101,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,
@@ -1074,6 +1114,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,
@@ -1086,6 +1127,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,
@@ -1098,6 +1140,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 v2 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>
---
Changes v2:
- Add this patch

 drivers/net/phy/as21xxx.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/as21xxx.c b/drivers/net/phy/as21xxx.c
index e2988cb10c04..db1a7cd68cef 100644
--- a/drivers/net/phy/as21xxx.c
+++ b/drivers/net/phy/as21xxx.c
@@ -325,7 +325,9 @@ static int aeon_firmware_boot(struct phy_device *phydev, const u8 *data,
 
 	/* Firmware is always aligned to u16 */
 	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 v2 1/4] net: phy: as21xxx: handle corner case with link and autoneg complete

[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 corner case
  with link and autoneg complete" describes the wrong condition.

--

net: phy: as21xxx: handle corner case with link and autoneg complete

This commit makes as21xxx_read_link() store the autoneg complete bit
from MDIO_STAT1 in phydev->autoneg_complete. When autoneg is enabled
but has not completed, it reports the link as down. This is the same
approach genphy_update_link() takes.

> Add missing case in custom read_link, when autoneg is started, autoneg
> complete bit is reset but link is still not up.

[Severity: Low]
Is this the condition the new code actually handles?

If the link is not up, MDIO_STAT1_LSTATUS is already clear. That means
phydev->link is already 0 before the new check runs:

	phydev->link = !!(status & MDIO_STAT1_LSTATUS);
	phydev->autoneg_complete = !!(status & MDIO_AN_STAT1_COMPLETE);
	...
	if (phydev->autoneg == AUTONEG_ENABLE && !phydev->autoneg_complete)
		phydev->link = 0;

The check only changes anything when MDIO_STAT1_LSTATUS is still set
and MDIO_AN_STAT1_COMPLETE has already been cleared. The in-code
comment, copied from genphy_update_link(), describes it that way:

	/* Consider the case that autoneg was started and "aneg complete"
	 * bit has been reset, but "link up" bit not yet.
	 */

Could the commit message be reworded to match? The case being handled
is that the link up bit has not been reset yet, not that the link is
not up.

It could also help stable backporters to state the user-visible effect.
Without this check, as21xxx_read_status() can move from link down to
link up while genphy_c45_read_lpa() is taking its
!MDIO_AN_STAT1_COMPLETE branch:

	if (!(val & MDIO_AN_STAT1_COMPLETE)) {
		linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
				   phydev->lp_advertising);
		mii_10gbt_stat_mod_linkmode_lpa_t(phydev->lp_advertising, 0);
		mii_adv_mod_linkmode_adv_t(phydev->lp_advertising, 0);
		...
		return 0;
	}

In that case phy_resolve_aneg_linkmode() resolves speed and duplex
after the base page and 10G link partner bits have been cleared. The
link can then be reported up with an unknown or stale speed.

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

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 · 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 enabled, as21xxx_read_status()
  (drivers/net/phy/as21xxx.c) now takes the duplex for 1000/100/10 links…
- [Medium] With autoneg enabled, as21xxx_read_status() now reads and
  decodes VEND1_SPEED_STATUS (MMD VEND1 0x4002) whenever the cached…
- [Low] The commit message does not match several behaviour changes in
  the patch.

--

net: phy: as21xxx: fix read_status speed handling

This commit reworks as21xxx_read_status() so that the speed always comes
from the vendor VEND1_SPEED_STATUS register, including when autoneg is
enabled. It also changes the switch to mask with VEND1_SPEED_MASK. The
generic link partner registers are now used only to resolve pause, through
phy_resolve_aneg_pause().

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

[ ... ]

> Rework the read_status code to always read the speed from the vendor
> register and parse the generic bit only for the pause frame.

[Severity: Low]
Could the commit message explain why phy_resolve_aneg_linkmode() was not
enough?

The removed genphy_c45_read_lpa() already read MDIO_AN_10GBT_STAT and set
the partner's 2.5G/5G/10G bits before resolution. The new code reads the
same register to fill lp_advertising:

    val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_10GBT_STAT);
    ...
    mii_10gbt_stat_mod_linkmode_lpa_t(phydev->lp_advertising, val);

The message doesn't say whether that register is reliable on this PHY. If
it isn't, the partner modes reported to userspace would still be wrong.

The message also leaves out a few behaviour changes:

- In as21xxx_read_c22_lpa(), the base page partner advertisement now
  comes from the mapped C22 MII_LPA instead of C45 MDIO_AN_LPA. The
  partner Autoneg bit therefore comes from LPA_LPACK instead of
  MDIO_AN_STAT1_LPABLE.

- Forced 10/100 used to always report DUPLEX_FULL and now reports the
  BMCR_FULLDPLX bit. Only the v2 changelog below the --- line mentions
  this, and that part is dropped when the patch is applied.

- The default -EINVAL return in the vendor speed switch can now be
  reached with autoneg enabled.

Since the patch carries a Fixes: tag, could the commit message describe
these changes too?

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

[ ... ]

> @@ -720,58 +729,73 @@ static int as21xxx_read_status(struct phy_device *phydev)
>  	phydev->asym_pause = 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;
> +		}

[Severity: Medium]
Can phydev->autoneg_complete be stale when this guard checks it?

as21xxx_read_link() returns early when ANRESTART is set, before it updates
autoneg_complete:

    if (*bmcr & BMCR_ANRESTART) {
        phydev->link = 0;
        return 0;
    }

If the previous poll saw the link up, autoneg_complete is still 1, so
renegotiation skips this guard. One example is
_phy_start_aneg()->phy_config_aneg()->phy_check_link_status() running
before ANRESTART self-clears.

The removed genphy_c45_read_lpa() re-read MDIO_STAT1 on every call. The
code also falls through when AN complete is still set but LSTATUS has
dropped.

In both cases the code goes on to decode VEND1_SPEED_STATUS while the link
is down or renegotiating, which can cause two problems.

First, VEND1_SPEED_10 is encoded as 0, so a zero or unresolved register
value is reported as a 10 Mbps link. If the register lags behind link up,
the "why bother the PHY if nothing can have changed" shortcut keeps that
speed until the link drops.

Second, any value outside the six documented codes hits the default
-EINVAL. phy_state_machine() turns that into phy_error_precise() and
PHY_ERROR. Before this patch, that path could only be reached with autoneg
disabled.

Should this guard use a fresh AN status read, or should the vendor speed
read be skipped when phydev->link is 0? The later patches in the series
don't seem to change this code.

>  
>  		ret = as21xxx_read_c22_lpa(phydev);
>  		if (ret)
>  			return ret;
>  
> -		phy_resolve_aneg_linkmode(phydev);
> -	} else {
> -		int speed;

[ ... ]

> +	switch (speed & VEND1_SPEED_MASK) {

[ ... ]

> +	case VEND1_SPEED_1000:
> +		phydev->speed = SPEED_1000;
> +		if (bmcr & BMCR_FULLDPLX)
>  			phydev->duplex = DUPLEX_FULL;

[ ... ]

> +		else
> +			phydev->duplex = DUPLEX_HALF;
> +		break;

[Severity: Medium]
With autoneg enabled, does BMCR_FULLDPLX report the negotiated duplex for
the 1000/100/10 cases?

Before this patch, the autoneg duplex came from
phy_resolve_aneg_linkmode(), which intersects advertising with
lp_advertising. BMCR bit 8 is a forced mode control bit (IEEE 802.3
22.2.4.1.8). genphy_read_status_fixed() reads it only for
AUTONEG_DISABLE.

Nothing on the autoneg path writes this bit. genphy_restart_aneg() only
sets BMCR_ANENABLE | BMCR_ANRESTART, while genphy_setup_forced() rewrites
the whole BMCR.

For example, if the user forces 100/half and then turns autoneg back on,
BMCR_FULLDPLX stays clear. Every later autonegotiated 10/100/1000 link
would then be reported as half duplex. A 1000/half result also looks
impossible here, because 1000baseT_Half doesn't seem to be advertised.

The reverse also applies: if the bit is left at 1, a link that negotiated
half duplex would be reported as full.

A wrong duplex also disables pause, because phy_resolve_aneg_pause() only
resolves it for full duplex:

    if (phydev->duplex == DUPLEX_FULL) {
        phydev->pause = linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT, ...

Unless the vendor mapped C22 BMCR mirrors the resolved duplex, should the
autoneg case get the duplex from the advertisement intersection, as it did
before? The later patches in the series don't seem to change this either.

[ ... ]

> +	if (phydev->autoneg == AUTONEG_ENABLE)
> +		phy_resolve_aneg_pause(phydev);
> +
>  	return 0;
>  }

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

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 · 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] The new as21xxx_config_aneg() in drivers/net/phy/as21xxx.c
  never checks phydev->autoneg, so a forced speed/duplex request is…
- [Medium] Installing the custom as21xxx_config_aneg() also drops
  master/slave (timing role) programming.

--

net: phy: as21xxx: force C45 OPs for AUTONEG

This patch clears the C22 bit from devices_in_package in as21xxx_probe().
It adds a custom as21xxx_config_aneg() that uses the generic C45 AN helpers
and also writes the 1000BASE-T advertisement through the mapped C22
CTRL1000 register in MMD 7. The new callback is hooked into all of the
post-firmware driver entries.

> diff --git a/drivers/net/phy/as21xxx.c b/drivers/net/phy/as21xxx.c
> index 519b78d3579d3..e2988cb10c047 100644
> --- a/drivers/net/phy/as21xxx.c
> +++ b/drivers/net/phy/as21xxx.c

[ ... ]

> @@ -633,6 +640,32 @@ 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;
> +	int ret;
> +	u16 adv;
> +
> +	ret = genphy_c45_an_config_aneg(phydev);

[Severity: Medium]
What happens here when phydev->autoneg == AUTONEG_DISABLE? There is no
forced-mode branch, so a request like "ethtool -s ethX autoneg off speed 100
duplex full" goes through:

phy_ethtool_ksettings_set()
  _phy_start_aneg()
    phy_config_aneg()
      as21xxx_config_aneg()
        genphy_c45_an_config_aneg()
        genphy_c45_check_and_restart_aneg()
          genphy_c45_restart_aneg()

genphy_c45_restart_aneg() sets MDIO_AN_CTRL1_ENABLE | MDIO_AN_CTRL1_RESTART.
So autoneg stays enabled, or gets turned back on, and no forced speed or
duplex is programmed.

Before this patch there was no .config_aneg, so phy_config_aneg() used
genphy_config_aneg()->__genphy_config_aneg(). That path handled forced mode
with genphy_setup_forced() below 1000, and for 1000 it advertised only the
requested mode. genphy_c45_config_aneg(), which this code seems to be
based on, also starts with:

	if (phydev->autoneg == AUTONEG_DISABLE)
		return genphy_c45_pma_setup_forced(phydev);

The config_aneg kernel-doc in include/linux/phy.h says the callback "forces
the speed to the current settings in phydev if phydev->autoneg is off".
as21xxx_read_status() also still has an AUTONEG_DISABLE branch.

Is this a regression for forced link modes? Against a partner forced to
10/100 full duplex, this side would fall back to parallel detection and end
up at half duplex. A forced 1000 request would also no longer restrict the
advertisement, so the link could come up at 2.5G or higher.

Would it make sense to add an AUTONEG_DISABLE branch at the top of
as21xxx_config_aneg(), for example returning
genphy_c45_pma_setup_forced(phydev)?

> +	if (ret < 0)
> +		return ret;
> +	if (ret > 0)
> +		changed = true;
> +
> +	adv = 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, adv);

[Severity: Medium]
Does this drop master/slave (timing role) programming? The mask only covers
ADVERTISE_1000FULL | ADVERTISE_1000HALF. genphy_c45_an_config_aneg() only
writes the advertisement registers, so nothing in this callback uses
phydev->master_slave_set.

Before this patch, __genphy_config_aneg() called genphy_setup_master_slave().
That function does:

	return phy_modify_changed(phydev, MII_CTRL1000,
				  (CTL1000_ENABLE_MASTER | CTL1000_AS_MASTER |
				   CTL1000_PREFER_MASTER), ctl);

Its result was also counted toward the AN restart decision.
genphy_c45_config_aneg() does the same through
genphy_c45_an_setup_master_slave().

The ethtool paths can't reach this for this PHY, because master_slave_get is
never set and stays MASTER_SLAVE_CFG_UNSUPPORTED. A DT "timing-role"
property still sets master_slave_set through
phy_probe()->of_set_phy_timing_role().

With this patch that role is no longer written to the PHY. A role change on
its own also no longer causes an AN restart. If the link partner is forced
to the same role, master/slave resolution would fail and the link would not
come up.

as21xxx_read_c22_lpa() still prints "Master/Slave resolution failed, maybe
conflicting manual settings?", which suggests manual roles are meant to work.

Should the CTL1000 role bits be programmed in the mapped register as well,
or genphy_c45_an_setup_master_slave() be called, with the result folded
into changed?

> +	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/20261022222955.698722-1-ansuelsmth%40gmail.com

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

[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] In aeon_firmware_boot() (drivers/net/phy/as21xxx.c), the comment
  directly above the firmware upload loop still reads "/* Firmware is…

--

net: phy: as21xxx: handle odd firmware on loading

This commit changes the firmware upload loop in aeon_firmware_boot() so it
checks the index before reading data[i + 1]. If the firmware image has an
odd size, the last byte is now sent to the PHY mailbox as a zero-padded
16-bit word.

> diff --git a/drivers/net/phy/as21xxx.c b/drivers/net/phy/as21xxx.c
> index e2988cb10c047..db1a7cd68cef1 100644
> --- a/drivers/net/phy/as21xxx.c
> +++ b/drivers/net/phy/as21xxx.c
> @@ -325,7 +325,9 @@ static int aeon_firmware_boot(struct phy_device *phydev, const u8 *data,
>  
>  	/* Firmware is always aligned to u16 */
>  	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;

[Severity: Low]
This isn't a bug, but should the comment above the loop be updated too?

It still says "Firmware is always aligned to u16". The new i + 1 < size
check exists only because size can be odd. The commit message also says
"we can't assume this will be the same forever".

Nothing enforces an even size either. aeon_firmware_load() passes the
size from request_firmware() straight through:

    ret = aeon_firmware_boot(phydev, fw->data, fw->size);

Someone who trusts the comment later could remove the guard as redundant.
That would bring back the out-of-bounds read of data[size] that this
patch fixes.

Maybe the comment could say something like "Firmware should be u16
aligned, zero-pad a trailing odd byte"?

>  
>  		ret = phy_write_mmd(phydev, MDIO_MMD_VEND1,
>  				    VEND1_GLB_REG_MDIO_INDIRECT_LOAD, val);

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