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

[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 X-Powers AC200/AC300 EPHY driver

This adds a single PHY driver for the Fast Ethernet link PHY found in the
X-Powers AC200 and AC300 companion ICs, with two package backends: AC200
control registers reached over an I2C regmap, and AC300 control registers
reached through the PHY package helpers at base address plus 16.

A few questions below about the AC200 backend, the resume/suspend return
values and the power-state bookkeeping.

> diff --git a/drivers/net/phy/xpowers/ac200.c b/drivers/net/phy/xpowers/ac200.c
> new file mode 100644
> index 0000000000000..17011b5f5e21c
> --- /dev/null
> +++ b/drivers/net/phy/xpowers/ac200.c
> @@ -0,0 +1,308 @@

[ ... ]

> +static int ac200_ephy_ctl_power_on(struct acx00_ephy_control *control,
> +				   unsigned int phy_addr)
> +{
> +	struct ac200_ephy_ctl *priv =
> +		container_of(control, struct ac200_ephy_ctl, control);
> +	u16 ephy_ctl;
> +	int ret;
> +
> +	if (phy_addr > FIELD_MAX(AC200_EPHY_PHY_ADDR_MASK))
> +		return -EINVAL;
> +
> +	if (priv->powered && priv->phy_addr == phy_addr)
> +		return 0;
> +
> +	if (priv->powered) {
> +		ret = ac200_ephy_ctl_disable(priv);
> +		if (ret)
> +			return ret;
> +	}
> +	priv->phy_addr = phy_addr;
> +
> +	if (!priv->supply_enabled) {
> +		ret = regulator_enable(priv->vcc);
> +		if (ret)
> +			return ret;
> +		priv->supply_enabled = true;
> +	}

[Severity: Medium]

What serializes these check-then-set updates of priv->supply_enabled and
priv->powered?

The same flags are read and written from ac200_ephy_ctl_power_on(),
ac200_ephy_ctl_disable() and ac200_ephy_ctl_set_interface(), reached from
four phylib entry points with different lock contexts:

  .resume    -> acx00_ephy_resume()        (phydev->lock held,
                                            __phy_resume() has
                                            lockdep_assert_held())
  .suspend   -> acx00_ephy_suspend()       (no phylib lock; phy_suspend()
                                            takes none, and
                                            _phy_state_machine_post_work()
                                            calls it after
                                            phy_state_machine() unlocked)
  .soft_reset-> acx00_ephy_soft_reset()    (via exported phy_init_hw(),
                                            no phylib lock)
  devres     -> acx00_ephy_control_release() (unbind, no lock)

If two of those interleave, the regulator enable count (and for AC300 the
clk_prepare_enable() count guarded by the same priv->powered flag) becomes
unbalanced, and the multi-register bring-up and shutdown sequences
interleave since only the regmap or MDIO bus lock is held per access. Would
a mutex in the backend private data be appropriate here?

> +
> +	ephy_ctl = ac200_ephy_ctl_config(priv);
> +
> +	/* Start from a disabled state before applying the configuration. */
> +	ret = ac200_ephy_ctl_write(priv, AC200_SYS_EPHY_CTL0_REG, 0);
> +	if (ret)
> +		goto err_disable;
> +
> +	ret = ac200_ephy_ctl_write(priv, AC200_SYS_EPHY_CTL1_REG,
> +				   AC200_EPHY_MII_IO_ENABLE);
> +	if (ret)
> +		goto err_disable;
> +
> +	ret = ac200_ephy_ctl_write(priv, AC200_EPHY_CTL_REG,
> +				   ephy_ctl | AC200_EPHY_SHUTDOWN);
> +	if (ret)
> +		goto err_disable;
> +
> +	ret = ac200_ephy_ctl_write(priv, AC200_SYS_EPHY_CTL0_REG,
> +				   AC200_EPHY_RESET_DEASSERT |
> +				   AC200_EPHY_SYSCLK_ENABLE);
> +	if (ret)
> +		goto err_disable;
> +
> +	ret = ac200_ephy_ctl_write(priv, AC200_EPHY_CTL_REG, ephy_ctl);
> +	if (ret)
> +		goto err_disable;

[Severity: Low]

Is the AC200 power-on sequence really free of timing requirements?

ac200_ephy_ctl_power_on() writes the control registers immediately after
regulator_enable(), and clears AC200_EPHY_SHUTDOWN in the write directly
following the reset-deassert plus sysclk-enable write. The callers
(acx00_ephy_soft_reset() and acx00_ephy_power_on_and_resume() ->
genphy_resume()) then start Clause 22 accesses to the link PHY right away.

The AC300 backend, driving the same PHY block, treats the equivalent steps
as timed:

	/* Wait for the power-on reset interval specified by the manual. */
	fsleep(10000);
	...
	ret = ac300_ephy_ctl_write(priv, AC300_EPHY_CONFIG_REG,
				   ac300_ephy_ctl_config(priv) |
				  AC300_EPHY_SHUTDOWN);
	...
	fsleep(10000);

Is the asymmetry intentional, i.e. does the AC200 datasheet specify no
power-on-reset interval and no settling time around the shutdown
transition? I could not confirm the AC200 timing from anything in tree.

[ ... ]

> +	ac200_node = of_parse_phandle(package_node, "x-powers,ac200", 0);
> +	if (!ac200_node)
> +		return ERR_PTR(dev_err_probe(dev, -EINVAL,
> +					     "missing x-powers,ac200 reference\n"));
> +	client = of_find_i2c_device_by_node(ac200_node);
> +	of_node_put(ac200_node);
> +	if (!client) {
> +		ret = IS_ENABLED(CONFIG_I2C) ? -EPROBE_DEFER : -ENODEV;
> +		return ERR_PTR(dev_err_probe(dev, ret,
> +					     "AC200 device is not registered\n"));
> +	}
> +
> +	if (!device_link_add(dev, &client->dev,
> +			     DL_FLAG_AUTOREMOVE_CONSUMER)) {
> +		ret = dev_err_probe(dev, -EINVAL,
> +				    "failed to link AC200 device\n");
> +		goto out_put_client;
> +	}
> +
> +	device_lock(&client->dev);
> +	if (device_is_bound(&client->dev))
> +		priv->regmap = dev_get_regmap(&client->dev, NULL);
> +	device_unlock(&client->dev);
> +	if (!priv->regmap) {
> +		ret = dev_err_probe(dev, -EPROBE_DEFER,
> +				    "AC200 driver is not ready\n");
> +		goto out_put_client;
> +	}

[Severity: Low]

Can this gate ever be satisfied in mainline?

The AC200 path requires that the I2C device referenced by the
x-powers,ac200 phandle is bound to a driver which has registered a regmap.
Searching the tree at this commit, x-powers,ac200 matches only the new
binding and the two new driver files, there is no AC200 companion-IC driver
(nothing in drivers/mfd), and no DTS instantiates such a node. The only
other AC200-ish DTS matches are unrelated (regulator-name = "vcc-ac200",
allwinner,sun55i-a523-gmac200).

So on any DT using x-powers,ac200-ephy-package or
x-powers,acx00-ephy-package with an AC200, of_i2c_register_devices() will
create the i2c_client, but device_is_bound() can never become true and the
probe defers forever. Everything below this point (the 0x0014, 0x0016,
0x6000 and 0x8004 register contract, the eFuse calibration read, the clock
rate selection and the whole power sequence) is then unreachable.

The Kconfig help text says:

	  Enable the Fast Ethernet PHY driver shared by the X-Powers AC200
	  and AC300 companion ICs.

and the commit message says:

    Firmware must make the selected AC200 provider path available
    before the PHY probes.

Would it be more accurate to state that the AC200 half depends on a
companion-IC MFD driver that is not merged yet, or to post that driver in
the same series so the AC200 code is testable?

> +
> +	if (!has_calibration) {
> +		ret = regmap_read(priv->regmap, AC200_EFUSE_EPHY_REG,
> +				  &internal_calibration);
> +		if (ret)
> +			goto out_error;
> +		calibration = internal_calibration;
> +	}

[ ... ]

> +	clk = clk_get(&client->dev, NULL);
> +	if (IS_ERR(clk)) {
> +		ret = PTR_ERR(clk);
> +		goto out_error;
> +	}
> +
> +	clk_rate = clk_get_rate(clk);
> +	clk_put(clk);
> +
> +	switch (clk_rate) {
> +	case AC200_EPHY_CLK_RATE_24_MHZ:
> +		priv->ephy_ctl |= AC200_EPHY_CLK_SEL_24_MHZ;
> +		break;
> +	case AC200_EPHY_CLK_RATE_27_MHZ:
> +		break;
> +	default:
> +		ret = dev_err_probe(dev, -EINVAL,
> +				    "unsupported AC200 clock rate %lu Hz\n",
> +				    clk_rate);
> +		goto out_put_client;
> +	}

[Severity: Medium]

Can the sampled rate go stale here?

ac200_ephy_ctl_create() reads the rate once, folds the result into
priv->ephy_ctl as AC200_EPHY_CLK_SEL_24_MHZ, drops the clock reference, and
every later ac200_ephy_ctl_power_on() re-writes that latched value. There is
no devm_clk_rate_exclusive_get() and no clk_prepare_enable() anywhere in
ac200.c, so if another consumer calls clk_set_rate() on the shared provider
after probe, CLK_SEL no longer describes the frequency reaching the EPHY and
the link timing is off with no error reported.

The AC300 backend handles the identical constraint the other way:

	ret = devm_add_action_or_reset(dev, ac300_ephy_clk_put, priv->clk);
	...
	ret = devm_clk_rate_exclusive_get(dev, priv->clk);

plus clk_prepare_enable() and clk_disable_unprepare() around the powered
state. Should the AC200 path pin the rate the same way? Related, the commit
message says:

    Obtain and manage the selected package supply, calibration and input
    clock

which as written seems to hold only for the AC300 backend. I could not check
whether the (unmerged) AC200 companion driver is expected to own and enable
that clock instead.

[Severity: Low]

Where is this AC200 clock requirement documented?

The package binding describes clocks as "AC300 input clock, running at 24,
25, or 27 MHz", requires it for x-powers,ac300-ephy-package and
x-powers,acx00-ephy-package, and forbids it for
x-powers,ac200-ephy-package. But the AC200 backend never looks at the
package clocks; it does clk_get(&client->dev, NULL) on the AC200 companion
I2C device and fails probe with -EINVAL unless that clock reports exactly
24 MHz or 27 MHz (and propagates the clk_get() error if the companion node
has no clock at all).

So a DT author has no in-tree description of the AC200 companion clock or
its permitted rates. Could the binding for the AC200 companion node (or at
least the package binding text) state that constraint?

> diff --git a/drivers/net/phy/xpowers/acx00.c b/drivers/net/phy/xpowers/acx00.c
> new file mode 100644
> index 0000000000000..98f2b0b639096
> --- /dev/null
> +++ b/drivers/net/phy/xpowers/acx00.c
> @@ -0,0 +1,536 @@

[ ... ]

> +static int acx00_ephy_soft_reset(struct phy_device *phydev)
> +{
> +	struct acx00_ephy_priv *priv = phydev->priv;
> +	int ret;
> +
> +	ret = acx00_ephy_set_interface(phydev);
> +	if (ret)
> +		return ret;
> +
> +	ret = acx00_ephy_control_power_on(priv);
> +	if (ret)
> +		return ret;
> +
> +	/* ACx00 can acknowledge reset in power-down without restarting. */
> +	ret = genphy_resume(phydev);
> +	if (ret)
> +		return ret;
> +
> +	return genphy_soft_reset(phydev);
> +}

[ ... ]

> +out_restore_page:
> +	ret = phy_restore_page(phydev, oldpage, ret);
> +	if (ret)
> +		return ret;
> +
> +	/* Restore the standard EEE policy retained by phylib across resets. */
> +	return genphy_c45_an_config_eee_aneg(phydev);
> +}

[Severity: Medium]

Should this positive return value be consumed rather than propagated?

genphy_c45_an_config_eee_aneg() returns 1 when it actually changed an EEE
advertisement register, because genphy_c45_write_eee_adv() returns changed:

drivers/net/phy/phy-c45.c:genphy_c45_write_eee_adv() {
	...
		if (val > 0)
			changed = 1;
	...
	return changed;
}

acx00_ephy_config_init() forwards that 1 to acx00_ephy_power_on_and_resume()
and from there to acx00_ephy_resume(), which is the .resume callback. In
phylib:

drivers/net/phy/phy_device.c:__phy_resume() {
	...
	ret = phydrv->resume(phydev);
	if (!ret)
		phydev->suspended = false;
	...
}

so a fully successful resume that happened to change the advertisement is
reported as an error and leaves phydev->suspended set. phy_suspend() then
short-circuits:

drivers/net/phy/phy_device.c:phy_suspend() {
	if (phydev->suspended || !phydrv)
		return 0;
	...
}

which means acx00_ephy_suspend(), the only path that powers the package
down, is skipped for the rest of the device's life. Note phy_init_hw() does
not always mask this, since phylink_prepare_resume() calls phy_resume()
directly when phydev->suspended is set.

Separately, if the advertisement did change, does this path need to restart
autonegotiation for the new advertisement?

> +
> +static int acx00_ephy_power_on_and_resume(struct phy_device *phydev)
> +{
> +	struct acx00_ephy_priv *priv = phydev->priv;
> +	int ret;
> +
> +	ret = acx00_ephy_set_interface(phydev);
> +	if (ret)
> +		return ret;
> +
> +	ret = acx00_ephy_control_power_on(priv);
> +	if (ret)
> +		return ret;
> +
> +	ret = genphy_resume(phydev);
> +	if (ret) {
> +		acx00_ephy_control_power_off(priv);
> +		return ret;
> +	}
> +
> +	/* Powering off the control block loses the vendor-page state. */
> +	return acx00_ephy_config_init(phydev);
> +}

[Severity: Medium]

Does the acx00_ephy_config_init() error path leave the package powered?

The genphy_resume() failure branch calls acx00_ephy_control_power_off(), but
a failure in the trailing acx00_ephy_config_init() returns with
priv->powered and priv->supply_enabled still true. acx00_ephy_soft_reset()
above has the same shape: it powers on, then returns genphy_resume() or
genphy_soft_reset() errors without a matching power off.

Because phylib only clears the suspended flag after a successful
.soft_reset:

drivers/net/phy/phy_device.c:phy_init_hw() {
	if (phydev->drv->soft_reset) {
		ret = phydev->drv->soft_reset(phydev);
		if (ret < 0)
			return ret;

		/* see comment in genphy_soft_reset for an explanation */
		phydev->suspended = 0;
	}
	...
}

and mdio_bus_phy_resume() aborts before phy_resume() when phy_init_hw()
fails, a single failed MDIO or I2C transaction on this path leaves
phydev->suspended set. phy_suspend() then returns early on that flag, so
the package supply (and the AC300 input clock enabled via
clk_prepare_enable()) stays on for all later suspend cycles until unbind
runs the devres release action. Should both paths power the control block
back off on failure?

> +
> +static int acx00_ephy_resume(struct phy_device *phydev)
> +{
> +	return acx00_ephy_power_on_and_resume(phydev);
> +}
> +
> +static int acx00_ephy_suspend(struct phy_device *phydev)
> +{
> +	struct acx00_ephy_priv *priv = phydev->priv;
> +	int resume_ret;
> +	int ret;
> +
> +	ret = genphy_suspend(phydev);
> +	if (ret)
> +		return ret;
> +
> +	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));
> +	}
> +
> +	return ret;
> +}

[Severity: Medium]

Is it safe to run the resume sequence from the .suspend callback?

acx00_ephy_power_on_and_resume() performs genphy_resume() (BMCR modify) and
acx00_ephy_config_init(), which does phy_select_page() plus paged vendor
writes and then genphy_c45_an_config_eee_aneg(), reading phydev->autoneg,
phydev->eee_cfg.eee_enabled and phydev->advertising_eee.

phylib requires phydev->lock for the resume operation:

drivers/net/phy/phy_device.c:__phy_resume() {
	lockdep_assert_held(&phydev->lock);
	...
}

but .suspend runs with no phylib lock held (phy_suspend() takes none, and
_phy_state_machine_post_work() invokes it after phy_state_machine() has
dropped the lock). So when the control-block write fails, this recovery
executes resume code unlocked, concurrently with phydev->lock holders such
as the state machine or phy_ethtool_set_eee(), which write the same fields
and registers it reads and rewrites. Should the recovery be restricted to
the control-block power-on, or moved to a context that holds phydev->lock?