Re: [PATCH v2 net-next 1/7] net: dsa: mt7530: get ctrl phy addr using a function

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

Critical: 0 · High: 0 · Medium: 0 · Low: 2

- [Low] mt753x_ctrl_phy_addr() (drivers/net/dsa/mt7530.c:49-57) reports
  the !priv->mdiodev case only via WARN_ON_ONCE() and then returns 0 — a…
- [Low] The commit message states the control-PHY address is found by
  'taking the address of mdiodev (the switch) add 1 and modulo 31', but…

--

net: dsa: mt7530: get ctrl phy addr using a function

This replaces the MT753X_CTRL_PHY_ADDR() macro with a new helper,
mt753x_ctrl_phy_addr(), which warns and falls back when priv->mdiodev is
NULL. The intent is to give MMIO based MT7530 implementations a place to
supply their own control PHY address.

A couple of questions below.

    The MDIO MT7530 has 5 integrated PHYs, of these, the first PHY has
    the additional registers that are used for such things as configuring
    the clock configuration for the switch. The switch is typically on
    MDIO address 31 and the PHYs are 0, 1, 2, 3, and 4 with 0 being the
    special one. So taking the address of mdiodev (the switch) add 1 and
    modulo 31 finds the first PHY.

[Severity: Low]
Should "modulo 31" read "modulo 32" here?  The helper computes:

	return (priv->mdiodev->addr + 1) & (PHY_MAX_ADDR - 1);

and include/linux/phy.h has:

	#define PHY_MAX_ADDR	32

so the mask is 31 and the arithmetic is modulo 32.  For the typical strap
described in the message, (31 + 1) % 32 == 0, which is the first integrated
PHY, whereas (31 + 1) % 31 == 1.  Perhaps "modulo 32", or "masked with 31",
would describe it better.

[Severity: Low]
Is 0 a usable way to signal the !priv->mdiodev case?  0 is also the normal
result for a switch strapped at MDIO address 31, so callers cannot tell the
two apart, and none of the three call sites checks the return value.

There is a related question about what happens after the warning fires.
priv->bus and priv->mdiodev are only assigned together in mt7530-mdio.c,
while mt7530-mmio.c mt7988_probe() does:

	priv->bus = NULL;

and never sets priv->mdiodev.  So the WARN_ON_ONCE() branch and a NULL
priv->bus are the same condition, and in core_write()/core_rmw() at this
commit the warning falls through into the bus dereference below rather than
aborting.  Note mt7530_mutex_lock()/mt7530_mutex_unlock() in the same path
do guard on priv->bus.

I could not find an in-tree path that reaches the warning at this commit:
core_write()/core_rmw() are reached from mt7530_pll_setup() and
mt7530_setup_port5/6() (ID_MT7530/ID_MT7621), and mt7531_setup() runs for
ID_MT7531, all of which are probed through mt7530-mdio.c.  Later in the
series, "net: dsa: mediatek: support PLL setup on MMIO MT7530" adds

	if (!bus)
		bus = priv->child_bus;

	if (WARN_ON_ONCE(!bus))
		return;

to both functions, and mt753x_ctrl_phy_addr() gains an ID_EN751221 case
returning 12, so both aspects are handled inside the same series.  Would it
still be preferable to return a negative errno here and check it at the call
sites, so the fallback value is not a valid PHY address?

[ ... ]

[ ... ]

Same question here: ctl_phy is used unchecked as the address for the
CORE_PLL_GROUP4 read/write and as the base of the EEE advertisement loop,
so a fallback of 0 would silently target PHY address 0 through 4.

[ ... ]

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