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

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: phy: Add driver for Motorcomm Quad 2.5GbE phy
    
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.

> diff --git a/drivers/net/phy/motorcomm.c b/drivers/net/phy/motorcomm.c
> index c5a2cda8d31bd..6ca6618993182 100644
> --- a/drivers/net/phy/motorcomm.c
> +++ b/drivers/net/phy/motorcomm.c

[ ... ]

> @@ -538,15 +538,15 @@
> +static int phy8824_page_write_with_lock(struct phy_device *phydev, int page)
> +{
> +	int ret;
> +
> +	phy_lock_mdio_bus(phydev);
> +	ret = ytphy_read_top_ext(phydev, YT8521_REG_SPACE_SELECT_REG);
> +	if (ret < 0)
> +		goto err;
> +	ret &= ~YT8824_RSSR_SPACE_MASK;
> +	ret |= (page & YT8824_RSSR_SPACE_MASK);
> +	ret = ytphy_write_top_ext(phydev, YT8521_REG_SPACE_SELECT_REG, ret);
> +
> +err:
> +	phy_unlock_mdio_bus(phydev);
> +	return ret;
> +}

[ ... ]

> @@ -881,18 +881,18 @@
> +static int yt8824_sds_isolate_and_softreset_paged(struct phy_device *phydev)
> +{
> +	int val = 0;
> +	int ret = -1;
> +	int r;
> +
> +	ret = phy8824_page_write_with_lock(phydev, YT8824_RSSR_USXGMII_SPACE);
> +	if (ret < 0)
> +		goto err_restore_page;
> +
> +	/* sds softreset and enable isolate */
> +	ret = phy_modify(phydev, MII_BMCR, BMCR_RESET | BMCR_ISOLATE,
> +			 BMCR_RESET & ~BMCR_ISOLATE);
> +	if (ret < 0)
> +		goto err_restore_page;
> +
> +	ret = phy_read_poll_timeout(phydev, MII_BMCR, val,
> +				    !(val & BMCR_RESET),
> +				    50000, 600000, true);

[Severity: High]
Does swapping the global register space and then releasing the mdio bus lock
expose other generic phylib operations to silent data corruption?

Because phy8824_page_write_with_lock() acquires and then immediately
releases mdio_lock after modifying YT8521_REG_SPACE_SELECT_REG, the global
page is left pointing to USXGMII_SPACE while this function runs its 600ms
polling loop.

If a generic PHY operation (like a background status poll or an ioctl from
userspace) runs on another port during this window, it will only acquire
mdio_lock, not the driver's private shared_lock, and end up reading or
writing the wrong register space.

[ ... ]

> @@ -1181,18 +1181,18 @@
> +static int yt8824_extern_config_sds_init_paged(struct phy_device *phydev)
> +{
> +	struct yt8824_shared_priv *shared_priv;
> +	int val_1, val_2, val_3, tmp;
> +	int port;
> +	int val;
> +	int ret;
> +	int r;
> +
> +	shared_priv = phy_package_get_priv(phydev);
> +	port = phydev->mdio.addr - shared_priv->offset;
> +	if (port < 0)
> +		return -EINVAL;
> +	ret = phy8824_page_write_with_lock(phydev, YT8824_RSSR_USXGMII_SPACE);
> +	if (ret < 0)
> +		goto err_restore_page;

[ ... ]

> +	if (port == 0) {
> +		/* Serdes optimization */
> +		ret = ytphy_write_ext_with_lock(phydev, 0x04be, 0x000d);

[Severity: Medium]
Will the shared package-level SerDes remain uninitialized if the user or
system brings up secondary ports without ever bringing up Port 0?

Because this global SerDes optimization is gated behind the port == 0 check
during the per-port config_init phase, it seems the hardware may be left
with degraded 2.5G performance or a failing link if Port 0 happens to be
unused in a given configuration.

[ ... ]

> @@ -4494,9 +4494,9 @@
> +static int yt8824_config_aneg(struct phy_device *phydev)
> +{
> +	int phy_ctrl = 0;
> +	int ret = 0;
> +
> +	ret = phy8824_page_write_with_lock(phydev, YT8824_RSSR_UTP_SPACE);

[Severity: High]
Does this function need to hold the package's shared_lock?

If auto-negotiation is triggered on one port concurrently with the
initialization of another port, yt8824_config_aneg() will force the global
page to UTP_SPACE. The port performing initialization (which holds
shared_lock and assumes the page is set to USXGMII_SPACE) will then
accidentally write its configuration to the UTP registers.

[ ... ]

> @@ -4523,14 +4523,14 @@
> +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_NA;
> +	priv->offset = INTERNAL_PHY8824_ADDR_UTP0;
> +	if (!of_property_read_string(np, "motorcomm,interface-mode",
> +				     &interface_mode_name)) {

[Severity: High]
How does the driver behave if the motorcomm,interface-mode device tree
property is missing?

If of_property_read_string() returns an error because the property is absent,
priv->interface_mode remains PHY_INTERFACE_MODE_NA and priv->offset remains
INTERNAL_PHY8824_ADDR_UTP0 (which evaluates to 4).

Later during initialization, yt8824_config_init() checks:
if (priv->interface_mode == PHY_INTERFACE_MODE_INTERNAL)

Since PHY_INTERFACE_MODE_NA does not equal PHY_INTERFACE_MODE_INTERNAL, the
code falls through to the external USXGMII initialization path. For an
internal PHY, the port index calculation (4 - 4 = 0) succeeds, causing the
driver to actively misconfigure the internal PHY with external SerDes
settings. For an external PHY (0 - 4 = -4), it results in a silent -EINVAL
abort.

Should there be a definitive default or a clean probe failure if this
property is missing?
-- 
pw-bot: cr