Re: [PATCH net-next 2/2] net: dsa: realtek: Rewrite RTL8366RB MTU handling

[Vladimir Oltean <olteanv@gmail.com>]

On Mon, Dec 11, 2023 at 06:07:56PM -0300, Luiz Angelo Daros de Luca wrote:
> /*
> * MTU change functionality. Switches can also adjust their MRU through
> * this method. By MTU, one understands the SDU (L2 payload) length.
> * If the switch needs to account for the DSA tag on the CPU port, this
> -* method needs to do so privately.
> +* method needs to do so privately. An MTU change will also be
> +* propagated to every CPU port when the largest MTU in the switch
> +* changes, either up or down. Switches with only a global MTU setting
> +* can adjust the MTU based only on these calls targeting CPU ports.
> */
> int (*port_change_mtu)(struct dsa_switch *ds, int port,
>    int new_mtu);

If only this comment were correct. In cascaded switch trees, we may have
switches which don't have a CPU port. Do as instructed here, and they
will not program the MTU to hardware.

> Do we really need to expand the API? A new ds_ops.set_global_mtu() is
> just giving the same but less specific info as
> ds_ops.port_change_mtu(). I prefer to work in an API with fewer
> functions than more optional functions for each specific HW design. It
> just makes writing a new driver more complex. The code in the DSA side
> will also be more complex with more code paths when only
> set_global_mtu is defined.

The alternative is distributing that complexity to individual drivers,
with more chances of getting it wrong. Let's be honest about it, that
has happened.

> If setting the MTU multiple times is a problem for a switch, the
> driver must keep track of the current global MTU value and do nothing
> when not needed. In the case of rtl8366rb, the MTU works in specific
> ranges. In many cases, changing the MTU inside a range would not have
> an effect on the switch (although the code is still writing the
> register with the same value).

It's not a practical problem except for the wasted time baked into the
design. Just worth mentioning. It's a slow path anyway, not a big deal.

> > If the chip uses set_max_frame_size() - which is not per port - then it
> > will accept any latest value, and not look just at the largest_mtu.
> 
> It might just work as the latest value will be the CPU port that is
> guaranteed to be the largest one. However, it might temporarily lower
> the global MTU between the user and the CPU MTU change. In order to
> fix that, it just needs to ignore user port changes.
> 
> > b53_change_mtu() also looks like it suffers from a similar problem, it
> > always programs the latest per-port value to a global register.
> 
> Latest will, in the end, just work because it is a CPU port and the
> MTU the largest one. Maybe the sequence could change in a
> multithreaded system but I didn't investigate that further. Anyway,
> focusing on the CPU port would just work.

The calling sequence in dsa_user_change_mtu() is single-threaded and
starts with dsa_port_mtu_change(), which is the cross-chip notifier
that programs all CPU and DSA ports, and ends with the direct
ds->ops->port_change_mtu() call which programs the user port.

So I hope you now understand why I'm saying they are buggy. No "might work".

> > So I guess there is ample opportunity to get this wrong, and maybe
> > making the global MTU "core functionality" is worth considering.
> > As "net-next" material - I think the bugs are sufficiently artificial,
> > and workarounds exist, to not bother the stable kernels with fixes over
> > the existing API.
> >
> > Would you volunteer to do that?
> 
> I don't believe we should expand the API but I will volunteer to
> update the port_change_mtu comment if accepted. I can also suggest
> changes to other drivers when needed but I prefer to not do that
> without a proper HW to test it.

With N drivers trying to save the same problem (working around the same
framework design), but in subtly different ways, the responsibility
moves to review to make sure they are all correct.

So the options for switches with global MTU are:

- keep an array of MTUs per port, calculate the maximum, program the
  maximum. This is what RTL8366RB does, you don't like that.

- reverse the calling order in dsa_user_change_mtu() between the
  ds->ops->port_change_mtu() on the user port and the dsa_port_mtu_change()
  cross-chip notifier, such that the order is accidentally fine for
  mv88e6xxx and b53. I don't think this qualifies as the "core
  functionality" you've been asking for, so let's move on.

- only act on MTU changes on the CPU port. A few drivers do this,
  obviously it works for them, and their reliance on the framework is
  reasonable. You're suggesting this as a workaround worth promoting
  to a recommendation in include/net/dsa.h, which won't be a good
  recommendation there as-is.

- ignore MTU changes on user ports, implicitly acting on all CPU and
  DSA (upstream and downstream) ports. Functionally ok as a general
  recommendation, but no driver follows it. You seem to be reluctant to
  make changes to drivers you can't test. The risk with both this and
  the previous option is that developers don't realize they need to
  perform the workaround.

- add a new ds->ops->change_global_mtu() which is called once per switch
  from dsa_switch_mtu(). This eliminates the back and forth between the
  driver and the framework. It would make the framework a bit more
  cumbersome, but the drivers generally simpler, except for mv88e6xxx
  which needs to implement both ds->ops->port_change_mtu() and
  ds->ops->change_global_mtu(). But even there, one needs to consider
  that the chip->info->ops->set_max_frame_size() call path needs fixing,
  and we'd end up having 2 call paths (one returning early for user
  ports) within the same mv88e6xxx_change_mtu() function.

- add a new ds->mtu_is_global bit, similar to ds->vlan_filtering_is_global.
  Document that when set, ds->port_change_mtu() will be called once per
  switch, with an invalid port argument which should be ignored. It has
  the same advantages as the ds->ops->change_global_mtu(), but the
  dsa_switch_ops API is a bit more quirky to save space for a different
  function pointer.