Re: [PATCH net-next v7 1/6] 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: 1 · Medium: 0 · Low: 0

- [High] drivers/net/dsa/motorcomm/chip.c: the newly added
  yt92xx_register_switch() calls yt921x_reg_read(priv, YT921X_CHIP_ID,…

--

net: dsa: motorcomm: initialize dsa_switch based on chipid

This adds a yt92xx_series descriptor table and a new
yt92xx_register_switch() helper that reads the chip ID, looks up the
matching series and fills in the dsa_switch fields, replacing the
hardcoded assignments previously done in yt921x_mdio_probe().

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index d663af010f431..1f9c0c67ad4a6 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c
> @@ -4679,6 +4679,61 @@ static const struct dsa_switch_ops yt921x_dsa_switch_ops = {
>  	.setup			= yt921x_dsa_setup,
>  };
>
> +static const struct yt92xx_series yt92xx_series_table[] = {

[ ... ]

> +static int yt92xx_register_switch(struct dsa_switch *ds)
> +{
> +	struct yt921x_priv *priv = to_yt921x_priv(ds);
> +	const struct yt92xx_series *series;
> +	u32 chipid;
> +	u32 major;
> +	int res;
> +
> +	res = yt921x_reg_read(priv, YT921X_CHIP_ID, &chipid);
> +	if (res)
> +		return res;

[Severity: High]

Should this read be done with priv->reg_lock held?

yt921x_reg_read() asserts the lock in drivers/net/dsa/motorcomm/smi.c:

int yt921x_reg_read(struct yt921x_priv *priv, u32 reg, u32 *valp)
{
	lockdep_assert_held_once(&priv->reg_lock);

	return priv->reg_ops->read(priv->reg_ctx, reg, valp);
}

yt921x_mdio_probe() only calls mutex_init(&priv->reg_lock) before invoking
yt92xx_register_switch(), so the lock is never taken on this path.  With
CONFIG_PROVE_LOCKING enabled, doesn't that make every probe of this driver
emit a WARN_ON_ONCE splat (and panic when booted with panic_on_warn)?

Every other register access in the driver takes the mutex first, for
example yt921x_dsa_setup():

	mutex_lock(&priv->reg_lock);
	res = yt921x_chip_reset(priv);
	mutex_unlock(&priv->reg_lock);

and the comment on the field in chip.h describes it as protecting the
register access:

	/* protect the access to the switch registers */
	struct mutex reg_lock;

Since probe context can sleep, would wrapping the YT921X_CHIP_ID read in
mutex_lock()/mutex_unlock(&priv->reg_lock) be the intended fix?

> +
> +	major = FIELD_GET(YT921X_CHIP_ID_MAJOR, chipid);
> +	series = yt92xx_series_lookup(major);
> +	if (!series)
> +		return -ENODEV;
> +	priv->series = series;

[ ... ]

> @@ -4754,15 +4810,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;
>
>  	mdiodev_set_drvdata(mdiodev, priv);

[ ... ]

One more note on this: checking the tip of the series (the commit adding
support for Motorcomm YT922x), yt92xx_register_switch() still performs the
chip ID read without taking priv->reg_lock and is still called from probe,
so this does not appear to be addressed by a later patch.

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