Re: [PATCH net-next v9 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 4 potential
issue(s) to consider.

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

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] yt92xx_register_switch() in drivers/net/dsa/motorcomm/chip.c
  declares `u32 chipid;` with no initializer.
- [Low] The commit message does not match the patch.
- [Low] At this commit, enum yt92xx_mode declares YT92XX_MODE_YT922X and
  YT92XX_MODE_MAX, but yt92xx_series_table[] fills only…
- [Low] yt921x_mdio_probe() calls mutex_init(&priv->reg_lock)
  (chip.c:4806) and now returns early if yt92xx_register_switch() fails…

--

net: dsa: motorcomm: initialize dsa_switch based on chipid

This patch moves the dsa_switch setup out of yt921x_mdio_probe() into a
new yt92xx_register_switch() helper. The helper reads the chip ID at probe
time and fills in the dsa_switch fields from a per-series table entry.

> Replace the hardcoded chip handling in yt92xx_register_switch() with a
> data-driven loopup based on the chip ID.

[Severity: Low]
Is this description accurate? yt92xx_register_switch() is added by this
patch. The hardcoded ds-> assignments it removes were in
yt921x_mdio_probe().

There's also a typo: "loopup" should be "lookup".

The lookup is also not really data-driven. yt92xx_series_lookup() writes
the majors into an if-statement:

    if (major == YT9215_MAJOR || major == YT9218_MAJOR)
        return &yt92xx_series_table[YT92XX_MODE_YT921X];

yt921x_mdio_probe() now reads YT921X_CHIP_ID over MDIO. It can fail with
an I/O error or -ENODEV before dsa_register_switch() is reached. Before
this patch, the chip ID was only read later, from yt921x_chip_detect() in
the DSA setup path. Could the message mention this change?

It could also say this is groundwork for YT922X, since nothing in this
patch uses YT92XX_MODE_YT922X or YT92XX_MODE_MAX.

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index 8b0ac5fc8c8b..d4a565994940 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c
> @@ -4679,6 +4679,68 @@ static const struct dsa_switch_ops yt921x_dsa_switch_ops = {

[ ... ]

> +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);

[Severity: Medium]
Can chipid be uninitialized when it is printed here? yt921x_reg_read()
ends up in yt921x_reg_mdio_read(), which stores to *valp only on success:

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:
    ...
}

If any of the MDIO accesses fail, this prints leftover stack contents as
the chip ID, and a bus failure looks like an ID mismatch.

The message seems to come from yt921x_chip_detect(). There it is printed
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 read failure get its own message? The chip ID could then be
printed in the unknown-major branch below, which currently leaves it out.

This code is unchanged at the end of the series, in "net: dsa: motorcomm:
Add support for Motorcomm YT922x".

> +		return res;
> +	}
> +
> +	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;
> +	}

[ ... ]

> @@ -4754,15 +4817,10 @@ static int yt921x_mdio_probe(struct mdio_device *mdiodev)
>  
>  	ds = &priv->ds;
>  	ds->dev = dev;
> -	ds->assisted_learning_on_cpu_port = true;
> -	ds->dscp_prio_mapping_is_global = true;
>  	ds->priv = priv;
> -	ds->ops = &yt921x_dsa_switch_ops;
> -	ds->ageing_time_min = 1 * 5000;
> -	ds->ageing_time_max = U16_MAX * 5000;
> -	ds->phylink_mac_ops = &yt921x_phylink_mac_ops;
> -	ds->num_lag_ids = YT921X_LAG_NUM;
> -	ds->num_ports = YT921X_PORT_NUM;
> +	res = yt92xx_register_switch(ds);
> +	if (res)
> +		return res;

[Severity: Low]
This isn't a bug, but this new early return comes after
mutex_init(&priv->reg_lock) and skips mutex_destroy(). The only
mutex_destroy() call is in yt921x_mdio_remove(), which doesn't run when
probe fails.

Nothing leaks. priv is devm allocated, and mutex_destroy() only poisons
the magic field when CONFIG_DEBUG_MUTEXES is set. The existing
dsa_register_switch() failure path already skips it too.

Is it worth adding mutex_destroy() to the probe error paths for symmetry?

[ ... ]

> diff --git a/drivers/net/dsa/motorcomm/chip.h b/drivers/net/dsa/motorcomm/chip.h
> index 83cd454955dd..c446aea449ed 100644
> --- a/drivers/net/dsa/motorcomm/chip.h
> +++ b/drivers/net/dsa/motorcomm/chip.h
> @@ -960,9 +960,29 @@ struct yt921x_reg_ops {
>  	int (*write)(void *context, u32 reg, u32 val);
>  };
>  
> +enum yt92xx_mode {
> +	YT92XX_MODE_YT921X,
> +	YT92XX_MODE_YT922X,
> +	YT92XX_MODE_MAX,
> +};
> +
> +struct yt92xx_series {
> +	enum yt92xx_mode mode;
> +	const char *name;

[Severity: Low]
At this commit, yt92xx_series_table[] fills only [YT92XX_MODE_YT921X].
There is no table entry for YT92XX_MODE_YT922X, and no BUILD_BUG_ON ties
the table size to YT92XX_MODE_MAX. The later patch "net: dsa: motorcomm:
Add support for Motorcomm YT922x" adds the YT922X entry, so this part is
resolved by the end of the series.

Are the mode and name fields needed? Nothing reads series->mode or
series->name, even at the end of the series. The name field also
duplicates yt921x_info.name, which is the name that gets printed.

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