Re: [PATCH net-next v7 13/15] net: phy: ncn26000: Support for loopback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] `ncn26000_config_aneg` uses `phy_write` to set `MII_BMCR`, inadvertently clearing `BMCR_LOOPBACK` and breaking the newly added loopback functionality.
--

--- Patch [13]: [PATCH net-next v7 13/15] net: phy: ncn26000: Support for loopback ---
commit 13f8b3ef4991385d79e8674e52b9e5000a330738
Author: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>

net: phy: ncn26000: Support for loopback

Adding loopback support for S2500 internal PHY to help running
loopback test through ethtool. It introduces a new API implementation
that manipulates only the loopback bit in the PHY.

> 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 functionality get silently disabled by
ncn26000_config_aneg() when the link is re-initialized?

If loopback is enabled via ethtool and an operation triggers the PHY state
machine to re-initialize the link (such as an interface down and up):

phy_start() -> _phy_start_aneg() -> ncn26000_config_aneg()

This call chain leads to MII_BMCR being overwritten:

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

Using phy_write() here clears the BMCR_LOOPBACK bit that was configured by
genphy_loopback_fixed_speed(), leading to a state mismatch where
phydev->loopback_enabled is true but the PHY loopback bit is actually
cleared.

Should ncn26000_config_aneg() be updated to use phy_modify() to preserve
the loopback state?

>  		.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/20260708-s2500-mac-phy-support-v7-0-478c877aa1a9@onsemi.com?part=13