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