Re: [PATCH net-next v22 5/9] net: phy: Add Airoha AN8855 Internal Switch Gigabit PHY

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] net/phy: flawed calibration lookup logic corrupts valid efuse values
- [Medium] net/phy: phy_write_mmd zeroing out adjacent fields in RXADC registers
- [Low] net/phy: downshift tunable ignores requested count violating ethtool API
--

commit 86e9fad6319a9402e15a273e59be80e0d645c59c
Author: Christian Marangi <ansuelsmth@gmail.com>

net: phy: Add Airoha AN8855 Internal Switch Gigabit PHY

This commit adds a simple PHY driver to support the Airoha AN8855
Internal Switch Gigabit PHY, including configuration and calibration
using NVMEM cells.

> diff --git a/drivers/net/phy/air_an8855.c b/drivers/net/phy/air_an8855.c
> new file mode 100644
> index 0000000000000..bc22256fd5f95
> --- /dev/null
> +++ b/drivers/net/phy/air_an8855.c
[ ... ]
> +static int an8855_get_r50ohm_val(struct device *dev, const char *calib_name,
> +				 u8 *dest)
> +{
> +	u32 shift_sel, val;
> +	int ret;
> +	int i;
> +
> +	ret = nvmem_cell_read_u32(dev, calib_name, &val);
> +	if (ret)
> +		return ret;
> +
> +	shift_sel = FIELD_GET(AN8855_SWITCH_EFUSE_R50O, val);
> +	for (i = 0; i < ARRAY_SIZE(dsa_r50ohm_table); i++)
> +		if (dsa_r50ohm_table[i] == shift_sel)
> +			break;
> +
> +	if (i < 8 || i >= ARRAY_SIZE(dsa_r50ohm_table))
> +		*dest = dsa_r50ohm_table[25];
> +	else
> +		*dest = dsa_r50ohm_table[i - 8];
> +
> +	return 0;
> +}

[Severity: High]
Does this logic incorrectly map valid top-end EFUSE values to a fallback?

If the hardware EFUSE provides 127 (which is the first entry in
dsa_r50ohm_table), the loop immediately matches at i = 0. The code then
evaluates if (i < 8) as true, forcing dest to a hardcoded fallback of
dsa_r50ohm_table[25] (which is 94). 

However, a slightly lower value like 126 matches at i = 17, bypasses the
check, and maps to dsa_r50ohm_table[9] (which correctly yields 127). Will
this discontinuity erroneously penalize top-end calibration values and
drop them to 94, potentially corrupting PHY TX pair calibration?

[ ... ]
> +static int an8855_set_downshift(struct phy_device *phydev, u8 cnt)
> +{
> +	u16 ds = cnt != DOWNSHIFT_DEV_DISABLE ? AN8855_PHY_EN_DOWN_SHIFT : 0;
> +
> +	return phy_modify_paged(phydev, AN8855_PHY_PAGE_EXTENDED_1,
> +				AN8855_PHY_EXT_REG_14, AN8855_PHY_EN_DOWN_SHIFT,
> +				ds);
> +}

[Severity: Low]
Does this downshift tunable ignore the requested retry count?

The ethtool API for ETHTOOL_PHY_DOWNSHIFT accepts a specific retry count via
the cnt parameter, but here cnt != DOWNSHIFT_DEV_DISABLE is used as a simple
boolean toggle. 

If userspace requests a specific unsupported downshift count, won't the driver
enable the hardware default and return 0 (success) instead of rejecting the
invalid count with -EOPNOTSUPP or -EINVAL?

[ ... ]
> +static int an8855_config_init(struct phy_device *phydev)
> +{
[ ... ]
> +	/* Apply values to reduce signal noise */
> +	ret = phy_modify_mmd(phydev, MDIO_MMD_VEND1, AN8855_PHY_TX_PAIR_DLY_SEL_GBE,
> +			     AN8855_PHY_CR_DA_TX_PAIR_DELAY_SEL_A_GBE |
> +			     AN8855_PHY_CR_DA_TX_PAIR_DELAY_SEL_C_GBE,
> +			     FIELD_PREP(AN8855_PHY_CR_DA_TX_PAIR_DELAY_SEL_A_GBE, 0x4) |
> +			     FIELD_PREP(AN8855_PHY_CR_DA_TX_PAIR_DELAY_SEL_C_GBE, 0x4));
> +	if (ret)
> +		return ret;
> +	ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, AN8855_PHY_RXADC_CTRL,
> +			       AN8855_PHY_RG_AD_SAMPLE_PHSEL_A |
> +			       AN8855_PHY_RG_AD_SAMPLE_PHSEL_C);
> +	if (ret)
> +		return ret;
> +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, AN8855_PHY_RXADC_REV_0,
> +			    FIELD_PREP(AN8855_PHY_RG_AD_RESERVE0_A, 0x1));
> +	if (ret)
> +		return ret;
> +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, AN8855_PHY_RXADC_REV_1,
> +			    FIELD_PREP(AN8855_PHY_RG_AD_RESERVE0_C, 0x1));
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}

[Severity: Medium]
Will phy_write_mmd() inadvertently zero out adjacent fields in these
RXADC registers?

Earlier in this function, phy_modify_mmd() and phy_set_bits_mmd() are used
to carefully preserve the state of pairs B and D. But for the REV_0 and
REV_1 registers, phy_write_mmd() completely overwrites the 16-bit register.

By writing FIELD_PREP(AN8855_PHY_RG_AD_RESERVE0_A, 0x1) directly, won't this
implicitly zero out the adjacent AN8855_PHY_RG_AD_RESERVE0_B (bits 7:0),
breaking the read-modify-write pattern and wiping out hardware defaults for
pairs B and D?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260915123812.14747-1-ansuelsmth@gmail.com?part=5