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

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] New, undocumented device-tree ABI for YT8824.
- [Medium] Lock-initialization race: priv->shared_lock is mutex_init()'d
  (motorcomm.c:4624) and priv->interface_mode published (4625) *after*…
- [Low] yt8824_restore_working_status()'s kernel-doc summary says "called
  to do store working status" while the function stores nothing: it…

--

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, sharing the top extended register space through
the phy_package helpers. It also exports a new phylib helper,
genphy_c45_template_testmode(), and adds the 10GBASE-T test mode register
definitions to the uapi mdio header.

> diff --git a/drivers/net/phy/motorcomm.c b/drivers/net/phy/motorcomm.c
> index 90a4f86f2758..86f1e93aa82d 100644
> --- a/drivers/net/phy/motorcomm.c
> +++ b/drivers/net/phy/motorcomm.c

[ ... ]

> @@ -626,11 +722,1056 @@ static int ytphy_set_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)

[ ... ]

> +/**
> + * yt8824_restore_working_status() - called to do store working status
> + * @phydev: a pointer to a &struct phy_device
> + * @ret: operation's return code
> + *
> + * Returns: 0 or negative errno code
> + */
> +static int yt8824_restore_working_status(struct phy_device *phydev, int ret)
> +{
> +	struct yt8824_shared_priv *priv = phy_package_get_priv(phydev);
> +	int r;
> +
> +	/* configure normal test mode */
> +	r = yt8824_utp_normal_test_mode_paged(phydev);
> +	if (ret >= 0 && r < 0)
> +		ret = r;

[Severity: Low]
This isn't a bug, but the kernel-doc summary says "called to do store
working status" while the body stores nothing.  It calls
yt8824_utp_normal_test_mode_paged() and, for the non-internal interface
mode, yt8824_sds_isolate_and_softreset_paged(), so it restores the PHY to
its working configuration.  Should the summary say something like
"restore the PHY working configuration"?

[ ... ]

> +/**
> + * yt8824_phy_package_probe_once()  - init phy packet 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, "motorcomm,interface-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, motorcomm,interface-mode missing in DTS.\n",
> +			    __func__);
> +	}
> +
> +	return 0;
> +}

[Severity: Medium]
Is there a device tree binding update planned for this?  This introduces a
new vendor property, motorcomm,interface-mode, read from the PHY package
node, with the legal values fixed by
phy_modes(PHY_INTERFACE_MODE_USXGMII) and
phy_modes(PHY_INTERFACE_MODE_INTERNAL).

The only Motorcomm PHY schema,
Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml, is not touched
by this patch and lists only:

    enum:
      - ethernet-phy-id4f51.e91a
      - ethernet-phy-id4f51.e91b

so there is no YT8824 compatible (ethernet-phy-id4f51.e8b8), no
description of the package node layout, and no description of the new
property.  Without that, can dt_binding_check or dtbs_check validate boards
using this part, and how do device tree authors know which node carries
the property?

An in-tree example of a documented vendor property on a PHY package node is
Documentation/devicetree/bindings/net/qcom,qca807x.yaml.

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

[Severity: Medium]
Related to the binding question above: this call is unconditional, and
of_phy_package_join() in drivers/net/phy/phy_package.c requires a specific,
currently undocumented device tree topology:

    if (!of_node_name_eq(package_node, "ethernet-phy-package")) {
        ret = -EINVAL;

It also requires the PHY to have an OF node and the package node to carry a
reg property.  Does a board that describes this part as four ordinary PHY
nodes, or a non-OF system, then fail probe with -EINVAL and leave the ports
unusable?

> +
> +	if (phy_package_probe_once(phydev)) {
> +		ret = yt8824_phy_package_probe_once(phydev);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}

[Severity: Medium]
Can a sibling PHY of the same package use priv->shared_lock before it has
been initialised?

phy_package_probe_once() is only an election:

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

It does not wait for the winner to finish, so:

CPU0                                CPU1
yt8824_probe()
  phy_package_probe_once() -> true
                                    yt8824_probe()
                                      phy_package_probe_once() -> false
                                      return 0
                                    phy_probe()
                                      yt8824_get_features()
                                        mutex_lock(&priv->shared_lock);
  yt8824_phy_package_probe_once()
    mutex_init(&priv->shared_lock);
    priv->interface_mode = ...;

The shared priv is allocated zeroed by phy_package_join(), so CPU1 would
lock a mutex that has not been through mutex_init() and would read
priv->interface_mode as 0 (PHY_INTERFACE_MODE_NA), which selects the
external SerDes paths in yt8824_config_init() and
yt8824_restore_working_status().

dp83640_probe() wraps the same pattern in the package lock for this reason:

drivers/net/phy/dp83640.c:dp83640_probe() {
    ...
	/* Ensure other PHY probes wait for shared clock initialization. */
	phy_package_lock(phydev);
	if (phy_package_probe_once(phydev))
		dp83640_clock_init(clock);
	phy_package_unlock(phydev);
    ...
}

Would phy_package_lock()/phy_package_unlock() around the once-block here be
appropriate too?

Note also that when the property parse returns -EINVAL, the once bit stays
set, so no other port in the package will retry the shared initialisation.

[ ... ]

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