Re: [PATCH net-next v17 2/2] 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: 1 · Medium: 1 · Low: 2

- [High] YT8824 MAC-side interface mode is not taken from
  phydev->interface (as every other PHY in motorcomm.c does) but from a…
- [Medium] All package-wide state (mutex_init(&priv->shared_lock) at
  drivers/net/phy/motorcomm.c:4626 and priv->interface_mode at 4627) is…
- [Low] The YT8824 .aneg_done callback (yt8824_aneg_done,
  drivers/net/phy/motorcomm.c:4278) reports link state instead of auto-…
- [Low] The YT8824 binding example names the package node ethernet-phy-
  package@0 but gives it reg = <9>…

--

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

This adds YT8824 quad-port 2.5G support to the existing motorcomm driver,
sharing the top extended register space through the phy_package helpers,
and exports a new phylib helper genphy_c45_template_testmode().

A few questions about the new YT8824 code below.

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

[ ... ]

> @@ -3104,6 +4247,429 @@ static int yt8821_resume(struct phy_device *phydev)

[ ... ]

> +/**
> + * yt8824_aneg_done()  - check negotiation state.
> + * @phydev: a pointer to a &struct phy_device
> + *
> + * Returns: link 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 link = 0;
> +	int ret = 0;
> +
> +	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, YTPHY_SPECIFIC_STATUS_REG);
> +	if (ret < 0)
> +		goto err;
> +	mutex_unlock(&priv->shared_lock);
> +	link = !!(ret & YTPHY_SSR_LINK);
> +
> +	phydev_dbg(phydev, "%s, phy addr: %d, link_utp: %d\n",
> +		   __func__, phydev->mdio.addr, link);
> +	return link;

[Severity: Low]
Should .aneg_done report auto-negotiation completion rather than link
state?  The kernel-doc above says "Returns: link status", and the value
returned here is the vendor YTPHY_SSR_LINK bit.

The phylib contract in phy_aneg_done() is:

drivers/net/phy/phy.c:
 * Returns > 0 on success or < 0 on error. 0 means that auto-negotiation
 * is still pending.

and the generic implementation genphy_aneg_done() derives that from
BMSR_ANEGCOMPLETE, which is a different condition from link-up (forced
links, or auto-negotiation completing before the link is usable).

The one in-tree consumer, phy_poll_aneg_done(), loops on zero:

drivers/net/phy/phy.c:phy_poll_aneg_done() {
	do {
		msleep(100);
		ret = phy_aneg_done(phydev);
	} while (!ret && --retries);

	if (!ret)
		return -ETIMEDOUT;
}

so with the link down a caller such as phy_speed_down() spends the full
poll interval and then gets -ETIMEDOUT.  This isn't new behaviour for this
driver, since yt8821_aneg_done() and yt8521_aneg_done_paged() do the same
thing, but should the new part follow the documented contract?

[ ... ]

> +/**
> + * 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 *interface_mode_name;
> +
> +	/* Initialise shared lock for YT8824 */
> +	mutex_init(&priv->shared_lock);
> +	priv->interface_mode = PHY_INTERFACE_MODE_INTERNAL;
> +	if (!of_property_read_string(np, "phy-mode",
> +				     &interface_mode_name)) {
> +		if (!strcasecmp(interface_mode_name,
> +				phy_modes(PHY_INTERFACE_MODE_USXGMII))) {
> +			priv->interface_mode = PHY_INTERFACE_MODE_USXGMII;
> +		} else if (!strcasecmp
> +				(interface_mode_name,
> +				 phy_modes(PHY_INTERFACE_MODE_INTERNAL))) {
> +			priv->interface_mode = PHY_INTERFACE_MODE_INTERNAL;
> +		} else {
> +			return -EINVAL;
> +		}
> +	} else {
> +		phydev_warn(phydev, "%s, phy-mode missing in DTS.\n",
> +			    __func__);
> +	}
> +
> +	return 0;
> +}

[Severity: High]
Why is the MAC-side mode parsed out of a phy-mode string in the package
node instead of coming from phydev->interface?  YT8511, YT8521, YT8531S
and YT8821 in this same file all use phydev->interface, and the new YT8824
code never reads it.

Can a missing or unparsable property leave a wrong configuration in place?
The default is assigned before the read, and any of_property_read_string()
failure only warns and then returns success, so priv->interface_mode stays
PHY_INTERFACE_MODE_INTERNAL.

For an external YT8824 wired over USXGMII, yt8824_config_init() then takes
the internal branch:

drivers/net/phy/motorcomm.c:yt8824_config_init() {
	if (priv->interface_mode == PHY_INTERFACE_MODE_INTERNAL) {
		ret = yt8824_internal_config_init_paged(phydev);
		...
	} else {
		ret = yt8824_extern_config_sds_init_paged(phydev);
		...
		ret = yt8824_extern_config_utp_init_paged(phydev);
	}
}

which means yt8824_extern_config_sds_init_paged() never runs, so the
efuse-derived PLL/VCO trim (ext 0x0429/0x0441/0x042b), the SerDes LDO trim
(0x04b5/0x04b4), the CDR/RX/TX settings, the min-preamble and fill-preamble
configuration and the SerDes calibration restart are all skipped.

The same value also gates yt8824_sds_isolate_and_softreset_paged() from
yt8824_restore_working_status(), yt8824_power_on(), yt8824_power_down() and
yt8824_soft_reset(), so the SerDes can stay in whatever isolate/reset state
it powered up in, while probe and config_init both report success.

The binding added by the preceding patch in the series,
Documentation/devicetree/bindings/net/motorcomm,yt8824.yaml, lists
phy-mode under required: but never declares it under properties:, and
ethernet-phy-package.yaml does not define it either.  So the documented
ABI has no type or value set, while the driver rejects with -EINVAL every
other schema-legal value and uses strcasecmp, accepting spellings the
lowercase DT convention does not use.  Could the legal values be
enumerated in the binding, and could an absent or unparsable required
property be a hard error rather than a silent default?

> +
> +/**
> + * 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;
> +
> +	if (phy_package_probe_once(phydev)) {
> +		ret = yt8824_phy_package_probe_once(phydev);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}

[Severity: Medium]
Should this be wrapped in phy_package_lock()/phy_package_unlock(), the way
dp83640_probe() does it, so the other members wait for the shared
initialisation?

phy_package_probe_once() only publishes a one-shot bit:

drivers/net/phy/phy_package.c:__phy_package_set_once() {
	return !test_and_set_bit(b, &shared->flags);
}

It returns false for the other members immediately, before
yt8824_phy_package_probe_once() has run mutex_init(&priv->shared_lock) and
assigned priv->interface_mode.  The shared area comes from
phy_package_join():

drivers/net/phy/phy_package.c:phy_package_join() {
	shared->priv = kzalloc(priv_size, GFP_KERNEL);
}

So can a losing member's probe return 0 and have phy_probe() call
.get_features straight away, where yt8824_get_features() does
mutex_lock(&priv->shared_lock) on an all-zero mutex and reads
interface_mode == 0 (PHY_INTERFACE_MODE_NA, which selects the external
branch everywhere)?

Separately, when yt8824_phy_package_probe_once() returns -EINVAL the
one-shot bit stays set, since it is only cleared when the shared struct is
freed as the last member leaves.  Any member that already joined can then
never redo the initialisation, and the remaining members run with the
zeroed state instead of the configuration error being propagated.  Would
recording the init result in the shared priv and checking it from the other
members address that?

[ ... ]

[Severity: Low]
One more on the binding in the preceding series patch, "dt-bindings: net:
Document Motorcomm YT8824 PHY package".  The example names the package node

        ethernet-phy-package@0 {
            ...
            compatible = "motorcomm,yt8824-package";
            reg = <9>;

Should the unit address match the first reg address, that is
ethernet-phy-package@9?  As written dt_binding_check should emit a
unit_address_vs_reg warning for the example.

The child PHYs in the same example are at reg 4 to 7, all below the
declared package base of 9, which also reads oddly against
ethernet-phy-package.yaml:

  reg:
    minimum: 0
    maximum: 31
    description:
      The base ID number for the PHY package.
      Commonly the ID of the first PHY in the PHY package.

Is reg = <9> intended here?

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