Re: [PATCH net-next v17 13/13] ax88796b: Add support for AX88772D, AX88179A and AX88279

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] net: phy: ax88796b: missing aneg restart and race in asix_ax88279_config_aneg
- [Medium] net: phy: ax88796b: incomplete clearance of 1000baseT capabilities for AX88772D
--

commit bc582050068fafd1fce54d8108ca8616b6ee048a
Author: Birger Koblitz <mail@birger-koblitz.de>

ax88796b: Add support for AX88772D, AX88179A and AX88279

This commit adds support for the AX88772D, AX88179A and AX88279 PHYs found
in ASIX USB-Ethernet controllers. It introduces vendor-specific configuration
and status reading functions to support 100MBit, 1GBit, and 2.5GBit link modes.

> diff --git a/drivers/net/phy/ax88796b.c b/drivers/net/phy/ax88796b.c
> index f20ddf6491499..a427292759a24 100644
> --- a/drivers/net/phy/ax88796b.c
> +++ b/drivers/net/phy/ax88796b.c

[ ... ]

> +static int asix_ax88279_config_aneg(struct phy_device *phydev)
> +{
> +	bool adv_2500;
> +	int ret;
> +
> +	if (phydev->autoneg == AUTONEG_DISABLE) {
> +		phydev_warn(phydev, "Disabling autoneg is not supported\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	ret = genphy_config_aneg(phydev);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	adv_2500 = linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, phydev->advertising);
> +	ret = phy_modify(phydev, MII_ADVERTISE, AX_ADVERTISE_2500,
> +			 adv_2500 ? AX_ADVERTISE_2500 : 0);
> +
> +	return ret;
> +}

[Severity: High]
Can this ordering cause autonegotiation to either miss a restart or race
with the 2.5G advertisement update?

genphy_config_aneg() evaluates changes and restarts autonegotiation
immediately if required. Modifying MII_ADVERTISE afterward bypasses the
change detection for the 2.5G bit and races with the already-started
autonegotiation.

Would it be better to check for changes to both standard and custom bits,
apply all register modifications, and only then restart autonegotiation?

[ ... ]

> +static int asix_ax88772D_get_features(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	/* MDIO_DEVS1/2 empty, so set mmds_present bits to allow reading abilities */
> +	phydev->c45_ids.mmds_present |= MDIO_DEVS_PMAPMD | MDIO_DEVS_AN;
> +
> +	ret = genphy_read_abilities(phydev);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* AX88772D does not support reported 1000baseT mode */
> +	linkmode_clear_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, phydev->supported);

[Severity: Medium]
Does this completely clear the errantly reported gigabit capabilities?

genphy_read_abilities() reads MII_ESTATUS and can also populate
ETHTOOL_LINK_MODE_1000baseT_Half_BIT and
ETHTOOL_LINK_MODE_1000baseX_Full_BIT if they are present.

Since this is a 100MBit PHY, should all gigabit modes be explicitly cleared
to prevent broken autonegotiation if the remote link partner attempts to
negotiate them?

> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261006-ax88179a-v17-0-252814fe35ea@birger-koblitz.de?part=13