Re: [PATCH net-next v6 11/15] net: phy: ncn26000: Support for loopback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The config_aneg callback unconditionally overwrites the MII_BMCR register, silently clearing previously set bits such as loopback.
- [Medium] The interrupt handler incorrectly uses a register address macro as a bitmask to check for link status changes.
--

commit 80f0753c539b77ecac87c47e5aa0620f69037283
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. This sets genphy_loopback_fixed_speed
as the set_loopback callback.

> 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,

[Severity: Medium]
This is a pre-existing issue, but does the interrupt handler use the wrong
mask to check for link status changes?

ncn26000_handle_interrupt() checks the interrupt status using the register
address macro (17 / 0x11) instead of the proper bitmask:

drivers/net/phy/ncn26000.c:ncn26000_handle_interrupt() {
	...
	ret = phy_read(phydev, NCN26000_REG_IRQ_STATUS);

	// check only link status changes
	if (ret < 0 || (ret & NCN26000_REG_IRQ_STATUS) == 0)
		return IRQ_NONE;
	...
}

Because NCN26000_REG_IRQ_STATUS is 17 (which has bit 0 set), it coincidentally
matches NCN26000_IRQ_LINKST_BIT (BIT(0)). However, it will also incorrectly
match other interrupts like bit 4 (NCN26000_IRQ_PLCAREC_BIT).

Should this be using NCN26000_IRQ_LINKST_BIT instead?

> +		.set_loopback          = genphy_loopback_fixed_speed,

[Severity: High]
This isn't a bug introduced by this patch, but will configuring loopback
here be silently broken by ncn26000_config_aneg()?

When a user configures loopback via ethtool, it sets BMCR_LOOPBACK in MII_BMCR.
However, if the PHY state machine later triggers a re-configuration,
ncn26000_config_aneg() unconditionally overwrites 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);
}

Will this wipe out the loopback bit and cause the hardware state to be lost
while the software state machine assumes loopback is still 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/20260629-s2500-mac-phy-support-v6-0-18ce79500371@onsemi.com?part=11