Re: [PATCH v16 1/2] net: phy: Add driver for Motorcomm Quad 2.5GbE phy

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Missing `return 0;` on success path causes unintended execution of error cleanup code.
--

commit 9c78ead4e6ec97a6c3f67882266f516e5930cc7c
Author: Kyle Switch <kyle.switch@motor-comm.com>

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

This commit adds support for the Motorcomm YT8824 quad-port 2.5G PHY to the
existing motorcomm driver. It implements the phy_package helpers to handle
the shared top extended register space and introduces a new exported phylib
helper, genphy_c45_template_testmode.

> diff --git a/drivers/net/phy/motorcomm.c b/drivers/net/phy/motorcomm.c
> index c5a2cda8d31bd..d535fd384e7bb 100644
> --- a/drivers/net/phy/motorcomm.c
> +++ b/drivers/net/phy/motorcomm.c
[ ... ]
> +static int yt8824_internal_config_init_paged(struct phy_device *phydev)
> +{
[ ... ]
> +	ret = genphy_c45_template_testmode
> +		(phydev, YT8824_UTP_TEMPLATE_TEST_MODE1);
> +	if (ret)
> +		goto err_restore_normal;
> +	/* reset */
> +	ret = phy_modify(phydev, MII_BMCR, BMCR_RESET | BMCR_ANENABLE,
> +			 BMCR_RESET | BMCR_ANENABLE);
> +	if (ret < 0)
> +		goto err_restore_normal;
> +	ret = phy_read_poll_timeout(phydev, MII_BMCR, val,
> +				    !(val & BMCR_RESET),
> +				    50000, 600000, true);
> +	if (ret < 0)
> +		goto err_restore_normal;
> +
> +	ret = genphy_c45_template_testmode
> +		(phydev, YT8824_UTP_TEMPLATE_TEST_NORMAL);
> +	if (ret < 0)
> +		goto err_restore_normal;
> +

[Severity: Medium]
Does this code accidentally fall through into the error cleanup path?

In yt8824_internal_config_init_paged(), the success path executes 
genphy_c45_template_testmode() but misses a return statement before the 
err_restore label. As a result, it unconditionally falls through to:

err_restore:
	r = phy_modify(phydev, MII_BMCR, BMCR_PDOWN, 0);

This will inadvertently clear the BMCR_PDOWN bit at the end of the 
initialization function instead of keeping it in the expected power-down
state until it is explicitly resumed. Sister functions like 
yt8824_extern_config_utp_init_paged() appear to handle this correctly by
including a return 0; statement here.

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910074624.2426899-1-kyle.switch@motor-comm.com?part=1