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

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

[Oleksij Rempel]

[-- Attachment #1: Type: text/plain, Size: 1210 bytes --]

Hi Vladimir,

I have a regression after this patch:
|commit bfcb813203e619a8960a819bf533ad2a108d8105                                                      
|Author:     Vladimir Oltean <vladimir.oltean@nxp.com>                                                
|
|  net: dsa: configure the MTU for switch ports

with following log:
[    3.044065] sja1105 spi1.0: Probed switch chip: SJA1105Q                                          
[    3.071385] sja1105 spi1.0: Enabled switch tagging                                                
[    3.076484] sja1105 spi1.0: error -34 setting MTU on port 0                                       
[    3.082795] sja1105: probe of spi1.0 failed with error -34

this is devicetree snippet for the port 0:
	port@0 {
		reg = <0>;
		label = "usb";
		phy-handle = <&usbeth_phy>;
		phy-mode = "rmii";
	};


Is it know issue?

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Vladimir Oltean]

Hi Oleksij,

On Tue, 21 Apr 2020 at 14:33, Oleksij Rempel <o.rempel@pengutronix.de> wrote:
>
> Hi Vladimir,
>
> I have a regression after this patch:
> |commit bfcb813203e619a8960a819bf533ad2a108d8105
> |Author:     Vladimir Oltean <vladimir.oltean@nxp.com>
> |
> |  net: dsa: configure the MTU for switch ports
>
> with following log:
> [    3.044065] sja1105 spi1.0: Probed switch chip: SJA1105Q
> [    3.071385] sja1105 spi1.0: Enabled switch tagging
> [    3.076484] sja1105 spi1.0: error -34 setting MTU on port 0
> [    3.082795] sja1105: probe of spi1.0 failed with error -34
>
> this is devicetree snippet for the port 0:
>         port@0 {
>                 reg = <0>;
>                 label = "usb";
>                 phy-handle = <&usbeth_phy>;
>                 phy-mode = "rmii";
>         };
>
>
> Is it know issue?
>
> Regards,
> Oleksij
> --
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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)

Thanks,
-Vladimir

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

[Oleksij Rempel]

On Tue, Apr 21, 2020 at 02:56:37PM +0300, Vladimir Oltean wrote:
> Hi Oleksij,
> 
> On Tue, 21 Apr 2020 at 14:33, Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> >
> > Hi Vladimir,
> >
> > I have a regression after this patch:
> > |commit bfcb813203e619a8960a819bf533ad2a108d8105
> > |Author:     Vladimir Oltean <vladimir.oltean@nxp.com>
> > |
> > |  net: dsa: configure the MTU for switch ports
> >
> > with following log:
> > [    3.044065] sja1105 spi1.0: Probed switch chip: SJA1105Q
> > [    3.071385] sja1105 spi1.0: Enabled switch tagging
> > [    3.076484] sja1105 spi1.0: error -34 setting MTU on port 0
> > [    3.082795] sja1105: probe of spi1.0 failed with error -34
> >
> > this is devicetree snippet for the port 0:
> >         port@0 {
> >                 reg = <0>;
> >                 label = "usb";
> >                 phy-handle = <&usbeth_phy>;
> >                 phy-mode = "rmii";
> >         };
> >
> >
> > Is it know issue?
> >
 
> 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.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[Andrew Lunn]

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

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