Re: [PATCH net-next v28 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 1 potential issue(s) to consider:
- [High] net: phy: motorcomm: broken sds_initialized tracking in yt8824_resume()
--

commit 4e05c00d9c48d262e045a1a6c3ea0d87e1c1718c
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 driver, using phy_package helpers to manage the shared top
extended register space.

> diff --git a/drivers/net/phy/motorcomm.c b/drivers/net/phy/motorcomm.c
> index 463e1ea3efc93..492e236ec9e31 100644
> --- a/drivers/net/phy/motorcomm.c
> +++ b/drivers/net/phy/motorcomm.c

[ ... ]

> @@ -642,11 +950,1168 @@ static int ytphy_set_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)

[ ... ]

> +static int yt8824_config_init(struct phy_device *phydev)
> +{
> +	struct yt8824_shared_priv *priv = phy_package_get_priv(phydev);
> +	int ret;
> +
> +	mutex_lock(&priv->shared_lock);
> +	if (priv->package_mode == PHY_INTERFACE_MODE_INTERNAL) {
> +		ret = yt8824_config_utp_init_paged(phydev);
> +		if (ret < 0)
> +			goto err;
> +	} else {
> +		if (!priv->sds_initialized) {
> +			ret = yt8824_extern_config_sds_init_paged(phydev);
> +			if (ret < 0)
> +				goto err;
> +			priv->sds_initialized = true;
> +		}
> +		/*
> +		 * Configuring the MII registers of the SDS,
> +		 * which are private to each channel.
> +		 */
> +		ret = yt8824_extern_sds_softreset(phydev);
> +		if (ret < 0)
> +			goto err;
> +
> +		ret = yt8824_config_utp_init_paged(phydev);
> +		if (ret < 0)
> +			goto err;
> +	}
> +	mutex_unlock(&priv->shared_lock);
> +	ret = yt8824_soft_reset(phydev);
> +
> +	phydev_dbg(phydev, "%s done, phy addr: %d\n", __func__,
> +		   phydev->mdio.addr);
> +	return ret;
> +err:
> +	mutex_unlock(&priv->shared_lock);
> +	return ret;
> +}

[ ... ]

> @@ -3240,6 +4705,455 @@ static int yt8821_resume(struct phy_device *phydev)

[ ... ]

> +static int yt8824_resume(struct phy_device *phydev)
> +{
> +	struct yt8824_shared_priv *priv = phy_package_get_priv(phydev);
> +	int ret;
> +
> +	mutex_lock(&priv->shared_lock);
> +	ret = yt8824_power_on(phydev);
> +	if (!ret)
> +		priv->sds_initialized = false;
> +	mutex_unlock(&priv->shared_lock);
> +
> +	return ret;
> +}

[Severity: High]
Does clearing priv->sds_initialized in yt8824_resume() cause the shared
SerDes to be reset and recalibrated repeatedly as each port in the package
is attached or started, while also inverting the initialization order on
system resume?

In struct yt8824_shared_priv, sds_initialized is a package-wide flag meant
to ensure yt8824_config_init() runs yt8824_extern_config_sds_init_paged()
only once for the 4-port package when priv->package_mode is
PHY_INTERFACE_MODE_10G_QXGMII.

However, during normal port attachment in phy_attach_direct(), phylib calls
phy_init_hw() followed immediately by phy_resume():

drivers/net/phy/phy_device.c:phy_attach_direct() {
	...
	err = phy_init_hw(phydev);
	if (err)
		goto error;

	if (phydev->mdio.bus->notify_phy_attach) {
		err = phydev->mdio.bus->notify_phy_attach(phydev);
		if (err)
			goto error;
	}

	phy_resume(phydev);
	...
}

When the first port is attached, phy_init_hw() -> yt8824_config_init() runs
yt8824_extern_config_sds_init_paged() and sets priv->sds_initialized = true,
and then phy_resume() -> yt8824_resume() immediately resets
priv->sds_initialized = false. Bringing an interface up via phy_start() ->
__phy_resume() -> yt8824_resume() also resets priv->sds_initialized = false.

As a result, when each of the remaining three ports in the YT8824 package is
attached, priv->sds_initialized is false again, so yt8824_config_init()
re-runs yt8824_extern_config_sds_init_paged() on each port. That function
asserts the shared SerDes global reset (YT8824_CSR_US_GLB_RESET_N = 0) and
restarts SerDes calibration (YT8824_CALIB_SEQ_SW_RST and
YT8824_RESTART_CAL_CTRL_REG) while sibling ports in the package are already
initialized or active.

Conversely, during system resume in mdio_bus_phy_resume(), phylib calls
phy_init_hw() before phy_resume():

drivers/net/phy/phy_device.c:mdio_bus_phy_resume() {
	...
	ret = phy_init_hw(phydev);
	if (ret < 0)
		return ret;

	ret = phy_resume(phydev);
	...
}

Because yt8824_suspend() does not clear priv->sds_initialized, if
priv->sds_initialized is true prior to suspend, the first port to resume
sees priv->sds_initialized == true in yt8824_config_init() (skipping SerDes
re-initialization) and only clears priv->sds_initialized = false afterward
in yt8824_resume(), causing the second port to re-run package-wide SerDes
initialization after the first port has already completed
yt8824_config_init().

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