Re: [PATCH net-next v25 05/10] 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:
- [Medium] net: phy: air_an8855: phy_write_mmd clobbers adjacent reserved fields
- [Medium] net: phy: air_an8855: Array linear search underflow causes valid high efuse values to fall back to default
--

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

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
> @@ -0,0 +1,275 @@

[ ... ]

> +static const u8 dsa_r50ohm_table[] = {
> +	127, 127, 127, 127, 127, 127, 127, 127, 127, 127,
> +	127, 127, 127, 127, 127, 127, 127, 126, 122, 117,
> +	112, 109, 104, 101,  97,  94,  90,  88,  84,  80,
> +	78,  74,  72,  68,  66,  64,  61,  58,  56,  53,
> +	51,  48,  47,  44,  42,  40,  38,  36,  34,  32,
> +	31,  28,  27,  24,  24,  22,  20,  18,  16,  16,
> +	14,  12,  11,   9
> +};
> +
> +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: Medium]
Can a valid high efuse value cause an array search underflow here?

Since dsa_r50ohm_table contains 17 occurrences of 127 at the beginning of the
array, a forward linear search for 127 breaks at i = 0.

The subsequent logic if (i < 8) intercepts i = 0 as an error. Consequently, an
input of 127 falls back to index 25 (94), whereas an input of 126 correctly
matches at index 17 and maps to table[17-8] (127). The forward search breaks
the - 8 shift algorithm for saturated values.

> +
> +	return 0;
> +}

[ ... ]

> +static int an8855_config_init(struct phy_device *phydev)
> +{

[ ... ]

> +	/* Apply values to reduce signal noise */
> +	ret = phy_modify_mmd(phydev, MDIO_MMD_VEND1, AN8855_PHY_TX_PAIR_DLY_SEL_GBE,
> +			     AN8855_PHY_CR_DA_TX_PAIR_DELAY_SEL_A_GBE |
> +			     AN8855_PHY_CR_DA_TX_PAIR_DELAY_SEL_C_GBE,
> +			     FIELD_PREP(AN8855_PHY_CR_DA_TX_PAIR_DELAY_SEL_A_GBE, 0x4) |
> +			     FIELD_PREP(AN8855_PHY_CR_DA_TX_PAIR_DELAY_SEL_C_GBE, 0x4));
> +	if (ret)
> +		return ret;
> +	ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, AN8855_PHY_RXADC_CTRL,
> +			       AN8855_PHY_RG_AD_SAMPLE_PHSEL_A |
> +			       AN8855_PHY_RG_AD_SAMPLE_PHSEL_C);
> +	if (ret)
> +		return ret;
> +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, AN8855_PHY_RXADC_REV_0,
> +			    FIELD_PREP(AN8855_PHY_RG_AD_RESERVE0_A, 0x1));

[Severity: Medium]
Does this inadvertently clear the adjacent reserved fields for pair B?

phy_write_mmd() unconditionally overwrites the entire 16-bit register. By
writing FIELD_PREP(AN8855_PHY_RG_AD_RESERVE0_A, 0x1), this inadvertently zeroes
out the adjacent 8-bit field AN8855_PHY_RG_AD_RESERVE0_B (bits 7:0). Should
phy_modify_mmd() be used here instead?

> +	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]
Similarly, does this inadvertently clear the adjacent reserved fields for
pair D (AN8855_PHY_RG_AD_RESERVE0_D)?

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

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