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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 3 · Low: 4

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message is three lines long for a ~1900-line driver
  addition and leaves out behaviour a maintainer needs to judge it.
- [Medium] priv->sds_initialized is only reset in
  yt8824_phy_package_probe_once() and is set to true after the first…
- [Medium] net: phy: motorcomm: yt8824_read_status_paged() clears forced
  speed and duplex
- [Low] yt8824_probe() calls phy_package_probe_once() before
  yt8824_phy_package_probe_once() has validated the package.
- [Low] In yt8824_config_aneg() the forced-2.5G clause sets
  MDIO_AN_10GBT_CTRL_ADV2_5G only if 2500baseT_Full is also in…
- [Low] yt8824_read_status_paged() calls genphy_c45_read_lpa() only when
  autoneg is enabled and autoneg_complete is set.
- [Low] yt8824_sds_isolate_and_softreset_paged() and
  yt8824_extern_sds_softreset() poll BMCR_RESET with…

--

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

This adds support for the Motorcomm YT8824 quad-port 2.5G PHY to the
existing motorcomm driver. It uses the phy_package helpers to access the
shared top extended register space. Two package modes are supported:
internal and 10g-qxgmii.

> Add support for Motorcomm YT8824 quad-port 2.5G PHY to the existing
> motorcomm driver, using the phy_package helpers for the shared top
> extended register space.

[Severity: Medium]

Could the commit message describe some of the less obvious behaviour
this adds?

yt8824_soft_reset(), yt8824_power_on() and yt8824_power_down() put the
PMA into 10GBASE-T template test mode 1 around every reset or power
transition, and switch it back to normal afterwards. In non-internal
mode they also isolate the SERDES and soft reset it. yt8824_config_init()
reaches the same sequence through yt8824_soft_reset(). The only
explanation in the code is "NOTE: need WA like softreset".

The changelog also doesn't mention the two package modes (internal and
10g-qxgmii). Nor does it mention the one-time package-wide SERDES
calibration, which only runs in 10g-qxgmii mode.

The ethernet-phy-package node must carry the "motorcomm,package-mode"
property, or yt8824_phy_package_probe_once() returns -EINVAL. The
property is documented by "dt-bindings: net: Document Motorcomm YT8824
PHY package" earlier in this series. Could the changelog say that this
property is required?

> diff --git a/drivers/net/phy/motorcomm.c b/drivers/net/phy/motorcomm.c
> index 463e1ea3efc9..6628ebcca081 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_sds_isolate_and_softreset_paged(struct phy_device *phydev)
> +{
> +	int old_page = YT8824_RSSR_UTP_SPACE;
> +	int val = 0;
> +	int ret = -1;
> +
> +	old_page = phy_select_page(phydev, YT8824_RSSR_SERDES_SPACE);
> +	if (old_page < 0)
> +		goto err_restore_page;
> +
> +	/* sds softreset and disable isolate */
> +	ret = __phy_modify(phydev, MII_BMCR, BMCR_RESET | BMCR_ISOLATE,
> +			   BMCR_RESET & ~BMCR_ISOLATE);
> +	if (ret < 0)
> +		goto err_restore_page;
> +
> +	/* poll while still holding the lock */
> +	ret = read_poll_timeout(__phy_read, val,
> +				(val < 0) || !(val & BMCR_RESET), 50000, 600000,
> +				true, phydev, MII_BMCR);

[Severity: Low]

phy_select_page() takes the bus-wide mdio_lock and phy_restore_page()
releases it. sleep_before_read is true, so this poll holds mdio_lock for
at least 50ms and up to 600ms. Is it intended that every other user of
the MDIO bus is blocked for that long? That includes the other YT8824
ports, any other PHYs, and any switch on the bus.

In 10g-qxgmii mode this runs per port from yt8824_soft_reset(), which
phy_init_hw() reaches twice: once as .soft_reset and again through
yt8824_config_init(). It also runs on suspend, on resume and on the
restore path.

yt8824_extern_sds_softreset() has the same poll under the bus lock, and
it runs on every yt8824_config_init().

Could the page be restored and the lock dropped between poll reads
instead?

> +	if (val < 0)
> +		ret = val;
> +
> +err_restore_page:
> +	return phy_restore_page(phydev, old_page, ret);
> +}

[ ... ]

> +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;
> +		}

[Severity: Medium]

priv->sds_initialized is only cleared in yt8824_phy_package_probe_once().
The shared priv lives as long as any port in the package stays bound.
What happens if the package loses its SERDES configuration after the
flag has been set?

One case is a PHY with reset-gpios that is detached and attached again:

phy_detach()
  phy_device_reset(phydev, 1)
phy_attach_direct()
  phy_init_hw()
    phy_device_reset(phydev, 0)
    yt8824_config_init()

Another is a system suspend that removes PHY power, followed by
mdio_bus_phy_resume()->phy_init_hw().

In both cases yt8824_config_init() would skip
yt8824_extern_config_sds_init_paged(). It would only redo the per-channel
SDS soft reset and the UTP init.

Would the SERDES PLL/CDR/AFE/DFE/TX tuning then stay at reset defaults
in 10g-qxgmii mode until the driver is rebound?

[ ... ]

> @@ -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: Low]

Can the 2500baseT_Full bit in phydev->lp_advertising go stale here?

When the link drops or negotiation restarts, autoneg_complete is 0. In
that case genphy_read_status()->genphy_read_lpa() only clears the clause
22 and 1000BASE-T partner bits:

	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;
	}

genphy_c45_read_lpa() is skipped in that case, so its own
mii_10gbt_stat_mod_linkmode_lpa_t(..., 0) clearing never runs.

Would ethtool then keep reporting a 2.5G link partner capability while
no negotiation is complete? yt8821_read_status() has the same pattern,
and it appears to have been copied here.

[ ... ]

> +static int yt8824_config_aneg(struct phy_device *phydev)
> +{

[ ... ]

> +	/*
> +	 * Only advertise 2.5G when autoneg is enabled, or when 2.5G is
> +	 * explicitly forced.  When a different speed is forced, clear
> +	 * ADV2_5G so a 2.5G-capable link partner cannot negotiate 2.5G.
> +	 * __genphy_config_aneg() only rewrites the
> +	 * clause 22 registers on the forced-speed path, so it will not
> +	 * clear this bit.
> +	 */
> +	if ((phydev->autoneg == AUTONEG_ENABLE ||
> +	     phydev->speed == SPEED_2500) &&
> +	    linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
> +			      phydev->advertising))
> +		phy_ctrl = MDIO_AN_10GBT_CTRL_ADV2_5G;

[Severity: Low]

Does this condition match the comment above it? For forced speeds of
1000 and above, __genphy_config_aneg() builds the advertisement from
phydev->supported, not phydev->advertising:

		c = phy_caps_lookup(phydev->speed, phydev->duplex,
				    phydev->supported, true);

Suppose 2.5G is forced after 2500baseT_Full was removed from
phydev->advertising. This clears ADV2_5G, and nothing usable is left in
the advertisement.

This can't be reached today. phy_ethtool_ksettings_set() rejects
AUTONEG_DISABLE with any speed other than 10/100/1000, so the
phydev->speed == SPEED_2500 clause is dead code.

Should that clause be dropped together with the "explicitly forced"
part of the comment? Alternatively, could it stop depending on
phydev->advertising?

[ ... ]

> +static int yt8824_phy_package_probe_once(struct phy_device *phydev)
> +{
> +	struct yt8824_shared_priv *priv = phy_package_get_priv(phydev);
> +	struct device_node *np = phy_package_get_node(phydev);
> +	const char *mode;
> +
> +	if (!priv || !np) {
> +		phydev_err(phydev, "%s: missing shared priv or DT node\n",
> +			   __func__);
> +		return -EINVAL;
> +	}
> +	/* Initialise shared lock for YT8824 */
> +	mutex_init(&priv->shared_lock);
> +	priv->sds_initialized = false;
> +	priv->package_mode = PHY_INTERFACE_MODE_INTERNAL;
> +	if (!of_property_read_string(np, "motorcomm,package-mode",
> +				     &mode)) {

[ ... ]

> +	} else {
> +		phydev_err(phydev, "missing package-mode in PHY package node.\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}

[ ... ]

> +static int yt8824_probe(struct phy_device *phydev)
> +{

[ ... ]

> +	/* Ensure other PHY probes wait for shared mutex initialization. */
> +	phy_package_lock(phydev);
> +	if (phy_package_probe_once(phydev)) {
> +		ret = yt8824_phy_package_probe_once(phydev);
> +		if (ret) {
> +			phy_package_unlock(phydev);
> +			return ret;
> +		}
> +	}
> +	phy_package_unlock(phydev);
> +
> +	return 0;
> +}

[Severity: Low]

What happens to the sibling ports if yt8824_phy_package_probe_once()
fails here?

phy_package_probe_once() does an unconditional test_and_set_bit() on
shared->flags. The INTERNAL default is stored in priv->package_mode
before the property is checked. If "motorcomm,package-mode" is missing
or unsupported, this port fails with -EINVAL, but the failure is not
recorded anywhere.

A sibling PHY in the same package may join before this port's devres
runs phy_package_leave(). Its reference keeps the shared object, and the
once bit, alive. phy_package_probe_once() then returns false for the
sibling, so it skips validation, returns 0, and runs in INTERNAL mode
whatever the DT says.

phylib probes synchronously, so this needs two ports of the package to
probe at the same time. One example is driver_attach() from a module
load racing with mdiobus_register().

Could the validation happen before the once bit is used up? Another
option would be to keep a validated or failed state in the shared priv
for siblings to check.

As a side note, the !priv || !np check can't trigger.
of_phy_package_join() always sets np, and it always allocates priv when
priv_size is non-zero.

[ ... ]

> @@ -3240,6 +4701,450 @@ static int yt8821_resume(struct phy_device *phydev)
>  	return yt8821_modify_utp_fiber_bmcr(phydev, BMCR_PDOWN, 0);
>  }
>  
> +/**
> + * yt8824_get_features - read mmd register to get 2.5G capability
> + * @phydev: target phy_device struct
> + *
> + * Returns: 0 or negative errno code
> + */
> +static int yt8824_get_features(struct phy_device *phydev)
> +{
> +	struct yt8824_shared_priv *priv = phy_package_get_priv(phydev);
> +	int ret;
> +
> +	mutex_lock(&priv->shared_lock);
> +	ret = phy8824_page_write_with_lock(phydev, YT8824_RSSR_UTP_SPACE);
> +	if (ret < 0)
> +		goto err;
> +	ret = yt8821_get_features(phydev);
> +
> +err:
> +	mutex_unlock(&priv->shared_lock);
> +	return ret;
> +}
> +
> +/**
> + * yt8824_aneg_done()  - check negotiation state.
> + * @phydev: a pointer to a &struct phy_device
> + *
> + * Returns: auto-negotiation complete status or negative errno code
> + */
> +static int yt8824_aneg_done(struct phy_device *phydev)
> +{
> +	struct yt8824_shared_priv *priv = phy_package_get_priv(phydev);
> +	int auto_neg;
> +	int ret;
> +
> +	mutex_lock(&priv->shared_lock);
> +	ret = phy8824_page_write_with_lock(phydev, YT8824_RSSR_UTP_SPACE);
> +	if (ret < 0)
> +		goto err;
> +
> +	ret = phy_read(phydev, MII_BMSR);
> +	if (ret < 0)
> +		goto err;
> +	mutex_unlock(&priv->shared_lock);
> +	auto_neg = !!(ret & BMSR_ANEGCOMPLETE);
> +
> +	phydev_dbg(phydev, "%s, phy addr: %d, auto negotiation done: %d\n",
> +		   __func__, phydev->mdio.addr, auto_neg);
> +	return auto_neg;
> +err:
> +	mutex_unlock(&priv->shared_lock);
> +	return ret;
> +}
> +
> +/**
> + * yt8824_read_status_paged() -  determines the speed and duplex of one page
> + * @phydev: a pointer to a &struct phy_device
> + *
> + * Returns: 0 or negative errno code
> + */
> +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;
> +	}
> +
> +	if (!phydev->link) {
> +		phydev->speed = SPEED_UNKNOWN;
> +		phydev->duplex = DUPLEX_UNKNOWN;
> +		if (phydev->autoneg == AUTONEG_ENABLE)
> +			phy_resolve_aneg_pause(phydev);
> +		return 0;
> +	}
> +
> +	ret = phy_read(phydev, YTPHY_SPECIFIC_STATUS_REG);
> +	if (ret < 0)
> +		return ret;
> +
> +	val = ret;
> +
> +	yt8821_adjust_status(phydev, val);
> +
> +	if (phydev->autoneg == AUTONEG_ENABLE)
> +		phy_resolve_aneg_pause(phydev);
> +
> +	return 0;
> +}
> +
> +/**
> + * yt8824_read_status() -  determines the negotiated speed and duplex
> + * @phydev: a pointer to a &struct phy_device
> + *
> + * Returns: 0 or negative errno code
> + */
> +static int yt8824_read_status(struct phy_device *phydev)
> +{
> +	struct yt8824_shared_priv *priv = phy_package_get_priv(phydev);
> +	int ret;
> +
> +	mutex_lock(&priv->shared_lock);
> +	ret = yt8824_read_status_paged(phydev);
> +	mutex_unlock(&priv->shared_lock);
> +
> +	return ret;
> +}
> +
> +/**
> + * yt8824_utp_power_on(): utp power on.
> + * @phydev: a pointer to a &struct phy_device
> + *
> + * Returns: 0 or negative errno code
> + */
> +static int yt8824_utp_power_on(struct phy_device *phydev)
> +{
> +	int ret = 0;
> +
> +	ret = phy8824_page_write_with_lock(phydev, YT8824_RSSR_UTP_SPACE);
> +	if (ret < 0)
> +		return ret;
> +
> +	return phy_modify(phydev, MII_BMCR, BMCR_PDOWN | BMCR_ISOLATE, 0x0);
> +}
> +
> +/**
> + * yt8824_utp_power_down(): utp power down.
> + * @phydev: a pointer to a &struct phy_device
> + *
> + * Returns: 0 or negative errno code
> + */
> +static int yt8824_utp_power_down(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = phy8824_page_write_with_lock(phydev, YT8824_RSSR_UTP_SPACE);
> +	if (ret < 0)
> +		return ret;
> +
> +	return phy_modify(phydev, MII_BMCR, BMCR_PDOWN, BMCR_PDOWN);
> +}
> +
> +/**
> + * yt8824_power_on()  - set utp power on.
> + * @phydev: a pointer to a &struct phy_device
> + *
> + * NOTE: need WA like softreset
> + *
> + * Returns: 0 or negative errno code
> + */
> +static int yt8824_power_on(struct phy_device *phydev)
> +{
> +	struct yt8824_shared_priv *priv = phy_package_get_priv(phydev);
> +	int ret;
> +	int r;
> +
> +	if (priv->package_mode == PHY_INTERFACE_MODE_INTERNAL) {
> +		ret = yt8824_utp_set_template_test_mode(phydev,
> +							MDIO_PMA_10GBT_TESTMODE_1);
> +		if (ret < 0)
> +			goto retry;
> +
> +		ret = yt8824_utp_power_on(phydev);
> +		if (ret < 0)
> +			goto retry;
> +
> +		ret = yt8824_utp_set_template_test_mode(phydev,
> +							MDIO_PMA_10GBT_TESTMODE_NORMAL);
> +		if (ret < 0)
> +			goto retry;
> +	} else {
> +		ret = yt8824_utp_set_template_test_mode(phydev,
> +							MDIO_PMA_10GBT_TESTMODE_1);
> +		if (ret < 0)
> +			goto retry;
> +
> +		ret = yt8824_sds_isolate_paged(phydev);
> +		if (ret < 0)
> +			goto retry;
> +
> +		ret = yt8824_utp_power_on(phydev);
> +		if (ret < 0)
> +			goto retry;
> +
> +		ret = yt8824_utp_set_template_test_mode(phydev,
> +							MDIO_PMA_10GBT_TESTMODE_NORMAL);
> +		if (ret < 0)
> +			goto retry;
> +
> +		ret = yt8824_sds_isolate_and_softreset_paged(phydev);
> +		if (ret < 0)
> +			goto retry;
> +	}
> +	return 0;
> +
> +retry:
> +	/*
> +	 * If the PHY up operation succeeds but the subsequent operation
> +	 * fails, revert to the default state.
> +	 */
> +	r = yt8824_utp_power_down(phydev);
> +	if (ret >= 0 && r < 0)
> +		ret = r;
> +	ret = yt8824_restore_working_status(phydev, ret);
> +	return ret;
> +}
> +
> +/**
> + * yt8824_resume() - resume the hardware
> + * @phydev: a pointer to a &struct phy_device
> + *
> + * Returns: 0 or negative errno code
> + */
> +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);
> +	mutex_unlock(&priv->shared_lock);
> +
> +	return ret;
> +}
> +
> +/**
> + * yt8824_power_down()  - set utp power down.
> + * @phydev: a pointer to a &struct phy_device
> + *
> + * NOTE: need WA like softreset
> + *
> + * Returns: 0 or negative errno code
> + */
> +static int yt8824_power_down(struct phy_device *phydev)
> +{
> +	struct yt8824_shared_priv *priv = phy_package_get_priv(phydev);
> +	int ret;
> +	int r;
> +
> +	if (priv->package_mode == PHY_INTERFACE_MODE_INTERNAL) {
> +		ret = yt8824_utp_set_template_test_mode(phydev,
> +							MDIO_PMA_10GBT_TESTMODE_1);
> +		if (ret < 0)
> +			goto retry;
> +
> +		ret = yt8824_utp_power_down(phydev);
> +		if (ret < 0)
> +			goto retry;
> +
> +		ret = yt8824_utp_set_template_test_mode(phydev,
> +							MDIO_PMA_10GBT_TESTMODE_NORMAL);
> +		if (ret < 0)
> +			goto retry;
> +	} else {
> +		ret = yt8824_utp_set_template_test_mode(phydev,
> +							MDIO_PMA_10GBT_TESTMODE_1);
> +		if (ret < 0)
> +			goto retry;
> +
> +		ret = yt8824_sds_isolate_paged(phydev);
> +		if (ret < 0)
> +			goto retry;
> +
> +		ret = yt8824_utp_power_down(phydev);
> +		if (ret < 0)
> +			goto retry;
> +
> +		ret = yt8824_utp_set_template_test_mode(phydev,
> +							MDIO_PMA_10GBT_TESTMODE_NORMAL);
> +		if (ret < 0)
> +			goto retry;
> +
> +		ret = yt8824_sds_isolate_and_softreset_paged(phydev);
> +		if (ret < 0)
> +			goto retry;
> +	}
> +	return 0;
> +
> +retry:
> +	/*
> +	 * If the PHY down operation succeeds but the subsequent operation
> +	 * fails, revert to the default state.
> +	 */
> +	r = yt8824_utp_power_on(phydev);
> +	if (ret >= 0 && r < 0)
> +		ret = r;
> +	ret = yt8824_restore_working_status(phydev, ret);
> +	return ret;
> +}
> +
> +/**
> + * yt8824_suspend() - suspend the hardware
> + * @phydev: a pointer to a &struct phy_device
> + *
> + * Returns: 0 or negative errno code
> + */
> +static int yt8824_suspend(struct phy_device *phydev)
> +{
> +	struct yt8824_shared_priv *priv = phy_package_get_priv(phydev);
> +	int ret;
> +
> +	mutex_lock(&priv->shared_lock);
> +	ret = yt8824_power_down(phydev);
> +	mutex_unlock(&priv->shared_lock);
> +
> +	return ret;
> +}
> +
> +/**
> + * yt8824_config_aneg() - config negotiation
> + * @phydev: a pointer to a &struct phy_device
> + *
> + * Returns: 0 or negative errno code
> + */
> +static int yt8824_config_aneg(struct phy_device *phydev)
> +{
> +	struct yt8824_shared_priv *priv = phy_package_get_priv(phydev);
> +	int phy_ctrl = 0;
> +	int ret;
> +
> +	mutex_lock(&priv->shared_lock);
> +	ret = phy8824_page_write_with_lock(phydev, YT8824_RSSR_UTP_SPACE);
> +	if (ret < 0)
> +		goto err;
> +
> +	/*
> +	 * Only advertise 2.5G when autoneg is enabled, or when 2.5G is
> +	 * explicitly forced.  When a different speed is forced, clear
> +	 * ADV2_5G so a 2.5G-capable link partner cannot negotiate 2.5G.
> +	 * __genphy_config_aneg() only rewrites the
> +	 * clause 22 registers on the forced-speed path, so it will not
> +	 * clear this bit.
> +	 */
> +	if ((phydev->autoneg == AUTONEG_ENABLE ||
> +	     phydev->speed == SPEED_2500) &&
> +	    linkmode_test_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
> +			      phydev->advertising))
> +		phy_ctrl = MDIO_AN_10GBT_CTRL_ADV2_5G;
> +
> +	ret = phy_modify_mmd_changed(phydev, MDIO_MMD_AN, MDIO_AN_10GBT_CTRL,
> +				     MDIO_AN_10GBT_CTRL_ADV2_5G, phy_ctrl);
> +	if (ret < 0)
> +		goto err;
> +
> +	ret = __genphy_config_aneg(phydev, ret);
> +
> +err:
> +	mutex_unlock(&priv->shared_lock);
> +	return ret;
> +}
> +
> +/**
> + * phy_mode_check() - check if mode names the PHY interface.
> + * @mode: PHY interface mode string
> + * @interface: PHY interface mode to compare against
> + *
> + * Return: true if @mode matches @interface, false otherwise.
> + */
> +static bool phy_mode_check(const char *mode, int interface)
> +{
> +	return !strcasecmp(mode, phy_modes(interface));
> +}
> +
> +/**
> + * yt8824_phy_package_probe_once()  - init phy package for phy8824.
> + * @phydev: a pointer to a &struct phy_device
> + *
> + * Returns: 0 or negative errno code
> + */
> +static int yt8824_phy_package_probe_once(struct phy_device *phydev)
> +{
> +	struct yt8824_shared_priv *priv = phy_package_get_priv(phydev);
> +	struct device_node *np = phy_package_get_node(phydev);
> +	const char *mode;
> +
> +	if (!priv || !np) {
> +		phydev_err(phydev, "%s: missing shared priv or DT node\n",
> +			   __func__);
> +		return -EINVAL;
> +	}
> +	/* Initialise shared lock for YT8824 */
> +	mutex_init(&priv->shared_lock);
> +	priv->sds_initialized = false;
> +	priv->package_mode = PHY_INTERFACE_MODE_INTERNAL;
> +	if (!of_property_read_string(np, "motorcomm,package-mode",
> +				     &mode)) {
> +		if (phy_mode_check(mode, PHY_INTERFACE_MODE_INTERNAL)) {
> +			priv->package_mode = PHY_INTERFACE_MODE_INTERNAL;
> +		} else if (phy_mode_check(mode,
> +					  PHY_INTERFACE_MODE_10G_QXGMII)) {
> +			priv->package_mode = PHY_INTERFACE_MODE_10G_QXGMII;
> +		} else {
> +			phydev_err(phydev, "unsupported package-mode %s\n",
> +				   mode);
> +			return -EINVAL;
> +		}
> +	} else {
> +		phydev_err(phydev, "missing package-mode in PHY package node.\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * yt8824_probe() - phy8824 probe.
> + * @phydev: a pointer to a &struct phy_device
> + *
> + * Returns: 0 or negative errno code
> + */
> +static int yt8824_probe(struct phy_device *phydev)
> +{
> +	struct device *dev = &phydev->mdio.dev;
> +	struct yt8824_shared_priv *shared_priv;
> +	int ret;
> +
> +	ret = devm_of_phy_package_join(dev, phydev, sizeof(*shared_priv));
> +	if (ret)
> +		return ret;
> +
> +	/* Ensure other PHY probes wait for shared mutex initialization. */
> +	phy_package_lock(phydev);
> +	if (phy_package_probe_once(phydev)) {
> +		ret = yt8824_phy_package_probe_once(phydev);
> +		if (ret) {
> +			phy_package_unlock(phydev);
> +			return ret;
> +		}
> +	}
> +	phy_package_unlock(phydev);
> +
> +	return 0;
> +}
> +
>  static struct phy_driver motorcomm_phy_drvs[] = {
>  	{
>  		PHY_ID_MATCH_EXACT(PHY_ID_YT8511),

[Severity: Medium]
When the link is down, this block overwrites phydev->speed and
phydev->duplex with SPEED_UNKNOWN and DUPLEX_UNKNOWN, whatever
phydev->autoneg is set to. Is that intended for the forced case?

With AUTONEG_DISABLE, phydev->speed and phydev->duplex are more than
status. They also hold the forced mode the user configured.
phy_ethtool_ksettings_get() reports them back. _phy_start_aneg()
passes them to phy_sanitize_settings() before phy_config_aneg().
genphy_read_status() has just refreshed them from BMCR through
genphy_read_status_fixed(), and this block then discards them.

Take "ethtool -s ethX autoneg off speed 100 duplex full" with the
cable unplugged. phy_check_link_status() runs straight after
phy_config_aneg() and loses the forced speed and duplex. Now take the
interface down and up, or suspend and resume. _phy_start_aneg() calls
phy_sanitize_settings(), which calls phy_caps_lookup() with
SPEED_UNKNOWN and exact == false. That lookup falls back to the lowest
supported mode, so the PHY ends up forced to 10/Half rather than the
100/Full the user asked for.

While the link stays down, a partial linkmodes update over ethtool
netlink also starts from these unknown values.
phy_ethtool_ksettings_set() then rejects AUTONEG_DISABLE with an
unknown speed or duplex and returns -EINVAL.

With autoneg enabled, genphy_read_status() already resets speed,
duplex and pause before reading the link partner, so the block looks
redundant there. The phy_resolve_aneg_pause() call is also a no-op.
Duplex has just been set to DUPLEX_UNKNOWN, and
phy_resolve_aneg_pause() only does anything for DUPLEX_FULL.

yt8821_read_status() does not do this. It only calls
yt8821_adjust_status() when the link is up. Could the !phydev->link
branch become a plain return 0, or be limited to AUTONEG_ENABLE?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261020021929.3028490-1-kyle.switch%40motor-comm.com