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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] net: phy: ax88796b: missing 10M half-duplex cleanup causing mismatch
- [Medium] net: phy: ax88796b: missing aneg restart when changing only 2.5G advertisement
- [Low] net: phy: asix: Dead code in asix_ax88772D_get_features()
--

commit 86e7d1e2ad82ba6062c7e69eccd21649d9c81f47
Author: Birger Koblitz <mail@birger-koblitz.de>
Subject: ax88796b: Add support for AX88772D, AX88179A and AX88279

This commit adds driver support for 100MBit, 1GBit, and 2.5 GBit PHYs
found in ASIX USB-Ethernet controllers.

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

[ ... ]

> @@ -83,6 +96,107 @@ static int asix_ax88772a_read_status(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +static int asix_ax88279_read_status(struct phy_device *phydev)
> +{

[ ... ]

> +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;
> +	}
> +
> +	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);
> +	if (ret < 0)
> +		return ret;
> +
> +	return genphy_config_aneg(phydev);

[Severity: Medium]
Will this fail to restart auto-negotiation if a user only changes the
2.5G advertisement?

Because phy_modify() returns 0 on success, the changed state of the
AX_ADVERTISE_2500 bit is lost. When the rest of the configuration is delegated
to genphy_config_aneg(), it internally evaluates standard advertisement bits
using a mask that excludes this custom 2.5G bit. If no standard changes are
detected, genphy_config_aneg() returns without restarting hardware
auto-negotiation, and the link partner won't be notified of the new 2.5G
advertisement.

Could we check if the register was actually modified, perhaps by using
phy_modify_changed(), and manually trigger a restart or pass the changed state
to __genphy_config_aneg()?

> +}
> +
> +static int asix_ax88279_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;
> +
> +	linkmode_set_bit_array(phy_basic_ports_array, ARRAY_SIZE(phy_basic_ports_array),
> +			       phydev->supported);
> +
> +	ret = genphy_c45_pma_read_abilities(phydev);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* AX88279 does not support reported 100baseT-half duplex mode */
> +	linkmode_clear_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT, phydev->supported);

[Severity: High]
Does this code also need to clear the 10M half-duplex capability?

The read status function unconditionally forces phydev->duplex to DUPLEX_FULL,
indicating the device only supports full duplex. However,
genphy_c45_pma_read_abilities() populates both 10baseT_Full_BIT and
10baseT_Half_BIT. Since the 10M half-duplex bit isn't cleared here along
with the 100M one, the PHY might still advertise 10M half-duplex capability.

If it negotiates 10M half-duplex with a link partner, could this lead to a
duplex mismatch between the MAC (forced to full) and the link partner?

> +
> +	return 0;
> +}

[ ... ]

> +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;

[Severity: Low]
Is the assignment to mmds_present needed here?

It seems this might be dead code, as genphy_read_abilities() and its helpers
like genphy_c45_read_eee_abilities() do not appear to check the mmds_present
mask to determine capabilities.

> +
> +	ret = genphy_read_abilities(phydev);
> +	if (ret < 0)
> +		return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-ax88179a-v18-0-6fa1d4c40422@birger-koblitz.de?part=13