Re: [PATCH net-next v21 5/9] 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] 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 [off-list ref]
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.

[ ... ]

+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