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

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c
> @@ -2574,6 +2574,67 @@ static const struct dsa_switch_ops yt921x_dsa_switch_ops = {
>  	.setup			= yt921x_dsa_setup,
>  };
>  
> +static const struct yt92xx_series yt92xx_series_table[] = {
> +	[YT92XX_MODE_YT921X] = {
> +		.name = "YT921x",
> +		.max_ports = YT921X_PORT_NUM,
> +		.num_lag_ids = YT921X_LAG_NUM,
> +		.ageing_time_min = 1 * 5000,
> +		.ageing_time_max = U16_MAX * 5000,
> +		.dscp_prio_mapping_is_global = true,
> +		.assisted_learning_on_cpu_port = true,
> +		.switch_ops = &yt921x_dsa_switch_ops,
> +		.mac_ops = &yt921x_phylink_mac_ops
> +	},
> +};
> +
> +static const struct yt92xx_series *yt92xx_series_lookup(u32 major)
> +{
> +	if (major == YT9215_MAJOR || major == YT9218_MAJOR)
> +		return &yt92xx_series_table[YT92XX_MODE_YT921X];
> +	else
> +		return NULL;
> +}
> +
> +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);
> +		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;
> +	}
> +	priv->series = series;
> +
> +	ds->assisted_learning_on_cpu_port =
> +		priv->series->assisted_learning_on_cpu_port;
> +	ds->dscp_prio_mapping_is_global =
> +		priv->series->dscp_prio_mapping_is_global;
> +	ds->ageing_time_min = priv->series->ageing_time_min;
> +	ds->ageing_time_max = priv->series->ageing_time_max;
> +	ds->num_lag_ids = priv->series->num_lag_ids;
> +	ds->num_ports = priv->series->max_ports;
> +	ds->ops = priv->series->switch_ops;
> +	ds->phylink_mac_ops = priv->series->mac_ops;
> +
> +	return 0;
> +}
> +
>  static void yt921x_mdio_shutdown(struct mdio_device *mdiodev)
>  {
>  	struct yt921x_priv *priv = mdiodev_get_drvdata(mdiodev);

[Severity: Medium]
If yt921x_reg_read() fails, yt921x_reg_mdio_read() returns before
it writes *valp. That leaves chipid as uninitialized stack data when
it is passed to dev_err() here.

The message is also misleading. On this path the read failed and the
chip ID was never obtained, so "Unexpected chipid" followed by a
random value is no help to someone debugging a broken MDIO bus. KMSAN
would also report it as an uninit-value use.

Could you print the error code from res here instead? The chip ID can
be reported in the !series branch, where it has actually been read
and is the value that is unexpected.

The YT922x patch later in the series already moves the message there.
Please fold that change into this patch so each commit stands on its
own and the series stays bisectable.
-- 
pw-bot: cr