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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unconditional assertion of CHIP_RESET on first power-on wipes external-supply configuration.
--

--- Patch [6]: [PATCH net-next v2 6/8] net: phy: add X-Powers AC300 EPHY control driver ---
commit 22de676d43131faa2cdf14d10397704b51076320
Author: James Hilliard <[email protected]>

net: phy: add X-Powers AC300 EPHY control driver

This commit adds an MDIO control driver for the Fast Ethernet PHY in the
X-Powers AC300 companion IC. It programs the PHY calibration, interface mode,
clock, and I/O controls.

diff --git a/drivers/net/phy/xpowers-ac300-ctl.c b/drivers/net/phy/xpowers-ac300-ctl.c
--- /dev/null
+++ b/drivers/net/phy/xpowers-ac300-ctl.c

[ ... ]

> +static int ac300_ephy_ctl_power_on(struct acx00_ephy_control *control,
> +				   unsigned int phy_addr)
> +{
> +	struct ac300_ephy_ctl *priv =
> +		container_of(control, struct ac300_ephy_ctl, control);

[ ... ]

> +	ret = clk_prepare_enable(priv->clk);
> +	if (ret)
> +		goto out_unlock;
> +	priv->powered = true;
> +
> +	/* Keep the external-supply configuration across subsequent resets. */
> +	reset_value = priv->package_known && !priv->internal_dldo ?
> +		      AC300_CHIP_RESET_DEASSERT : 0;
> +	ret = mdiodev_write(priv->mdiodev, AC300_SYS_CONTROL_REG, reset_value);

[Severity: High]
Does this code inadvertently wipe the external-supply configuration on the
first probe?

When ac300_ephy_ctl_power_on() is called for the first time,
priv->package_known is false because it is zero-initialized by
devm_kzalloc().

This causes reset_value to evaluate to 0, which is then written to
AC300_SYS_CONTROL_REG, unconditionally asserting CHIP_RESET for all package
types before the package status is ever read.

[ ... ]

> +static int ac300_ephy_ctl_probe(struct mdio_device *mdiodev)
> +{

[ ... ]

> +	priv->control.power_on = ac300_ephy_ctl_power_on;
> +	priv->control.power_off = ac300_ephy_ctl_power_off;
> +	priv->control.set_interface = ac300_ephy_ctl_set_interface;
> +	/* MII is the reset default used until the MAC supplies its interface. */
> +	priv->interface = PHY_INTERFACE_MODE_MII;
> +	mdiodev_set_drvdata(mdiodev, &priv->control);
> +
> +	/* Validate the package while the control endpoint is known to respond. */
> +	phy_addr = ac300_ephy_ctl_link_addr(priv);
> +	ret = ac300_ephy_ctl_power_on(&priv->control, phy_addr);

[Severity: High]
Could this first call to ac300_ephy_ctl_power_on() trigger the destructive
reset mentioned above?

Since priv->package_known is still false at this point during the probe,
the driver seems to wipe out the configuration it intends to preserve.

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