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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] net/phy: ax88279 leaves 10baseT_Half_BIT in supported modes despite being full-duplex only
--

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

ax88796b: Add support for AX88772D, AX88179A and AX88279

The AX88772D, AX88179A and AX88279 are 100MBit, 1GBit and respectively
2.5 GBit PHYs that are found in ASIX USB-Ethernet controllers of the
same name.

> diff --git a/drivers/net/phy/ax88796b.c b/drivers/net/phy/ax88796b.c
> index f20ddf6491499..0df44e0e236ab 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)
> +{

[ ... ]

> +	/* MDIO_AN_10GBT_STAT_LP2_5G is broken, but we can deduce that
> +	 * the link-partner advertised 2500M if remotely AN succceded
> +	 * for link speed > 1000M and we locally have a link speed of
> +	 * 2500M
> +	 */
> +	linkmode_clear_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, phydev->lp_advertising);
> +	val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_10GBT_STAT);
> +	if (val >= 0 && val & MDIO_AN_10GBT_STAT_REMOK) {
> +		linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
> +				 phydev->lp_advertising,
> +				 phydev->speed == SPEED_2500);
> +	}
> +	/* Only supports full duplex */
> +	phydev->duplex = DUPLEX_FULL;

[Severity: Medium]
Since the driver unconditionally forces the MAC to full-duplex here, could
this cause a duplex mismatch if 10M half-duplex is negotiated?

In asix_ax88279_get_features() below, genphy_c45_pma_read_abilities()
evaluates extended abilities and will set ETHTOOL_LINK_MODE_10baseT_Half_BIT
if 10Mbps is supported. If a peer supports only half-duplex (or
auto-negotiation otherwise selects it), the link will come up in half-duplex
on the wire while the driver unconditionally reports DUPLEX_FULL, leading to
late collisions and packet loss.

> +
> +	/* PHY switches interface between 2.5GBit and slower modes */
> +	if (phydev->speed == SPEED_2500)
> +		phydev->interface = PHY_INTERFACE_MODE_2500BASEX;
> +	else
> +		phydev->interface = PHY_INTERFACE_MODE_SGMII;
> +
> +	return 0;
> +}

[ ... ]

> +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: Medium]
Does this code also need to clear ETHTOOL_LINK_MODE_10baseT_Half_BIT?

The driver manually clears 100baseT-Half due to a lack of support, but
appears to leave 10baseT-Half enabled in the supported modes despite the PHY
being full-duplex only.

> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261010-ax88179a-v19-0-7475173cfb56@birger-koblitz.de?part=13