Re: [PATCH net-next 2/4] net: phy: Add Allwinner AC300 EPHY driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] net: phy: ac300: leaked PHY page state on error paths
- [High] net: phy: ac300: race condition in paged PHY register access
- [Medium] pm: incorrect PHY suspend sequence causes MDIO operations on powered-down device
--

commit f19e9d7452b2e059755510b20bf6ea442e025e0f
Author: Alastair D'Silva <alastair@d-silva.org>

net: phy: Add Allwinner AC300 EPHY driver

Adds support for the Allwinner AC300 integrated Fast Ethernet Physical
Layer (EPHY) co-packaged on Allwinner H616 and H618 SoCs.

> diff --git a/drivers/net/phy/ac300-phy.c b/drivers/net/phy/ac300-phy.c
> new file mode 100644
> index 0000000000000..c70f567c31794
> --- /dev/null
> +++ b/drivers/net/phy/ac300-phy.c

[ ... ]

> +static int ac300_phy_config_init(struct phy_device *phydev)
> +{
> +	struct ac300_phy_priv *priv = phydev->priv;
> +	int ret;
> +
> +	/* Write page-based EPHY transceiver and signal path optimizations */
> +	ret = phy_write(phydev, 0x1f, 0x0100); /* Switch to Page 1 */
> +	if (ret)
> +		return ret;
> +	ret = phy_write(phydev, 0x12, 0x4824); /* Disable APS */

[Severity: High]
Can a concurrent MDIO operation interleave during this unlocked window?
phy_write() internally acquires and immediately releases the shared mdio_lock
for each individual register access. This leaves the MDIO bus unlocked between
the page switch and the data write. Should the driver hold mdio_lock across
the entire page manipulation sequence, or implement the .write_page callback
and use phy_write_paged() instead?

> +	if (ret)
> +		return ret;

[Severity: High]
If a transient MDIO error occurs and triggers this early return, does this
leave the PHY permanently stuck on Page 1 instead of restoring it to Page 0?
Subsequent PHY operations, including generic link status polling like
genphy_read_status(), assume the PHY is on Page 0 and might read garbage data
if left on the wrong page.

> +
> +	ret = phy_write(phydev, 0x1f, 0x0200); /* Switch to Page 2 */

[ ... ]

> +static int ac300_phy_suspend(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = ac300_phy_disable(phydev);
> +	if (ret)
> +		return ret;
> +	return genphy_suspend(phydev);

[Severity: Medium]
Will genphy_suspend() fail or produce warnings when called here?
ac300_phy_disable() powers down and resets the PHY, but genphy_suspend()
subsequently issues standard IEEE MDIO writes to MII_BMCR to set BMCR_PDOWN.
Should genphy_suspend() be called while the hardware is still active, before
powering it down?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260916044119.475666-1-alastair@d-silva.org?part=2