Message-ID: <20220810133531.wia2oznylkjrgje2@skbuf>
Date:   Wed, 10 Aug 2022 16:35:31 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Sergei Antonov <saproj@...il.com>
Cc:     netdev@...r.kernel.org, Florian Fainelli <f.fainelli@...il.com>
Subject: Re: [PATCH] net: dsa: mv88e6060: report max mtu 1536

On Wed, Aug 10, 2022 at 03:00:20PM +0300, Sergei Antonov wrote:
> > >       val = addr[0] << 8 | addr[1];
> > >
> > >       /* The multicast bit is always transmitted as a zero, so the switch uses
> > > @@ -212,6 +211,11 @@ static int mv88e6060_setup(struct dsa_switch *ds)
> > >       return 0;
> > >  }
> > >
> > > +static int mv88e6060_port_max_mtu(struct dsa_switch *ds, int port)
> > > +{
> > > +     return MV88E6060_MAX_MTU;
> > > +}
> >
> > Does this solve any problem? It's ok for the hardware MTU to be higher
> > than advertised. The problem is when the hardware doesn't accept what
> > the stack thinks it should.
> 
> I need some time to reconstruct the problem. IIRC there was an attempt
> to set MTU 1504 (1500 + a switch overhead), but can not reproduce it
> at the moment.

What kernel are you using? According to Documentation/process/maintainer-netdev.rst,
you should test the patches you submit against the master branch from one of
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git
or
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git
depending on whether it's a new feature or if it fixes a problem.

Currently, both net and net-next contain the same thing (we are in a
merge window so net-next will not progress until kernel 6.0-rc1 is cut),
which is that dsa_slave_change_mtu() will not do anything because of
this:

	if (!ds->ops->port_change_mtu)
		return -EOPNOTSUPP;

(which mv88e6060 does not implement)

So I am slightly doubtful that anyone attempts an MTU change for this
switch, as you say.

The DSA master (host port, not switch), on the other hand, is a
different story. Its MTU is updated to 1504 by dsa_master_setup().

> > You're the first person to submit a patch on mv88e6060 that I see.
> > Is there a board with this switch available somewhere? Does the driver
> > still work?
> 
> Very nice to get your feedback. Because, yes, I am working with a
> device which has mv88e6060, it is called MOXA NPort 6610.
> 
> The driver works now. There was one problem which I had to workaround.
> Inside my device only ports 2 and 5 are used, so I initially wrote in
> .dts:
>         switch@0 {
>                 compatible = "marvell,mv88e6060";
>                 reg = <16>;

reg = <16> for switch@0? Something is wrong, probably switch@0.

> 
>                 ports {
>                         #address-cells = <1>;
>                         #size-cells = <0>;
> 
>                         port@2 {
>                                 reg = <2>;
>                                 label = "lan2";
>                         };
> 
>                         port@5 {
>                                 reg = <5>;
>                                 label = "cpu";
>                                 ethernet = <&mac1>;
>                         };
>                 };
>         };
> and the driver crashed in mv88e6060_setup_port() on a null pointer.
> Two workarounds are possible:
> 1. Describe ports 0, 1, 3, 4 in .dts too.

No, it should work with just port@2 and port@5 defined.

> 2. Insert this code at the beginning of mv88e6060_setup_port():
> if(!dsa_is_cpu_port(priv->ds, p) && !dsa_to_port(priv->ds, p)->cpu_dp)
>     return 0;
> 'cpu_dp' was the null pointer the driver crashed at.

You mean here:

			(dsa_is_cpu_port(priv->ds, p) ?
			 dsa_user_ports(priv->ds) :
			 BIT(dsa_to_port(priv->ds, p)->cpu_dp->index)));

Yes, this is a limitation that has been made worse by blind code
conversions (nobody seems to have the hardware or to know someone who
does; I've been tempted to delete the driver a few times or at least to
move it to staging, because of the unrealistically long delays until
someone chirps that something is broken for it, even when it obviously is).
The driver assumes that if the port isn't a CPU port, it's a user port.
That's clearly false.

You can probably put this at the beginning of mv88e6060_setup_port():

	if (dsa_is_unused_port(priv->ds, p))
		return 0;

The bug seems to have been introduced by commit 0abfd494deef ("net: dsa:
use dedicated CPU port"), because, although before we'd be uselessly
programming the port VLAN for a disabled port, now in doing so, we
dereference a NULL pointer.

FWIW, in case there is ever a need to backport, the vintage-correct fix
would be to use something like this:

	if (!dsa_port_is_valid(priv->ds->ports[p]))
		return 0;

but in that case the process is:
- send patch against current "net" tree
- wait until patch is queued up for "linux-stable" and backported as far
  as possible
- email will be sent that patch failed to apply to the still-maintained
  LTS branches as far as the Fixes: tag required (this is why it is
  important to populate the Fixes: tag correctly)
- reply to that email with a manually backported patch, just for that
  stable tree (linux-4.14.y etc)

> 
> One more observation. Generating and setting a random MAC in
> mv88e6060_setup_addr() is not necessary - the switch works without it
> (at least in my case).

The GLOBAL_MAC address that the switch uses there will be used as MAC SA
in PAUSE frames (802.3 flow control). Not clear if you were aware of
that fact when saying that the switch "works without it". In other words,
if you make a change in that area, I expect that flow control is what
you test, and not, say, ping.

It's true that some other switches use a MAC SA of 00:00:00:00:00:00 for
PAUSE frames (ocelot_init_port) and this hasn't caused a problem for them.
I don't know if the 6060 supports this mode. If it does, it's worth a shot.

Powered by blists - more mailing lists