Re: [PATCH net-next v14 5/7] net: dsa: mv88e6xxx: rmu: Add functionality to get RMON

[Vladimir Oltean <olteanv@gmail.com>]

On Mon, Sep 19, 2022 at 01:08:45PM +0200, Mattias Forsblad wrote:
> @@ -255,6 +299,15 @@ int mv88e6xxx_rmu_setup(struct mv88e6xxx_chip *chip)
>  	chip->rmu.smi_ops = chip->smi_ops;
>  	chip->rmu.ds_ops = chip->ds->ops;
>  
> +	/* Change rmu ops with our own pointer */
> +	chip->rmu.smi_rmu_ops = (struct mv88e6xxx_bus_ops *)chip->rmu.smi_ops;
> +	chip->rmu.smi_rmu_ops->get_rmon = mv88e6xxx_rmu_stats_get;

The patch splitting is still so poor, that I can't even complain about
this bug properly, since no one uses this pointer for now (and when it
will be used, it will not be obvious how it's assigned).

In short, it's called like this:

static void mv88e6xxx_get_ethtool_stats(struct dsa_switch *ds, int port,
					uint64_t *data)
{
	struct mv88e6xxx_chip *chip = ds->priv;

	chip->smi_ops->get_rmon(chip, port, data);
}

When the switch doesn't support RMU operations, or when the RMU setup
simply failed, "ethtool -S" will dereference a very NULL pointer during
that indirect call, because mv88e6xxx_rmu_setup() is unconditionally
called for every switch during setup. Probably not a wise choice to
start off with the RMU ops for ethtool -S.

Also, not clear, if RMU fails, why we don't make an effort to fall back
to MDIO for that user space request.

> +
> +	/* Also change get stats strings for our own */

Who is "our own", and implicitly, who are they? You'd expect that there
aren't tribalist factions within the same driver.

> +	chip->rmu.ds_rmu_ops = (struct dsa_switch_ops *)chip->ds->ops;
> +	chip->rmu.ds_rmu_ops->get_sset_count = mv88e6xxx_stats_get_sset_count_rmu;
> +	chip->rmu.ds_rmu_ops->get_strings = mv88e6xxx_stats_get_strings_rmu;
> +

Those who cast a const to a non-const pointer and proceed to modify the
read-only structure behind it should go to patchwork arrest for one week.

>  	/* Start disabled, we'll enable RMU in master_state_change */
>  	if (chip->info->ops->rmu_disable)
>  		return chip->info->ops->rmu_disable(chip);
> -- 
> 2.25.1
>