[PATCH net 0/3] MTU fixes for mv88e6xxx

[PATCH net 0/3] MTU fixes for mv88e6xxx

[Andrew Lunn]

Adding support for jumbo packets broke MTU change for a couple of
mv88e6xxx family members. The wrong way of configuring the MTU was
used for 6161, and a mixup between MTU and frame size broke other
devices. Additionally, when changing the MTU on the CPU port, the DSA
overhead needs to be taken into account.

Thanks to 曹煜 for reporting and helping debugging these problems.

Andrew Lunn (3):
  dsa: mv88e6xxx: 6161: Use chip wide MAX MTU
  dsa: mv88e6xxx: Fix MTU definition
  net: dsa: Include tagger overhead when setting MTU for DSA and CPU
    ports

 drivers/net/dsa/mv88e6xxx/chip.c | 14 +++++++-------
 drivers/net/dsa/mv88e6xxx/port.c |  2 ++
 net/dsa/switch.c                 | 16 ++++++++++++++--
 3 files changed, 23 insertions(+), 9 deletions(-)

-- 
2.31.1

[PATCH net 1/3] dsa: mv88e6xxx: 6161: Use chip wide MAX MTU

[Andrew Lunn]

The datasheets suggests the 6161 uses a per port setting for jumbo
frames. Testing has however shown this is not correct, it uses the old
style chip wide MTU control. Change the ops in the 6161 structure to
reflect this.

Fixes: 1baf0fac10fb ("net: dsa: mv88e6xxx: Use chip-wide max frame size for MTU")
Reported by: 曹煜 <cao88yu@gmail.com>
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index eca285aaf72f..cbedaee3cefd 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3638,7 +3638,6 @@ static const struct mv88e6xxx_ops mv88e6161_ops = {
 	.port_set_ucast_flood = mv88e6352_port_set_ucast_flood,
 	.port_set_mcast_flood = mv88e6352_port_set_mcast_flood,
 	.port_set_ether_type = mv88e6351_port_set_ether_type,
-	.port_set_jumbo_size = mv88e6165_port_set_jumbo_size,
 	.port_egress_rate_limiting = mv88e6097_port_egress_rate_limiting,
 	.port_pause_limit = mv88e6097_port_pause_limit,
 	.port_disable_learn_limit = mv88e6xxx_port_disable_learn_limit,
@@ -3663,6 +3662,7 @@ static const struct mv88e6xxx_ops mv88e6161_ops = {
 	.avb_ops = &mv88e6165_avb_ops,
 	.ptp_ops = &mv88e6165_ptp_ops,
 	.phylink_validate = mv88e6185_phylink_validate,
+	.set_max_frame_size = mv88e6185_g1_set_max_frame_size,
 };
 
 static const struct mv88e6xxx_ops mv88e6165_ops = {
-- 
2.31.1

[PATCH net 2/3] dsa: mv88e6xxx: Fix MTU definition

[Andrew Lunn]

The MTU passed to the DSA driver is the payload size, typically 1500.
However, the switch uses the frame size when applying restrictions.
Adjust the MTU with the size of the Ethernet header and the frame
checksum.

Fixes: 1baf0fac10fb ("net: dsa: mv88e6xxx: Use chip-wide max frame size for MTU")
Reported by: 曹煜 <cao88yu@gmail.com>
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 12 ++++++------
 drivers/net/dsa/mv88e6xxx/port.c |  2 ++
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index cbedaee3cefd..593dc734582e 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2775,8 +2775,8 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
 	if (err)
 		return err;
 
-	/* Port Control 2: don't force a good FCS, set the maximum frame size to
-	 * 10240 bytes, disable 802.1q tags checking, don't discard tagged or
+	/* Port Control 2: don't force a good FCS, set the MTU size to
+	 * 10222 bytes, disable 802.1q tags checking, don't discard tagged or
 	 * untagged frames on this port, do a destination address lookup on all
 	 * received packets as usual, disable ARP mirroring and don't send a
 	 * copy of all transmitted/received frames on this port to the CPU.
@@ -2795,7 +2795,7 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
 		return err;
 
 	if (chip->info->ops->port_set_jumbo_size) {
-		err = chip->info->ops->port_set_jumbo_size(chip, port, 10240);
+		err = chip->info->ops->port_set_jumbo_size(chip, port, 10222);
 		if (err)
 			return err;
 	}
@@ -2885,10 +2885,10 @@ static int mv88e6xxx_get_max_mtu(struct dsa_switch *ds, int port)
 	struct mv88e6xxx_chip *chip = ds->priv;
 
 	if (chip->info->ops->port_set_jumbo_size)
-		return 10240;
+		return 10240 - ETH_HLEN - ETH_FCS_LEN;
 	else if (chip->info->ops->set_max_frame_size)
-		return 1632;
-	return 1522;
+		return 1632 - ETH_HLEN - ETH_FCS_LEN;
+	return 1522 - ETH_HLEN - ETH_FCS_LEN;
 }
 
 static int mv88e6xxx_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
index f77e2ee64a60..28664ea91244 100644
--- a/drivers/net/dsa/mv88e6xxx/port.c
+++ b/drivers/net/dsa/mv88e6xxx/port.c
@@ -1277,6 +1277,8 @@ int mv88e6165_port_set_jumbo_size(struct mv88e6xxx_chip *chip, int port,
 	u16 reg;
 	int err;
 
+	size += ETH_HLEN + ETH_FCS_LEN;
+
 	err = mv88e6xxx_port_read(chip, port, MV88E6XXX_PORT_CTL2, &reg);
 	if (err)
 		return err;
-- 
2.31.1

[PATCH net 3/3] net: dsa: Include tagger overhead when setting MTU for DSA and CPU ports

[Andrew Lunn]

Same members of the Marvell Ethernet switches impose MTU restrictions
on ports used for connecting to the CPU or DSA. If the MTU is set too
low, tagged frames will be discarded. Ensure the tagger overhead is
included in setting the MTU for DSA and CPU ports.

Fixes: 1baf0fac10fb ("net: dsa: mv88e6xxx: Use chip-wide max frame size for MTU")
Reported by: 曹煜 <cao88yu@gmail.com>
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 net/dsa/switch.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index 9bf8e20ecdf3..48c737b0b802 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -67,14 +67,26 @@ static bool dsa_switch_mtu_match(struct dsa_switch *ds, int port,
 static int dsa_switch_mtu(struct dsa_switch *ds,
 			  struct dsa_notifier_mtu_info *info)
 {
-	int port, ret;
+	struct dsa_port *cpu_dp;
+	int port, ret, overhead;
 
 	if (!ds->ops->port_change_mtu)
 		return -EOPNOTSUPP;
 
 	for (port = 0; port < ds->num_ports; port++) {
 		if (dsa_switch_mtu_match(ds, port, info)) {
-			ret = ds->ops->port_change_mtu(ds, port, info->mtu);
+			overhead = 0;
+			if (dsa_is_cpu_port(ds, port)) {
+				cpu_dp = dsa_to_port(ds, port);
+				overhead = cpu_dp->tag_ops->overhead;
+			}
+			if (dsa_is_dsa_port(ds, port)) {
+					cpu_dp = dsa_to_port(ds, port)->cpu_dp;
+					overhead = cpu_dp->tag_ops->overhead;
+			}
+
+			ret = ds->ops->port_change_mtu(ds, port,
+						       info->mtu + overhead);
 			if (ret)
 				return ret;
 		}
-- 
2.31.1

Re: [PATCH net 2/3] dsa: mv88e6xxx: Fix MTU definition

[Vladimir Oltean]

On Mon, May 24, 2021 at 11:33:12PM +0200, Andrew Lunn wrote:
> The MTU passed to the DSA driver is the payload size, typically 1500.
> However, the switch uses the frame size when applying restrictions.
> Adjust the MTU with the size of the Ethernet header and the frame
> checksum.
> 
> Fixes: 1baf0fac10fb ("net: dsa: mv88e6xxx: Use chip-wide max frame size for MTU")
> Reported by: 曹煜 <cao88yu@gmail.com>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  drivers/net/dsa/mv88e6xxx/chip.c | 12 ++++++------
>  drivers/net/dsa/mv88e6xxx/port.c |  2 ++
>  2 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index cbedaee3cefd..593dc734582e 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -2775,8 +2775,8 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
>  	if (err)
>  		return err;
>  
> -	/* Port Control 2: don't force a good FCS, set the maximum frame size to
> -	 * 10240 bytes, disable 802.1q tags checking, don't discard tagged or
> +	/* Port Control 2: don't force a good FCS, set the MTU size to
> +	 * 10222 bytes, disable 802.1q tags checking, don't discard tagged or
>  	 * untagged frames on this port, do a destination address lookup on all
>  	 * received packets as usual, disable ARP mirroring and don't send a
>  	 * copy of all transmitted/received frames on this port to the CPU.
> @@ -2795,7 +2795,7 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
>  		return err;
>  
>  	if (chip->info->ops->port_set_jumbo_size) {
> -		err = chip->info->ops->port_set_jumbo_size(chip, port, 10240);
> +		err = chip->info->ops->port_set_jumbo_size(chip, port, 10222);
>  		if (err)
>  			return err;
>  	}
> @@ -2885,10 +2885,10 @@ static int mv88e6xxx_get_max_mtu(struct dsa_switch *ds, int port)
>  	struct mv88e6xxx_chip *chip = ds->priv;
>  
>  	if (chip->info->ops->port_set_jumbo_size)
> -		return 10240;
> +		return 10240 - ETH_HLEN - ETH_FCS_LEN;
>  	else if (chip->info->ops->set_max_frame_size)
> -		return 1632;
> -	return 1522;
> +		return 1632 - ETH_HLEN - ETH_FCS_LEN;
> +	return 1522 - ETH_HLEN - ETH_FCS_LEN;
>  }
>  
>  static int mv88e6xxx_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
> diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
> index f77e2ee64a60..28664ea91244 100644
> --- a/drivers/net/dsa/mv88e6xxx/port.c
> +++ b/drivers/net/dsa/mv88e6xxx/port.c
> @@ -1277,6 +1277,8 @@ int mv88e6165_port_set_jumbo_size(struct mv88e6xxx_chip *chip, int port,
>  	u16 reg;
>  	int err;
>  
> +	size += ETH_HLEN + ETH_FCS_LEN;
> +
>  	err = mv88e6xxx_port_read(chip, port, MV88E6XXX_PORT_CTL2, &reg);
>  	if (err)
>  		return err;
> -- 
> 2.31.1
> 

Does the hardware account for VLAN-tagged frames automatically? If not,
it is not uncommon to use VLAN_ETH_HLEN instead of ETH_HLEN (the VLAN
header is not part of the L2 SDU).

Re: [PATCH net 3/3] net: dsa: Include tagger overhead when setting MTU for DSA and CPU ports

[Vladimir Oltean]

On Mon, May 24, 2021 at 11:33:13PM +0200, Andrew Lunn wrote:
> Same members of the Marvell Ethernet switches impose MTU restrictions
> on ports used for connecting to the CPU or DSA. If the MTU is set too
> low, tagged frames will be discarded. Ensure the tagger overhead is
> included in setting the MTU for DSA and CPU ports.
> 
> Fixes: 1baf0fac10fb ("net: dsa: mv88e6xxx: Use chip-wide max frame size for MTU")
> Reported by: 曹煜 <cao88yu@gmail.com>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---

Some switches account for the DSA tag automatically in hardware. So far
it has been the convention that if a switch doesn't do that, the driver
should, not DSA.

>  net/dsa/switch.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/net/dsa/switch.c b/net/dsa/switch.c
> index 9bf8e20ecdf3..48c737b0b802 100644
> --- a/net/dsa/switch.c
> +++ b/net/dsa/switch.c
> @@ -67,14 +67,26 @@ static bool dsa_switch_mtu_match(struct dsa_switch *ds, int port,
>  static int dsa_switch_mtu(struct dsa_switch *ds,
>  			  struct dsa_notifier_mtu_info *info)
>  {
> -	int port, ret;
> +	struct dsa_port *cpu_dp;
> +	int port, ret, overhead;
>  
>  	if (!ds->ops->port_change_mtu)
>  		return -EOPNOTSUPP;
>  
>  	for (port = 0; port < ds->num_ports; port++) {
>  		if (dsa_switch_mtu_match(ds, port, info)) {
> -			ret = ds->ops->port_change_mtu(ds, port, info->mtu);
> +			overhead = 0;
> +			if (dsa_is_cpu_port(ds, port)) {
> +				cpu_dp = dsa_to_port(ds, port);
> +				overhead = cpu_dp->tag_ops->overhead;
> +			}
> +			if (dsa_is_dsa_port(ds, port)) {
> +					cpu_dp = dsa_to_port(ds, port)->cpu_dp;
> +					overhead = cpu_dp->tag_ops->overhead;

Too Much Indentation.

> +			}
> +
> +			ret = ds->ops->port_change_mtu(ds, port,
> +						       info->mtu + overhead);
>  			if (ret)
>  				return ret;
>  		}
> -- 
> 2.31.1
> 

Re: [PATCH net 3/3] net: dsa: Include tagger overhead when setting MTU for DSA and CPU ports

[Andrew Lunn]

On Mon, May 24, 2021 at 10:04:01PM +0000, Vladimir Oltean wrote:
> On Mon, May 24, 2021 at 11:33:13PM +0200, Andrew Lunn wrote:
> > Same members of the Marvell Ethernet switches impose MTU restrictions
> > on ports used for connecting to the CPU or DSA. If the MTU is set too
> > low, tagged frames will be discarded. Ensure the tagger overhead is
> > included in setting the MTU for DSA and CPU ports.
> > 
> > Fixes: 1baf0fac10fb ("net: dsa: mv88e6xxx: Use chip-wide max frame size for MTU")
> > Reported by: 曹煜 <cao88yu@gmail.com>
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > ---
> 
> Some switches account for the DSA tag automatically in hardware. So far
> it has been the convention that if a switch doesn't do that, the driver
> should, not DSA.

O.K.

This is going to be a little bit interesting with Tobias's support for
changing the tag protocol. I need to look at the ordering.

	 Andrew

Re: [PATCH net 3/3] net: dsa: Include tagger overhead when setting MTU for DSA and CPU ports

[Vladimir Oltean]

On Tue, May 25, 2021 at 04:53:39AM +0200, Andrew Lunn wrote:
> On Mon, May 24, 2021 at 10:04:01PM +0000, Vladimir Oltean wrote:
> > On Mon, May 24, 2021 at 11:33:13PM +0200, Andrew Lunn wrote:
> > > Same members of the Marvell Ethernet switches impose MTU restrictions
> > > on ports used for connecting to the CPU or DSA. If the MTU is set too
> > > low, tagged frames will be discarded. Ensure the tagger overhead is
> > > included in setting the MTU for DSA and CPU ports.
> > > 
> > > Fixes: 1baf0fac10fb ("net: dsa: mv88e6xxx: Use chip-wide max frame size for MTU")
> > > Reported by: 曹煜 <cao88yu@gmail.com>
> > > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > > ---
> > 
> > Some switches account for the DSA tag automatically in hardware. So far
> > it has been the convention that if a switch doesn't do that, the driver
> > should, not DSA.
> 
> O.K.
> 
> This is going to be a little bit interesting with Tobias's support for
> changing the tag protocol. I need to look at the ordering.

The dsa_switch_change_tag_proto() notifier handler already iterates
through user ports and calls dsa_slave_change_mtu(), which triggers the
whole shebang (calculates the largest_mtu of the ds, changes the master
MTU to that value plus the tagger overhead, emits a notifier for the CPU
port MTU change and another one for the user port MTU change, then
triggers the MTU bridge normalization logic if the MTU is in fact a
MRU).

So when the tagging protocol changes, you get re-notified to change the
MTU on the CPU port to the largest_mtu. That part should work correctly.

What could be interesting, and this is something I had to check, is to
see if the proper MTU values are propagated correctly to the DSA links.
dsa_slave_change_mtu() calls dsa_port_mtu_change() with
propagate_upstream == true for the CPU port (which is programmed with
the largest_mtu of the switch) and that triggers this matching logic:

static bool dsa_switch_mtu_match(struct dsa_switch *ds, int port,
				 struct dsa_notifier_mtu_info *info)
{
	if (ds->index == info->sw_index)
		return (port == info->port) || dsa_is_dsa_port(ds, port);

	if (!info->propagate_upstream)
		return false;

	if (dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port)) <- returns true for all DSA links here
		return true;

	return false;
}

the "propagate_upstream" is a bit of a misnomer, since it is "propagate
to other switches" - what I really needed at the time, with
"propagate_upstream == false", was a way to send a targeted MTU change
(for the user port itself) that bypasses the cross-chip notifiers.

My updated cross-chip notifier simulator
(https://patchwork.kernel.org/project/netdevbpf/patch/20210222120248.1415075-1-olteanv@gmail.com/)
shows this "heat map" for a notifier emitted on the CPU port (port 0 of
switch 0) with propagate_upstream == true:

Heat map for test notifier emitted on sw0p0:

   sw0p0     sw0p1     sw0p2     sw0p3     sw0p4
[  cpu  ] [  user ] [  user ] [  dsa  ] [  user ]
[   x   ] [       ] [       ] [   x   ] [       ]
                                  |
                                  +---------+
                                            |
   sw1p0     sw1p1     sw1p2     sw1p3     sw1p4
[  user ] [  user ] [  user ] [  dsa  ] [  dsa  ]
[       ] [       ] [       ] [   x   ] [   x   ]
                                  |
                                  +---------+
                                            |
   sw2p0     sw2p1     sw2p2     sw2p3     sw2p4
[  user ] [  user ] [  user ] [  user ] [  dsa  ]
[       ] [       ] [       ] [       ] [   x   ]

So the largest_mtu in this case ends up being programmed in all DSA
links.

There are 2 potential problems I see:
(a) the largest_mtu is calculated as the maximum over all user ports of
    @ds. But since it is propagated in the entire tree, maybe it should
    be the maximum across the entire @dst, to avoid this situation:

ip link set sw0p1 mtu 9000
ip link set sw1p1 mtu 1500 # oops, this changes the largest_mtu of the CPU port, breaking termination for sw0p1

(b) I don't remember why I didn't make the targeted notifier
    (propagate_upstream == false) even more targeted towards only the
    port on which it was emitted. Instead DSA links of that switch are
    targeted too, and that is probably a mistake:

	if (ds->index == info->sw_index)
		return (port == info->port) || dsa_is_dsa_port(ds, port);

because if the DSA links of the entire dst were programmed in a previous
round to the largest_mtu via a "propagate_upstream == true" notification,
then the dsa_port_mtu_change(propagate_upstream == false) call that is
immediately upcoming will break the MTU on the one DSA link which is
chip-wise local to the dp whose MTU is changing right now.

Example:

ip link set sw0p1 mtu 9000
ip link set sw2p1 mtu 9000 # at this stage, sw0p1 and sw2p1 can talk to one another using jumbo frames
ip link set sw0p2 mtu 1500 # this programs the sw0p3 DSA link first to
                           # the largest_mtu of 9000, then reprograms it to 1500 with the
                           # "propagate_upstream == false" notifier, breaking communication between
                           # sw0p1 and sw2p1