Re: [net-next PATCH 16/19] net: dsa: qca8k: enable mtu_enforcement_ingress

[Vladimir Oltean]

On Fri, Nov 19, 2021 at 03:28:48AM +0100, Ansuel Smith wrote:
> On Fri, Nov 19, 2021 at 04:20:08AM +0200, Vladimir Oltean wrote:
> > On Wed, Nov 17, 2021 at 10:04:48PM +0100, Ansuel Smith wrote:
> > > qca8k have a global MTU. Inform DSA of this as the change MTU port
> > > function checks the max MTU across all port and sets the max value
> > > anyway as this switch doesn't support per port MTU.
> > > 
> > > Signed-off-by: Ansuel Smith <ansuelsmth@xxxxxxxxx>
> > > ---
> > >  drivers/net/dsa/qca8k.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> > > index c3234988aabf..cae58753bb1f 100644
> > > --- a/drivers/net/dsa/qca8k.c
> > > +++ b/drivers/net/dsa/qca8k.c
> > > @@ -1315,6 +1315,9 @@ qca8k_setup(struct dsa_switch *ds)
> > >  	/* Set max number of LAGs supported */
> > >  	ds->num_lag_ids = QCA8K_NUM_LAGS;
> > >  
> > > +	/* Global MTU. Inform dsa that per port MTU is not supported */
> > > +	ds->mtu_enforcement_ingress = true;
> > > +
> > >  	return 0;
> > >  }
> > >  
> > > -- 
> > > 2.32.0
> > > 
> > 
> > This doesn't do what you think it does. If you want the dev->mtu of all
> > interfaces to get updated at once, you need to do that yourself. Setting
> > ds->mtu_enforcement_ingress will only update the MTU for ports belonging
> > to the same bridge, and for a different reason. Or I'm missing the
> > reason why you're making this change now.
> 
> Got confused by the Documentation. Just to confirm in DSA we don't have
> a way to handle the case where we have one MTU reg that is applied to
> every port, correct?

No we don't, because usually it is not a problem. The interface MTU
represents the minimum L2 payload size that can be accepted and
transmitted by the interface. It can be larger than that.

> We already handle this by checking the max MTU set to all port in the
> port_change_mtu but I was searching a cleaner way to handle this as
> currently we use an array to store the MTU of all port and seems a bit
> hacky and a waste of space. 

It depends on what your goal is. The current implementation seems to get
the job done fine, unless I'm missing something. If you set one
interface's MTU to e.g. 4000, the MTU of the other interfaces will
remain the same at 1500, although they can also receive larger frames.
But to also transmit larger frames on those other ports, you'd have to
increase their MTU as well, and even though that wouldn't increase
anything in hardware, it would tell the kernel to use the larger value.
This is in line with the user interaction that would be required if the
MTU was a per-port value, which seems fine to me.