Re: [PATCH net-next v9 2/7] net: dsa: motorcomm: use max_ports in series for port bounds checking

[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] Concern: the commit overclaims, and nothing ties
  series->max_ports to the fixed-size port storage.

Pre-existing issues:
- [Low] Concern: yt921x_mbus_int_read()/write() and phy_mask bound the
  internal MDIO bus by the total DSA port count (series->max_ports = 11…

--

net: dsa: motorcomm: use max_ports in series for port bounds checking

This patch replaces the hardcoded YT921X_PORT_NUM with the per-series
priv->series->max_ports in three places: the internal MDIO bus bounds
checks in yt921x_mbus_int_read() and yt921x_mbus_int_write(), and the
phy_mask computed in yt921x_mbus_int_init().

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index d4a565994940..b95f3c355511 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c
> @@ -357,7 +357,7 @@ static int yt921x_mbus_int_read(struct mii_bus *mbus, int port, int reg)
>  	u16 val;
>  	int res;
>  
> -	if (port >= YT921X_PORT_NUM)
> +	if (port >= priv->series->max_ports)
>  		return U16_MAX;

[Severity: Low]
The commit message says this "removes family-specific constants from the
common code path". Nothing in the patch ties series->max_ports to the size
of the per-port storage, though. Is that intended?

struct yt921x_priv still sizes its port array with the YT921x constant:

drivers/net/dsa/motorcomm/chip.h:struct yt921x_priv {
    ...
	struct yt921x_port ports[YT921X_PORT_NUM];
    ...
};

YT921X_PORT_NUM also still bounds loops in these functions:

  yt921x_lag_set()
  yt921x_isolate()
  yt921x_bridge() (twice)
  yt921x_chip_setup_dsa() (twice)

Meanwhile ds->num_ports now comes from priv->series->max_ports.

With the current table this can't happen. At the end of the series,
YT921X has max_ports = 11 and YT922X has max_ports = 9, so both fit. The
remaining YT921X_PORT_NUM loops are also only reachable from YT921x-only
code.

If a future series entry had max_ports greater than YT921X_PORT_NUM,
though, could priv->ports[] be indexed out of bounds? Would a
BUILD_BUG_ON() or a probe-time check that max_ports is no larger than
ARRAY_SIZE(priv->ports) be worth adding?

[ ... ]

> @@ -402,7 +403,8 @@ yt921x_mbus_int_init(struct yt921x_priv *priv, struct device_node *mnp)
>  	mbus->read = yt921x_mbus_int_read;
>  	mbus->write = yt921x_mbus_int_write;
>  	mbus->parent = dev;
> -	mbus->phy_mask = (u32)~GENMASK(YT921X_PORT_NUM - 1, 0);
> +	max_ports = priv->series->max_ports;
> +	mbus->phy_mask = (u32)~GENMASK(max_ports - 1, 0);

[Severity: Low]
This is a pre-existing issue, and this patch didn't introduce it. It
shows up here because the bound is being changed. max_ports counts every
DSA port, not only the integrated PHYs:

drivers/net/dsa/motorcomm/chip.h:
/* 8 internal + 2 external + 1 mcu */
#define YT921X_PORT_NUM			11

As a result, yt921x_mbus_int_read() and yt921x_mbus_int_write() still
allow internal MDIO bus transactions to addresses 8-10 if the DT puts a
PHY there. The phy_mask computed here doesn't limit scanning either,
because __of_mdiobus_register() overwrites it with ~0.

The baseline used the same value, so "No functional change for existing
YT921X devices" is accurate.

Bounding by info->internal_mask may not be right either. Later in the
series, YT922x uses internal bus addresses that are not integrated UTP
PHYs, via yt922x_sds_phyaddr_get(): SerDes addresses 0 and 8, and
YT922X_COMMON_EXT_PHYADDR (9).

Should the internal bus bound describe the addresses that actually exist
on that bus, rather than the total port count? Or is using max_ports here
intentional?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260924094146.4128703-1-kyle.switch%40motor-comm.com