[PATCH net 0/2] MTU fixes for DSA

[PATCH net 0/2] MTU fixes for DSA

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

This small series aims to address the complaints about a regression in
DSA that were formulated here:

https://lore.kernel.org/netdev/CA+h21ho2YnUfzMja1Y7=B7Yrqk=WD6jm-OoKKzX4uS3WJiU5aw@mail.gmail.com/T/#t

Vladimir Oltean (2):
  net: dsa: be compatible with DSA masters with max_mtu of 1500 or less
  net: dsa: don't fail to probe if we couldn't set the MTU

 net/dsa/slave.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

-- 
2.17.1

[PATCH net 1/2] net: dsa: be compatible with DSA masters with max_mtu of 1500 or less

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

It would be ideal if the DSA switch ports would support an MTU of 1500
bytes by default, same as any other net device. But there are 2 cases of
issues with trying to do that:

- Drivers that are legitimately MTU-challenged and don't support
  anything larger than ETH_DATA_LEN. A very quick search shows that
  sungem.c is one such example - there may be many others.

- Drivers that simply don't populate netdev->max_mtu. In that case, it
  seems that the ether_setup function sets dev->max_mtu to a default
  value of ETH_DATA_LEN. And due to the above cases which really are
  MTU-challenged, we can't really make any guesses.

So for these cases, if the max_mtu of the master net_device is lower
than 1500, use that (minus the tagger overhead) as the max MTU of the
switch ports.

Fixes: bfcb813203e6 ("net: dsa: configure the MTU for switch ports")
Reported-by: Oleksij Rempel <o.rempel@pengutronix.de>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/slave.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index e94eb1aac602..3776a6f6d312 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1723,6 +1723,7 @@ int dsa_slave_create(struct dsa_port *port)
 	const char *name = port->name;
 	struct net_device *slave_dev;
 	struct dsa_slave_priv *p;
+	int mtu;
 	int ret;
 
 	if (!ds->num_tx_queues)
@@ -1767,8 +1768,10 @@ int dsa_slave_create(struct dsa_port *port)
 	p->xmit = cpu_dp->tag_ops->xmit;
 	port->slave = slave_dev;
 
+	mtu = min_t(int, master->max_mtu - cpu_dp->tag_ops->overhead,
+		    ETH_DATA_LEN);
 	rtnl_lock();
-	ret = dsa_slave_change_mtu(slave_dev, ETH_DATA_LEN);
+	ret = dsa_slave_change_mtu(slave_dev, mtu);
 	rtnl_unlock();
 	if (ret && ret != -EOPNOTSUPP) {
 		dev_err(ds->dev, "error %d setting MTU on port %d\n",
-- 
2.17.1

Re: [PATCH net 1/2] net: dsa: be compatible with DSA masters with max_mtu of 1500 or less

[Andrew Lunn]

On Tue, Apr 21, 2020 at 03:31:09PM +0300, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> It would be ideal if the DSA switch ports would support an MTU of 1500
> bytes by default, same as any other net device. But there are 2 cases of
> issues with trying to do that:
> 
> - Drivers that are legitimately MTU-challenged and don't support
>   anything larger than ETH_DATA_LEN. A very quick search shows that
>   sungem.c is one such example - there may be many others.
> 
> - Drivers that simply don't populate netdev->max_mtu. In that case, it
>   seems that the ether_setup function sets dev->max_mtu to a default
>   value of ETH_DATA_LEN. And due to the above cases which really are
>   MTU-challenged, we can't really make any guesses.
> 
> So for these cases, if the max_mtu of the master net_device is lower
> than 1500, use that (minus the tagger overhead) as the max MTU of the
> switch ports.

I don't like this. I suspect this will also break in subtle ways.

Please go back to the original behaviour. Make the call to request the
minimum needed for DSA. And don't care at all if it fails. For jumbo
frames then you can error out.

       Andrew

Re: [PATCH net 1/2] net: dsa: be compatible with DSA masters with max_mtu of 1500 or less

[Vladimir Oltean]

Hi Andrew,

On Tue, 21 Apr 2020 at 16:33, Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Tue, Apr 21, 2020 at 03:31:09PM +0300, Vladimir Oltean wrote:
> > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> >
> > It would be ideal if the DSA switch ports would support an MTU of 1500
> > bytes by default, same as any other net device. But there are 2 cases of
> > issues with trying to do that:
> >
> > - Drivers that are legitimately MTU-challenged and don't support
> >   anything larger than ETH_DATA_LEN. A very quick search shows that
> >   sungem.c is one such example - there may be many others.
> >
> > - Drivers that simply don't populate netdev->max_mtu. In that case, it
> >   seems that the ether_setup function sets dev->max_mtu to a default
> >   value of ETH_DATA_LEN. And due to the above cases which really are
> >   MTU-challenged, we can't really make any guesses.
> >
> > So for these cases, if the max_mtu of the master net_device is lower
> > than 1500, use that (minus the tagger overhead) as the max MTU of the
> > switch ports.
>
> I don't like this. I suspect this will also break in subtle ways.
>
> Please go back to the original behaviour. Make the call to request the
> minimum needed for DSA.

In what sense "minimum needed"? It is minimum needed. If
master->max_mtu is 1500, the MTU will be set to 1496.

> And don't care at all if it fails. For jumbo
> frames then you can error out.

Yes, that is patch 2/2.

>
>        Andrew

Thanks,
-Vladimir

Re: [PATCH net 1/2] net: dsa: be compatible with DSA masters with max_mtu of 1500 or less

[Andrew Lunn]

On Tue, Apr 21, 2020 at 04:42:41PM +0300, Vladimir Oltean wrote:
> Hi Andrew,
> 
> On Tue, 21 Apr 2020 at 16:33, Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > On Tue, Apr 21, 2020 at 03:31:09PM +0300, Vladimir Oltean wrote:
> > > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> > >
> > > It would be ideal if the DSA switch ports would support an MTU of 1500
> > > bytes by default, same as any other net device. But there are 2 cases of
> > > issues with trying to do that:
> > >
> > > - Drivers that are legitimately MTU-challenged and don't support
> > >   anything larger than ETH_DATA_LEN. A very quick search shows that
> > >   sungem.c is one such example - there may be many others.
> > >
> > > - Drivers that simply don't populate netdev->max_mtu. In that case, it
> > >   seems that the ether_setup function sets dev->max_mtu to a default
> > >   value of ETH_DATA_LEN. And due to the above cases which really are
> > >   MTU-challenged, we can't really make any guesses.
> > >
> > > So for these cases, if the max_mtu of the master net_device is lower
> > > than 1500, use that (minus the tagger overhead) as the max MTU of the
> > > switch ports.
> >
> > I don't like this. I suspect this will also break in subtle ways.
> >
> > Please go back to the original behaviour. Make the call to request the
> > minimum needed for DSA.
> 
> In what sense "minimum needed"? It is minimum needed. If
> master->max_mtu is 1500, the MTU will be set to 1496.

Ah, sorry. This is the slave. I was thinking it was the master.

We have always assumed the slave can send normal sized frames,
independent of what the master supports. This is just a follow on from
the fact we ignore errors setting the MTU on the master for the DSA
overhead for normal size frames. So don't set the MTU to 1496, leave
it at 1500. For all working boards out in the wild, 1500 will work.

	 Andrew

Re: [PATCH net 1/2] net: dsa: be compatible with DSA masters with max_mtu of 1500 or less

[Vladimir Oltean]

On Tue, 21 Apr 2020 at 17:06, Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Tue, Apr 21, 2020 at 04:42:41PM +0300, Vladimir Oltean wrote:
> > Hi Andrew,
> >
> > On Tue, 21 Apr 2020 at 16:33, Andrew Lunn <andrew@lunn.ch> wrote:
> > >
> > > On Tue, Apr 21, 2020 at 03:31:09PM +0300, Vladimir Oltean wrote:
> > > > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > >
> > > > It would be ideal if the DSA switch ports would support an MTU of 1500
> > > > bytes by default, same as any other net device. But there are 2 cases of
> > > > issues with trying to do that:
> > > >
> > > > - Drivers that are legitimately MTU-challenged and don't support
> > > >   anything larger than ETH_DATA_LEN. A very quick search shows that
> > > >   sungem.c is one such example - there may be many others.
> > > >
> > > > - Drivers that simply don't populate netdev->max_mtu. In that case, it
> > > >   seems that the ether_setup function sets dev->max_mtu to a default
> > > >   value of ETH_DATA_LEN. And due to the above cases which really are
> > > >   MTU-challenged, we can't really make any guesses.
> > > >
> > > > So for these cases, if the max_mtu of the master net_device is lower
> > > > than 1500, use that (minus the tagger overhead) as the max MTU of the
> > > > switch ports.
> > >
> > > I don't like this. I suspect this will also break in subtle ways.
> > >
> > > Please go back to the original behaviour. Make the call to request the
> > > minimum needed for DSA.
> >
> > In what sense "minimum needed"? It is minimum needed. If
> > master->max_mtu is 1500, the MTU will be set to 1496.
>
> Ah, sorry. This is the slave. I was thinking it was the master.
>
> We have always assumed the slave can send normal sized frames,
> independent of what the master supports. This is just a follow on from
> the fact we ignore errors setting the MTU on the master for the DSA
> overhead for normal size frames. So don't set the MTU to 1496, leave
> it at 1500. For all working boards out in the wild, 1500 will work.
>
>          Andrew

Does iperf3 TCP work on your Vybrid board with the master MTU equal to
the slave MTU equal to 1500 (without IP fragmentation, that is)? If it
does, ok, this patch can maybe be dropped.

qca7000 doesn't support packets larger than 1500 MTU either, neither
does broadcom b44, and neither do probably more adapters which I
didn't find now.

Why would I not limit the MTU to 1496 if the master's max_mtu is 1500?
Just to provoke more warnings in people's logs? I just don't want to
have further issue reports, but I am basically reading your reply as
"don't try to be correct". If you don't like this behavior for FEC,
you can set its max_mtu since it doesn't do that by itself.

Thanks,
-Vladimir

Re: [PATCH net 1/2] net: dsa: be compatible with DSA masters with max_mtu of 1500 or less

[Andrew Lunn]

> > We have always assumed the slave can send normal sized frames,
> > independent of what the master supports. This is just a follow on from
> > the fact we ignore errors setting the MTU on the master for the DSA
> > overhead for normal size frames. So don't set the MTU to 1496, leave
> > it at 1500. For all working boards out in the wild, 1500 will work.
> >
> >          Andrew
> 
> Does iperf3 TCP work on your Vybrid board with the master MTU equal to
> the slave MTU equal to 1500 (without IP fragmentation, that is)? If it
> does, ok, this patch can maybe be dropped.

Yes it does.

> qca7000 doesn't support packets larger than 1500 MTU either, neither
> does broadcom b44, and neither do probably more adapters which I
> didn't find now.

And unfortunately, there are probably a similar number of devices
which do work, and don't have correct MTU settings. So long as the IP
stack does MTU correctly, it does not matter what the network device
does with frames bigger than the MTU. And so network devices whos MTU
handling is wrong never causes issues, and so never get detected and
fixed.

We have to live in a world where trying to be correct is going to
cause regressions because of poor decisions in the past. But we can do
jumbo correctly, since it is new, it cannot regress. And doing jumbo
correct will help find some of these network drivers which do MTU
settings wrong.

      Andrew

[PATCH net 2/2] net: dsa: don't fail to probe if we couldn't set the MTU

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

There is no reason to fail the probing of the switch if the MTU couldn't
be configured correctly (either the switch port itself, or the host
port) for whatever reason. MTU-sized traffic probably won't work, sure,
but we can still probably limp on and support some form of communication
anyway, which the users would probably appreciate more.

Fixes: bfcb813203e6 ("net: dsa: configure the MTU for switch ports")
Reported-by: Oleksij Rempel <o.rempel@pengutronix.de>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/slave.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 3776a6f6d312..34071d9bab3f 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1773,11 +1773,9 @@ int dsa_slave_create(struct dsa_port *port)
 	rtnl_lock();
 	ret = dsa_slave_change_mtu(slave_dev, mtu);
 	rtnl_unlock();
-	if (ret && ret != -EOPNOTSUPP) {
+	if (ret)
 		dev_err(ds->dev, "error %d setting MTU on port %d\n",
 			ret, port->index);
-		goto out_free;
-	}
 
 	netif_carrier_off(slave_dev);
 
-- 
2.17.1