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

[Vladimir Oltean]

On Tue, 21 Apr 2020 at 16:27, Andrew Lunn <andrew@xxxxxxx> 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@iMX6QP 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