Re: [PATCH net-next v14 4/7] net: dsa: mv88e6xxxx: Add RMU functionality.

[Vladimir Oltean <olteanv@gmail.com>]

On Tue, Sep 20, 2022 at 01:53:36PM +0200, Mattias Forsblad wrote:
> On 2022-09-20 00:39, Vladimir Oltean wrote:
> >> +void mv88e6xxx_master_state_change(struct dsa_switch *ds, const struct net_device *master,
> >> +				   bool operational)
> >> +{
> >> +	if (operational && chip->info->ops->rmu_enable) {
> > 
> > This all needs to be rewritten. Like here, if the master is operational
> > but the chip->info->ops->rmu_enable method is not populated, you call
> > mv88e6xxx_disable_rmu(). Why?
> 
> So what should we do in this case?

Nothing, obviously.

> If the master is operational but we cannot enable rmu (bc no funcptr),
> we cannot use RMU -> disable RMU.

Again, the RMU should start as disabled. Then why
would you call mv88e6xxx_disable_rmu() a million times as the master
goes up and down, if the switch doesn't support chip->info->ops->rmu_enable()?

In fact, the RMU _is_ disabled, since mv88e6xxx_rmu_setup() has:

int mv88e6xxx_rmu_setup(struct mv88e6xxx_chip *chip)
{
	mutex_init(&chip->rmu.mutex);

	/* Remember original ops for restore */
	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;

	/* Also change get stats strings for our own */
	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;

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

	return 0;
}

But mv88e6xxx_disable_rmu() has:

static void mv88e6xxx_disable_rmu(struct mv88e6xxx_chip *chip)
{
	chip->smi_ops = chip->rmu.smi_ops;
	chip->ds->ops = chip->rmu.ds_rmu_ops;
	chip->rmu.master_netdev = NULL;

	if (chip->info->ops->rmu_disable)
		chip->info->ops->rmu_disable(chip);
}

Notice in mv88e6xxx_disable_rmu() how:

- all calls to chip->info->ops->rmu_disable() are redundant when
  chip->info->ops->rmu_enable() isn't available.

- the mumbo jumbo pointer logic with chip->smi_ops and chip->ds->ops is
  buggy but at the same time not in the obvious way. What is obvious is
  that you surely don't mean to assign "chip->ds->ops = chip->rmu.ds_rmu_ops;",
  but rather "chip->ds->ops = chip->rmu.ds_ops;". But this does not
  truly matter.

This is because juggling the chip->ds->ops pointer itself is not how you
make mv88e6xxx_get_ethtool_stats() call MDIO or Ethernet-based ops. This
is because in reality in your implementation, ds_rmu_ops and ds_ops (and
same goes for smi_ops and smi_rmu_ops) point to the same data structure.
And when you do this:

	/* 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;

you change the get_rmon() operation of _both_ smi_rmu_ops and smi_ops,
because you dereference two pointers which have the same value.

Therefore, when you attempt to collect ethtool stats, you dereference
"our own" RMU based pointer, regardless of whether RMU is available or
not:

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

This will proceed to access stuff that isn't available, such as the
master netdev, and crash the kernel.