Re: [PATCH net-next v24 05/10] net: phy: Add Airoha AN8855 Internal Switch Gigabit PHY

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] net: phy: an8855_probe() fails if nvmem-cells exists for non-calibration purposes
- [High] net: phy: exact-match lookup in an8855_get_r50ohm_val() breaks calibration
- [Medium] net: phy: an8855_set_downshift() silently ignores unsupported counts
- [Medium] net: phy: phy_write_mmd() in an8855_config_init() clobbers adjacent RXADC_REV lanes
--

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

    net: phy: Add Airoha AN8855 Internal Switch Gigabit PHY

This patch adds support for the Airoha AN8855 Internal Switch Gigabit PHY. It
includes a driver to configure and calibrate the PHY 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];

[Severity: High]
Does this exact-match loop lookup load incorrect calibration values?

Since 127 occurs 17 times at the start of dsa_r50ohm_table, searching for 127
will always break at i = 0. This triggers the i < 8 fallback, applying a value
of 94 instead of the intended 127.

Also, any intermediate EFUSE values missing from the sparse table (e.g., 125,
124) will fail the exact match loop entirely (i = 64) and also fall back to
94, which could cause massive discontinuities.

> +
> +	return 0;
> +}
> +
> +static int an8855_probe(struct phy_device *phydev)
> +{
> +	struct device *dev = &phydev->mdio.dev;
> +	struct air_an8855_priv *priv;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->needs_calibration = of_property_present(dev->of_node,
> +						      "nvmem-cells");
> +
> +	if (priv->needs_calibration) {
> +		int ret;
> +
> +		ret = an8855_get_r50ohm_val(dev, "tx_a", &priv->calibration_data[0]);
> +		if (ret)
> +			return ret;

[Severity: High]
Does this broad property presence check break probing on valid device trees?

If the nvmem-cells property is present for another standard purpose (like
providing a MAC address) but the calibration cell (tx_a) is missing,
nvmem_cell_read_u32() in an8855_get_r50ohm_val() will return -ENOENT.

Because this error is returned directly, the PHY driver completely fails to
probe on devices that legally use nvmem-cells for non-calibration data.

[ ... ]
> +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: Medium]
Should this function return an error such as -EOPNOTSUPP or -EINVAL for
unsupported downshift counts?

The requested cnt is completely ignored unless it is DOWNSHIFT_DEV_DISABLE,
always enabling the default downshift behavior. Userspace configuration will
desynchronize from the actual hardware state, as ethtool will report success
for counts the PHY did not actually apply.

[ ... ]
> +	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]
Will these raw writes clobber the adjacent RXADC_REV lanes?

Using phy_write_mmd() rather than a modify wipes out the RESERVE0_B and
RESERVE0_D fields located in the lower byte of these registers, destroying
any hardware defaults or prior configurations for lanes B and D.

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

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