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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Broken exact-match search causes silent calibration fallback for intermediate eFuse values
- [Medium] API contract violation: an8855_set_downshift silently ignores unsupported requested retry counts
--

--- Patch [5]: [PATCH net-next v20 05/10] net: phy: Add Airoha AN8855 Internal Switch Gigabit PHY ---
commit e5962bf38329639f7d4d409e75314ddeedd25edf
Author: Christian Marangi <[email protected]>

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

[ ... ]

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

[Severity: High]
Will this exact match condition fail for intermediate physical calibration
values from the eFuse?

If the hardware reads a valid intermediate value like 100 or 102 that isn't
explicitly present in the dsa_r50ohm_table, the loop will exhaust the array
without matching. This would cause the search to silently discard the
calibration data and fall back to the default uncalibrated value.

Should this use a closest-match or bound search to safely handle
intermediate ADC/calibration values without dropping them?

> +
> +	if (i < 8 || i >= ARRAY_SIZE(dsa_r50ohm_table))
> +		*dest = dsa_r50ohm_table[25];
> +	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;
> +
> +	return phy_modify_paged(phydev, AN8855_PHY_PAGE_EXTENDED_1,
> +				AN8855_PHY_EXT_REG_14, AN8855_PHY_EN_DOWN_SHIFT,
> +				ds);
> +}

[Severity: Medium]
Does this silently ignore unsupported downshift retry counts?

The function treats the user-provided count only as a boolean toggle. If
userspace configures ETHTOOL_PHY_DOWNSHIFT with a specific retry count
(e.g. via ethtool), the driver unconditionally enables downshift but ignores
the requested count.

If the driver only supports a fixed internal count, should it validate the
user's input and return -EOPNOTSUPP or -EINVAL when the requested count
doesn't match DOWNSHIFT_DEV_DISABLE or DOWNSHIFT_DEV_DEFAULT_COUNT?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5