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

[netdev-bot+sashiko]

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.

> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 14692497e1a61..54d931002bc14 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -46,6 +46,16 @@ static const struct mt7530_mib_desc mt7530_mib[] = {
>  	MIB_DESC(1, MT7530_PORT_MIB_RX_ARL_DROP, "RxArlDrop"),
>  };
>  
> +static int
> +mt753x_ctrl_phy_addr(struct mt7530_priv *priv)
> +{
> +	if (WARN_ON_ONCE(!priv->mdiodev))
> +		return 0;
> +
> +	/* Default is 1st PHY */
> +	return (priv->mdiodev->addr + 1) & (PHY_MAX_ADDR - 1);
> +}
> +

[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?

>  static void
>  mt7530_mutex_lock(struct mt7530_priv *priv)
>  {
> @@ -63,32 +73,30 @@ mt7530_mutex_unlock(struct mt7530_priv *priv)
>  static void
>  core_write(struct mt7530_priv *priv, u32 reg, u32 val)
>  {
> +	int ctl_phy = mt753x_ctrl_phy_addr(priv);
>  	struct mii_bus *bus = priv->bus;
>  	int ret;
>  
>  	mt7530_mutex_lock(priv);
>  
>  	/* Write the desired MMD Devad */
> -	ret = bus->write(bus, MT753X_CTRL_PHY_ADDR(priv->mdiodev->addr),
> -			 MII_MMD_CTRL, MDIO_MMD_VEND2);
> +	ret = bus->write(bus, ctl_phy, MII_MMD_CTRL, MDIO_MMD_VEND2);
>  	if (ret < 0)
>  		goto err;

[ ... ]

> @@ -99,6 +107,7 @@ core_write(struct mt7530_priv *priv, u32 reg, u32 val)
>  static void
>  core_rmw(struct mt7530_priv *priv, u32 reg, u32 mask, u32 set)
>  {
> +	int ctl_phy = mt753x_ctrl_phy_addr(priv);
>  	struct mii_bus *bus = priv->bus;
>  	u32 val;
>  	int ret;

[ ... ]

> @@ -2674,8 +2679,11 @@ mt7531_setup(struct dsa_switch *ds)
>  {
>  	struct mt7530_priv *priv = ds->priv;
>  	u32 val, id;
> +	int ctl_phy;
>  	int ret, i;
>  
> +	ctl_phy = mt753x_ctrl_phy_addr(priv);
> +

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