[PATCH net v2 1/2] net: dsa: mt7530: fix NULL dereference on unbind of MT7531 and MT7621

[netdev-bot+sashiko at kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 (2) · Low: 0 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The regulator enable reference is taken in the DSA `.setup`
  callback (mt7530_setup(), drivers/net/dsa/mt7530.c:2493-2509) but…

Pre-existing issues:
- [Medium] On the ID_MT7530 path the patch keeps
  regulator_disable(core_pwr)/regulator_disable(io_pwr) ahead of…
- [Medium] In mt7530_setup() (drivers/net/dsa/mt7530.c:2494 and :2502)
  the return values of `regulator_set_voltage(priv->core_pwr, 1000000,…
- [Low] Binding/driver disagreement about which compatibles the `core-
  supply`/`io-supply` properties apply to.

--

net: dsa: mt7530: fix NULL dereference on unbind of MT7531 and MT7621

The core and io supplies are only requested for ID_MT7530, but
mt7530_remove() disabled them unconditionally, so unbinding an MT7531 or
MT7621 called regulator_disable() on a NULL pointer.  The patch gates the
two regulator_disable() calls on priv->id == ID_MT7530.

> diff --git a/drivers/net/dsa/mt7530-mdio.c b/drivers/net/dsa/mt7530-mdio.c
> index 784dd58a71589..de42f70afcfa1 100644
> --- a/drivers/net/dsa/mt7530-mdio.c
> +++ b/drivers/net/dsa/mt7530-mdio.c
> @@ -227,15 +227,17 @@ mt7530_remove(struct mdio_device *mdiodev)
>  	if (!priv)
>  		return;
>  
> -	ret = regulator_disable(priv->core_pwr);
> -	if (ret < 0)
> -		dev_err(priv->dev,
> -			"Failed to disable core power: %d\n", ret);
> -
> -	ret = regulator_disable(priv->io_pwr);
> -	if (ret < 0)
> -		dev_err(priv->dev, "Failed to disable io pwr: %d\n",
> -			ret);
> +	if (priv->id == ID_MT7530) {

[Severity: Low]
This isn't a bug introduced by this patch, but the id test here is now the
third place that encodes "these supplies belong to ID_MT7530", while the
binding does not say that.

In Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml both
core-supply and io-supply are defined as generic top-level properties and
are only listed under required: for the mediatek,mt7530 branch.  The
mediatek,mt7531 branch disallows other properties explicitly:

      properties:
        gpio-controller: false
        mediatek,mcm: false

but leaves the two supplies permitted, and the mediatek,mt7621 branch only
adds mediatek,mcm to required:.

So a schema-valid DT with core-supply on compatible = "mediatek,mt7531" has
that phandle silently ignored, since mt7530_probe() never calls
devm_regulator_get() for any other id:

	if (priv->id == ID_MT7530) {
		priv->core_pwr = devm_regulator_get(&mdiodev->dev, "core");
		...
	}

Should the binding set core-supply: false / io-supply: false on the
mt7531 and mt7621 branches so the two sides agree?

> +		ret = regulator_disable(priv->core_pwr);
> +		if (ret < 0)
> +			dev_err(priv->dev,
> +				"Failed to disable core power: %d\n", ret);
> +
> +		ret = regulator_disable(priv->io_pwr);
> +		if (ret < 0)
> +			dev_err(priv->dev, "Failed to disable io pwr: %d\n",
> +				ret);
> +	}

[Severity: Medium]
Is priv->id the right condition for the disable side?  It records that the
supplies were requested in mt7530_probe(), not that they were ever enabled.
The enable happens in a different callback, mt7530_setup() in mt7530.c,
reached through ds->ops->setup.

Can this produce an unbalanced disable on an ID_MT7530 board?
mt7530_probe() -> dsa_register_switch() -> dsa_tree_setup() can return
success without ever running ds->ops->setup:

net/dsa/dsa.c:dsa_tree_setup() {
	...
	complete = dsa_tree_setup_routing_table(dst);
	if (!complete)
		return 0;
	...
}

A later unbind then takes this new ID_MT7530 branch with
regulator->enable_count still zero:

drivers/regulator/core.c:_regulator_disable() {
	if (WARN(regulator->enable_count == 0,
		 "unbalanced disables for %s\n", rdev_get_name(rdev)))
		return -EIO;
	...
}

which also means the dev_err() above prints -EIO.  A tree teardown and
re-setup cycle, e.g. a peer switch in the same tree unbinding and binding
again, runs mt7530_setup() and therefore regulator_enable() more than once
against this single disable.

Does the opposite direction leak the enable reference?  Every error return
in mt7530_setup() after the core rail is up:

	if (priv->id == ID_MT7530) {
		regulator_set_voltage(priv->core_pwr, 1000000, 1000000);
		ret = regulator_enable(priv->core_pwr);
		...
		regulator_set_voltage(priv->io_pwr, 3300000, 3300000);
		ret = regulator_enable(priv->io_pwr);
		if (ret < 0) {
			dev_err(priv->dev, "Failed to enable io pwr: %d\n",
				ret);
			return ret;
		}
	}

	/* Waiting for MT7530 got to stable */
	INIT_MT7530_DUMMY_POLL(&p, priv, MT753X_TRAP);
	ret = readx_poll_timeout(_mt7530_read, &p, val, val != 0,
				 20, 1000000);
	if (ret < 0) {
		dev_err(priv->dev, "reset timeout\n");
		return ret;

propagates out through mt753x_setup() and dsa_register_switch() to
mt7530_probe(), so .remove is never called.  The devm release path only
puts the regulator:

drivers/regulator/core.c:_regulator_put() {
	...
	/* Docs say you must disable before calling regulator_put() */
	WARN_ON(regulator->enable_count);
	...
}

so the 1.0 V core and 3.3 V I/O rails stay enabled for the lifetime of the
system, with a WARN.  The MT7530_CREV id mismatch and the 20 MHz XTAL
rejection reach the same point.

Would pairing the enable with the existing symmetric hook,
mt753x_teardown() in mt7530.c, or tracking an explicit "enabled" flag and
unwinding the mt7530_setup() error paths, cover the NULL case this patch
targets as well as these two?

>  
>  	mt7530_remove_common(priv);
>  

[Severity: Medium]
This is a pre-existing ordering issue rather than something this patch
introduces, and the cover text says as much, but it may be worth recording
here since the ID_MT7530 path keeps it.

Everything mt7530_remove_common() still does runs against a die whose rails
have just been switched off:

drivers/net/dsa/mt7530.c:mt7530_remove_common() {
	dsa_unregister_switch(priv->ds);

	mutex_destroy(&priv->reg_mutex);
}

dsa_unregister_switch() reaches mt753x_teardown(), the per-port
mt7530_port_disable() PCR read-modify-writes such as

	mt7530_rmw(priv, MT7530_PCR_P(port), PCR_MATRIX_MASK, PCR_MATRIX_CLR);

the mac_link_down() PMCR writes and the internal-PHY suspend writes that go
through the switch's own MDIO bus.  Are those writes simply lost at that
point, leaving the driver's bookkeeping describing a state the hardware
never reached?

Can the MIB poller also read an unpowered die?  mt753x_teardown() is the
only place the self-rearming stats work is cancelled:

drivers/net/dsa/mt7530.c:mt753x_teardown() {
	struct mt7530_priv *priv = ds->priv;

	if (priv->bus)
		cancel_delayed_work_sync(&priv->stats_work);
}

and it runs from inside dsa_unregister_switch(), i.e. after the disables
above.  mt7530_stats_poll() re-arms itself unconditionally:

		mt7530_stats_refresh(priv);
		schedule_delayed_work(&priv->stats_work,
				      MT7530_STATS_POLL_INTERVAL);

so it can be doing MDIO reads while mt7530_remove() drops the rails,
folding all-ones reads into the cached per-port counters.

[Severity: Medium]
This is also pre-existing and untouched by the patch, but while in this
area: mt7530_setup() in drivers/net/dsa/mt7530.c discards the return value
of both regulator_set_voltage() calls:

	if (priv->id == ID_MT7530) {
		regulator_set_voltage(priv->core_pwr, 1000000, 1000000);
		ret = regulator_enable(priv->core_pwr);
		...
		regulator_set_voltage(priv->io_pwr, 3300000, 3300000);
		ret = regulator_enable(priv->io_pwr);

If machine constraints do not permit the requested range, or
REGULATOR_CHANGE_VOLTAGE is not allowed, or a second consumer holds a
shared rail at another voltage, the rail is still enabled and the driver
goes on to reset the die, poll MT753X_TRAP and read MT7530_CREV as if the
1.0 V core / 3.3 V I/O operating points had been applied.  Should these two
calls be checked so the failure surfaces as an error instead of as
intermittent register access failures?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918015020.2518315-1-f%40lex.la