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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Forward loop over `dsa_r50ohm_table` with duplicate values causes a severe calibration discontinuity for an EFUSE value of 127.
- [Medium] `an8855_set_downshift` silently ignores specific downshift counts, treating any non-zero value as a generic 'enable'.
--

commit 836fad6c28da3b4cc1aa1daa6879476d597247ba
Author: Christian Marangi <ansuelsmth@gmail.com>
net: phy: Add Airoha AN8855 Internal Switch Gigabit PHY

This commit adds a new PHY driver for the Airoha AN8855 Internal Switch
Gigabit PHY. It configures and calibrates the PHY for the AN8855 switch
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
> @@ -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];
> +	else
> +		*dest = dsa_r50ohm_table[i - 8];

[Severity: High]
Does this loop unintentionally apply the fallback value instead of the correct
resistance mapping when shift_sel is 127? 

When the EFUSE calibration data read from NVMEM contains a shift_sel value
of 127, the forward for loop finds the first match at index i = 0. The
subsequent bounds check if (i < 8) evaluates to true, overriding the mapped
value with the fallback dsa_r50ohm_table[25] (which is 94).

Additionally, if intermediate values not explicitly defined in the table 
are encountered, the strict equality check fails and i reaches 
ARRAY_SIZE(dsa_r50ohm_table), which also falls back to 94 instead of 
mapping to the closest index.

> +
> +	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 the specific downshift count requested by the user?

If userspace requests a specific downshift count, the driver checks
cnt != DOWNSHIFT_DEV_DISABLE, evaluating to true and setting the hardware
enable bit AN8855_PHY_EN_DOWN_SHIFT. However, it configures the hardware
without actually applying the requested count or returning an error 
(such as -EOPNOTSUPP) for unsupported values. 

Tools like ethtool will report success, but the hardware will silently use
its default count instead.

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