[PATCH net-next 1/5] net: dsa: mv88e6xxx: Improve isolation of standalone ports

[PATCH net-next 1/5] net: dsa: mv88e6xxx: Improve isolation of standalone ports

[Tobias Waldekranz]

Clear MapDA on standalone ports to bypass any ATU lookup that might
point the packet in the wrong direction. This means that all packets
are flooded using the PVT config. So make sure that standalone ports
are only allowed to communicate with the CPU port.

Here is a scenario in which this is needed:

   CPU
    |     .----.
.---0---. | .--0--.
|  sw0  | | | sw1 |
'-1-2-3-' | '-1-2-'
      '---'

- sw0p1 and sw1p1 are bridged
- sw0p2 and sw1p2 are in standalone mode
- Learning must be enabled on sw0p3 in order for hardware forwarding
  to work properly between bridged ports

1. A packet with SA :aa comes in on sw1p2
   1a. Egresses sw1p0
   1b. Ingresses sw0p3, ATU adds an entry for :aa towards port 3
   1c. Egresses sw0p0

2. A packet with DA :aa comes in on sw0p2
   2a. If an ATU lookup is done at this point, the packet will be
       incorrectly forwarded towards sw0p3. With this change in place,
       the ATU is pypassed and the packet is forwarded in accordance
       whith the PVT, which only contains the CPU port.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 32 +++++++++++++++++++++++++-------
 drivers/net/dsa/mv88e6xxx/port.c |  7 +++++--
 drivers/net/dsa/mv88e6xxx/port.h |  2 +-
 include/net/dsa.h                | 12 ++++++++++++
 4 files changed, 43 insertions(+), 10 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 1023e4549359..dde6a8d0ca36 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1290,8 +1290,15 @@ static u16 mv88e6xxx_port_vlan(struct mv88e6xxx_chip *chip, int dev, int port)
 
 	pvlan = 0;
 
-	/* Frames from user ports can egress any local DSA links and CPU ports,
-	 * as well as any local member of their bridge group.
+	/* Frames from standalone user ports can only egress on the
+	 * CPU port.
+	 */
+	if (!dsa_port_bridge_dev_get(dp))
+		return BIT(dsa_switch_upstream_port(ds));
+
+	/* Frames from bridged user ports can egress any local DSA
+	 * links and CPU ports, as well as any local member of their
+	 * bridge group.
 	 */
 	dsa_switch_for_each_port(other_dp, ds)
 		if (other_dp->type == DSA_PORT_TYPE_CPU ||
@@ -2487,6 +2494,10 @@ static int mv88e6xxx_port_bridge_join(struct dsa_switch *ds, int port,
 	if (err)
 		goto unlock;
 
+	err = mv88e6xxx_port_set_map_da(chip, port, true);
+	if (err)
+		return err;
+
 	err = mv88e6xxx_port_commit_pvid(chip, port);
 	if (err)
 		goto unlock;
@@ -2521,6 +2532,12 @@ static void mv88e6xxx_port_bridge_leave(struct dsa_switch *ds, int port,
 	    mv88e6xxx_port_vlan_map(chip, port))
 		dev_err(ds->dev, "failed to remap in-chip Port VLAN\n");
 
+	err = mv88e6xxx_port_set_map_da(chip, port, false);
+	if (err)
+		dev_err(ds->dev,
+			"port %d failed to restore map-DA: %pe\n",
+			port, ERR_PTR(err));
+
 	err = mv88e6xxx_port_commit_pvid(chip, port);
 	if (err)
 		dev_err(ds->dev,
@@ -2918,12 +2935,13 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
 		return err;
 
 	/* 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.
+	 * 10222 bytes, disable 802.1q tags checking, don't discard
+	 * tagged or untagged frames on this port, skip destination
+	 * address lookup on user ports, disable ARP mirroring and don't
+	 * send a copy of all transmitted/received frames on this port
+	 * to the CPU.
 	 */
-	err = mv88e6xxx_port_set_map_da(chip, port);
+	err = mv88e6xxx_port_set_map_da(chip, port, !dsa_is_user_port(ds, port));
 	if (err)
 		return err;
 
diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
index ab41619a809b..ceb450113f88 100644
--- a/drivers/net/dsa/mv88e6xxx/port.c
+++ b/drivers/net/dsa/mv88e6xxx/port.c
@@ -1278,7 +1278,7 @@ int mv88e6xxx_port_drop_untagged(struct mv88e6xxx_chip *chip, int port,
 	return mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_CTL2, new);
 }
 
-int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port)
+int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port, bool map)
 {
 	u16 reg;
 	int err;
@@ -1287,7 +1287,10 @@ int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port)
 	if (err)
 		return err;
 
-	reg |= MV88E6XXX_PORT_CTL2_MAP_DA;
+	if (map)
+		reg |= MV88E6XXX_PORT_CTL2_MAP_DA;
+	else
+		reg &= ~MV88E6XXX_PORT_CTL2_MAP_DA;
 
 	return mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_CTL2, reg);
 }
diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h
index 03382b66f800..5c347cc58baf 100644
--- a/drivers/net/dsa/mv88e6xxx/port.h
+++ b/drivers/net/dsa/mv88e6xxx/port.h
@@ -425,7 +425,7 @@ int mv88e6185_port_get_cmode(struct mv88e6xxx_chip *chip, int port, u8 *cmode);
 int mv88e6352_port_get_cmode(struct mv88e6xxx_chip *chip, int port, u8 *cmode);
 int mv88e6xxx_port_drop_untagged(struct mv88e6xxx_chip *chip, int port,
 				 bool drop_untagged);
-int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port);
+int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port, bool map);
 int mv88e6095_port_set_upstream_port(struct mv88e6xxx_chip *chip, int port,
 				     int upstream_port);
 int mv88e6xxx_port_set_mirror(struct mv88e6xxx_chip *chip, int port,
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 57b3e4e7413b..30f3192616e5 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -581,6 +581,18 @@ static inline bool dsa_is_upstream_port(struct dsa_switch *ds, int port)
 	return port == dsa_upstream_port(ds, port);
 }
 
+/* Return the local port used to reach the CPU port */
+static inline unsigned int dsa_switch_upstream_port(struct dsa_switch *ds)
+{
+	int p;
+
+	for (p = 0; p < ds->num_ports; p++)
+		if (!dsa_is_unused_port(ds, p))
+			return dsa_upstream_port(ds, p);
+
+	return ds->num_ports;
+}
+
 /* Return true if @upstream_ds is an upstream switch of @downstream_ds, meaning
  * that the routing port from @downstream_ds to @upstream_ds is also the port
  * which @downstream_ds uses to reach its dedicated CPU.
-- 
2.25.1

[PATCH net-next 2/5] net: dsa: mv88e6xxx: Support policy entries in the VTU

[Tobias Waldekranz]

A VTU entry with policy enabled is used in combination with a port's
VTU policy setting to override normal switching behavior for frames
assigned to the entry's VID.

A typical example is to Treat all frames in a particular VLAN as
control traffic, and trap them to the CPU. In which case the relevant
user port's VTU policy would be set to TRAP.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/net/dsa/mv88e6xxx/chip.h        | 1 +
 drivers/net/dsa/mv88e6xxx/global1.h     | 1 +
 drivers/net/dsa/mv88e6xxx/global1_vtu.c | 5 ++++-
 3 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index 438cee853d07..80dc7b549e81 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -179,6 +179,7 @@ struct mv88e6xxx_vtu_entry {
 	u16	fid;
 	u8	sid;
 	bool	valid;
+	bool	policy;
 	u8	member[DSA_MAX_PORTS];
 	u8	state[DSA_MAX_PORTS];
 };
diff --git a/drivers/net/dsa/mv88e6xxx/global1.h b/drivers/net/dsa/mv88e6xxx/global1.h
index 4f3dbb015f77..2c1607c858a1 100644
--- a/drivers/net/dsa/mv88e6xxx/global1.h
+++ b/drivers/net/dsa/mv88e6xxx/global1.h
@@ -46,6 +46,7 @@
 
 /* Offset 0x02: VTU FID Register */
 #define MV88E6352_G1_VTU_FID		0x02
+#define MV88E6352_G1_VTU_FID_VID_POLICY	0x1000
 #define MV88E6352_G1_VTU_FID_MASK	0x0fff
 
 /* Offset 0x03: VTU SID Register */
diff --git a/drivers/net/dsa/mv88e6xxx/global1_vtu.c b/drivers/net/dsa/mv88e6xxx/global1_vtu.c
index ae12c981923e..b1bd9274a562 100644
--- a/drivers/net/dsa/mv88e6xxx/global1_vtu.c
+++ b/drivers/net/dsa/mv88e6xxx/global1_vtu.c
@@ -27,7 +27,7 @@ static int mv88e6xxx_g1_vtu_fid_read(struct mv88e6xxx_chip *chip,
 		return err;
 
 	entry->fid = val & MV88E6352_G1_VTU_FID_MASK;
-
+	entry->policy = !!(val & MV88E6352_G1_VTU_FID_VID_POLICY);
 	return 0;
 }
 
@@ -36,6 +36,9 @@ static int mv88e6xxx_g1_vtu_fid_write(struct mv88e6xxx_chip *chip,
 {
 	u16 val = entry->fid & MV88E6352_G1_VTU_FID_MASK;
 
+	if (entry->policy)
+		val |= MV88E6352_G1_VTU_FID_VID_POLICY;
+
 	return mv88e6xxx_g1_write(chip, MV88E6352_G1_VTU_FID, val);
 }
 
-- 
2.25.1

[PATCH net-next 3/5] net: dsa: mv88e6xxx: Enable port policy support on 6097

[Tobias Waldekranz]

This chip has support for the same per-port policy actions found in
later versions of LinkStreet devices.

Fixes: f3a2cd326e44 ("net: dsa: mv88e6xxx: introduce .port_set_policy")
Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index dde6a8d0ca36..8896709b9103 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3652,6 +3652,7 @@ static const struct mv88e6xxx_ops mv88e6097_ops = {
 	.port_sync_link = mv88e6185_port_sync_link,
 	.port_set_speed_duplex = mv88e6185_port_set_speed_duplex,
 	.port_tag_remap = mv88e6095_port_tag_remap,
+	.port_set_policy = mv88e6352_port_set_policy,
 	.port_set_frame_mode = mv88e6351_port_set_frame_mode,
 	.port_set_ucast_flood = mv88e6352_port_set_ucast_flood,
 	.port_set_mcast_flood = mv88e6352_port_set_mcast_flood,
-- 
2.25.1

[PATCH net-next 4/5] net: dsa: mv88e6xxx: Improve multichip isolation of standalone ports

[Tobias Waldekranz]

Given that standalone ports are now configured to bypass the ATU and
forward all frames towards the upstream port, extend the ATU bypass to
multichip systems.

Load VID 0 (standalone) into the VTU with the policy bit set. Since
VID 4095 (bridged) is already loaded, we now know that all VIDs in use
are always available in all VTUs. Therefore, we can safely enable
802.1Q on DSA ports.

Setting the DSA ports' VTU policy to TRAP means that all incoming
frames on VID 0 will be classified as MGMT - as a result, the ATU is
bypassed on all subsequent switches.

With this isolation in place, we are able to support configurations
that are simultaneously very quirky and very useful. Quirky because it
involves looping cables between local switchports like in this
example:

   CPU
    |     .------.
.---0---. | .----0----.
|  sw0  | | |   sw1   |
'-1-2-3-' | '-1-2-3-4-'
  $ @ '---'   $ @ % %

We have three physically looped pairs ($, @, and %).

This is very useful because it allows us to run the kernel's
kselftests for the bridge on mv88e6xxx hardware.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 63 ++++++++++++++++++++++----------
 1 file changed, 44 insertions(+), 19 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 8896709b9103..d0d766354669 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1630,21 +1630,11 @@ static int mv88e6xxx_fid_map_vlan(struct mv88e6xxx_chip *chip,
 
 int mv88e6xxx_fid_map(struct mv88e6xxx_chip *chip, unsigned long *fid_bitmap)
 {
-	int i, err;
-	u16 fid;
-
 	bitmap_zero(fid_bitmap, MV88E6XXX_N_FID);
 
-	/* Set every FID bit used by the (un)bridged ports */
-	for (i = 0; i < mv88e6xxx_num_ports(chip); ++i) {
-		err = mv88e6xxx_port_get_fid(chip, i, &fid);
-		if (err)
-			return err;
-
-		set_bit(fid, fid_bitmap);
-	}
-
-	/* Set every FID bit used by the VLAN entries */
+	/* Every FID has an associated VID, so walking the VTU
+	 * will discover the full set of FIDs in use.
+	 */
 	return mv88e6xxx_vtu_walk(chip, mv88e6xxx_fid_map_vlan, fid_bitmap);
 }
 
@@ -1657,10 +1647,7 @@ static int mv88e6xxx_atu_new(struct mv88e6xxx_chip *chip, u16 *fid)
 	if (err)
 		return err;
 
-	/* The reset value 0x000 is used to indicate that multiple address
-	 * databases are not needed. Return the next positive available.
-	 */
-	*fid = find_next_zero_bit(fid_bitmap, MV88E6XXX_N_FID, 1);
+	*fid = find_first_zero_bit(fid_bitmap, MV88E6XXX_N_FID);
 	if (unlikely(*fid >= mv88e6xxx_num_databases(chip)))
 		return -ENOSPC;
 
@@ -2152,6 +2139,9 @@ static int mv88e6xxx_port_vlan_join(struct mv88e6xxx_chip *chip, int port,
 	if (!vlan.valid) {
 		memset(&vlan, 0, sizeof(vlan));
 
+		if (vid == MV88E6XXX_VID_STANDALONE)
+			vlan.policy = true;
+
 		err = mv88e6xxx_atu_new(chip, &vlan.fid);
 		if (err)
 			return err;
@@ -2949,8 +2939,43 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
 	if (err)
 		return err;
 
+	/* On chips that support it, set all DSA ports' VLAN policy to
+	 * TRAP. In combination with loading MV88E6XXX_VID_STANDALONE
+	 * as a policy entry in the VTU, this provides a better
+	 * isolation barrier between standalone ports, as the ATU is
+	 * bypassed on any intermediate switches between the incoming
+	 * port and the CPU.
+	 */
+	if (!dsa_is_user_port(ds, port) && chip->info->ops->port_set_policy) {
+		err = chip->info->ops->port_set_policy(chip, port,
+						MV88E6XXX_POLICY_MAPPING_VTU,
+						MV88E6XXX_POLICY_ACTION_TRAP);
+		if (err)
+			return err;
+	}
+
+	/* User ports start out in standalone mode and 802.1Q is
+	 * therefore disabled. On DSA ports, all valid VIDs are always
+	 * loaded in the VTU - therefore, enable 802.1Q in order to take
+	 * advantage of VLAN policy on chips that supports it.
+	 */
 	err = mv88e6xxx_port_set_8021q_mode(chip, port,
-				MV88E6XXX_PORT_CTL2_8021Q_MODE_DISABLED);
+				dsa_is_user_port(ds, port) ?
+				MV88E6XXX_PORT_CTL2_8021Q_MODE_DISABLED :
+				MV88E6XXX_PORT_CTL2_8021Q_MODE_SECURE);
+	if (err)
+		return err;
+
+	/* Bind MV88E6XXX_VID_STANDALONE to MV88E6XXX_FID_STANDALONE by
+	 * virtue of the fact that mv88e6xxx_atu_new() will pick it as
+	 * the first free FID. This will be used as the private PVID for
+	 * unbridged ports. Shared (DSA and CPU) ports must also be
+	 * members of this VID, in order to trap all frames assigned to
+	 * it to the CPU.
+	 */
+	err = mv88e6xxx_port_vlan_join(chip, port, MV88E6XXX_VID_STANDALONE,
+				       MV88E6XXX_G1_VTU_DATA_MEMBER_TAG_UNMODIFIED,
+				       false);
 	if (err)
 		return err;
 
@@ -2963,7 +2988,7 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
 	 * relying on their port default FID.
 	 */
 	err = mv88e6xxx_port_vlan_join(chip, port, MV88E6XXX_VID_BRIDGED,
-				       MV88E6XXX_G1_VTU_DATA_MEMBER_TAG_UNTAGGED,
+				       MV88E6XXX_G1_VTU_DATA_MEMBER_TAG_UNMODIFIED,
 				       false);
 	if (err)
 		return err;
-- 
2.25.1

[PATCH net-next 5/5] selftests: net: bridge: Parameterize ageing timeout

[Tobias Waldekranz]

Allow the ageing timeout that is set on bridges to be customized from
forwarding.config. This allows the tests to be run on hardware which
does not support a 10s timeout (e.g. mv88e6xxx).

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 tools/testing/selftests/net/forwarding/bridge_vlan_aware.sh  | 5 +++--
 .../testing/selftests/net/forwarding/bridge_vlan_unaware.sh  | 5 +++--
 .../selftests/net/forwarding/forwarding.config.sample        | 2 ++
 tools/testing/selftests/net/forwarding/lib.sh                | 1 +
 4 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/net/forwarding/bridge_vlan_aware.sh b/tools/testing/selftests/net/forwarding/bridge_vlan_aware.sh
index b90dff8d3a94..64bd00fe9a4f 100755
--- a/tools/testing/selftests/net/forwarding/bridge_vlan_aware.sh
+++ b/tools/testing/selftests/net/forwarding/bridge_vlan_aware.sh
@@ -28,8 +28,9 @@ h2_destroy()
 
 switch_create()
 {
-	# 10 Seconds ageing time.
-	ip link add dev br0 type bridge vlan_filtering 1 ageing_time 1000 \
+	ip link add dev br0 type bridge \
+		vlan_filtering 1 \
+		ageing_time $LOW_AGEING_TIME \
 		mcast_snooping 0
 
 	ip link set dev $swp1 master br0
diff --git a/tools/testing/selftests/net/forwarding/bridge_vlan_unaware.sh b/tools/testing/selftests/net/forwarding/bridge_vlan_unaware.sh
index c15c6c85c984..1c8a26046589 100755
--- a/tools/testing/selftests/net/forwarding/bridge_vlan_unaware.sh
+++ b/tools/testing/selftests/net/forwarding/bridge_vlan_unaware.sh
@@ -27,8 +27,9 @@ h2_destroy()
 
 switch_create()
 {
-	# 10 Seconds ageing time.
-	ip link add dev br0 type bridge ageing_time 1000 mcast_snooping 0
+	ip link add dev br0 type bridge \
+		ageing_time $LOW_AGEING_TIME \
+		mcast_snooping 0
 
 	ip link set dev $swp1 master br0
 	ip link set dev $swp2 master br0
diff --git a/tools/testing/selftests/net/forwarding/forwarding.config.sample b/tools/testing/selftests/net/forwarding/forwarding.config.sample
index b0980a2efa31..4a546509de90 100644
--- a/tools/testing/selftests/net/forwarding/forwarding.config.sample
+++ b/tools/testing/selftests/net/forwarding/forwarding.config.sample
@@ -41,6 +41,8 @@ NETIF_CREATE=yes
 # Timeout (in seconds) before ping exits regardless of how many packets have
 # been sent or received
 PING_TIMEOUT=5
+# Minimum ageing_time (in centiseconds) supported by hardware
+LOW_AGEING_TIME=1000
 # Flag for tc match, supposed to be skip_sw/skip_hw which means do not process
 # filter by software/hardware
 TC_FLAG=skip_hw
diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
index 7da783d6f453..e7e434a4758b 100644
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -24,6 +24,7 @@ PING_COUNT=${PING_COUNT:=10}
 PING_TIMEOUT=${PING_TIMEOUT:=5}
 WAIT_TIMEOUT=${WAIT_TIMEOUT:=20}
 INTERFACE_TIMEOUT=${INTERFACE_TIMEOUT:=600}
+LOW_AGEING_TIME=${LOW_AGEING_TIME:=1000}
 REQUIRE_JQ=${REQUIRE_JQ:=yes}
 REQUIRE_MZ=${REQUIRE_MZ:=yes}
 
-- 
2.25.1

Re: [PATCH net-next 5/5] selftests: net: bridge: Parameterize ageing timeout

[Petr Machata]

Tobias Waldekranz <tobias@waldekranz.com> writes:

> Allow the ageing timeout that is set on bridges to be customized from
> forwarding.config. This allows the tests to be run on hardware which
> does not support a 10s timeout (e.g. mv88e6xxx).
>
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>

Reviewed-by: Petr Machata <petrm@nvidia.com>

Re: [PATCH net-next 1/5] net: dsa: mv88e6xxx: Improve isolation of standalone ports

[Vladimir Oltean]

Hi Tobias,

On Mon, Jan 31, 2022 at 04:46:51PM +0100, Tobias Waldekranz wrote:
> Clear MapDA on standalone ports to bypass any ATU lookup that might
> point the packet in the wrong direction. This means that all packets
> are flooded using the PVT config. So make sure that standalone ports
> are only allowed to communicate with the CPU port.

Actually "CPU port" != "upstream port" (the latter can be an
upstream-facing DSA port). The distinction is quite important.

> 
> Here is a scenario in which this is needed:
> 
>    CPU
>     |     .----.
> .---0---. | .--0--.
> |  sw0  | | | sw1 |
> '-1-2-3-' | '-1-2-'
>       '---'
> 
> - sw0p1 and sw1p1 are bridged

Do sw0p1 and sw1p1 even matter?

> - sw0p2 and sw1p2 are in standalone mode
> - Learning must be enabled on sw0p3 in order for hardware forwarding
>   to work properly between bridged ports
> 
> 1. A packet with SA :aa comes in on sw1p2
>    1a. Egresses sw1p0
>    1b. Ingresses sw0p3, ATU adds an entry for :aa towards port 3
>    1c. Egresses sw0p0
> 
> 2. A packet with DA :aa comes in on sw0p2
>    2a. If an ATU lookup is done at this point, the packet will be
>        incorrectly forwarded towards sw0p3. With this change in place,
>        the ATU is pypassed and the packet is forwarded in accordance

s/pypassed/bypassed/

>        whith the PVT, which only contains the CPU port.

s/whith/with/

What you describe is a bit convoluted, so let me try to rephrase it.
The mv88e6xxx driver configures all standalone ports to use the same
DefaultVID(0)/FID(0), and configures standalone user ports with no
learning via the Port Association Vector. Shared (cascade + CPU) ports
have learning enabled so that cross-chip bridging works without floods.
But since learning is per port and not per FID, it means that we enable
learning in FID 0, the one where the ATU was supposed to be always empty.
So we may end up taking wrong forwarding decisions for standalone ports,
notably when we should do software forwarding between ports of different
switches. By clearing MapDA, we force standalone ports to bypass any ATU
entries that might exist.

Question: can we disable learning per FID? I searched for this in the
limited documentation that I have, but I didn't see such option.
Doing this would be advantageous because we'd end up with a bit more
space in the ATU. With your solution we're just doing damage control.

> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
> ---
>  drivers/net/dsa/mv88e6xxx/chip.c | 32 +++++++++++++++++++++++++-------
>  drivers/net/dsa/mv88e6xxx/port.c |  7 +++++--
>  drivers/net/dsa/mv88e6xxx/port.h |  2 +-
>  include/net/dsa.h                | 12 ++++++++++++
>  4 files changed, 43 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 1023e4549359..dde6a8d0ca36 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -1290,8 +1290,15 @@ static u16 mv88e6xxx_port_vlan(struct mv88e6xxx_chip *chip, int dev, int port)
>  
>  	pvlan = 0;
>  
> -	/* Frames from user ports can egress any local DSA links and CPU ports,
> -	 * as well as any local member of their bridge group.
> +	/* Frames from standalone user ports can only egress on the
> +	 * CPU port.
> +	 */
> +	if (!dsa_port_bridge_dev_get(dp))
> +		return BIT(dsa_switch_upstream_port(ds));
> +
> +	/* Frames from bridged user ports can egress any local DSA
> +	 * links and CPU ports, as well as any local member of their
> +	 * bridge group.
>  	 */
>  	dsa_switch_for_each_port(other_dp, ds)
>  		if (other_dp->type == DSA_PORT_TYPE_CPU ||
> @@ -2487,6 +2494,10 @@ static int mv88e6xxx_port_bridge_join(struct dsa_switch *ds, int port,
>  	if (err)
>  		goto unlock;
>  
> +	err = mv88e6xxx_port_set_map_da(chip, port, true);
> +	if (err)
> +		return err;
> +
>  	err = mv88e6xxx_port_commit_pvid(chip, port);
>  	if (err)
>  		goto unlock;
> @@ -2521,6 +2532,12 @@ static void mv88e6xxx_port_bridge_leave(struct dsa_switch *ds, int port,
>  	    mv88e6xxx_port_vlan_map(chip, port))
>  		dev_err(ds->dev, "failed to remap in-chip Port VLAN\n");
>  
> +	err = mv88e6xxx_port_set_map_da(chip, port, false);
> +	if (err)
> +		dev_err(ds->dev,
> +			"port %d failed to restore map-DA: %pe\n",
> +			port, ERR_PTR(err));
> +
>  	err = mv88e6xxx_port_commit_pvid(chip, port);
>  	if (err)
>  		dev_err(ds->dev,
> @@ -2918,12 +2935,13 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
>  		return err;
>  
>  	/* 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.
> +	 * 10222 bytes, disable 802.1q tags checking, don't discard
> +	 * tagged or untagged frames on this port, skip destination
> +	 * address lookup on user ports, disable ARP mirroring and don't
> +	 * send a copy of all transmitted/received frames on this port
> +	 * to the CPU.
>  	 */
> -	err = mv88e6xxx_port_set_map_da(chip, port);
> +	err = mv88e6xxx_port_set_map_da(chip, port, !dsa_is_user_port(ds, port));
>  	if (err)
>  		return err;
>  
> diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
> index ab41619a809b..ceb450113f88 100644
> --- a/drivers/net/dsa/mv88e6xxx/port.c
> +++ b/drivers/net/dsa/mv88e6xxx/port.c
> @@ -1278,7 +1278,7 @@ int mv88e6xxx_port_drop_untagged(struct mv88e6xxx_chip *chip, int port,
>  	return mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_CTL2, new);
>  }
>  
> -int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port)
> +int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port, bool map)
>  {
>  	u16 reg;
>  	int err;
> @@ -1287,7 +1287,10 @@ int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port)
>  	if (err)
>  		return err;
>  
> -	reg |= MV88E6XXX_PORT_CTL2_MAP_DA;
> +	if (map)
> +		reg |= MV88E6XXX_PORT_CTL2_MAP_DA;
> +	else
> +		reg &= ~MV88E6XXX_PORT_CTL2_MAP_DA;
>  
>  	return mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_CTL2, reg);
>  }
> diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h
> index 03382b66f800..5c347cc58baf 100644
> --- a/drivers/net/dsa/mv88e6xxx/port.h
> +++ b/drivers/net/dsa/mv88e6xxx/port.h
> @@ -425,7 +425,7 @@ int mv88e6185_port_get_cmode(struct mv88e6xxx_chip *chip, int port, u8 *cmode);
>  int mv88e6352_port_get_cmode(struct mv88e6xxx_chip *chip, int port, u8 *cmode);
>  int mv88e6xxx_port_drop_untagged(struct mv88e6xxx_chip *chip, int port,
>  				 bool drop_untagged);
> -int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port);
> +int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port, bool map);
>  int mv88e6095_port_set_upstream_port(struct mv88e6xxx_chip *chip, int port,
>  				     int upstream_port);
>  int mv88e6xxx_port_set_mirror(struct mv88e6xxx_chip *chip, int port,
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 57b3e4e7413b..30f3192616e5 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -581,6 +581,18 @@ static inline bool dsa_is_upstream_port(struct dsa_switch *ds, int port)
>  	return port == dsa_upstream_port(ds, port);
>  }
>  
> +/* Return the local port used to reach the CPU port */
> +static inline unsigned int dsa_switch_upstream_port(struct dsa_switch *ds)
> +{
> +	int p;
> +
> +	for (p = 0; p < ds->num_ports; p++)
> +		if (!dsa_is_unused_port(ds, p))
> +			return dsa_upstream_port(ds, p);

dsa_switch_for_each_available_port

Although to be honest, the caller already has a dp, I wonder why you
need to complicate things and don't just call dsa_upstream_port(ds,
dp->index) directly.

> +
> +	return ds->num_ports;
> +}
> +
>  /* Return true if @upstream_ds is an upstream switch of @downstream_ds, meaning
>   * that the routing port from @downstream_ds to @upstream_ds is also the port
>   * which @downstream_ds uses to reach its dedicated CPU.
> -- 
> 2.25.1
> 

Re: [PATCH net-next 1/5] net: dsa: mv88e6xxx: Improve isolation of standalone ports

[Vladimir Oltean]

On Tue, Feb 01, 2022 at 07:06:34PM +0200, Vladimir Oltean wrote:
> Question: can we disable learning per FID? I searched for this in the
> limited documentation that I have, but I didn't see such option.
> Doing this would be advantageous because we'd end up with a bit more
> space in the ATU. With your solution we're just doing damage control.

Answering my own question...

Your patch 4/5 does disable learning for packets coming from standalone
ports, but not by FID, but by VTU policy. So that's good. Reading on...

Re: [PATCH net-next 4/5] net: dsa: mv88e6xxx: Improve multichip isolation of standalone ports

[Vladimir Oltean]

On Mon, Jan 31, 2022 at 04:46:54PM +0100, Tobias Waldekranz wrote:
> Given that standalone ports are now configured to bypass the ATU and
> forward all frames towards the upstream port, extend the ATU bypass to
> multichip systems.
> 
> Load VID 0 (standalone) into the VTU with the policy bit set. Since
> VID 4095 (bridged) is already loaded, we now know that all VIDs in use
> are always available in all VTUs. Therefore, we can safely enable
> 802.1Q on DSA ports.
> 
> Setting the DSA ports' VTU policy to TRAP means that all incoming
> frames on VID 0 will be classified as MGMT - as a result, the ATU is
> bypassed on all subsequent switches.
> 
> With this isolation in place, we are able to support configurations
> that are simultaneously very quirky and very useful. Quirky because it
> involves looping cables between local switchports like in this
> example:
> 
>    CPU
>     |     .------.
> .---0---. | .----0----.
> |  sw0  | | |   sw1   |
> '-1-2-3-' | '-1-2-3-4-'
>   $ @ '---'   $ @ % %
> 
> We have three physically looped pairs ($, @, and %).
> 
> This is very useful because it allows us to run the kernel's
> kselftests for the bridge on mv88e6xxx hardware.
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
> ---
>  drivers/net/dsa/mv88e6xxx/chip.c | 63 ++++++++++++++++++++++----------
>  1 file changed, 44 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 8896709b9103..d0d766354669 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -1630,21 +1630,11 @@ static int mv88e6xxx_fid_map_vlan(struct mv88e6xxx_chip *chip,
>  
>  int mv88e6xxx_fid_map(struct mv88e6xxx_chip *chip, unsigned long *fid_bitmap)
>  {
> -	int i, err;
> -	u16 fid;
> -
>  	bitmap_zero(fid_bitmap, MV88E6XXX_N_FID);
>  
> -	/* Set every FID bit used by the (un)bridged ports */
> -	for (i = 0; i < mv88e6xxx_num_ports(chip); ++i) {
> -		err = mv88e6xxx_port_get_fid(chip, i, &fid);
> -		if (err)
> -			return err;
> -
> -		set_bit(fid, fid_bitmap);
> -	}
> -
> -	/* Set every FID bit used by the VLAN entries */
> +	/* Every FID has an associated VID, so walking the VTU
> +	 * will discover the full set of FIDs in use.
> +	 */

So practically, regardless of whether the switch supports VTU policy or
not, we still load VID 0 in the VTU, and this simplifies the driver a
bit. Could we also simplify mv88e6xxx_port_db_dump() by deleting the
mv88e6xxx_port_get_fid() from there (and then delete this function
altogether)?

I think the mv88e6xxx_port_set_fid() call is now useless too?

>  	return mv88e6xxx_vtu_walk(chip, mv88e6xxx_fid_map_vlan, fid_bitmap);
>  }
>  
> @@ -1657,10 +1647,7 @@ static int mv88e6xxx_atu_new(struct mv88e6xxx_chip *chip, u16 *fid)
>  	if (err)
>  		return err;
>  
> -	/* The reset value 0x000 is used to indicate that multiple address
> -	 * databases are not needed. Return the next positive available.
> -	 */
> -	*fid = find_next_zero_bit(fid_bitmap, MV88E6XXX_N_FID, 1);
> +	*fid = find_first_zero_bit(fid_bitmap, MV88E6XXX_N_FID);
>  	if (unlikely(*fid >= mv88e6xxx_num_databases(chip)))
>  		return -ENOSPC;
>  
> @@ -2152,6 +2139,9 @@ static int mv88e6xxx_port_vlan_join(struct mv88e6xxx_chip *chip, int port,
>  	if (!vlan.valid) {
>  		memset(&vlan, 0, sizeof(vlan));
>  
> +		if (vid == MV88E6XXX_VID_STANDALONE)
> +			vlan.policy = true;
> +
>  		err = mv88e6xxx_atu_new(chip, &vlan.fid);
>  		if (err)
>  			return err;
> @@ -2949,8 +2939,43 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
>  	if (err)
>  		return err;
>  
> +	/* On chips that support it, set all DSA ports' VLAN policy to
> +	 * TRAP. In combination with loading MV88E6XXX_VID_STANDALONE
> +	 * as a policy entry in the VTU, this provides a better
> +	 * isolation barrier between standalone ports, as the ATU is
> +	 * bypassed on any intermediate switches between the incoming
> +	 * port and the CPU.
> +	 */
> +	if (!dsa_is_user_port(ds, port) && chip->info->ops->port_set_policy) {

Will this not also affect FWD frames sent on behalf of VLAN-unaware
bridges as they are received on CPU ports and upstream-facing DSA ports?
Somehow I think you intend to make this match only on downstream-facing
DSA ports.

> +		err = chip->info->ops->port_set_policy(chip, port,
> +						MV88E6XXX_POLICY_MAPPING_VTU,
> +						MV88E6XXX_POLICY_ACTION_TRAP);
> +		if (err)
> +			return err;
> +	}
> +
> +	/* User ports start out in standalone mode and 802.1Q is
> +	 * therefore disabled. On DSA ports, all valid VIDs are always
> +	 * loaded in the VTU - therefore, enable 802.1Q in order to take
> +	 * advantage of VLAN policy on chips that supports it.
> +	 */

Is this really needed? I thought cascade ports parse the VID from the
DSA header regardless of 802.1Q mode.

>  	err = mv88e6xxx_port_set_8021q_mode(chip, port,
> -				MV88E6XXX_PORT_CTL2_8021Q_MODE_DISABLED);
> +				dsa_is_user_port(ds, port) ?
> +				MV88E6XXX_PORT_CTL2_8021Q_MODE_DISABLED :
> +				MV88E6XXX_PORT_CTL2_8021Q_MODE_SECURE);
> +	if (err)
> +		return err;
> +
> +	/* Bind MV88E6XXX_VID_STANDALONE to MV88E6XXX_FID_STANDALONE by
> +	 * virtue of the fact that mv88e6xxx_atu_new() will pick it as
> +	 * the first free FID. This will be used as the private PVID for
> +	 * unbridged ports. Shared (DSA and CPU) ports must also be
> +	 * members of this VID, in order to trap all frames assigned to
> +	 * it to the CPU.
> +	 */
> +	err = mv88e6xxx_port_vlan_join(chip, port, MV88E6XXX_VID_STANDALONE,
> +				       MV88E6XXX_G1_VTU_DATA_MEMBER_TAG_UNMODIFIED,
> +				       false);
>  	if (err)
>  		return err;
>  
> @@ -2963,7 +2988,7 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
>  	 * relying on their port default FID.
>  	 */
>  	err = mv88e6xxx_port_vlan_join(chip, port, MV88E6XXX_VID_BRIDGED,
> -				       MV88E6XXX_G1_VTU_DATA_MEMBER_TAG_UNTAGGED,
> +				       MV88E6XXX_G1_VTU_DATA_MEMBER_TAG_UNMODIFIED,

I think the idea with UNTAGGED here was that packets sent by tag_dsa.c
with TX forwarding offload on behalf of a VLAN-unaware bridge have VID
4095. By setting the port as untagged, that VID is stripped on egress.
If you make it UNMODIFIED, the outside world will see it. Or am I wrong?

>  				       false);
>  	if (err)
>  		return err;
> -- 
> 2.25.1
> 

Re: [PATCH net-next 1/5] net: dsa: mv88e6xxx: Improve isolation of standalone ports

[Tobias Waldekranz]

On Tue, Feb 01, 2022 at 19:06, Vladimir Oltean <olteanv@gmail.com> wrote:
> Hi Tobias,
>
> On Mon, Jan 31, 2022 at 04:46:51PM +0100, Tobias Waldekranz wrote:
>> Clear MapDA on standalone ports to bypass any ATU lookup that might
>> point the packet in the wrong direction. This means that all packets
>> are flooded using the PVT config. So make sure that standalone ports
>> are only allowed to communicate with the CPU port.
>
> Actually "CPU port" != "upstream port" (the latter can be an
> upstream-facing DSA port). The distinction is quite important.

Yes that was sloppy of me, I'll rephrase.

>> 
>> Here is a scenario in which this is needed:
>> 
>>    CPU
>>     |     .----.
>> .---0---. | .--0--.
>> |  sw0  | | | sw1 |
>> '-1-2-3-' | '-1-2-'
>>       '---'
>> 
>> - sw0p1 and sw1p1 are bridged
>
> Do sw0p1 and sw1p1 even matter?

Strictly speaking, no - it was just to illustrate...

>> - sw0p2 and sw1p2 are in standalone mode
>> - Learning must be enabled on sw0p3 in order for hardware forwarding
>>   to work properly between bridged ports

... this point, i.e. a clear example of why learning can't be disabled
on DSA ports.

>> 1. A packet with SA :aa comes in on sw1p2
>>    1a. Egresses sw1p0
>>    1b. Ingresses sw0p3, ATU adds an entry for :aa towards port 3
>>    1c. Egresses sw0p0
>> 
>> 2. A packet with DA :aa comes in on sw0p2
>>    2a. If an ATU lookup is done at this point, the packet will be
>>        incorrectly forwarded towards sw0p3. With this change in place,
>>        the ATU is pypassed and the packet is forwarded in accordance
>
> s/pypassed/bypassed/
>
>>        whith the PVT, which only contains the CPU port.
>
> s/whith/with/
>
> What you describe is a bit convoluted, so let me try to rephrase it.
> The mv88e6xxx driver configures all standalone ports to use the same
> DefaultVID(0)/FID(0), and configures standalone user ports with no
> learning via the Port Association Vector. Shared (cascade + CPU) ports
> have learning enabled so that cross-chip bridging works without floods.
> But since learning is per port and not per FID, it means that we enable
> learning in FID 0, the one where the ATU was supposed to be always empty.
> So we may end up taking wrong forwarding decisions for standalone ports,
> notably when we should do software forwarding between ports of different
> switches. By clearing MapDA, we force standalone ports to bypass any ATU
> entries that might exist.

Are you saying you want me to replace the initial paragraph with your
version, or are you saying the the example is convoluted and should be
replaced by this text? Or is it only for the benefit of other readers?

> Question: can we disable learning per FID? I searched for this in the
> limited documentation that I have, but I didn't see such option.
> Doing this would be advantageous because we'd end up with a bit more
> space in the ATU. With your solution we're just doing damage control.

As you discovered, and as I tried to lay out in the cover, this is only
one part of the whole solution.

>> 
>> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
>> ---
>>  drivers/net/dsa/mv88e6xxx/chip.c | 32 +++++++++++++++++++++++++-------
>>  drivers/net/dsa/mv88e6xxx/port.c |  7 +++++--
>>  drivers/net/dsa/mv88e6xxx/port.h |  2 +-
>>  include/net/dsa.h                | 12 ++++++++++++
>>  4 files changed, 43 insertions(+), 10 deletions(-)
>> 
>> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
>> index 1023e4549359..dde6a8d0ca36 100644
>> --- a/drivers/net/dsa/mv88e6xxx/chip.c
>> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
>> @@ -1290,8 +1290,15 @@ static u16 mv88e6xxx_port_vlan(struct mv88e6xxx_chip *chip, int dev, int port)
>>  
>>  	pvlan = 0;
>>  
>> -	/* Frames from user ports can egress any local DSA links and CPU ports,
>> -	 * as well as any local member of their bridge group.
>> +	/* Frames from standalone user ports can only egress on the
>> +	 * CPU port.
>> +	 */
>> +	if (!dsa_port_bridge_dev_get(dp))
>> +		return BIT(dsa_switch_upstream_port(ds));
>> +
>> +	/* Frames from bridged user ports can egress any local DSA
>> +	 * links and CPU ports, as well as any local member of their
>> +	 * bridge group.
>>  	 */
>>  	dsa_switch_for_each_port(other_dp, ds)
>>  		if (other_dp->type == DSA_PORT_TYPE_CPU ||
>> @@ -2487,6 +2494,10 @@ static int mv88e6xxx_port_bridge_join(struct dsa_switch *ds, int port,
>>  	if (err)
>>  		goto unlock;
>>  
>> +	err = mv88e6xxx_port_set_map_da(chip, port, true);
>> +	if (err)
>> +		return err;
>> +
>>  	err = mv88e6xxx_port_commit_pvid(chip, port);
>>  	if (err)
>>  		goto unlock;
>> @@ -2521,6 +2532,12 @@ static void mv88e6xxx_port_bridge_leave(struct dsa_switch *ds, int port,
>>  	    mv88e6xxx_port_vlan_map(chip, port))
>>  		dev_err(ds->dev, "failed to remap in-chip Port VLAN\n");
>>  
>> +	err = mv88e6xxx_port_set_map_da(chip, port, false);
>> +	if (err)
>> +		dev_err(ds->dev,
>> +			"port %d failed to restore map-DA: %pe\n",
>> +			port, ERR_PTR(err));
>> +
>>  	err = mv88e6xxx_port_commit_pvid(chip, port);
>>  	if (err)
>>  		dev_err(ds->dev,
>> @@ -2918,12 +2935,13 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
>>  		return err;
>>  
>>  	/* 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.
>> +	 * 10222 bytes, disable 802.1q tags checking, don't discard
>> +	 * tagged or untagged frames on this port, skip destination
>> +	 * address lookup on user ports, disable ARP mirroring and don't
>> +	 * send a copy of all transmitted/received frames on this port
>> +	 * to the CPU.
>>  	 */
>> -	err = mv88e6xxx_port_set_map_da(chip, port);
>> +	err = mv88e6xxx_port_set_map_da(chip, port, !dsa_is_user_port(ds, port));
>>  	if (err)
>>  		return err;
>>  
>> diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
>> index ab41619a809b..ceb450113f88 100644
>> --- a/drivers/net/dsa/mv88e6xxx/port.c
>> +++ b/drivers/net/dsa/mv88e6xxx/port.c
>> @@ -1278,7 +1278,7 @@ int mv88e6xxx_port_drop_untagged(struct mv88e6xxx_chip *chip, int port,
>>  	return mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_CTL2, new);
>>  }
>>  
>> -int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port)
>> +int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port, bool map)
>>  {
>>  	u16 reg;
>>  	int err;
>> @@ -1287,7 +1287,10 @@ int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port)
>>  	if (err)
>>  		return err;
>>  
>> -	reg |= MV88E6XXX_PORT_CTL2_MAP_DA;
>> +	if (map)
>> +		reg |= MV88E6XXX_PORT_CTL2_MAP_DA;
>> +	else
>> +		reg &= ~MV88E6XXX_PORT_CTL2_MAP_DA;
>>  
>>  	return mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_CTL2, reg);
>>  }
>> diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h
>> index 03382b66f800..5c347cc58baf 100644
>> --- a/drivers/net/dsa/mv88e6xxx/port.h
>> +++ b/drivers/net/dsa/mv88e6xxx/port.h
>> @@ -425,7 +425,7 @@ int mv88e6185_port_get_cmode(struct mv88e6xxx_chip *chip, int port, u8 *cmode);
>>  int mv88e6352_port_get_cmode(struct mv88e6xxx_chip *chip, int port, u8 *cmode);
>>  int mv88e6xxx_port_drop_untagged(struct mv88e6xxx_chip *chip, int port,
>>  				 bool drop_untagged);
>> -int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port);
>> +int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port, bool map);
>>  int mv88e6095_port_set_upstream_port(struct mv88e6xxx_chip *chip, int port,
>>  				     int upstream_port);
>>  int mv88e6xxx_port_set_mirror(struct mv88e6xxx_chip *chip, int port,
>> diff --git a/include/net/dsa.h b/include/net/dsa.h
>> index 57b3e4e7413b..30f3192616e5 100644
>> --- a/include/net/dsa.h
>> +++ b/include/net/dsa.h
>> @@ -581,6 +581,18 @@ static inline bool dsa_is_upstream_port(struct dsa_switch *ds, int port)
>>  	return port == dsa_upstream_port(ds, port);
>>  }
>>  
>> +/* Return the local port used to reach the CPU port */
>> +static inline unsigned int dsa_switch_upstream_port(struct dsa_switch *ds)
>> +{
>> +	int p;
>> +
>> +	for (p = 0; p < ds->num_ports; p++)
>> +		if (!dsa_is_unused_port(ds, p))
>> +			return dsa_upstream_port(ds, p);
>
> dsa_switch_for_each_available_port
>
> Although to be honest, the caller already has a dp, I wonder why you
> need to complicate things and don't just call dsa_upstream_port(ds,
> dp->index) directly.

Because dp refers to the port we are determining the permissions _for_,
and ds refers to the chip we are configuring the PVT _on_.

I think other_dp and dp should swap names with each other. Because it is
very easy to get confused. Or maybe s/dp/remote_dp/ and s/other_dp/dp/?

>> +
>> +	return ds->num_ports;
>> +}
>> +
>>  /* Return true if @upstream_ds is an upstream switch of @downstream_ds, meaning
>>   * that the routing port from @downstream_ds to @upstream_ds is also the port
>>   * which @downstream_ds uses to reach its dedicated CPU.
>> -- 
>> 2.25.1
>> 

Re: [PATCH net-next 1/5] net: dsa: mv88e6xxx: Improve isolation of standalone ports

[Vladimir Oltean]

On Tue, Feb 01, 2022 at 08:56:32PM +0100, Tobias Waldekranz wrote:
> >> - sw0p1 and sw1p1 are bridged
> >
> > Do sw0p1 and sw1p1 even matter?
> 
> Strictly speaking, no - it was just to illustrate...
> 
> >> - sw0p2 and sw1p2 are in standalone mode
> >> - Learning must be enabled on sw0p3 in order for hardware forwarding
> >>   to work properly between bridged ports
> 
> ... this point, i.e. a clear example of why learning can't be disabled
> on DSA ports.

Ok, I understand now. It wasn't too clear.

> >> 1. A packet with SA :aa comes in on sw1p2
> >>    1a. Egresses sw1p0
> >>    1b. Ingresses sw0p3, ATU adds an entry for :aa towards port 3
> >>    1c. Egresses sw0p0
> >> 
> >> 2. A packet with DA :aa comes in on sw0p2
> >>    2a. If an ATU lookup is done at this point, the packet will be
> >>        incorrectly forwarded towards sw0p3. With this change in place,
> >>        the ATU is pypassed and the packet is forwarded in accordance
> >
> > s/pypassed/bypassed/
> >
> >>        whith the PVT, which only contains the CPU port.
> >
> > s/whith/with/
> >
> > What you describe is a bit convoluted, so let me try to rephrase it.
> > The mv88e6xxx driver configures all standalone ports to use the same
> > DefaultVID(0)/FID(0), and configures standalone user ports with no
> > learning via the Port Association Vector. Shared (cascade + CPU) ports
> > have learning enabled so that cross-chip bridging works without floods.
> > But since learning is per port and not per FID, it means that we enable
> > learning in FID 0, the one where the ATU was supposed to be always empty.
> > So we may end up taking wrong forwarding decisions for standalone ports,
> > notably when we should do software forwarding between ports of different
> > switches. By clearing MapDA, we force standalone ports to bypass any ATU
> > entries that might exist.
> 
> Are you saying you want me to replace the initial paragraph with your
> version, or are you saying the the example is convoluted and should be
> replaced by this text? Or is it only for the benefit of other readers?

Just for the sake of discussion, I wanted to make sure I understand what
you describe.

> > Question: can we disable learning per FID? I searched for this in the
> > limited documentation that I have, but I didn't see such option.
> > Doing this would be advantageous because we'd end up with a bit more
> > space in the ATU. With your solution we're just doing damage control.
> 
> As you discovered, and as I tried to lay out in the cover, this is only
> one part of the whole solution.

I'm not copied to the cover letter :) and I have some issues with my
email client / vger, where emails that I receive through the mailing list
sometimes take days to reach my inbox, whereas emails sent directly to
me reach my inbox instantaneously. So don't assume I read email that
wasn't targeted directly to me, sorry.

> >> diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h
> >> index 03382b66f800..5c347cc58baf 100644
> >> --- a/drivers/net/dsa/mv88e6xxx/port.h
> >> +++ b/drivers/net/dsa/mv88e6xxx/port.h
> >> @@ -425,7 +425,7 @@ int mv88e6185_port_get_cmode(struct mv88e6xxx_chip *chip, int port, u8 *cmode);
> >>  int mv88e6352_port_get_cmode(struct mv88e6xxx_chip *chip, int port, u8 *cmode);
> >>  int mv88e6xxx_port_drop_untagged(struct mv88e6xxx_chip *chip, int port,
> >>  				 bool drop_untagged);
> >> -int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port);
> >> +int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port, bool map);
> >>  int mv88e6095_port_set_upstream_port(struct mv88e6xxx_chip *chip, int port,
> >>  				     int upstream_port);
> >>  int mv88e6xxx_port_set_mirror(struct mv88e6xxx_chip *chip, int port,
> >> diff --git a/include/net/dsa.h b/include/net/dsa.h
> >> index 57b3e4e7413b..30f3192616e5 100644
> >> --- a/include/net/dsa.h
> >> +++ b/include/net/dsa.h
> >> @@ -581,6 +581,18 @@ static inline bool dsa_is_upstream_port(struct dsa_switch *ds, int port)
> >>  	return port == dsa_upstream_port(ds, port);
> >>  }
> >>  
> >> +/* Return the local port used to reach the CPU port */
> >> +static inline unsigned int dsa_switch_upstream_port(struct dsa_switch *ds)
> >> +{
> >> +	int p;
> >> +
> >> +	for (p = 0; p < ds->num_ports; p++)
> >> +		if (!dsa_is_unused_port(ds, p))
> >> +			return dsa_upstream_port(ds, p);
> >
> > dsa_switch_for_each_available_port
> >
> > Although to be honest, the caller already has a dp, I wonder why you
> > need to complicate things and don't just call dsa_upstream_port(ds,
> > dp->index) directly.
> 
> Because dp refers to the port we are determining the permissions _for_,
> and ds refers to the chip we are configuring the PVT _on_.
> 
> I think other_dp and dp should swap names with each other. Because it is
> very easy to get confused. Or maybe s/dp/remote_dp/ and s/other_dp/dp/?

Sorry, my mistake, I was looking at the patch in the email client and
didn't recognize from the context that this is mv88e6xxx_port_vlan(),
and that the port is remote. So I retract the part about calling
dsa_upstream_port() directly, but please still consider using a more
appropriate port iterator for the implementation of dsa_switch_upstream_port().

Re: [PATCH net-next 4/5] net: dsa: mv88e6xxx: Improve multichip isolation of standalone ports

[Tobias Waldekranz]

On Tue, Feb 01, 2022 at 19:55, Vladimir Oltean <olteanv@gmail.com> wrote:
> On Mon, Jan 31, 2022 at 04:46:54PM +0100, Tobias Waldekranz wrote:
>> Given that standalone ports are now configured to bypass the ATU and
>> forward all frames towards the upstream port, extend the ATU bypass to
>> multichip systems.
>> 
>> Load VID 0 (standalone) into the VTU with the policy bit set. Since
>> VID 4095 (bridged) is already loaded, we now know that all VIDs in use
>> are always available in all VTUs. Therefore, we can safely enable
>> 802.1Q on DSA ports.
>> 
>> Setting the DSA ports' VTU policy to TRAP means that all incoming
>> frames on VID 0 will be classified as MGMT - as a result, the ATU is
>> bypassed on all subsequent switches.
>> 
>> With this isolation in place, we are able to support configurations
>> that are simultaneously very quirky and very useful. Quirky because it
>> involves looping cables between local switchports like in this
>> example:
>> 
>>    CPU
>>     |     .------.
>> .---0---. | .----0----.
>> |  sw0  | | |   sw1   |
>> '-1-2-3-' | '-1-2-3-4-'
>>   $ @ '---'   $ @ % %
>> 
>> We have three physically looped pairs ($, @, and %).
>> 
>> This is very useful because it allows us to run the kernel's
>> kselftests for the bridge on mv88e6xxx hardware.
>> 
>> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
>> ---
>>  drivers/net/dsa/mv88e6xxx/chip.c | 63 ++++++++++++++++++++++----------
>>  1 file changed, 44 insertions(+), 19 deletions(-)
>> 
>> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
>> index 8896709b9103..d0d766354669 100644
>> --- a/drivers/net/dsa/mv88e6xxx/chip.c
>> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
>> @@ -1630,21 +1630,11 @@ static int mv88e6xxx_fid_map_vlan(struct mv88e6xxx_chip *chip,
>>  
>>  int mv88e6xxx_fid_map(struct mv88e6xxx_chip *chip, unsigned long *fid_bitmap)
>>  {
>> -	int i, err;
>> -	u16 fid;
>> -
>>  	bitmap_zero(fid_bitmap, MV88E6XXX_N_FID);
>>  
>> -	/* Set every FID bit used by the (un)bridged ports */
>> -	for (i = 0; i < mv88e6xxx_num_ports(chip); ++i) {
>> -		err = mv88e6xxx_port_get_fid(chip, i, &fid);
>> -		if (err)
>> -			return err;
>> -
>> -		set_bit(fid, fid_bitmap);
>> -	}
>> -
>> -	/* Set every FID bit used by the VLAN entries */
>> +	/* Every FID has an associated VID, so walking the VTU
>> +	 * will discover the full set of FIDs in use.
>> +	 */
>
> So practically, regardless of whether the switch supports VTU policy or
> not, we still load VID 0 in the VTU, and this simplifies the driver a
> bit. Could we also simplify mv88e6xxx_port_db_dump() by deleting the
> mv88e6xxx_port_get_fid() from there (and then delete this function
> altogether)?

db_dump could be simplified, as there is no reason to go looking for
entries in MV88E6XXX_FID_STANDALONE. We still need this function to
collect all in-use FIDs in order for mv88e6xxx_atu_new to be able to
pick a free one.

> I think the mv88e6xxx_port_set_fid() call is now useless too?

I don't think so. Since standalone ports do not have 1Q enabled, they
will source their FID from there. This is important because of the
awkward way in which these chips behave when learning is "disabled". The
quotes are there because disabling learning essentially means that the
switch will just load an invalid entry for incoming SA:s. Which means
that they must still be isolated in their own FID to avoid poisoning
other databases.

>>  	return mv88e6xxx_vtu_walk(chip, mv88e6xxx_fid_map_vlan, fid_bitmap);
>>  }
>>  
>> @@ -1657,10 +1647,7 @@ static int mv88e6xxx_atu_new(struct mv88e6xxx_chip *chip, u16 *fid)
>>  	if (err)
>>  		return err;
>>  
>> -	/* The reset value 0x000 is used to indicate that multiple address
>> -	 * databases are not needed. Return the next positive available.
>> -	 */
>> -	*fid = find_next_zero_bit(fid_bitmap, MV88E6XXX_N_FID, 1);
>> +	*fid = find_first_zero_bit(fid_bitmap, MV88E6XXX_N_FID);
>>  	if (unlikely(*fid >= mv88e6xxx_num_databases(chip)))
>>  		return -ENOSPC;
>>  
>> @@ -2152,6 +2139,9 @@ static int mv88e6xxx_port_vlan_join(struct mv88e6xxx_chip *chip, int port,
>>  	if (!vlan.valid) {
>>  		memset(&vlan, 0, sizeof(vlan));
>>  
>> +		if (vid == MV88E6XXX_VID_STANDALONE)
>> +			vlan.policy = true;
>> +
>>  		err = mv88e6xxx_atu_new(chip, &vlan.fid);
>>  		if (err)
>>  			return err;
>> @@ -2949,8 +2939,43 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
>>  	if (err)
>>  		return err;
>>  
>> +	/* On chips that support it, set all DSA ports' VLAN policy to
>> +	 * TRAP. In combination with loading MV88E6XXX_VID_STANDALONE
>> +	 * as a policy entry in the VTU, this provides a better
>> +	 * isolation barrier between standalone ports, as the ATU is
>> +	 * bypassed on any intermediate switches between the incoming
>> +	 * port and the CPU.
>> +	 */
>> +	if (!dsa_is_user_port(ds, port) && chip->info->ops->port_set_policy) {
>
> Will this not also affect FWD frames sent on behalf of VLAN-unaware
> bridges as they are received on CPU ports and upstream-facing DSA ports?

No, because FORWARDs from non-filtering bridges will use
MV88E6XXX_VID_BRIDGED, which does not have the policy bit set in its VTU
entry.

If the CPU was to send a FORWARD on MV88E6XXX_VID_STANDALONE, then it
would bounce back. By definition though, no forward offloading ever
takes place on those ports, so only FROM_CPUs will be sent.

> Somehow I think you intend to make this match only on downstream-facing
> DSA ports.

That sounds like a good idea. It shouldn't change the behavior, but it
does seem more prudent.

So !dsa_is_user_port would become dsa_is_downstream_port, which in turn
would be something like:

/* Return true if this is a DSA port leading away from the CPU */
static inline bool dsa_is_downstream_port(struct dsa_switch *ds, int port)
{
        return dsa_is_dsa_port(ds, port) && !dsa_is_upstream_port(ds, port);
}

Agreed?

>> +		err = chip->info->ops->port_set_policy(chip, port,
>> +						MV88E6XXX_POLICY_MAPPING_VTU,
>> +						MV88E6XXX_POLICY_ACTION_TRAP);
>> +		if (err)
>> +			return err;
>> +	}
>> +
>> +	/* User ports start out in standalone mode and 802.1Q is
>> +	 * therefore disabled. On DSA ports, all valid VIDs are always
>> +	 * loaded in the VTU - therefore, enable 802.1Q in order to take
>> +	 * advantage of VLAN policy on chips that supports it.
>> +	 */
>
> Is this really needed? I thought cascade ports parse the VID from the
> DSA header regardless of 802.1Q mode.

I am pretty sure they will just forward the frame according to the PVT
config if 1Q is disabled.

>>  	err = mv88e6xxx_port_set_8021q_mode(chip, port,
>> -				MV88E6XXX_PORT_CTL2_8021Q_MODE_DISABLED);
>> +				dsa_is_user_port(ds, port) ?
>> +				MV88E6XXX_PORT_CTL2_8021Q_MODE_DISABLED :
>> +				MV88E6XXX_PORT_CTL2_8021Q_MODE_SECURE);
>> +	if (err)
>> +		return err;
>> +
>> +	/* Bind MV88E6XXX_VID_STANDALONE to MV88E6XXX_FID_STANDALONE by
>> +	 * virtue of the fact that mv88e6xxx_atu_new() will pick it as
>> +	 * the first free FID. This will be used as the private PVID for
>> +	 * unbridged ports. Shared (DSA and CPU) ports must also be
>> +	 * members of this VID, in order to trap all frames assigned to
>> +	 * it to the CPU.
>> +	 */
>> +	err = mv88e6xxx_port_vlan_join(chip, port, MV88E6XXX_VID_STANDALONE,
>> +				       MV88E6XXX_G1_VTU_DATA_MEMBER_TAG_UNMODIFIED,
>> +				       false);
>>  	if (err)
>>  		return err;
>>  
>> @@ -2963,7 +2988,7 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
>>  	 * relying on their port default FID.
>>  	 */
>>  	err = mv88e6xxx_port_vlan_join(chip, port, MV88E6XXX_VID_BRIDGED,
>> -				       MV88E6XXX_G1_VTU_DATA_MEMBER_TAG_UNTAGGED,
>> +				       MV88E6XXX_G1_VTU_DATA_MEMBER_TAG_UNMODIFIED,
>
> I think the idea with UNTAGGED here was that packets sent by tag_dsa.c
> with TX forwarding offload on behalf of a VLAN-unaware bridge have VID
> 4095. By setting the port as untagged, that VID is stripped on egress.
> If you make it UNMODIFIED, the outside world will see it. Or am I wrong?

Unmodified basically means: "Send it out in the same way that is was
received, in accordance with the port's frame mode". The frame mode then
determines the actual on-wire format.

For DSA ports, the frame mode is set to "DSA", in which case the port
will relay the frame with the DSA tag untouched.

For user ports, the frame mode is set to "Normal Network", which either
strips the DSA tag in case of an untagged frame, or converts it to a 1Q
frame in the case of a tagged frame. Which is why no DSA tags will ever
be sent.

>>  				       false);
>>  	if (err)
>>  		return err;
>> -- 
>> 2.25.1
>> 

Re: [PATCH net-next 1/5] net: dsa: mv88e6xxx: Improve isolation of standalone ports

[Tobias Waldekranz]

On Tue, Feb 01, 2022 at 22:11, Vladimir Oltean <olteanv@gmail.com> wrote:
> On Tue, Feb 01, 2022 at 08:56:32PM +0100, Tobias Waldekranz wrote:
>> >> - sw0p1 and sw1p1 are bridged
>> >
>> > Do sw0p1 and sw1p1 even matter?
>> 
>> Strictly speaking, no - it was just to illustrate...
>> 
>> >> - sw0p2 and sw1p2 are in standalone mode
>> >> - Learning must be enabled on sw0p3 in order for hardware forwarding
>> >>   to work properly between bridged ports
>> 
>> ... this point, i.e. a clear example of why learning can't be disabled
>> on DSA ports.
>
> Ok, I understand now. It wasn't too clear.
>
>> >> 1. A packet with SA :aa comes in on sw1p2
>> >>    1a. Egresses sw1p0
>> >>    1b. Ingresses sw0p3, ATU adds an entry for :aa towards port 3
>> >>    1c. Egresses sw0p0
>> >> 
>> >> 2. A packet with DA :aa comes in on sw0p2
>> >>    2a. If an ATU lookup is done at this point, the packet will be
>> >>        incorrectly forwarded towards sw0p3. With this change in place,
>> >>        the ATU is pypassed and the packet is forwarded in accordance
>> >
>> > s/pypassed/bypassed/
>> >
>> >>        whith the PVT, which only contains the CPU port.
>> >
>> > s/whith/with/
>> >
>> > What you describe is a bit convoluted, so let me try to rephrase it.
>> > The mv88e6xxx driver configures all standalone ports to use the same
>> > DefaultVID(0)/FID(0), and configures standalone user ports with no
>> > learning via the Port Association Vector. Shared (cascade + CPU) ports
>> > have learning enabled so that cross-chip bridging works without floods.
>> > But since learning is per port and not per FID, it means that we enable
>> > learning in FID 0, the one where the ATU was supposed to be always empty.
>> > So we may end up taking wrong forwarding decisions for standalone ports,
>> > notably when we should do software forwarding between ports of different
>> > switches. By clearing MapDA, we force standalone ports to bypass any ATU
>> > entries that might exist.
>> 
>> Are you saying you want me to replace the initial paragraph with your
>> version, or are you saying the the example is convoluted and should be
>> replaced by this text? Or is it only for the benefit of other readers?
>
> Just for the sake of discussion, I wanted to make sure I understand what
> you describe.
>
>> > Question: can we disable learning per FID? I searched for this in the
>> > limited documentation that I have, but I didn't see such option.
>> > Doing this would be advantageous because we'd end up with a bit more
>> > space in the ATU. With your solution we're just doing damage control.
>> 
>> As you discovered, and as I tried to lay out in the cover, this is only
>> one part of the whole solution.
>
> I'm not copied to the cover letter :) and I have some issues with my
> email client / vger, where emails that I receive through the mailing list
> sometimes take days to reach my inbox, whereas emails sent directly to
> me reach my inbox instantaneously. So don't assume I read email that
> wasn't targeted directly to me, sorry.

No worries. I have recently started using get_maintainers.pl to auto
generate the recipient list, with the result that the cover is only sent
to the list. Ideally I would like send-email to use the union of all
recipients for the cover letter, but I haven't figured that one out yet.

I actually gave up on getting my mailinglists from my email provider,
now I just download it directly from lore. I hacked together a script
that will scrape a public-inbox repo and convert it to a Maildir:

https://github.com/wkz/notmuch-lore

As you can tell from the name, it is tailored for plugging into notmuch,
but the guts are pretty generic.

>> >> diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h
>> >> index 03382b66f800..5c347cc58baf 100644
>> >> --- a/drivers/net/dsa/mv88e6xxx/port.h
>> >> +++ b/drivers/net/dsa/mv88e6xxx/port.h
>> >> @@ -425,7 +425,7 @@ int mv88e6185_port_get_cmode(struct mv88e6xxx_chip *chip, int port, u8 *cmode);
>> >>  int mv88e6352_port_get_cmode(struct mv88e6xxx_chip *chip, int port, u8 *cmode);
>> >>  int mv88e6xxx_port_drop_untagged(struct mv88e6xxx_chip *chip, int port,
>> >>  				 bool drop_untagged);
>> >> -int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port);
>> >> +int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port, bool map);
>> >>  int mv88e6095_port_set_upstream_port(struct mv88e6xxx_chip *chip, int port,
>> >>  				     int upstream_port);
>> >>  int mv88e6xxx_port_set_mirror(struct mv88e6xxx_chip *chip, int port,
>> >> diff --git a/include/net/dsa.h b/include/net/dsa.h
>> >> index 57b3e4e7413b..30f3192616e5 100644
>> >> --- a/include/net/dsa.h
>> >> +++ b/include/net/dsa.h
>> >> @@ -581,6 +581,18 @@ static inline bool dsa_is_upstream_port(struct dsa_switch *ds, int port)
>> >>  	return port == dsa_upstream_port(ds, port);
>> >>  }
>> >>  
>> >> +/* Return the local port used to reach the CPU port */
>> >> +static inline unsigned int dsa_switch_upstream_port(struct dsa_switch *ds)
>> >> +{
>> >> +	int p;
>> >> +
>> >> +	for (p = 0; p < ds->num_ports; p++)
>> >> +		if (!dsa_is_unused_port(ds, p))
>> >> +			return dsa_upstream_port(ds, p);
>> >
>> > dsa_switch_for_each_available_port
>> >
>> > Although to be honest, the caller already has a dp, I wonder why you
>> > need to complicate things and don't just call dsa_upstream_port(ds,
>> > dp->index) directly.
>> 
>> Because dp refers to the port we are determining the permissions _for_,
>> and ds refers to the chip we are configuring the PVT _on_.
>> 
>> I think other_dp and dp should swap names with each other. Because it is
>> very easy to get confused. Or maybe s/dp/remote_dp/ and s/other_dp/dp/?
>
> Sorry, my mistake, I was looking at the patch in the email client and
> didn't recognize from the context that this is mv88e6xxx_port_vlan(),
> and that the port is remote. So I retract the part about calling
> dsa_upstream_port() directly, but please still consider using a more
> appropriate port iterator for the implementation of dsa_switch_upstream_port().

Will do!

Re: [PATCH net-next 1/5] net: dsa: mv88e6xxx: Improve isolation of standalone ports

[Vladimir Oltean]

On Tue, Feb 01, 2022 at 10:22:13PM +0100, Tobias Waldekranz wrote:
> No worries. I have recently started using get_maintainers.pl to auto
> generate the recipient list, with the result that the cover is only sent
> to the list. Ideally I would like send-email to use the union of all
> recipients for the cover letter, but I haven't figured that one out yet.

Maybe auto-generating isn't the best solution? Wait until you need to
post a link to https://patchwork.kernel.org/project/netdevbpf/, and
get_maintainers.pl will draw in all the BPF maintainers for you...
The union appears when you run get_maintainer.pl on multiple patch
files. I typically run get_maintainer.pl on *.patch, and adjust the
git-send-email list from there.

> I actually gave up on getting my mailinglists from my email provider,
> now I just download it directly from lore. I hacked together a script
> that will scrape a public-inbox repo and convert it to a Maildir:
> 
> https://github.com/wkz/notmuch-lore
> 
> As you can tell from the name, it is tailored for plugging into notmuch,
> but the guts are pretty generic.

Thanks, I set that up, it's syncing right now, I'm also going to compare
the size of the git tree vs the maildir I currently have.

Re: [PATCH net-next 1/5] net: dsa: mv88e6xxx: Improve isolation of standalone ports

[Marek Behún]

On Thu, 3 Feb 2022 15:56:06 +0200
Vladimir Oltean <olteanv@gmail.com> wrote:

> On Tue, Feb 01, 2022 at 10:22:13PM +0100, Tobias Waldekranz wrote:
> > No worries. I have recently started using get_maintainers.pl to auto
> > generate the recipient list, with the result that the cover is only sent
> > to the list. Ideally I would like send-email to use the union of all
> > recipients for the cover letter, but I haven't figured that one out yet.  
> 
> Maybe auto-generating isn't the best solution? Wait until you need to
> post a link to https://patchwork.kernel.org/project/netdevbpf/, and
> get_maintainers.pl will draw in all the BPF maintainers for you...
> The union appears when you run get_maintainer.pl on multiple patch
> files. I typically run get_maintainer.pl on *.patch, and adjust the
> git-send-email list from there.
> 
> > I actually gave up on getting my mailinglists from my email provider,
> > now I just download it directly from lore. I hacked together a script
> > that will scrape a public-inbox repo and convert it to a Maildir:
> > 
> > https://github.com/wkz/notmuch-lore
> > 
> > As you can tell from the name, it is tailored for plugging into notmuch,
> > but the guts are pretty generic.  
> 
> Thanks, I set that up, it's syncing right now, I'm also going to compare
> the size of the git tree vs the maildir I currently have.

Hi Vladimir, please let me know the results.
Marek

Re: [PATCH net-next 1/5] net: dsa: mv88e6xxx: Improve isolation of standalone ports

[Vladimir Oltean]

On Thu, Feb 03, 2022 at 05:01:04PM +0100, Marek Behún wrote:
> On Thu, 3 Feb 2022 15:56:06 +0200
> Vladimir Oltean <olteanv@gmail.com> wrote:
> 
> > On Tue, Feb 01, 2022 at 10:22:13PM +0100, Tobias Waldekranz wrote:
> > > No worries. I have recently started using get_maintainers.pl to auto
> > > generate the recipient list, with the result that the cover is only sent
> > > to the list. Ideally I would like send-email to use the union of all
> > > recipients for the cover letter, but I haven't figured that one out yet.  
> > 
> > Maybe auto-generating isn't the best solution? Wait until you need to
> > post a link to https://patchwork.kernel.org/project/netdevbpf/, and
> > get_maintainers.pl will draw in all the BPF maintainers for you...
> > The union appears when you run get_maintainer.pl on multiple patch
> > files. I typically run get_maintainer.pl on *.patch, and adjust the
> > git-send-email list from there.
> > 
> > > I actually gave up on getting my mailinglists from my email provider,
> > > now I just download it directly from lore. I hacked together a script
> > > that will scrape a public-inbox repo and convert it to a Maildir:
> > > 
> > > https://github.com/wkz/notmuch-lore
> > > 
> > > As you can tell from the name, it is tailored for plugging into notmuch,
> > > but the guts are pretty generic.  
> > 
> > Thanks, I set that up, it's syncing right now, I'm also going to compare
> > the size of the git tree vs the maildir I currently have.
> 
> Hi Vladimir, please let me know the results.
> Marek

Sure, it's still syncing, at 19GB currently. Please remind me next week
if I forget to check on it.