Re: [PATCH net-next v8 2/6] 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] yt921x_mbus_int_read()/yt921x_mbus_int_write() (chip.c:360 and
  chip.c:378) now validate internal-MDIO addresses against…

Pre-existing issues:
- [Low] Claim of an inconsistency between the runtime per-series bound
  (ds->num_ports = priv->series->max_ports at chip.c:4734, and now the…

--

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

This patch replaces YT921X_PORT_NUM with priv->series->max_ports in the
internal MDIO bus read/write address checks and in the phy_mask
computation in yt921x_mbus_int_init().  The commit message states there
is no functional change for existing YT921X devices.

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index 564e45ece22cb..2156230a78aad 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;
>  
>  	mutex_lock(&priv->reg_lock);
> @@ -375,7 +375,7 @@ yt921x_mbus_int_write(struct mii_bus *mbus, int port, int reg, u16 data)
>  	struct yt921x_priv *priv = mbus->priv;
>  	int res;
>  
> -	if (port >= YT921X_PORT_NUM)
> +	if (port >= priv->series->max_ports)
>  		return -ENODEV;

[Severity: Low]
Is max_ports the right quantity for these two checks?  The address passed
to yt921x_mbus_int_read()/yt921x_mbus_int_write() ends up in the
YT921X_INT_MBUS_CTRL PORT field, which selects one of the integral GbE
PHYs, whereas max_ports is the total DSA port count including the
external MAC ports and the MCU port.

The driver keeps those as separate quantities:

drivers/net/dsa/motorcomm/chip.h:
	/* 8 internal + 2 external + 1 mcu */
	#define YT921X_PORT_NUM			11
	...
	#define yt921x_port_is_internal(port) ((port) < 8)
	#define yt921x_port_is_external(port) ((port) == 8 || (port) == 9)

and struct yt921x_info carries:

	/* Ports with integral GbE PHYs, not including MCU Port 10 */
	u16 internal_mask;

With max_ports == 11 the guard now accepts internal-bus transactions for
ports 8 and 9 (external MACs) and port 10 (MCU), none of which have an
integral PHY.  For a series whose integral PHYs are not the lowest port
indices, the accepted set diverges further: the YT922x entry added later
in this series sets max_ports = 9 while its integral UTP PHYs are only
ports 4..7, so ports 0..3 and 8 would also be accepted.

The effect is bounded: these callbacks only see addresses phylib has
bound from the "mdio" child node, so a DT would have to list a PHY at
such an address.  When it does, the driver runs a real INT_MBUS cycle
against a nonexistent PHY index, and yt921x_intif_read() already has a
dev_info() for the resulting unexpected read data.

Would keying the check off the integral-PHY count or mask (the
internal_mask / yt921x_port_is_internal equivalent) rather than
max_ports, which DSA consumes as ds->num_ports, match the hardware
field being programmed?

> @@ -390,6 +390,7 @@ yt921x_mbus_int_init(struct yt921x_priv *priv, struct device_node *mnp)
>  {
>  	struct device *dev = to_device(priv);
>  	struct mii_bus *mbus;
> +	u32 max_ports;
>  	int res;
>  
>  	mbus = devm_mdiobus_alloc(dev);
> @@ -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 isn't a bug introduced by this patch, but now that the runtime bound
comes from priv->series->max_ports, is it worth tying that value to the
compile-time array size?

yt92xx_register_switch() already does:

	ds->num_ports = priv->series->max_ports;

while the per-port state stays fixed size in chip.h:

	struct yt921x_port ports[YT921X_PORT_NUM];

and several helpers still loop on the old constant, for example in
yt921x_chip_setup_dsa():

	for (int i = 0; i < YT921X_PORT_NUM; i++)
		ctrl |= YT921X_ACT_UNK_ACTn_TRAP(i);

Nothing is broken today: the only two series entries are max_ports == 11
(YT921x) and max_ports == 9 (YT922x), both within ARRAY_SIZE(priv->ports),
and the remaining YT921X_PORT_NUM loops live in helpers reachable only
through yt921x_dsa_switch_ops, i.e. only when max_ports == 11.  A
BUILD_BUG_ON() relating max_ports to ARRAY_SIZE(priv->ports), or
converting those loops to max_ports, would make the invariant explicit
for the next series added.

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