Re: [PATCH net-next v24 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: 4 · Low: 3

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] yt8824_read_status_paged() sets phydev->speed/duplex to
  SPEED_UNKNOWN/DUPLEX_UNKNOWN whenever the link is down, even with…
- [Medium] priv->sds_initialized is set to true after the first
  successful package SERDES init in yt8824_config_init().
- [Medium] yt8824_probe() calls phy_package_probe_once(), which
  permanently sets the package's 'probed' bit via test_and_set_bit()…
- [Medium] After a negotiation with a 2.5G-capable partner,
  lp_advertising holds ETHTOOL_LINK_MODE_2500baseT_Full_BIT.
- [Low] The commit message (three lines for about 1900 added lines)
  leaves out behaviour a maintainer needs in order to review the driver.
- [Low] Some kernel-doc does not match the code.
- [Low] yt8824_config_utp_init_paged() writes the U0/U1 TX resetb bits to
  0 and later to 1, and pulses YT8824_PLL_DAC_RST 0→1.

--

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. The phy_package helpers are used to reach the
shared top extended register space. The package mode (internal or
10G-QXGMII) is read from the PHY package DT node.

> 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: Low]
Could the commit message say more about what the driver does? It is three
lines long for roughly 1900 added lines.

Five functions put the copper side into PMA template TESTMODE_1, do the
reset or power change, and then switch back to TESTMODE_NORMAL:
yt8824_soft_reset(), yt8824_power_on(), yt8824_power_down(),
yt8824_restore_working_status() and yt8824_config_utp_init_paged().

yt8824_soft_reset()
    yt8824_utp_set_template_test_mode(phydev, MDIO_PMA_10GBT_TESTMODE_1);
    ...
    yt8824_utp_softreset_paged(phydev);
    ...
    yt8824_utp_set_template_test_mode(phydev, MDIO_PMA_10GBT_TESTMODE_NORMAL);

This looks like a hardware workaround. However,
genphy_c45_template_testmode() was added in "net: phy: Add support for
Template Control register for PMA" as a helper "used for PHY validation".

Could the commit message explain the workaround and why it is needed?

Probe also now requires DT. devm_of_phy_package_join() needs an
ethernet-phy-package parent with a reg property. Without
motorcomm,package-mode, yt8824_phy_package_probe_once() returns -EINVAL.
The two package modes also take quite different init paths.

Could these points be mentioned as well, along with the source of the many
magic register values in the init sequences?

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

[ ... ]

> @@ -437,6 +684,70 @@ static int ytphy_read_ext_with_lock(struct phy_device *phydev, u16 regnum)
>  	return ret;
>  }
>  
> +/**
> + * ytphy_read_top_ext() - read a PHY's top extended register for YT8824
> + * @phydev: a pointer to a &struct phy_device
> + * @regnum: register number to read
> + *
> + * Returns: the value of regnum reg or negative error code
> + */
> +static int ytphy_read_top_ext(struct phy_device *phydev, u16 regnum)
> +{
> +	int ret;
> +
> +	lockdep_assert_held(&phydev->mdio.bus->mdio_lock);
> +	ret = __phy_package_write(phydev, 0, YTPHY_PAGE_SELECT, regnum);
> +	if (ret < 0)
> +		return ret;
> +
> +	return __phy_package_read(phydev, 0, YTPHY_PAGE_DATA);
> +}

[Severity: Low]
This isn't a bug, but the kernel-doc for ytphy_read_top_ext() and
ytphy_write_top_ext() says they access "a PHY's top extended register".

Both helpers call __phy_package_read()/__phy_package_write() with offset 0.
That reaches the package-shared block at the package reg address, not this
PHY's own address. Writing YT8521_REG_SPACE_SELECT_REG through them
therefore switches the register space for all four ports.

Could the comments say that?

The kernel-doc for yt8824_restore_working_status() has a similar problem.
It says "called to do store working status", but the function restores
state: it sets the template test mode back to NORMAL, and outside internal
mode it clears SERDES isolate and soft resets the SERDES.

[ ... ]

> @@ -622,15 +933,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)
> +{

[ ... ]

> +	ctrl = FIELD_PREP(YT8824_U0_CSR_RESETB_TX_CH0, 0);
> +	ctrl |= FIELD_PREP(YT8824_U0_CSR_RESETB_TX_CH1, 0);
> +	ctrl |= FIELD_PREP(YT8824_U0_CSR_RESETB_TX_CH2, 0);
> +	ctrl |= FIELD_PREP(YT8824_U0_CSR_RESETB_TX_CH3, 0);
> +	mask  = YT8824_U0_CSR_RESETB_TX_CH0 | YT8824_U0_CSR_RESETB_TX_CH1 |
> +		YT8824_U0_CSR_RESETB_TX_CH2 | YT8824_U0_CSR_RESETB_TX_CH3;
> +	ret = ytphy_modify_ext_with_lock(phydev, YT8824_U0_CRS_RESET_REG,
> +					 mask, ctrl);
> +	if (ret < 0)
> +		goto err_restore;

[ ... ]

> +err_restore:
> +	r = phy_modify(phydev, MII_BMCR, BMCR_PDOWN, 0);
> +	if (ret >= 0 && r < 0)
> +		ret = r;
> +	return ret;

[Severity: Low]
Suppose an MDIO access fails after the U0/U1 TX resetb bits have been
cleared, or between the two YT8824_PLL_DAC_RST writes. err_restore only
clears BMCR_PDOWN in that case.

Is it intended that the TX channels stay held in reset here?

In that case config_init returns an error, and a later phy_init_hw() runs
the whole sequence again, which deasserts the resets. So this may be fine.
If it is, a short comment saying so could help.

[ ... ]

> +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]
What resets priv->sds_initialized once the package has lost its register
state? The only place that sets it to false is
yt8824_phy_package_probe_once().

config_init is called again from phy_init_hw() in at least two places where
the hardware may have been reset:

phy_detach_internal()
    phy_device_reset(phydev, 1);
...
phy_attach_direct()
    phy_init_hw()
        yt8824_config_init()

mdio_bus_phy_resume()
    phy_init_hw()
        yt8824_config_init()

In both cases the shared priv is still alive, because the package is joined
at probe and sibling PHYs keep their references. So
yt8824_extern_config_sds_init_paged() is skipped.

In 10G-QXGMII mode, would the package-wide SERDES PLL/CDR/AFE and
calibration settings then stay at their reset defaults?

[ ... ]

> @@ -3104,6 +4565,450 @@ 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 lp_advertising keep a stale ETHTOOL_LINK_MODE_2500baseT_Full_BIT here?

When a link with a 2.5G partner goes down or renegotiates,
genphy_read_status()->genphy_read_lpa() takes its !autoneg_complete branch.
That branch only clears the clause 22 bits:

genphy_read_lpa()
    mii_stat1000_mod_linkmode_lpa_t(phydev->lp_advertising, 0);
    mii_lpa_mod_linkmode_lpa_t(phydev->lp_advertising, 0);

genphy_c45_read_lpa() would clear the 10GBT bits, but it only runs when
autoneg_complete is set. phy_ethtool_ksettings_get() copies lp_advertising
unchanged, so ethtool would still show a 2.5G-capable link partner.

rtl822x_read_status() avoids this by clearing the 10GBT bits first.

> +
> +	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]
Does this throw away a forced speed and duplex while the link is down?

With autoneg disabled, genphy_read_status() has just reloaded speed and
duplex from BMCR via genphy_read_status_fixed(). This block then replaces
them with SPEED_UNKNOWN/DUPLEX_UNKNOWN.

On the next phy_start(), for example after ifdown/ifup or resume, the
PHY_UP path does:

_phy_start_aneg()
    phy_sanitize_settings()
        phy_caps_lookup(SPEED_UNKNOWN, DUPLEX_UNKNOWN, supported, false)

That lookup matches nothing and returns the slowest supported mode. Then
yt8824_config_aneg()->__genphy_config_aneg()->genphy_setup_forced() would
program 10/half instead of the forced setting the user chose.

yt8821_read_status() in the same file does not clear speed on link down.
Could the clearing here be limited to the autoneg case?

[ ... ]

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

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

phy_package_probe_once() has already set the probed bit with
test_and_set_bit(), and nothing clears it later. priv->package_mode has
also already been set to PHY_INTERFACE_MODE_INTERNAL before
motorcomm,package-mode is checked.

With async probing, a sibling PHY can join through
devm_of_phy_package_join() before the failing PHY's devres drops its
reference. The shared struct then survives. The sibling sees
phy_package_probe_once() return false, skips the validation, and binds in
internal mode even though the DT never asked for that.

Would it be better to validate the property before the once bit is
consumed, so that every PHY in the package fails the same way?

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