Re: [PATCH net-next v20 05/10] net: phy: Add Airoha AN8855 Internal Switch Gigabit PHY
From: sashiko-bot@kernel.org
Date: 2026-08-18 09:03:20
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 [off-list ref] 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.
quoted hunk ↗ jump to hunk
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/20260817082034.20326-1-ansuelsmth@gmail.com?part=5