Message-ID: <YjzYK3oDDclLRmm2@Ansuel-xps.localdomain>
Date:   Thu, 24 Mar 2022 21:44:27 +0100
From:   Ansuel Smith <ansuelsmth@...il.com>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [net-next PATCH 1/4] drivers: net: dsa: qca8k: drop MTU tracking
 from qca8k_priv

On Thu, Mar 24, 2022 at 12:45:24PM +0200, Vladimir Oltean wrote:
> On Tue, Mar 22, 2022 at 03:03:36PM +0100, Ansuel Smith wrote:
> > On Tue, Mar 22, 2022 at 03:55:35PM +0200, Vladimir Oltean wrote:
> > > On Tue, Mar 22, 2022 at 02:38:08PM +0100, Ansuel Smith wrote:
> > > > On Tue, Mar 22, 2022 at 01:58:12PM +0200, Vladimir Oltean wrote:
> > > > > On Tue, Mar 22, 2022 at 02:45:03AM +0100, Ansuel Smith wrote:
> > > > > > Drop the MTU array from qca8k_priv and use slave net dev to get the max
> > > > > > MTU across all user port. CPU port can be skipped as DSA already make
> > > > > > sure CPU port are set to the max MTU across all ports.
> > > > > > 
> > > > > > Signed-off-by: Ansuel Smith <ansuelsmth@...il.com>
> > > > > > ---
> > > > > 
> > > > > I hardly find this to be an improvement and I would rather not see such
> > > > > unjustified complexity in a device driver. What are the concrete
> > > > > benefits, size wise?
> > > > >
> > > > 
> > > > The main idea here is, if the value is already present and accessible,
> > > > why should we duplicate it? Tracking the MTU in this custom way already
> > > > caused some bugs (check the comment i'm removing). We both use standard
> > > > way to track ports MTU and we save some additional space. At the cost of
> > > > 2 additional checks are are not that much of a problem.
> > > 
> > > Where is the bug?
> > 
> > There was a bug where we tracked the MTU with the FCS and L2 added and
> > then in the change_mtu code we added another time the FCS and L2 header
> > just because we used this custom way and nobody notice that we were adding
> > 2 times the same headers. (it's now fixed but still it's a reason why
> > using standard way to track MTU would have prevented that)
> 
> No, I'm sorry, this is completely unjustified complexity - not to
> mention it's buggy, too. Does qca8k support cascaded setups? Because if
> it does:
> 
> 	/* We have only have a general MTU setting. So check
> 	 * every port and set the max across all port.
> 	 */
> 	list_for_each_entry(dp, &ds->dst->ports, list) {
> 		/* We can ignore cpu port, DSA will itself chose
> 		 * the max MTU across all port
> 		 */
> 		if (!dsa_port_is_user(dp))
> 			continue;
> 
> 		if (dp->index == port)	// <- this will exclude from the max MTU calculation the ports in other switches that are numerically equal to @port.
> 			continue;
> 
> 		/* Address init phase where not every port have
> 		 * a slave device
> 		 */
> 		if (!dp->slave)
> 			continue;
> 
> 		if (mtu < dp->slave->mtu)
> 			mtu = dp->slave->mtu;
> 	}
> 
> Not to mention it's missing the blatantly obvious. DSA calls
> ->port_change_mtu() on the CPU port with the max MTU, every time that
> changes.
> 
> You need the max MTU.
> 
> Why calculate it again? Why don't you do what mt7530 does, which has a
> similar restriction, and just program the hardware when the CPU port MTU
> is updated?
>

I just checked and wow it was that easy...
Also wonder if I should add some check for jumbo frame... (I should
check what is the max MTU for the switch and if it can accept jumbo
frame+fcs+l2)

> You may think - does this work with multiple CPU ports? Well, yes it
> does, since DSA calculates the largest MTU across the entire tree, and
> not just across the user ports affine to a certain CPU port.
> 
> If it wasn't for this possibility, I would have been in favor of
> introducing a dsa_tree_largest_mtu(dst) helper in the DSA core, but I
> can't find it justifiable.
> 
> > > > Also from this I discovered that (at least on ipq806x that use stmmac)
> > > > when master needs to change MTU, stmmac complains that the interface is
> > > > up and it must be put down. Wonder if that's common across other drivers
> > > > or it's only specific to stmmac.
> > > 
> > > I never had the pleasure of dealing with such DSA masters. I wonder why
> > > can't stmmac_change_mtu() check if netif_running(), call dev_close and
> > > set a bool, and at the end, if the bool was set, call dev_open back?
> > 
> > Oh ok so it's not standard that stmmac_change_mtu() just refuse to
> > change the MTU instead of put the interface down, change MTU and reopen
> > it... Fun stuff...
> > 
> > From system side to change MTU to a new value (so lower MTU on any port
> > or set MTU to a higher value for one pot) I have to:
> > 1. ifconfig eth0 down
> > 2. ifconfig lan1 mtu 1600 up
> > 3. ifconfig eth up
> > 
> > If I just ifconfig lan1 mtu 1600 up it's just rejected with stmmac
> > complaining.
> 
> Not sure if there is any hard line on this. But I made a poll, and the
> crushing majority of drivers in drivers/net/ethernet/ do not require
> !netif_running() in ndo_change_mtu. The ones that do are:
> 
> nixge
> macb
> altera tse
> axienet
> renesas sh
> ksz884x
> bcm63xx_enet
> sundance
> stmmac
> 
> (compared to more than 100 that don't, and even have a dedicated code
> path for live changes)
> 
> By the way, an an interesting aside - I've found the xgene, atl1c and
> xgmac drivers to be obviously odd (meaning that more drivers might be
> odd in the same way, but in more subtle ways I haven't noticed):
> when netif_running() is false, they simply return 0, but they don't
> change dev->mtu either, they just ignore the request.
> 
> So on one hand you have drivers that _want_ to be down to change the
> MTU, and on the other you have drivers that silently ignore MTU changes
> when they're down. Hard for DSA to do something reasonable to handle
> both cases...

Wonder if I should propose a change for stmmac and just drop the
interface and restart it when the change is down.

-- 
	Ansuel

Powered by blists - more mailing lists