Re: [PATCH net-next 2/2] net: dsa: Set the master device's MTU to account for DSA overheads

[Andrew Lunn <andrew@lunn.ch>]

On Thu, Dec 06, 2018 at 12:21:31PM -0800, Florian Fainelli wrote:
> On 12/6/18 2:36 AM, Andrew Lunn wrote:
> > DSA tagging of frames sent over the master interface to the switch
> > increases the size of the frame. Such frames can then be bigger than
> > the normal MTU of the master interface, and it may drop them. Use the
> > overhead information from the tagger to set the MTU of the master
> > device to include this overhead.
> > 
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > ---
> >  net/dsa/master.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/net/dsa/master.c b/net/dsa/master.c
> > index c90ee3227dea..42f525bc68e2 100644
> > --- a/net/dsa/master.c
> > +++ b/net/dsa/master.c
> > @@ -158,8 +158,24 @@ static void dsa_master_ethtool_teardown(struct net_device *dev)
> >  	cpu_dp->orig_ethtool_ops = NULL;
> >  }
> >  
> > +void dsa_master_set_mtu(struct net_device *dev, struct dsa_port *cpu_dp)
> > +{
> > +	unsigned int mtu = ETH_DATA_LEN + cpu_dp->tag_ops->overhead;
> > +	int err;
> > +
> > +	rtnl_lock();
> > +	if (mtu <= dev->max_mtu) {
> > +		err = dev_set_mtu(dev, mtu);
> > +		if (err)
> > +			netdev_dbg(dev, "Unable to set MTU to include for DSA overheads\n");
> > +	}
> 
> Would it make sense to warn the user that there might be
> transmit/receive issues with the DSA tagging protocol if either
> dev_set_mtu() fails or mtu > dev->max_mtu?

I thought about that. But we have setups which work today with the
standard MTU. The master might not implement the set_mtu op, or might
impose the standard MTU, but be quite happy to deal with our DSA
packets. So i wanted to make this a hint to the master device, not a
strong requirement.

> Not that I think it matters too much to people because unbinding the
> switch driver and expecting the CPU port to continue operating is
> wishful thinking, but we should probably unwind that operation in
> dsa_master_teardown(), right?

That would make sense.

David has already accepted the patchset, so i will add a followup
patch.

	Andrew