Re: [PATCH net-next v23 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:
- [Medium] net: phy: air_an8855: incorrect r50ohm calibration for 127 due to forward search
- [Medium] net: phy: air_an8855: register clobbering in AN8855_PHY_RXADC_REV registers
- [Low] net: phy: air_an8855: an8855_set_downshift ignores specific count values
--

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

net: phy: Add Airoha AN8855 Internal Switch Gigabit PHY

Add support for Airoha AN8855 Internal Switch Gigabit PHY. This is a
simple PHY driver to configure and calibrate the PHY for the AN8855 Switch
with the use of 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
> @@ -0,0 +1,275 @@

[ ... ]

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

[Severity: Medium]
Will this mapping produce the wrong value for 127?

Since dsa_r50ohm_table starts with multiple 127s, the forward loop finds
the first match at index 0. Then the subsequent check sees i < 8 and assigns
94 (table index 25) instead of 127.

Should this use a reverse search (or find the last match) to map 127 to 127
at index 16 (where 16 - 8 = 8)?

> +	else
> +		*dest = dsa_r50ohm_table[i - 8];
> +
> +	return 0;
> +}

[ ... ]

> +static int an8855_set_downshift(struct phy_device *phydev, u8 cnt)
> +{
> +	u16 ds = cnt != DOWNSHIFT_DEV_DISABLE ? AN8855_PHY_EN_DOWN_SHIFT : 0;

[Severity: Low]
Does this code incorrectly ignore unsupported downshift counts?

If a user sets an unsupported downshift retry count via ethtool that isn't
DOWNSHIFT_DEV_DISABLE or DOWNSHIFT_DEV_DEFAULT_COUNT, this simply applies
AN8855_PHY_EN_DOWN_SHIFT and returns 0 (success).

Should it return -EOPNOTSUPP for unknown values to maintain consistency
between requested and actual hardware state?

> +
> +	return phy_modify_paged(phydev, AN8855_PHY_PAGE_EXTENDED_1,
> +				AN8855_PHY_EXT_REG_14, AN8855_PHY_EN_DOWN_SHIFT,
> +				ds);
> +}

[ ... ]

> +static int an8855_config_init(struct phy_device *phydev)
> +{

[ ... ]

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

[Severity: Medium]
Can this clobber the adjacent fields in the RXADC_REV registers?

phy_write_mmd() overwrites the entire 16-bit register. By preparing only
AN8855_PHY_RG_AD_RESERVE0_A and AN8855_PHY_RG_AD_RESERVE0_C, the lower
8 bits (RESERVE0_B and RESERVE0_D) will be unintentionally zeroed out.

Should phy_modify_mmd() be used here instead to preserve undocumented
hardware defaults?

> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}

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