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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In yt92xx_register_switch()
  (drivers/net/dsa/motorcomm/chip.c), `u32 chipid;` has no initializer.

--

net: dsa: motorcomm: initialize dsa_switch based on chipid

This patch moves the hardcoded dsa_switch setup out of
yt921x_mdio_probe() and into a new helper, yt92xx_register_switch().
The helper reads the chip ID, looks up a yt92xx_series table entry by
major number, and fills in the dsa_switch fields from that entry.

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index 3ff2de383157a..79422def16ffb 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 = to_yt921x_priv(ds);
> +	struct device *dev = 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);
> +		return res;
> +	}

[Severity: Medium]
Can this dev_err() print an uninitialized chipid?

chipid has no initializer. This branch runs only when yt921x_reg_read()
fails. The read goes through yt921x_reg_mdio_read(), and every MDIO
failure there jumps past the store to the caller's variable:

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 an MDIO bus error during yt921x_mdio_probe()->yt92xx_register_switch()
would log leftover stack contents as the chip ID. KMSAN would also flag
this as uninit-value.

The message is also misleading in this case, because no chip ID was
read. Could this branch print the error code instead, for example
"Failed to read chip ID: %d"?

> +
> +	major = FIELD_GET(YT921X_CHIP_ID_MAJOR, chipid);
> +	series = yt92xx_series_lookup(major);
> +	if (!series) {
> +		dev_err(dev, "Undefined series info for the corresponding major\n");
> +		return -ENODEV;
> +	}

This branch is where the chip ID really is unexpected, but the message
leaves out chipid.

The existing yt921x_chip_detect() handles both cases the other way
round. It returns res without a message when the read fails, and prints
"Unexpected chipid 0x%x" only after a successful read with an unknown
major:

drivers/net/dsa/motorcomm/chip.c:yt921x_chip_detect() {
    ...
	res = yt921x_reg_read(priv, YT921X_CHIP_ID, &chipid);
	if (res)
		return res;
    ...
	if (!info->name) {
		dev_err(dev, "Unexpected chipid 0x%x\n", chipid);
		return -ENODEV;
	}
    ...
}

Should the chipid value be printed here, in the unknown-major branch,
instead of in the read-failure branch above?

> +	priv->series = series;

[ ... ]

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