netdev - Re: dsa: sja1105: regression after patch: "net: dsa: configure the MTU for switch ports"

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CA+h21hoH4u4TGMPPGpuF9dgW5SHd3DYm4mR8AMmuVs=nevYSYg@mail.gmail.com>
Date:   Tue, 21 Apr 2020 16:35:40 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Oleksij Rempel <o.rempel@...gutronix.de>, mkl@...gutronix.de,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>, kernel@...gutronix.de,
        netdev <netdev@...r.kernel.org>,
        lkml <linux-kernel@...r.kernel.org>,
        David Jander <david@...tonic.nl>,
        Russell King - ARM Linux admin <linux@...linux.org.uk>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: dsa: sja1105: regression after patch: "net: dsa: configure the
 MTU for switch ports"

On Tue, 21 Apr 2020 at 16:27, Andrew Lunn <andrew@...n.ch> wrote:
>
> > > The code which is causing problems seems to be this one:
> > >
> > >     mtu_limit = min_t(int, master->max_mtu, dev->max_mtu);
> > >     old_master_mtu = master->mtu;
> > >     new_master_mtu = largest_mtu + cpu_dp->tag_ops->overhead;
> > >     if (new_master_mtu > mtu_limit)
> > >         return -ERANGE;
> > >
> > > called from
> > >
> > >     rtnl_lock();
> > >     ret = dsa_slave_change_mtu(slave_dev, ETH_DATA_LEN);
> > >     rtnl_unlock();
> > >     if (ret && ret != -EOPNOTSUPP) {
> > >         dev_err(ds->dev, "error %d setting MTU on port %d\n",
> > >             ret, port->index);
> > >         goto out_free;
> > >     }
> > >
> > > Before this patch, it was silently failing, now it's preventing the
> > > probing of the ports which I might agree with you is not better.
> > > Andrew warned about this, and I guess that during probe, we should
> > > warn but ignore any nonzero return code, not just EOPNOTSUPP. I'll
> > > send a patch out shortly to correct this.
> > >
> > > Out of curiosity, what DSA master port do you have? Does it not
> > > support an MTU of 1504 bytes? Does MTU-sized traffic pass correctly
> > > through your interface? (you can test with iperf3)
> >
> > It is FEC@...6QP attached to the port 4 of the sja1105 switch.
> > I'll try to make some tests tomorrow.
>
> Ah, interesting. I've been testing recently on a Vybrid, so also
> FEC. I had the warning, but it kept going.
>
> I don't particularly like this warning in this case. We have hardware
> which happy works, but is now issuing a warning on boot. I would
> prefer if it warned when only trying to configure an MTU bigger than
> the minimum needed for DSA, i.e. only the jumbo use case.
>
>     Andrew

Looks like FEC is one of those drivers that don't touch
netdev->max_mtu. So I sent a patch to reduce your switch MTU to 1496
or whereabouts. About the error, I caved in and turned it into an
warning, but with the new logic of limiting the MTU on bootup to the
limit given by the master there is really no reason to fail now, so I
think we shouldn't remove the print.

Thanks,
-Vladimir