Re: [PATCH net-next v27 3/3] net: phy: Add driver for Motorcomm Quad 2.5GbE phy

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] net: phy: motorcomm: yt8824_read_status_paged() clears forced speed and duplex
- [Medium] net: phy: motorcomm: stale 2.5G lp_advertising in yt8824_read_status_paged()
- [Medium] net: phy: motorcomm: BMCR_PDOWN not cleared in yt8824_config_utp_init_paged()
--

commit 5e6afad50de750fec66b609e0dfb0e9ed5ab8ac1
Author: Kyle Switch <kyle.switch@motor-comm.com>

net: phy: Add driver for Motorcomm Quad 2.5GbE phy

This commit adds support for the Motorcomm YT8824 quad-port 2.5G PHY to the
motorcomm PHY driver, using phy_package helpers to access the shared top
extended register space.

> diff --git a/drivers/net/phy/motorcomm.c b/drivers/net/phy/motorcomm.c
> index 463e1ea3efc93..6628ebcca0819 100644
> --- a/drivers/net/phy/motorcomm.c
> +++ b/drivers/net/phy/motorcomm.c
[ ... ]
> @@ -638,15 +949,1165 @@ static int ytphy_set_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
[ ... ]
> +static int yt8824_config_utp_init_paged(struct phy_device *phydev)
> +{
> +	int ctrl = 0;
> +	int mask = 0;
> +	int ret = 0;
> +	int val = 0;
> +	int r;
> +
> +	ret = phy8824_page_write_with_lock(phydev, YT8824_RSSR_UTP_SPACE);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = phy_modify(phydev, MII_BMCR, BMCR_PDOWN, BMCR_PDOWN);
> +	if (ret < 0)
> +		goto err_restore;

[ ... ]

> +	/* reset */
> +	ret = genphy_c45_template_testmode(phydev, MDIO_PMA_10GBT_TESTMODE_1);
> +	if (ret < 0)
> +		goto err_restore_normal;
> +
> +	ret = phy_modify(phydev, MII_BMCR, BMCR_RESET | BMCR_ANENABLE,
> +			 BMCR_RESET | BMCR_ANENABLE);
> +	if (ret < 0)
> +		goto err_restore_normal;
> +	ret = phy_read_poll_timeout(phydev, MII_BMCR, val, !(val & BMCR_RESET),
> +				    50000, 600000, true);
> +	if (ret < 0)
> +		goto err_restore_normal;
> +
> +	ret = genphy_c45_template_testmode(phydev,
> +					   MDIO_PMA_10GBT_TESTMODE_NORMAL);
> +	if (ret < 0)
> +		goto err_restore_normal;
> +	return 0;
> +
> +err_restore:
> +	r = phy_modify(phydev, MII_BMCR, BMCR_PDOWN, 0);
> +	if (ret >= 0 && r < 0)
> +		ret = r;
> +	return ret;
> +
> +err_restore_normal:
> +	r = genphy_c45_template_testmode(phydev,
> +					 MDIO_PMA_10GBT_TESTMODE_NORMAL);
> +	if (ret >= 0 && r < 0)
> +		ret = r;
> +	r = phy_modify(phydev, MII_BMCR, BMCR_PDOWN, 0);
> +	if (ret >= 0 && r < 0)
> +		ret = r;
> +	return ret;
> +}

[Severity: Medium]
Does yt8824_config_utp_init_paged() leave BMCR_PDOWN set in MII_BMCR on the
normal return path?

At the beginning of yt8824_config_utp_init_paged(), BMCR_PDOWN is set via
phy_modify(phydev, MII_BMCR, BMCR_PDOWN, BMCR_PDOWN). Near the end of the
function, the reset is issued via:

	ret = phy_modify(phydev, MII_BMCR, BMCR_RESET | BMCR_ANENABLE,
			 BMCR_RESET | BMCR_ANENABLE);

Because phy_modify() performs a read-modify-write and BMCR_PDOWN is not in
the mask, it writes BMCR_PDOWN | BMCR_RESET | BMCR_ANENABLE back to
MII_BMCR. The subsequent yt8824_soft_reset() -> yt8824_utp_softreset_paged()
call in yt8824_config_init() also uses phy_modify(phydev, MII_BMCR,
BMCR_RESET, BMCR_RESET), which preserves BMCR_PDOWN as well.

Both error paths (err_restore and err_restore_normal) explicitly clear
BMCR_PDOWN, but the success path returns 0 without clearing it. When
phy_init_hw() is called without a subsequent phy_resume() (for example via
phy_mii_ioctl() on SIOCSMIIREG with BMCR_RESET, or by a MAC driver resetting
an active PHY), phydev->suspended is set to 0 while the UTP port remains
powered down.

[ ... ]
> @@ -3240,6 +4701,450 @@ static int yt8821_resume(struct phy_device *phydev)
[ ... ]
> +static int yt8824_read_status_paged(struct phy_device *phydev)
> +{
> +	int ret;
> +	int val;
> +
> +	ret = phy8824_page_write_with_lock(phydev, YT8824_RSSR_UTP_SPACE);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = genphy_read_status(phydev);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) {
> +		ret = genphy_c45_read_lpa(phydev);
> +		if (ret < 0)
> +			return ret;
> +	}

[Severity: Medium]
Can guarding genphy_c45_read_lpa() with phydev->autoneg_complete leave
ETHTOOL_LINK_MODE_2500baseT_Full_BIT stale in phydev->lp_advertising when
the link goes down or autonegotiation restarts?

Inside genphy_read_status(), genphy_read_lpa() only clears Clause 22
link-partner advertisement bits when !phydev->autoneg_complete:

	if (phydev->autoneg == AUTONEG_ENABLE) {
		if (!phydev->autoneg_complete) {
			mii_stat1000_mod_linkmode_lpa_t(phydev->lp_advertising,
							0);
			mii_lpa_mod_linkmode_lpa_t(phydev->lp_advertising, 0);
			return 0;
		}

Meanwhile, genphy_c45_read_lpa() has its own cleanup path when
MDIO_AN_STAT1_COMPLETE is not set:

	if (!(val & MDIO_AN_STAT1_COMPLETE)) {
		linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
				   phydev->lp_advertising);
		mii_10gbt_stat_mod_linkmode_lpa_t(phydev->lp_advertising, 0);
		mii_adv_mod_linkmode_adv_t(phydev->lp_advertising, 0);
		phydev->pause = false;
		phydev->asym_pause = false;

		return 0;
	}

By skipping genphy_c45_read_lpa() when !phydev->autoneg_complete (and not
calling mii_10gbt_stat_mod_linkmode_lpa_t(phydev->lp_advertising, 0)), a
2.5G link that drops or restarts autonegotiation will keep
ETHTOOL_LINK_MODE_2500baseT_Full_BIT set in phydev->lp_advertising.

> +
> +	if (!phydev->link) {
> +		phydev->speed = SPEED_UNKNOWN;
> +		phydev->duplex = DUPLEX_UNKNOWN;
> +		if (phydev->autoneg == AUTONEG_ENABLE)
> +			phy_resolve_aneg_pause(phydev);
> +		return 0;
> +	}

[Severity: Medium]
Won't overwriting phydev->speed and phydev->duplex when !phydev->link break
forced speed modes (phydev->autoneg == AUTONEG_DISABLE)?

Earlier in yt8824_read_status_paged(), genphy_read_status() handles both
AUTONEG_ENABLE and AUTONEG_DISABLE: when autonegotiation is disabled, it
calls genphy_read_status_fixed() to populate phydev->speed and
phydev->duplex from MII_BMCR even while the link is down.

Unconditionally setting phydev->speed = SPEED_UNKNOWN and
phydev->duplex = DUPLEX_UNKNOWN when !phydev->link clobbers the forced
settings whenever the cable is unplugged or the link has not yet come up
after _phy_start_aneg() -> phy_check_link_status(). If the interface is
later cycled while the link is down, _phy_start_aneg() ->
phy_sanitize_settings() -> phy_caps_lookup() will see SPEED_UNKNOWN and
reset the forced link configuration to 10baseT/Half.

Also, in the same !phydev->link block, phy_resolve_aneg_pause() only updates
phydev->pause and phydev->asym_pause when phydev->duplex == DUPLEX_FULL, so
calling it right after setting phydev->duplex = DUPLEX_UNKNOWN has no
effect, and genphy_read_status() has already cleared phydev->speed,
phydev->duplex, phydev->pause, and phydev->asym_pause for the !phydev->link
case. Is this !phydev->link block needed after genphy_read_status()?

> +
> +	ret = phy_read(phydev, YTPHY_SPECIFIC_STATUS_REG);
> +	if (ret < 0)
> +		return ret;
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020021929.3028490-1-kyle.switch@motor-comm.com?part=3