Re: [PATCH net-next v22 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 4 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 2

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] In drivers/net/phy/motorcomm.c, yt8824_read_status_paged()
  clears phydev->speed and phydev->duplex to UNKNOWN whenever the link…
- [Medium] yt8824_read_status_paged() only calls genphy_c45_read_lpa()
  when `phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete`.
- [Low] yt8824_probe() calls phy_package_probe_once(), and
  __phy_package_set_once() does `!test_and_set_bit()`, so the shared…
- [Low] In yt8824_config_aneg(), the comment says ADV2_5G is advertised
  'when 2.5G is explicitly forced', and the condition has a…

--

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 for the shared
top extended register space, and a package-wide mutex to serialize UTP and
SerDes register space switching. The package mode comes from the
"motorcomm,package-mode" DT property.

> diff --git a/drivers/net/phy/motorcomm.c b/drivers/net/phy/motorcomm.c
> index 90a4f86f2758b..11672fb25020b 100644
> --- a/drivers/net/phy/motorcomm.c
> +++ b/drivers/net/phy/motorcomm.c

[ ... ]

> @@ -3104,6 +4230,461 @@ static int yt8821_resume(struct phy_device *phydev)
>  	return yt8821_modify_utp_fiber_bmcr(phydev, BMCR_PDOWN, 0);
>  }
>  

[ ... ]

> +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 ETHTOOL_LINK_MODE_2500baseT_Full_BIT stay set in
phydev->lp_advertising after a 2.5G link partner goes away?

If autoneg has not completed (link loss, or renegotiation in progress),
genphy_read_status()->genphy_read_lpa() clears only the C22 and 1000BASE-T
bits:

drivers/net/phy/phy_device.c:genphy_read_lpa() {
    ...
	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;
    ...
}

Only the !MDIO_AN_STAT1_COMPLETE branch of genphy_c45_read_lpa() clears
the 2.5G link partner bit:

	if (!(val & MDIO_AN_STAT1_COMPLETE)) {
		...
		mii_10gbt_stat_mod_linkmode_lpa_t(phydev->lp_advertising, 0);

The autoneg_complete gate here means that branch never runs. So ethtool
would keep showing 2500baseT/Full in the link partner advertisement until
the next negotiation completes. yt8821_read_status() has the same gate,
and this code copies it.

> +
> +	if (!phydev->link) {
> +		phydev->speed = SPEED_UNKNOWN;
> +		phydev->duplex = DUPLEX_UNKNOWN;

[Severity: Medium]
Does this throw away the forced speed and duplex when autoneg is disabled?

With autoneg off, genphy_read_status()->genphy_read_status_fixed() has
just loaded the forced speed and duplex from BMCR. Whenever the link is
down, this block overwrites them with SPEED_UNKNOWN and DUPLEX_UNKNOWN.

On the next restart (ip link set down/up, or phy_start() on resume), the
path looks like this:

_phy_start_aneg()
  phy_sanitize_settings()
    phy_caps_lookup(SPEED_UNKNOWN, DUPLEX_UNKNOWN, supported, false)
      /* no match, falls back to 'last', the slowest capability */
  yt8824_config_aneg()
    __genphy_config_aneg()
      genphy_setup_forced()

BMCR then gets the slowest supported mode, normally 10 Mb/s half duplex.
It no longer matches what was requested with, for example,
"ethtool -s ethX autoneg off speed 100 duplex full".

The PHY_HALTED handling in _phy_state_machine() avoids this by resetting
speed and duplex only when autoneg is enabled:

	case PHY_HALTED:
		if (phydev->link) {
			if (phydev->autoneg == AUTONEG_ENABLE) {
				phydev->speed = SPEED_UNKNOWN;
				phydev->duplex = DUPLEX_UNKNOWN;
			}

Should this block be limited to AUTONEG_ENABLE as well?

> +		if (phydev->autoneg == AUTONEG_ENABLE)
> +			phy_resolve_aneg_pause(phydev);
> +		return 0;
> +	}

[ ... ]

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

[Severity: Low]
Is the phydev->speed == SPEED_2500 part of this condition reachable?

phy_ethtool_ksettings_set() rejects AUTONEG_DISABLE for any speed other
than 10, 100 or 1000:

	if (autoneg == AUTONEG_DISABLE &&
	    ((speed != SPEED_1000 &&
	      speed != SPEED_100 &&
	      speed != SPEED_10) ||

phylink_ethtool_ksettings_set() goes through the same helper. The comment
about 2.5G being "explicitly forced" therefore describes a case the driver
cannot currently reach.

If it were reachable, the C45 advertisement would come from
phydev->advertising. The forced C22 advertisement in __genphy_config_aneg()
comes from phy_caps_lookup(speed, duplex, supported) instead, so the two
halves would use different sources.

Could the condition and the comment cover only the autoneg case?

> +
> +	ret = phy_modify_mmd_changed(phydev, MDIO_MMD_AN, MDIO_AN_10GBT_CTRL,
> +				     MDIO_AN_10GBT_CTRL_ADV2_5G, phy_ctrl);

[ ... ]

> +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->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)
> +{
> +	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);

[Severity: Low]
What happens to sibling PHYs in the package when
yt8824_phy_package_probe_once() fails?

phy_package_probe_once() ends up in __phy_package_set_once(), which does
!test_and_set_bit(). The once-bit is therefore set before the DT property
is validated. On -EINVAL the bit is not cleared, and the failure is not
recorded in the shared priv.

devm_of_phy_package_join() runs outside the package lock. A sibling that
joined earlier (async probing, or overlapping manual binds) keeps the
shared object alive:

PHY A                                 PHY B
devm_of_phy_package_join()            devm_of_phy_package_join()
phy_package_lock()
phy_package_probe_once() -> true
yt8824_phy_package_probe_once()
  package_mode = INTERNAL
  return -EINVAL
phy_package_unlock()
                                      phy_package_lock()
                                      phy_package_probe_once() -> false
                                      phy_package_unlock()
                                      return 0

PHY B then binds with the PHY_INTERFACE_MODE_INTERNAL default that was
written before the property check. Its config_init runs the internal-mode
sequence on a package whose DT was already rejected.

With the default synchronous probing, the failed probe's devres unwind
drops the refcount to zero first. This only shows up with async probing.

Could the first probe's result be stored in yt8824_shared_priv so that
later joiners also fail?

As a side note, the !priv || !np check can't trigger, because
of_phy_package_join() always sets both.

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