On Wed, Dec 16, 2020 at 21:04, Vladimir Oltean [off-list ref] wrote:

On Wed, Dec 16, 2020 at 05:00:55PM +0100, Tobias Waldekranz wrote:

quoted

Support offloading of LAGs to hardware. LAGs may be attached to a
bridge in which case VLANs, multicast groups, etc. are also offloaded
as usual.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c    | 298 +++++++++++++++++++++++++++-
 drivers/net/dsa/mv88e6xxx/global2.c |   8 +-
 drivers/net/dsa/mv88e6xxx/global2.h |   5 +
 drivers/net/dsa/mv88e6xxx/port.c    |  21 ++
 drivers/net/dsa/mv88e6xxx/port.h    |   5 +
 5 files changed, 329 insertions(+), 8 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index eafe6bedc692..bd80b3939157 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c

@@ -1189,7 +1189,8 @@ static int mv88e6xxx_set_mac_eee(struct dsa_switch *ds, int port,
 }
 
 /* Mask of the local ports allowed to receive frames from a given fabric port */
-static u16 mv88e6xxx_port_vlan(struct mv88e6xxx_chip *chip, int dev, int port)
+static u16 mv88e6xxx_port_vlan(struct mv88e6xxx_chip *chip, int dev, int port,
+			       struct net_device **lag)
 {
 	struct dsa_switch *ds = chip->ds;
 	struct dsa_switch_tree *dst = ds->dst;

@@ -1201,6 +1202,9 @@ static u16 mv88e6xxx_port_vlan(struct mv88e6xxx_chip *chip, int dev, int port)
 	list_for_each_entry(dp, &dst->ports, list) {
 		if (dp->ds->index == dev && dp->index == port) {
 			found = true;
+
+			if (dp->lag_dev && lag)
+				*lag = dp->lag_dev;
 			break;
 		}
 	}

I'll let Andrew and Vivien have the decisive word, who are vastly more
familiar with mv88e6xxx than I am, but to me it looks like a bit of a
hack to put this logic here, especially since one of the two callers
(i.e. half) doesn't even care about the LAG.

quoted

@@ -1396,14 +1402,21 @@ static int mv88e6xxx_mac_setup(struct mv88e6xxx_chip *chip)
 
 static int mv88e6xxx_pvt_map(struct mv88e6xxx_chip *chip, int dev, int port)
 {
+	struct net_device *lag = NULL;
 	u16 pvlan = 0;
 
 	if (!mv88e6xxx_has_pvt(chip))
 		return 0;
 
 	/* Skip the local source device, which uses in-chip port VLAN */
-	if (dev != chip->ds->index)
-		pvlan = mv88e6xxx_port_vlan(chip, dev, port);
+	if (dev != chip->ds->index) {
+		pvlan = mv88e6xxx_port_vlan(chip, dev, port, &lag);
+
+		if (lag) {
+			dev = MV88E6XXX_G2_PVT_ADRR_DEV_TRUNK;
+			port = dsa_lag_id(chip->ds->dst, lag);
+		}
+	}

What about the following, which should remove the need of modifying mv88e6xxx_port_vlan:

static int mv88e6xxx_pvt_map(struct mv88e6xxx_chip *chip, int dev, int port)
{
	struct dsa_switch *ds = chip->ds;
	struct net_device *lag = NULL;
	u16 pvlan = 0;

	if (!mv88e6xxx_has_pvt(chip))
		return 0;

	/* Skip the local source device, which uses in-chip port VLAN */
	if (dev != ds->index) {
		pvlan = mv88e6xxx_port_vlan(chip, dev, port);
		struct dsa_switch *other_ds;
		struct dsa_port *other_dp;

		other_ds = dsa_switch_find(ds->dst->index, dev);
		other_dp = dsa_to_port(other_ds, port);

		/* XXX needs an explanation for the reinterpreted values of
		 * dev and port
		 */
		if (other_dp->lag_dev) {
			dev = MV88E6XXX_G2_PVT_ADRR_DEV_TRUNK;
			port = dsa_lag_id(ds->dst, other_dp->lag_dev);
		}
	}

	return mv88e6xxx_g2_pvt_write(chip, dev, port, pvlan);
}

Yeah I agree. This is really a left-over from the RFC that I meant to
clean-up. I will change it for v5.

quoted

 
 	return mv88e6xxx_g2_pvt_write(chip, dev, port, pvlan);
 }

@@ -5375,6 +5388,271 @@ static int mv88e6xxx_port_egress_floods(struct dsa_switch *ds, int port,
 	return err;
 }