Re: [PATCH net-next v5 11/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:
- [High] The newly added loopback support is broken because the driver's config_aneg implementation blindly overwrites the MII_BMCR register, clearing the loopback bit.
--

commit 5a40363adea3a7ee8be993f04a56cb1d3004a24b
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.

> 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: High]
Does ncn26000_config_aneg() inadvertently clear the loopback bit that is
configured here?

Looking at the driver's implementation of ncn26000_config_aneg():

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

When the interface is brought up, the PHY state machine is asynchronously
scheduled and can transition from PHY_UP by unconditionally calling
ncn26000_config_aneg().

If the user activates the loopback selftest simultaneously, the loopback
bit is set via genphy_loopback_fixed_speed(). However, because
ncn26000_config_aneg() blindly overwrites the MII_BMCR register using
phy_write(), it silently clears the BMCR_LOOPBACK bit.

This causes the hardware loopback to be disabled while the software state
indicates it is active, which can lead to selftest failures and potentially
leak test packets onto the network wire.

Could ncn26000_config_aneg() use phy_modify() instead of phy_write() to
preserve orthogonal bits like BMCR_LOOPBACK?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260614-s2500-mac-phy-support-v5-0-89874b72f725@onsemi.com?part=11