Re: [PATCH net-next v4 12/16] net: phy: ncn26000: Support for loopback support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ncn26000_config_aneg overwrites MII_BMCR, clearing loopback state
--

commit 94cf320a690a5a9087c661b90073d1379cc73d25
Author: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>

net: phy: ncn26000: Support for loopback support

Adding loopback support for S2500 internal PHY to help running
loopback test through ethtool.

> diff --git a/drivers/net/phy/ncn26000.c b/drivers/net/phy/ncn26000.c
> index c3a34b2c524d5..afafa81dc22b8 100644
> --- a/drivers/net/phy/ncn26000.c
> +++ b/drivers/net/phy/ncn26000.c
> @@ -178,6 +178,7 @@ static struct phy_driver ncn26000_driver[] = {
>  		.config_aneg           = ncn26000_config_aneg,
>  		.read_status           = ncn26000_read_status,
>  		.handle_interrupt      = ncn26000_handle_interrupt,
> +		.set_loopback          = genphy_loopback_fixed_speed,

[Severity: Medium]
Will this newly added loopback state get silently cleared when the link is
configured?

Looking at ncn26000_config_aneg() which sets up the link, it uses phy_write()
rather than phy_modify() on MII_BMCR:

drivers/net/phy/ncn26000.c:ncn26000_config_aneg() {
	...
	// bring up the link
	return phy_write(phydev, MII_BMCR, NCN26000_BCMR_LINK_CTRL_BIT);
}

Since genphy_loopback_fixed_speed() manages the loopback state by toggling the
BMCR_LOOPBACK bit in MII_BMCR, won't this unconditional write clear the
loopback bit and cause loopback tests to fail if phy_config_aneg() is called
while loopback is active?

>  		.set_plca_cfg          = ncn26000_c45_plca_set_cfg,
>  		.get_plca_cfg          = genphy_c45_plca_get_cfg,
>  		.get_plca_status       = genphy_c45_plca_get_status,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260605-s2500-mac-phy-support-v4-0-de0fbc13c6d8@onsemi.com?part=12