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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The new helper mt753x_ctrl_phy_addr()
  (drivers/net/dsa/mt7530.c:49-57) handles a NULL priv->mdiodev by…
- [Low] The commit message says the control PHY is found by taking the
  switch's MDIO address, adding 1, and taking it "modulo 31".

--

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

This commit replaces the MT753X_CTRL_PHY_ADDR() macro with a
mt753x_ctrl_phy_addr() helper. The helper warns and returns 0 when
priv->mdiodev is NULL. It is also meant to give MMIO implementations a
place to supply their own control PHY address.

> 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]
Is "modulo 31" right here? The code computes:

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

PHY_MAX_ADDR is 32, so this masks with 31, which is modulo 32. For a
switch at address 31, (31 + 1) % 31 would give 1. The code gives 0,
which is the PHY the message calls the special one.

Should this say modulo 32, or say that it wraps around the 32-address
MDIO space?

> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 7781a63b4e6fd..d10194488bdee 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;

[Severity: Low]
When priv->mdiodev is NULL, this returns 0. That is also a valid PHY
address, so callers can't tell the fallback from a real result.

At this commit, core_write() and core_rmw() then use priv->bus without
checking it:

    struct mii_bus *bus = priv->bus;
    ...
    ret = bus->write(bus, ctl_phy, MII_MMD_CTRL, MDIO_MMD_VEND2);

On MMIO devices, mt7988_probe() sets priv->bus = NULL and never sets
mdiodev. If the fallback were hit there, could the WARN be followed by a
NULL bus dereference in core_write() or core_rmw()?

In mt7531_setup(), the same fallback would send the CORE_PLL_GROUP4
write and the EEE advertisement writes to PHYs 0 to 4.

This doesn't look reachable at this commit. Every caller runs only for
MT7530/MT7621/MT7531, which are probed through mt7530-mdio.c and always
have both priv->bus and priv->mdiodev set.

Two later patches in the series also cover this:

  - "net: dsa: mediatek: support PLL setup on MMIO MT7530" adds a
    child_bus fallback and a WARN_ON_ONCE(!bus) check to core_write()
    and core_rmw().
  - "net: dsa: mediatek: support EN751221 switch" adds the EN751221
    special case to this helper.

That leaves a design question. Should the helper return an error for a
missing mdiodev, so callers can bail out instead of using address 0?

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

[ ... ]

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