Re: [PATCH net 2/2] net: dsa: mt7530: unregister the switch before freeing its MDIO IRQs

[netdev-bot+sashiko@kernel.org]

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

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

- [High] Incomplete fix: mt7530_remove_common()
  (drivers/net/dsa/mt7530.c:3593-3602) still calls…

Pre-existing issues:
- [Medium] Power-sequencing ordering on the MDIO front end:
  mt7530_remove() in drivers/net/dsa/mt7530-mdio.c disables…

--

net: dsa: mt7530: unregister the switch before freeing its MDIO IRQs

This moves dsa_unregister_switch() ahead of the per-PHY interrupt
disposal in mt7530_remove_common(), so that phylib frees the nested PHY
interrupts before their mappings are destroyed. The reported symptom is a
remove_proc_entry() warning from irq_free_descs() followed by a NULL
dereference in handle_nested_irq().

> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 3e61eb3c2b1e2..90fd04665ebf1 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -3593,11 +3593,11 @@ EXPORT_SYMBOL_GPL(mt7530_probe_common);
>  void
>  mt7530_remove_common(struct mt7530_priv *priv)
>  {
> +	dsa_unregister_switch(priv->ds);
> +
>  	if (priv->irq_domain)
>  		mt7530_free_mdio_irq(priv);

[Severity: High]
Does this reorder actually close the handle_nested_irq() fault described
in the changelog, or does it only close the phylib half of it?

The parent threaded interrupt is owned by devres.  mt7530_setup_irq()
registers the chip with devm_regmap_add_irq_chip_fwnode() and keeps only
the domain pointer:

drivers/net/dsa/mt7530.c:mt7530_setup_irq() {
	ret = devm_regmap_add_irq_chip_fwnode(dev, dev_fwnode(dev),
					      priv->regmap, irq,
					      IRQF_ONESHOT,
					      0, chip,
					      &irq_data);
	...
	priv->irq_domain = regmap_irq_get_domain(irq_data);
}

so free_irq() on the parent and irq_domain_remove() happen only from the
devres release, devm_regmap_irq_chip_release() -> regmap_del_irq_chip(),
which runs after mt7530_remove() has returned.  Nothing in
mt7530_remove_common() disables, frees or synchronizes against that
parent IRQ, so the regmap-irq thread is still live while
mt7530_free_mdio_irq() destroys the mappings.

On the thread side there is no zero check on the lookup:

drivers/base/regmap/regmap-irq.c:regmap_irq_thread() {
	...
		data->status_buf[i] &= ~data->mask_buf[i];
	...
			handle_nested_irq(irq_find_mapping(data->domain, i));
}

mask_buf is read there without data->lock, so the thread can carry a
pre-mask snapshot past the filter.  Can the following still happen with
this patch applied, on a part with a PHY_LC bit latched?

  regmap_irq_thread()			mt7530_remove_common()
    read_irq_data(), PHY0_LC set
    passes the mask_buf filter
					dsa_unregister_switch()
					  phy_disconnect()
					    phy_free_interrupt()
					      free_irq(nested virq)
					mt7530_free_mdio_irq()
					  irq_dispose_mapping()
    irq_find_mapping() -> 0
    handle_nested_irq(0)

kernel/irq/chip.c:handle_nested_irq() has no NULL check:

	struct irq_desc *desc = irq_to_desc(irq);
	...
	scoped_guard(raw_spinlock_irq, &desc->lock) {

which is the read at handle_nested_irq+0xac quoted in the changelog.

Is the driver-side disposal loop needed at all?  regmap_del_irq_chip()
already walks the domain and disposes every mapping, and it does so in
the safe order:

drivers/base/regmap/regmap-irq.c:regmap_del_irq_chip() {
	free_irq(irq, d);

	/* Dispose all virtual irq from irq domain before removing it */
	...
}

Would dropping mt7530_free_mdio_irq() from the remove path (leaving the
disposal to regmap_del_irq_chip(), which runs after the parent IRQ is
freed) be a better fix than moving it after dsa_unregister_switch()?

> -	dsa_unregister_switch(priv->ds);
> -
>  	mutex_destroy(&priv->reg_mutex);
>  }
>  EXPORT_SYMBOL_GPL(mt7530_remove_common);

[Severity: Medium]
This is a pre-existing ordering problem and not something this patch
introduces, but it interacts with the new placement of
dsa_unregister_switch(), so it may be worth mentioning.

On the MDIO front end the supplies are dropped before any of this runs:

drivers/net/dsa/mt7530-mdio.c:mt7530_remove() {
	if (priv->id == ID_MT7530) {
		ret = regulator_disable(priv->core_pwr);
		...
		ret = regulator_disable(priv->io_pwr);
		...
	}

	mt7530_remove_common(priv);
}

After this patch the whole register-level teardown reached from
dsa_unregister_switch() - internal PHY power-down, port disable writes,
and the regmap-irq mask sync that clears the PHY_LC bits through
mt7530_irq_mask_sync() - is the first thing mt7530_remove_common() does,
i.e. entirely after core_pwr and io_pwr were released on ID_MT7530.

Can those writes still reach the chip?  MDIO writes are unacknowledged,
and regmap-irq updates its mask_buf shadow regardless of what
handle_mask_sync() returns:

drivers/net/dsa/mt7530.c:mt7530_irq_mask_sync() {
	ret = regmap_update_bits(priv->regmap, MT7530_SYS_INT_EN,
				 mask_buf_def, ~mask_buf);
	...
}

so MT7530_SYS_INT_EN can be left armed and MT7530_SYS_INT_STS latched
while software believes they are masked.  MT7530_TOP_SIG_CTRL's
TOP_SIG_CTRL_NORMAL bit, set in mt7530_setup_irq(), likewise has no
teardown counterpart.  Should the regulator_disable() calls move after
mt7530_remove_common() instead?

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