Re: [PATCH net-next v9 07/11] net: phy: ncn26000: Enable enhanced noise immunity

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] ncn26000_c45_plca_set_cfg() (drivers/net/phy/ncn26000.c) changes
  PLCA_EN (through genphy_c45_plca_set_cfg) and ENI (PHYCFG1 BIT(7)) as…

--

net: phy: ncn26000: Enable enhanced noise immunity

This commit wraps genphy_c45_plca_set_cfg() in a new
ncn26000_c45_plca_set_cfg() for the NCN26000. The wrapper sets the
vendor-specific ENI bit in PHYCFG1 when PLCA is enabled and clears it when
PLCA is disabled.

> diff --git a/drivers/net/phy/ncn26000.c b/drivers/net/phy/ncn26000.c
> index cabdd83c614f0..6d433f4e2c45d 100644
> --- a/drivers/net/phy/ncn26000.c
> +++ b/drivers/net/phy/ncn26000.c

[ ... ]

> @@ -100,6 +104,24 @@ static int ncn26000_read_status(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +/* Intercept PLCA enable/disable request to
> + * set the proprietary, ENI mode accordingly
> + */
> +static int ncn26000_c45_plca_set_cfg(struct phy_device *phydev,
> +				     const struct phy_plca_cfg *plca_cfg)
> +{
> +	int ret = genphy_c45_plca_set_cfg(phydev, plca_cfg);
> +	u16 eni_cfg = 0;
> +
> +	if (ret || plca_cfg->enabled < 0)
> +		return ret;

[Severity: Low]
Can PLCA_EN and ENI end up out of sync here when an MDIO access fails?

For a disable request (enabled == 0), genphy_c45_plca_set_cfg() clears
PLCA_EN first and only then does the other writes:

drivers/net/phy/phy-c45.c:genphy_c45_plca_set_cfg() {
    ...
	if (plca_cfg->enabled == 0) {
		ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2,
					 MDIO_OATC14_PLCA_CTRL0,
					 MDIO_OATC14_PLCA_EN);
    ...
}

Suppose a later write in the same request fails (PLCA_CTRL1, TOTMR or
BURST). The early return above then skips the ENI update. The PHY would
run CSMA/CD with the PLCA-only ENI bit still set.

> +
> +	eni_cfg = (plca_cfg->enabled) ? NCN26000_PHYCFG1_ENI : 0;
> +	return phy_modify_mmd(phydev, MDIO_MMD_VEND2,
> +			      NCN26000_REG_PHYCFG1,
> +			      NCN26000_PHYCFG1_ENI_MASK, eni_cfg);

For an enable request (enabled > 0), genphy_c45_plca_set_cfg() sets
PLCA_EN as its last step and returns 0. If this phy_modify_mmd() on
PHYCFG1 then fails, PLCA stays enabled with ENI clear. Userspace still
gets an error.

genphy_c45_plca_get_cfg() does not report ENI, so ethtool cannot show the
mismatch. A later request that leaves out enabled (enabled == -1) will not
fix it either.

genphy_c45_plca_set_cfg() already leaves partial state behind on failure,
and resending the same request would bring both bits back in line. Even so,
would it make sense to roll back PLCA_EN (or ENI) on the error paths so the
two bits stay consistent?

> +}
> +

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009-s2500-mac-phy-support-v9-0-dcefe1d0bf0d%40onsemi.com