Re: [PATCH net-next v2 8/8] net: phy: add X-Powers AC200/AC300 EPHY driver

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Premature devres action registration causes unbalanced power_off on probe failure.
- [High] Missing phy_restore_page() on phy_select_page() error causes MDIO bus deadlock.
- [High] Unbalanced power_on in soft_reset causes resource reference count leak.
- [High] Acquiring the supplier's device_lock in probe creates an AB-BA deadlock.
- [Medium] Incorrect error recovery in suspend causes an unbalanced power_on.
--

--- Patch [8]: [PATCH net-next v2 8/8] net: phy: add X-Powers AC200/AC300 EPHY driver ---
commit 5868cfc367391cc28058cf619b83005f8ecd1852
Author: James Hilliard <[email protected]>

net: phy: add X-Powers AC200/AC300 EPHY driver

This commit adds a shared PHY driver for the X-Powers AC200 and AC300 companion
ICs. It configures the common MDIO PHY registers and manages package-specific
power, calibration, and interface configurations.

> diff --git a/drivers/net/phy/xpowers-acx00.c b/drivers/net/phy/xpowers-acx00.c
> new file mode 100644
> index 0000000000000..f9e6be751252d
> --- /dev/null
> +++ b/drivers/net/phy/xpowers-acx00.c
[ ... ]
> +static int acx00_ephy_get_control(struct phy_device *phydev,
> +				  struct acx00_ephy_priv *priv)
> +{
[ ... ]
> +	/* The managed link keeps the provider and its operations bound. */
> +	device_lock(supplier);
> +	if (!device_is_bound(supplier)) {
> +		ret = -EPROBE_DEFER;
> +	} else {

[Severity: High]
Does this introduce a regression by creating a deadlock during device unbind?

The driver core's really_probe() holds the consumer's device_lock while
calling acx00_ephy_get_control(), which then acquires device_lock(supplier).

Conversely, the driver core's unbind path holds the supplier's device_lock and
attempts to acquire the consumer's device_lock to tear down the device link.

[ ... ]
> +static int acx00_ephy_probe(struct phy_device *phydev)
> +{
[ ... ]
> +	ret = devm_add_action_or_reset(dev, acx00_ephy_control_release, priv);
> +	if (ret)
> +		return ret;
> +
> +	ret = acx00_ephy_control_power_on(priv);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed to power on control block\n");

[Severity: High]
If acx00_ephy_control_power_on() fails, will the devres cleanup execute an
unbalanced power_off on the control block?

Since the devm action is registered before power_on completes, returning an
error here triggers acx00_ephy_control_release() on a device that failed to
power on.

[ ... ]
> +static int acx00_ephy_soft_reset(struct phy_device *phydev)
> +{
[ ... ]
> +	ret = acx00_ephy_set_interface(phydev);
> +	if (ret)
> +		return ret;
> +
> +	ret = acx00_ephy_control_power_on(priv);
> +	if (ret)
> +		return ret;

[Severity: High]
Does this cause a regression by leaking the provider's reference counts
(regulator, clock, etc.) on every soft reset?

acx00_ephy_control_power_on() is called unconditionally, but there is no
corresponding power_off in the reset cycle. Repeated interface up/down cycles
would monotonically increment the reference counts.

[ ... ]
> +static int acx00_ephy_config_init(struct phy_device *phydev)
> +{
[ ... ]
> +	oldpage = phy_select_page(phydev, ACX00_PAGE_1);
> +	if (oldpage < 0)
> +		return oldpage;

[Severity: High]
Does returning directly here cause a regression by leaving the mdio bus
permanently deadlocked?

phy_select_page() acquires the mdio bus lock before attempting to read the
page. If the read fails and returns an error, the caller must still invoke
phy_restore_page() to unlock the bus.

[ ... ]
> +static int acx00_ephy_suspend(struct phy_device *phydev)
> +{
[ ... ]
> +	ret = acx00_ephy_control_power_off(priv);
> +	if (ret) {
> +		resume_ret = acx00_ephy_power_on_and_resume(phydev);
> +		if (resume_ret)
> +			phydev_warn(phydev,
> +				    "failed to recover from suspend error: %pe\n",
> +				    ERR_PTR(resume_ret));
> +	}

[Severity: Medium]
Does this error recovery path cause a regression by forcing an unbalanced power
state?

If acx00_ephy_control_power_off() fails, the resource was never successfully
powered off. Invoking acx00_ephy_power_on_and_resume() here will execute an
additional power_on, resulting in a reference count leak.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8