Message-ID: <20220324104524.ou7jyqcbfj3fhpvo@skbuf>
Date:   Thu, 24 Mar 2022 12:45:24 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Ansuel Smith <ansuelsmth@...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 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?

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

Powered by blists - more mailing lists