Re: [PATCH v2 net-next 4/7] net: dsa: mediatek: support PLL setup on MMIO MT7530

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] On the path this patch newly enables (priv->bus == NULL, so
  bus = priv->child_bus), core_write() and core_rmw() execute a 4-5…
- [Low] Ordering of the new priv->child_bus producer vs. its consumers:
  priv->child_bus is assigned only in mt7530_setup_mdio()…
- [Low] The commit subject/message ('net: dsa: mediatek: support PLL
  setup on MMIO MT7530', 'begin using it for core switch PLL setup')…
- [Low] core_write() and core_rmw() compute `int ctl_phy =
  mt753x_ctrl_phy_addr(priv);` in the declaration initializer, before…
- [Low] The new kernel-doc for the @child_bus member of struct
  mt7530_priv is ungrammatical and does not describe the field's actual…

Pre-existing issues:
- [High] Pre-existing race on the MDIO path that this patch extends
  without addressing: core_write()/core_rmw() drive the control PHY's…

--

net: dsa: mediatek: support PLL setup on MMIO MT7530

This patch adds a new priv->child_bus pointer that records the MDIO bus
the switch driver itself creates in mt7530_setup_mdio(), and makes
core_write() and core_rmw() fall back to it when priv->bus is NULL, so
core (PLL) register access can work on MMIO-probed switches.

No Link: tags are present in the commit header.

The commit message says:

  In an MMIO implementation, the MDIO bus is part of the switch itself.
  This bus is already setup for PHY calibration so begin using it for
  core switch PLL setup.

[Severity: Low]
Is there any device in the tree at this commit that can reach the new
fallback?  mt7530-mmio.c matches airoha,an7583-switch,
airoha,en7581-switch, econet,en7528-switch and mediatek,mt7988-switch,
and all of those IDs use .sw_setup = mt7988_setup, which contains no
core_write()/core_rmw() call, and none of them has a .mac_port_config
that reaches mt7530_setup_port5()/mt7530_setup_port6().

The first MMIO consumer appears only with the later patch
"net: dsa: mediatek: support EN751221 switch", which adds ID_EN751221
with .mac_port_config = mt7530_mac_config and the control PHY address
for MMIO (if (priv->id == ID_EN751221) return 12;).

Would it be clearer to state in the message that this is a prerequisite
for the EN751221 MMIO variant, and that the MMIO control PHY address is
still missing here?

> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index b2344808457cb..b152b73315d50 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -77,6 +77,12 @@ core_write(struct mt7530_priv *priv, u32 reg, u32 val)
>  	struct mii_bus *bus = priv->bus;
>  	int ret;
>  
> +	if (!bus)
> +		bus = priv->child_bus;
> +
> +	if (WARN_ON_ONCE(!bus))
> +		return;
> +

[Severity: Low]
On the instances this fallback newly enables, what PHY address does the
MMD sequence end up using?  core_write() computes ctl_phy in the
declaration initializer, before the fallback runs:

	int ctl_phy = mt753x_ctrl_phy_addr(priv);
	struct mii_bus *bus = priv->bus;

and at this commit mt753x_ctrl_phy_addr() has no MMIO case:

	if (WARN_ON_ONCE(!priv->mdiodev))
		return 0;

	/* Default is 1st PHY */
	return (priv->mdiodev->addr + 1) & (PHY_MAX_ADDR - 1);

mt7988_probe() in mt7530-mmio.c sets priv->bus = NULL and never sets
priv->mdiodev, so an MMIO caller would hit that WARN and then issue the
whole vendor MMD sequence to child bus address 0, which is an ordinary
port PHY rather than the control PHY.

This is not reachable at this commit (no MMIO ID calls core_write() or
core_rmw()), and the later patch "net: dsa: mediatek: support EN751221
switch" adds the missing case for the only MMIO consumer.  Would it be
worth adding the MMIO control PHY handling in the same patch that adds
the bus fallback, so the two never appear apart?

[Severity: High]
This isn't a bug introduced by this patch, but since the same helper is
now extended to a second bus, would it make sense to fix the locking
once for both paths?

On the existing MDIO path, core_write() drives the control PHY MMD
window while holding only the parent bus lock:

	mt7530_mutex_lock(priv);   /* priv->bus->mdio_lock */
	ret = bus->write(bus, ctl_phy, MII_MMD_CTRL, MDIO_MMD_VEND2);
	ret = bus->write(bus, ctl_phy, MII_MMD_DATA, reg);
	ret = bus->write(bus, ctl_phy, MII_MMD_CTRL,
			 MDIO_MMD_VEND2 | MII_MMD_CTRL_NOINCR);
	ret = bus->write(bus, ctl_phy, MII_MMD_DATA, val);

That address, (priv->mdiodev->addr + 1) & (PHY_MAX_ADDR - 1), is the
same physical PHY that phylib manages as user port 0.  phylib's own
indirect MMD access in mmd_phy_indirect() holds the child bus lock, and
picks up the parent lock only per transfer, because the child bus ops go
through mt7530_phy_read_c22()/mt7530_phy_write_c22() ->
mdiobus_read_nested()/mdiobus_write_nested(priv->bus, ...).

Can the whole core_write() sequence then land between two steps of
phylib's sequence?  It leaves MII_MMD_CTRL set to
MDIO_MMD_VEND2 | MII_MMD_CTRL_NOINCR with a CORE_* register latched in
MII_MMD_DATA, so phylib's next MII_MMD_DATA access would write its own
value into that CORE_* register (CORE_PLL_GROUP7 or
CORE_TRGMII_GSW_CLK_CG, for example), or read switch PLL state instead
of EEE ability.

Both contexts run concurrently: mt7530_setup_port6() is reached from
mt753x_phylink_mac_config() in the phylink resolve worker, which holds
only pl->phydev_mutex and pl->state_mutex, while phy_check_link_status()
issues genphy_c45_eee_is_active() -> phy_read_mmd() on link up.

Would taking the PHY's own bus lock (the child bus mdio_lock) around the
MMD sequence, before the nested parent lock, be the right fix here?

>  	mt7530_mutex_lock(priv);
>  
>  	/* Write the desired MMD Devad */
> @@ -112,6 +118,12 @@ core_rmw(struct mt7530_priv *priv, u32 reg, u32 mask, u32 set)
>  	u32 val;
>  	int ret;
>  
> +	if (!bus)
> +		bus = priv->child_bus;
> +
> +	if (WARN_ON_ONCE(!bus))
> +		return;
> +
>  	mt7530_mutex_lock(priv);

[Severity: Medium]
When the child bus is used, is anything serializing the multi-step MMD
window?  mt7530_mutex_lock()/mt7530_mutex_unlock() are no-ops in exactly
that case:

static void
mt7530_mutex_lock(struct mt7530_priv *priv)
{
	if (priv->bus)
		mutex_lock_nested(&priv->bus->mdio_lock, MDIO_MUTEX_NESTED);
}

So with priv->bus == NULL the four writes in core_write(), and the five
transactions of the core_rmw() read-modify-write:

	ret = bus->read(bus, ctl_phy, MII_MMD_DATA);
	...
	val &= ~mask;
	val |= set;
	ret = bus->write(bus, ctl_phy, MII_MMD_DATA, val);

run with no lock covering the sequence, and the direct bus->write() /
bus->read() calls also bypass child_bus->mdio_lock, so the
lockdep_assert_held_once(&bus->mdio_lock) in __mdiobus_write() cannot
flag it.  The atomicity the MDIO path got from holding the parent bus
lock across the whole sequence is dropped for the MMIO path.

Individual transfers do stay atomic on MMIO (mt7531_ind_c22_phy_read()
and mt7531_ind_c22_phy_write() take priv->reg_mutex), so what is exposed
is the multi-step window on the control PHY: a second accessor of that
PHY address would see a read return, or a write land in, the wrong
CORE_* register.  I could not find such a second accessor for the
in-series EN751221 consumer, so the corruption itself is speculative,
but the dropped lock is not.

Would locking the bus that was actually selected work here, that is
mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED) plus
__mdiobus_read()/__mdiobus_write(), the way other DSA drivers do it?

>  
>  	/* Write the desired MMD Devad */
> @@ -2429,8 +2441,11 @@ mt7530_setup_mdio(struct mt7530_priv *priv)
>  	if (priv->irq_domain && !mnp)
>  		mt7530_setup_mdio_irq(priv);
>  
> +	priv->child_bus = bus;
> +
>  	ret = devm_of_mdiobus_register(dev, bus, mnp);
>  	if (ret) {
> +		priv->child_bus = NULL;
>  		dev_err(dev, "failed to register MDIO bus: %d\n", ret);
>  		if (priv->irq_domain && !mnp)
>  			mt7530_free_mdio_irq(priv);

[Severity: Low]
Is the producer of priv->child_bus ordered ahead of its consumers?
mt753x_setup() runs the switch specific setup first and only afterwards
creates the bus:

	int ret = priv->info->sw_setup(ds);
	...
	ret = mt7530_setup_mdio(priv);

So any core_write()/core_rmw() issued from an sw_setup callback on an
instance where priv->bus is NULL (mt7988_probe() sets priv->bus = NULL)
finds both pointers NULL, trips the new WARN_ON_ONCE(!bus) and returns
without performing the MMD sequence.  Should the bus be created before
sw_setup(), or should core_* return an error the caller can act on?

There is also a case where the pointer stays NULL for the whole device
lifetime.  mt7530_setup_mdio() returns 0 without allocating a bus when a
disabled mdio child node is present:

	if (mnp && !of_device_is_available(mnp))
		goto out;

After that, every runtime core_* access (the CORE_PLL_GROUP* and
CORE_TRGMII_GSW_CLK_CG programming in mt7530_setup_port6(), for example)
becomes a warning plus a no-op rather than a probe failure.  Is silently
skipping the clock programming the intended behaviour there?

For completeness, the path named in the commit message,
mt7530_setup() -> mt7530_pll_setup() -> core_clear()/core_write(), is
not reachable on a bus-less instance in this tree: mt7530_setup() is
.sw_setup only for the MDIO probed IDs, where priv->bus is a real parent
bus.  So this reads as a latent ordering trap plus the disabled mdio node
corner case rather than a failure any current board hits.

> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
> index 108c831dd0120..5b7394f6383c7 100644
> --- a/drivers/net/dsa/mt7530.h
> +++ b/drivers/net/dsa/mt7530.h
> @@ -886,6 +886,9 @@ struct mt753x_info {
>   * @dev:		The device pointer
>   * @ds:			The pointer to the dsa core structure
>   * @bus:		The bus used for the device and built-in PHY
> + * @child_bus:		The bus created by the device, on MDIO implementations
> + *			this is bus indirectly accesses `bus`, on MMIO
> + *			implementations this bus is part of the switch.

[Severity: Low]
The middle clause, "this is bus indirectly accesses `bus`", looks like it
is missing a word or two.

Also, "The bus created by the device" does not match the code:
mt7530_setup_mdio() returns 0 without allocating a bus when a disabled
mdio child node exists, and it resets the pointer back to NULL on
registration failure, so the field can legitimately be NULL for the
whole device lifetime.

Could the doc state when the field may be NULL, and which of priv->bus,
priv->child_bus and ds->user_mii_bus the core_* helpers are expected to
use?  The selection rule (if (!bus) bus = priv->child_bus;) is currently
copy-pasted into core_write() and core_rmw(), so every future core_*
helper has to remember to repeat it.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915174643.253660-1-cjd%40cjdns.fr