Re: [PATCH net-next v16 1/5] net: dsa: motorcomm: initialize dsa_switch based on chipid

[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] When yt921x_reg_read(priv, YT921X_CHIP_ID, &chipid) fails,
  yt92xx_register_switch() (drivers/net/dsa/motorcomm/chip.c) prints…

--

net: dsa: motorcomm: initialize dsa_switch based on chipid

This patch replaces the hardcoded dsa_switch setup in yt921x_mdio_probe()
with a new helper, yt92xx_register_switch(). The helper reads the chip ID,
looks up a per-series descriptor table and fills in the dsa_switch fields
from that descriptor.

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index d4fc20c52a58d..4add61b89310a 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c

[ ... ]

> +static int yt92xx_register_switch(struct dsa_switch *ds)
> +{
> +	struct yt921x_priv *priv = dsa_to_yt921x_priv(ds);
> +	struct device *dev = yt921x_priv_to_device(priv);
> +	const struct yt92xx_series *series;
> +	u32 chipid;
> +	u32 major;
> +	int res;
> +
> +	mutex_lock(&priv->reg_lock);
> +	res = yt921x_reg_read(priv, YT921X_CHIP_ID, &chipid);
> +	mutex_unlock(&priv->reg_lock);
> +	if (res) {
> +		dev_err(dev, "Unexpected chipid 0x%x\n", chipid);
                                                          ^^^^^^

[Severity: Low]
Can this print an uninitialized value?

chipid is a local u32 with no initializer. This dev_err() only runs when
yt921x_reg_read() has failed. With the MDIO backend, the output pointer is
written only on success. Every error path jumps over the store:

drivers/net/dsa/motorcomm/chip.c:yt921x_reg_mdio_read() {
    ...
	res = __mdiobus_read(bus, addr, reg_data);
	if (res < 0)
		goto end;
	val = (val << 16) | (u16)res;

	*valp = val;
	res = 0;

end:
	mutex_unlock(&bus->mdio_lock);
	return res;
}

So if an MDIO error happens during yt921x_mdio_probe(), the log line will
show leftover stack contents. The wording also suggests a bad ID was read,
when in fact the read failed.

The same string in yt921x_chip_detect() is printed only after a successful
read, when the major is not recognized.

Could the read-failure path just return res, or print a message that does
not use chipid?

It looks like the final patch of this series, "net: dsa: motorcomm: Add
support for Motorcomm YT922x", rewrites this branch as a plain "if (res)
return res;". After that change, chipid is printed only on the unknown-major
path. The intermediate patches in the series still contain this code.

> +		return res;
> +	}

[ ... ]

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