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

On 2022-09-20 00:49, Vladimir Oltean wrote:

On Mon, Sep 19, 2022 at 01:08:45PM +0200, Mattias Forsblad wrote:

quoted

@@ -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.

quoted

+
+	/* 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.

quoted

+	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.

quoted

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

This whole shebang was a suggestion from Andrew. I had a solution with
mv88e6xxx_rmu_available in mv88e6xxx_get_ethtool_stats which he wasn't fond of.
The mv88e6xxx_bus_ops is declared const and how am I to change the get_rmon
member? I'm not really sure on how to solve this in a better way?
Suggestions any? Maybe I've misunderstood his suggestion.

/Mattias