Re: [PATCH net-next v9 07/11] net: phy: ncn26000: Enable enhanced noise immunity
flat view
From: netdev-bot+sashiko@kernel.org
Date: 2026-10-10 19:40:22
Also in:
linux-devicetree, linux-doc, lkml
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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
+ + 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