[PATCH v8 net-next 0/2] mv88e6xxx: Add MAB offload support

[PATCH v8 net-next 0/2] mv88e6xxx: Add MAB offload support

[Hans J. Schultz]

This patchset adds MAB [1] offload support in mv88e6xxx.

Patch #1: Fix a problem when reading the FID needed to get the VID.

Patch #2: The MAB implementation for mv88e6xxx.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=a35ec8e38cdd1766f29924ca391a01de20163931

Hans J. Schultz (2):
  net: dsa: mv88e6xxx: allow reading FID when handling ATU violations
  net: dsa: mv88e6xxx: mac-auth/MAB implementation

 drivers/net/dsa/mv88e6xxx/Makefile      |  1 +
 drivers/net/dsa/mv88e6xxx/chip.c        | 19 ++++--
 drivers/net/dsa/mv88e6xxx/chip.h        | 10 ++++
 drivers/net/dsa/mv88e6xxx/global1_atu.c | 70 ++++++++++++++++++++--
 drivers/net/dsa/mv88e6xxx/port.h        |  6 ++
 drivers/net/dsa/mv88e6xxx/switchdev.c   | 77 +++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/switchdev.h   | 19 ++++++
 7 files changed, 190 insertions(+), 12 deletions(-)
 create mode 100644 drivers/net/dsa/mv88e6xxx/switchdev.c
 create mode 100644 drivers/net/dsa/mv88e6xxx/switchdev.h

-- 
2.34.1

[PATCH v8 net-next 1/2] net: dsa: mv88e6xxx: allow reading FID when handling ATU violations

[Hans J. Schultz]

The FID is needed to get hold of which VID was involved in a violation,
thus the need to be able to read the FID.

For convenience the function mv88e6xxx_g1_atu_op() has been used to read
ATU violations, but the function invalidates reading the fid, so to both
read ATU violations without zeroing the fid, and read the fid, functions
have been added to ensure both are done correctly.

Signed-off-by: Hans J. Schultz <netdev@kapio-technology.com>
---
 drivers/net/dsa/mv88e6xxx/global1_atu.c | 60 ++++++++++++++++++++++---
 1 file changed, 55 insertions(+), 5 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/global1_atu.c b/drivers/net/dsa/mv88e6xxx/global1_atu.c
index 40bd67a5c8e9..8a874b6fc8e1 100644
--- a/drivers/net/dsa/mv88e6xxx/global1_atu.c
+++ b/drivers/net/dsa/mv88e6xxx/global1_atu.c
@@ -114,6 +114,19 @@ static int mv88e6xxx_g1_atu_op_wait(struct mv88e6xxx_chip *chip)
 	return mv88e6xxx_g1_wait_bit(chip, MV88E6XXX_G1_ATU_OP, bit, 0);
 }
 
+static int mv88e6xxx_g1_read_atu_violation(struct mv88e6xxx_chip *chip)
+{
+	int err;
+
+	err = mv88e6xxx_g1_write(chip, MV88E6XXX_G1_ATU_OP,
+				 MV88E6XXX_G1_ATU_OP_BUSY |
+				 MV88E6XXX_G1_ATU_OP_GET_CLR_VIOLATION);
+	if (err)
+		return err;
+
+	return mv88e6xxx_g1_atu_op_wait(chip);
+}
+
 static int mv88e6xxx_g1_atu_op(struct mv88e6xxx_chip *chip, u16 fid, u16 op)
 {
 	u16 val;
@@ -159,6 +172,41 @@ int mv88e6xxx_g1_atu_get_next(struct mv88e6xxx_chip *chip, u16 fid)
 	return mv88e6xxx_g1_atu_op(chip, fid, MV88E6XXX_G1_ATU_OP_GET_NEXT_DB);
 }
 
+static int mv88e6xxx_g1_atu_fid_read(struct mv88e6xxx_chip *chip, u16 *fid)
+{
+	u16 val = 0, upper = 0, op = 0;
+	int err = -EOPNOTSUPP;
+
+	if (mv88e6xxx_num_databases(chip) > 256) {
+		err = mv88e6xxx_g1_read(chip, MV88E6352_G1_ATU_FID, &val);
+		val &= 0xfff;
+		if (err)
+			return err;
+	} else {
+		err = mv88e6xxx_g1_read(chip, MV88E6XXX_G1_ATU_OP, &op);
+		if (err)
+			return err;
+		if (mv88e6xxx_num_databases(chip) > 64) {
+			/* ATU DBNum[7:4] are located in ATU Control 15:12 */
+			err = mv88e6xxx_g1_read(chip, MV88E6XXX_G1_ATU_CTL,
+						&upper);
+			if (err)
+				return err;
+
+			upper = (upper >> 8) & 0x00f0;
+		} else if (mv88e6xxx_num_databases(chip) > 16) {
+			/* ATU DBNum[5:4] are located in ATU Operation 9:8 */
+			upper = (op >> 4) & 0x30;
+		}
+
+		/* ATU DBNum[3:0] are located in ATU Operation 3:0 */
+		val = (op & 0xf) | upper;
+	}
+	*fid = val;
+
+	return err;
+}
+
 /* Offset 0x0C: ATU Data Register */
 
 static int mv88e6xxx_g1_atu_data_read(struct mv88e6xxx_chip *chip,
@@ -353,14 +401,12 @@ static irqreturn_t mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
 {
 	struct mv88e6xxx_chip *chip = dev_id;
 	struct mv88e6xxx_atu_entry entry;
-	int spid;
-	int err;
-	u16 val;
+	int err, spid;
+	u16 val, fid;
 
 	mv88e6xxx_reg_lock(chip);
 
-	err = mv88e6xxx_g1_atu_op(chip, 0,
-				  MV88E6XXX_G1_ATU_OP_GET_CLR_VIOLATION);
+	err = mv88e6xxx_g1_read_atu_violation(chip);
 	if (err)
 		goto out;
 
@@ -368,6 +414,10 @@ static irqreturn_t mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
 	if (err)
 		goto out;
 
+	err = mv88e6xxx_g1_atu_fid_read(chip, &fid);
+	if (err)
+		goto out;
+
 	err = mv88e6xxx_g1_atu_data_read(chip, &entry);
 	if (err)
 		goto out;
-- 
2.34.1

[PATCH v8 net-next 2/2] net: dsa: mv88e6xxx: mac-auth/MAB implementation

[Hans J. Schultz]

This implementation for the Marvell mv88e6xxx chip series, is based on
handling ATU miss violations occurring when packets ingress on a port
that is locked with learning on. This will trigger a
SWITCHDEV_FDB_ADD_TO_BRIDGE event, that wil incurr the bridge module,
to add a locked FDB entry. This bridge FDB entry will not age out as
it has the extern_learn flag set.

Userspace daemons can listen to these events and either accept or deny
access for the host, by either replacing the locked FDB entry with a
simple entry or leave the locked entry.

If the host MAC address is already present on another port, a ATU
member violation will occur, but to no real effect. Statistics on these
violations can be shown with the command and example output of interest:

NIC statistics:
...
     atu_member_violation: 36
     atu_miss_violation: 23
...

Where ethX is the interface of the MAB enabled port.

Signed-off-by: Hans J. Schultz <netdev@kapio-technology.com>
---
 drivers/net/dsa/mv88e6xxx/Makefile      |  1 +
 drivers/net/dsa/mv88e6xxx/chip.c        | 19 ++++--
 drivers/net/dsa/mv88e6xxx/chip.h        | 10 ++++
 drivers/net/dsa/mv88e6xxx/global1_atu.c | 10 +++-
 drivers/net/dsa/mv88e6xxx/port.h        |  6 ++
 drivers/net/dsa/mv88e6xxx/switchdev.c   | 77 +++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/switchdev.h   | 19 ++++++
 7 files changed, 135 insertions(+), 7 deletions(-)
 create mode 100644 drivers/net/dsa/mv88e6xxx/switchdev.c
 create mode 100644 drivers/net/dsa/mv88e6xxx/switchdev.h

diff --git a/drivers/net/dsa/mv88e6xxx/Makefile b/drivers/net/dsa/mv88e6xxx/Makefile
index c8eca2b6f959..be903a983780 100644
--- a/drivers/net/dsa/mv88e6xxx/Makefile
+++ b/drivers/net/dsa/mv88e6xxx/Makefile
@@ -15,3 +15,4 @@ mv88e6xxx-objs += port_hidden.o
 mv88e6xxx-$(CONFIG_NET_DSA_MV88E6XXX_PTP) += ptp.o
 mv88e6xxx-objs += serdes.o
 mv88e6xxx-objs += smi.o
+mv88e6xxx-objs += switchdev.o
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index ccfa4751d3b7..efc0085a5616 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1726,11 +1726,11 @@ static int mv88e6xxx_vtu_get(struct mv88e6xxx_chip *chip, u16 vid,
 	return err;
 }
 
-static int mv88e6xxx_vtu_walk(struct mv88e6xxx_chip *chip,
-			      int (*cb)(struct mv88e6xxx_chip *chip,
-					const struct mv88e6xxx_vtu_entry *entry,
-					void *priv),
-			      void *priv)
+int mv88e6xxx_vtu_walk(struct mv88e6xxx_chip *chip,
+		       int (*cb)(struct mv88e6xxx_chip *chip,
+				 const struct mv88e6xxx_vtu_entry *entry,
+				 void *priv),
+		       void *priv)
 {
 	struct mv88e6xxx_vtu_entry entry = {
 		.vid = mv88e6xxx_max_vid(chip),
@@ -6524,7 +6524,7 @@ static int mv88e6xxx_port_pre_bridge_flags(struct dsa_switch *ds, int port,
 	const struct mv88e6xxx_ops *ops;
 
 	if (flags.mask & ~(BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD |
-			   BR_BCAST_FLOOD | BR_PORT_LOCKED))
+			   BR_BCAST_FLOOD | BR_PORT_LOCKED | BR_PORT_MAB))
 		return -EINVAL;
 
 	ops = chip->info->ops;
@@ -6582,12 +6582,19 @@ static int mv88e6xxx_port_bridge_flags(struct dsa_switch *ds, int port,
 			goto out;
 	}
 
+	if (flags.mask & BR_PORT_MAB) {
+		chip->ports[port].mab = !!(flags.val & BR_PORT_MAB);
+		err = 0;
+	}
+
 	if (flags.mask & BR_PORT_LOCKED) {
 		bool locked = !!(flags.val & BR_PORT_LOCKED);
 
 		err = mv88e6xxx_port_set_lock(chip, port, locked);
 		if (err)
 			goto out;
+
+		chip->ports[port].locked = locked;
 	}
 out:
 	mv88e6xxx_reg_unlock(chip);
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index e693154cf803..3b951cd0e6f8 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -280,6 +280,10 @@ struct mv88e6xxx_port {
 	unsigned int serdes_irq;
 	char serdes_irq_name[64];
 	struct devlink_region *region;
+
+	/* Locked port and MacAuth control flags */
+	bool locked;
+	bool mab;
 };
 
 enum mv88e6xxx_region_id {
@@ -802,6 +806,12 @@ static inline void mv88e6xxx_reg_unlock(struct mv88e6xxx_chip *chip)
 	mutex_unlock(&chip->reg_lock);
 }
 
+int mv88e6xxx_vtu_walk(struct mv88e6xxx_chip *chip,
+		       int (*cb)(struct mv88e6xxx_chip *chip,
+				 const struct mv88e6xxx_vtu_entry *entry,
+				 void *priv),
+		       void *priv);
+
 int mv88e6xxx_fid_map(struct mv88e6xxx_chip *chip, unsigned long *bitmap);
 
 #endif /* _MV88E6XXX_CHIP_H */
diff --git a/drivers/net/dsa/mv88e6xxx/global1_atu.c b/drivers/net/dsa/mv88e6xxx/global1_atu.c
index 8a874b6fc8e1..0a57f4e7dd46 100644
--- a/drivers/net/dsa/mv88e6xxx/global1_atu.c
+++ b/drivers/net/dsa/mv88e6xxx/global1_atu.c
@@ -12,6 +12,7 @@
 
 #include "chip.h"
 #include "global1.h"
+#include "switchdev.h"
 
 /* Offset 0x01: ATU FID Register */
 
@@ -426,6 +427,8 @@ static irqreturn_t mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
 	if (err)
 		goto out;
 
+	mv88e6xxx_reg_unlock(chip);
+
 	spid = entry.state;
 
 	if (val & MV88E6XXX_G1_ATU_OP_AGE_OUT_VIOLATION) {
@@ -446,6 +449,12 @@ static irqreturn_t mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
 				    "ATU miss violation for %pM portvec %x spid %d\n",
 				    entry.mac, entry.portvec, spid);
 		chip->ports[spid].atu_miss_violation++;
+
+		if (fid && chip->ports[spid].mab)
+			err = mv88e6xxx_handle_violation(chip, spid, &entry,
+							 fid, MV88E6XXX_G1_ATU_OP_MISS_VIOLATION);
+		if (err)
+			goto out;
 	}
 
 	if (val & MV88E6XXX_G1_ATU_OP_FULL_VIOLATION) {
@@ -454,7 +463,6 @@ static irqreturn_t mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
 				    entry.mac, entry.portvec, spid);
 		chip->ports[spid].atu_full_violation++;
 	}
-	mv88e6xxx_reg_unlock(chip);
 
 	return IRQ_HANDLED;
 
diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h
index aec9d4fd20e3..bd90a02865a0 100644
--- a/drivers/net/dsa/mv88e6xxx/port.h
+++ b/drivers/net/dsa/mv88e6xxx/port.h
@@ -377,6 +377,12 @@ int mv88e6xxx_port_set_pvid(struct mv88e6xxx_chip *chip, int port, u16 pvid);
 int mv88e6xxx_port_set_lock(struct mv88e6xxx_chip *chip, int port,
 			    bool locked);
 
+static inline bool mv88e6xxx_port_is_locked(struct mv88e6xxx_chip *chip,
+					    int port)
+{
+	return chip->ports[port].locked;
+}
+
 int mv88e6xxx_port_set_8021q_mode(struct mv88e6xxx_chip *chip, int port,
 				  u16 mode);
 int mv88e6095_port_tag_remap(struct mv88e6xxx_chip *chip, int port);
diff --git a/drivers/net/dsa/mv88e6xxx/switchdev.c b/drivers/net/dsa/mv88e6xxx/switchdev.c
new file mode 100644
index 000000000000..000778550532
--- /dev/null
+++ b/drivers/net/dsa/mv88e6xxx/switchdev.c
@@ -0,0 +1,77 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * switchdev.c
+ *
+ *	Authors:
+ *	Hans J. Schultz		<hans.schultz@westermo.com>
+ *
+ */
+
+#include <net/switchdev.h>
+#include "chip.h"
+#include "global1.h"
+
+struct mv88e6xxx_fid_search_ctx {
+	u16 fid_search;
+	u16 vid_found;
+};
+
+static int mv88e6xxx_find_vid(struct mv88e6xxx_chip *chip,
+			      const struct mv88e6xxx_vtu_entry *entry,
+			      void *priv)
+{
+	struct mv88e6xxx_fid_search_ctx *ctx = priv;
+
+	if (ctx->fid_search == entry->fid) {
+		ctx->vid_found = entry->vid;
+		return 1;
+	}
+
+	return 0;
+}
+
+int mv88e6xxx_handle_violation(struct mv88e6xxx_chip *chip, int port,
+			       struct mv88e6xxx_atu_entry *entry,
+			       u16 fid, u16 type)
+{
+	struct switchdev_notifier_fdb_info info = {
+		.addr = entry->mac,
+		.locked = true,
+	};
+	struct mv88e6xxx_fid_search_ctx ctx;
+	struct net_device *brport;
+	struct dsa_port *dp;
+	int err;
+
+	if (mv88e6xxx_is_invalid_port(chip, port))
+		return -ENODEV;
+
+	ctx.fid_search = fid;
+	mv88e6xxx_reg_lock(chip);
+	err = mv88e6xxx_vtu_walk(chip, mv88e6xxx_find_vid, &ctx);
+	mv88e6xxx_reg_unlock(chip);
+	if (err < 0)
+		return err;
+	if (err == 1)
+		info.vid = ctx.vid_found;
+	else
+		return -ENODATA;
+
+	switch (type) {
+	case MV88E6XXX_G1_ATU_OP_MISS_VIOLATION:
+		dp = dsa_to_port(chip->ds, port);
+
+		rtnl_lock();
+		brport = dsa_port_to_bridge_port(dp);
+		if (!brport) {
+			rtnl_unlock();
+			return -ENODEV;
+		}
+		err = call_switchdev_notifiers(SWITCHDEV_FDB_ADD_TO_BRIDGE,
+					       brport, &info.info, NULL);
+		rtnl_unlock();
+		break;
+	}
+
+	return err;
+}
diff --git a/drivers/net/dsa/mv88e6xxx/switchdev.h b/drivers/net/dsa/mv88e6xxx/switchdev.h
new file mode 100644
index 000000000000..ccc10a9d4072
--- /dev/null
+++ b/drivers/net/dsa/mv88e6xxx/switchdev.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * switchdev.h
+ *
+ *	Authors:
+ *	Hans J. Schultz		<hans.schultz@westermo.com>
+ *
+ */
+
+#ifndef DRIVERS_NET_DSA_MV88E6XXX_SWITCHDEV_H_
+#define DRIVERS_NET_DSA_MV88E6XXX_SWITCHDEV_H_
+
+#include "chip.h"
+
+int mv88e6xxx_handle_violation(struct mv88e6xxx_chip *chip, int port,
+			       struct mv88e6xxx_atu_entry *entry,
+			       u16 fid, u16 type);
+
+#endif /* DRIVERS_NET_DSA_MV88E6XXX_SWITCHDEV_H_ */
-- 
2.34.1

Re: [PATCH v8 net-next 0/2] mv88e6xxx: Add MAB offload support

[Jakub Kicinski]

On Sat, 12 Nov 2022 21:37:46 +0100 Hans J. Schultz wrote:
> This patchset adds MAB [1] offload support in mv88e6xxx.
> 
> Patch #1: Fix a problem when reading the FID needed to get the VID.
> 
> Patch #2: The MAB implementation for mv88e6xxx.
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=a35ec8e38cdd1766f29924ca391a01de20163931

Vladimir, Ido, ack?

Re: [PATCH v8 net-next 0/2] mv88e6xxx: Add MAB offload support

[Jakub Kicinski]

On Mon, 14 Nov 2022 18:57:04 -0800 Jakub Kicinski wrote:
> On Sat, 12 Nov 2022 21:37:46 +0100 Hans J. Schultz wrote:
> > This patchset adds MAB [1] offload support in mv88e6xxx.
> > 
> > Patch #1: Fix a problem when reading the FID needed to get the VID.
> > 
> > Patch #2: The MAB implementation for mv88e6xxx.
> > 
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=a35ec8e38cdd1766f29924ca391a01de20163931  
> 
> Vladimir, Ido, ack?

Ah, either way a v9 will be needed:

drivers/net/dsa/mv88e6xxx/switchdev.c:33:5: warning: no previous prototype for function 'mv88e6xxx_handle_violation' [-Wmissing-prototypes]
int mv88e6xxx_handle_violation(struct mv88e6xxx_chip *chip, int port,
    ^
drivers/net/dsa/mv88e6xxx/switchdev.c:33:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int mv88e6xxx_handle_violation(struct mv88e6xxx_chip *chip, int port,
^

Re: [PATCH v8 net-next 0/2] mv88e6xxx: Add MAB offload support

[Ido Schimmel]

On Sat, Nov 12, 2022 at 09:37:46PM +0100, Hans J. Schultz wrote:
> This patchset adds MAB [1] offload support in mv88e6xxx.
> 
> Patch #1: Fix a problem when reading the FID needed to get the VID.
> 
> Patch #2: The MAB implementation for mv88e6xxx.

Just to be sure, this was tested with bridge_locked_port.sh, right?

Re: [PATCH v8 net-next 2/2] net: dsa: mv88e6xxx: mac-auth/MAB implementation

[Ido Schimmel]

On Sat, Nov 12, 2022 at 09:37:48PM +0100, Hans J. Schultz wrote:
> diff --git a/drivers/net/dsa/mv88e6xxx/global1_atu.c b/drivers/net/dsa/mv88e6xxx/global1_atu.c
> index 8a874b6fc8e1..0a57f4e7dd46 100644
> --- a/drivers/net/dsa/mv88e6xxx/global1_atu.c
> +++ b/drivers/net/dsa/mv88e6xxx/global1_atu.c
> @@ -12,6 +12,7 @@
>  
>  #include "chip.h"
>  #include "global1.h"
> +#include "switchdev.h"
>  
>  /* Offset 0x01: ATU FID Register */
>  
> @@ -426,6 +427,8 @@ static irqreturn_t mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
>  	if (err)
>  		goto out;
>  
> +	mv88e6xxx_reg_unlock(chip);

Why? At minimum such a change needs to be explained in the commit
message and probably split to a separate preparatory patch, assuming the
change is actually required.

> +
>  	spid = entry.state;
>  
>  	if (val & MV88E6XXX_G1_ATU_OP_AGE_OUT_VIOLATION) {
> @@ -446,6 +449,12 @@ static irqreturn_t mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
>  				    "ATU miss violation for %pM portvec %x spid %d\n",
>  				    entry.mac, entry.portvec, spid);
>  		chip->ports[spid].atu_miss_violation++;
> +
> +		if (fid && chip->ports[spid].mab)
> +			err = mv88e6xxx_handle_violation(chip, spid, &entry,
> +							 fid, MV88E6XXX_G1_ATU_OP_MISS_VIOLATION);
> +		if (err)
> +			goto out;

On error the kernel will try to unlock a mutex that is already unlocked.


>  	}
>  
>  	if (val & MV88E6XXX_G1_ATU_OP_FULL_VIOLATION) {
> @@ -454,7 +463,6 @@ static irqreturn_t mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
>  				    entry.mac, entry.portvec, spid);
>  		chip->ports[spid].atu_full_violation++;
>  	}
> -	mv88e6xxx_reg_unlock(chip);
>  
>  	return IRQ_HANDLED;

Re: [PATCH v8 net-next 0/2] mv88e6xxx: Add MAB offload support

[netdev]

On 2022-11-15 10:30, Ido Schimmel wrote:
> On Sat, Nov 12, 2022 at 09:37:46PM +0100, Hans J. Schultz wrote:
>> This patchset adds MAB [1] offload support in mv88e6xxx.
>> 
>> Patch #1: Fix a problem when reading the FID needed to get the VID.
>> 
>> Patch #2: The MAB implementation for mv88e6xxx.
> 
> Just to be sure, this was tested with bridge_locked_port.sh, right?

As I have the phy regression I have given notice of, that has simply not
been possible. After maybe 50 resets it worked for me at a point
(something to do with timing), and I tested it manually.

When I have tried to run the selftests, I get errors related to the phy
problem, which I have not been able to find a way around.

Re: [PATCH v8 net-next 0/2] mv88e6xxx: Add MAB offload support

[Vladimir Oltean]

On Tue, Nov 15, 2022 at 11:26:55AM +0100, netdev@kapio-technology.com wrote:
> On 2022-11-15 10:30, Ido Schimmel wrote:
> > On Sat, Nov 12, 2022 at 09:37:46PM +0100, Hans J. Schultz wrote:
> > > This patchset adds MAB [1] offload support in mv88e6xxx.
> > > 
> > > Patch #1: Fix a problem when reading the FID needed to get the VID.
> > > 
> > > Patch #2: The MAB implementation for mv88e6xxx.
> > 
> > Just to be sure, this was tested with bridge_locked_port.sh, right?
> 
> As I have the phy regression I have given notice of, that has simply not
> been possible. After maybe 50 resets it worked for me at a point
> (something to do with timing), and I tested it manually.
> 
> When I have tried to run the selftests, I get errors related to the phy
> problem, which I have not been able to find a way around.

What PHY regression?

Re: [PATCH v8 net-next 2/2] net: dsa: mv88e6xxx: mac-auth/MAB implementation

[netdev]

On 2022-11-15 10:58, Ido Schimmel wrote:
> On Sat, Nov 12, 2022 at 09:37:48PM +0100, Hans J. Schultz wrote:
>> diff --git a/drivers/net/dsa/mv88e6xxx/global1_atu.c 
>> b/drivers/net/dsa/mv88e6xxx/global1_atu.c
>> index 8a874b6fc8e1..0a57f4e7dd46 100644
>> --- a/drivers/net/dsa/mv88e6xxx/global1_atu.c
>> +++ b/drivers/net/dsa/mv88e6xxx/global1_atu.c
>> @@ -12,6 +12,7 @@
>> 
>>  #include "chip.h"
>>  #include "global1.h"
>> +#include "switchdev.h"
>> 
>>  /* Offset 0x01: ATU FID Register */
>> 
>> @@ -426,6 +427,8 @@ static irqreturn_t 
>> mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
>>  	if (err)
>>  		goto out;
>> 
>> +	mv88e6xxx_reg_unlock(chip);
> 
> Why? At minimum such a change needs to be explained in the commit
> message and probably split to a separate preparatory patch, assuming 
> the
> change is actually required.

This was a change done long time ago related to that the violation 
handle function takes the NL lock,
which could lead to a double-lock deadlock afair if the chip lock is 
taken throughout the handler.

Re: [PATCH v8 net-next 0/2] mv88e6xxx: Add MAB offload support

[netdev]

On 2022-11-15 11:28, Vladimir Oltean wrote:
> On Tue, Nov 15, 2022 at 11:26:55AM +0100, netdev@kapio-technology.com 
> wrote:
>> On 2022-11-15 10:30, Ido Schimmel wrote:
>> > On Sat, Nov 12, 2022 at 09:37:46PM +0100, Hans J. Schultz wrote:
>> > > This patchset adds MAB [1] offload support in mv88e6xxx.
>> > >
>> > > Patch #1: Fix a problem when reading the FID needed to get the VID.
>> > >
>> > > Patch #2: The MAB implementation for mv88e6xxx.
>> >
>> > Just to be sure, this was tested with bridge_locked_port.sh, right?
>> 
>> As I have the phy regression I have given notice of, that has simply 
>> not
>> been possible. After maybe 50 resets it worked for me at a point
>> (something to do with timing), and I tested it manually.
>> 
>> When I have tried to run the selftests, I get errors related to the 
>> phy
>> problem, which I have not been able to find a way around.
> 
> What PHY regression?

I had a discussion with Jacub, which resulted in me sending a mail to
maintainers on this. The problem is shown below...


the phy is marvell/6097/88e3082

------------[ cut here ]------------
WARNING: CPU: 0 PID: 332 at drivers/net/phy/phy.c:975
phy_error+0x28/0x54
Modules linked in:
CPU: 0 PID: 332 Comm: kworker/0:0 Tainted: G        W          6.0.0 #17
Hardware name: Freescale i.MX27 (Device Tree Support)
Workqueue: events_power_efficient phy_state_machine
   unwind_backtrace from show_stack+0x18/0x1c
   show_stack from dump_stack_lvl+0x28/0x30
   dump_stack_lvl from __warn+0xb8/0x114
   __warn from warn_slowpath_fmt+0x80/0xbc
   warn_slowpath_fmt from phy_error+0x28/0x54
   phy_error from phy_state_machine+0xbc/0x218
   phy_state_machine from process_one_work+0x17c/0x244
   process_one_work from worker_thread+0x248/0x2cc
   worker_thread from kthread+0xb0/0xbc
   kthread from ret_from_fork+0x14/0x2c
Exception stack(0xc4a71fb0 to 0xc4a71ff8)
1fa0:                                     00000000 00000000 00000000
00000000
1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000
00000000
1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
---[ end trace 0000000000000000 ]---

Re: [PATCH v8 net-next 0/2] mv88e6xxx: Add MAB offload support

[Vladimir Oltean]

On Tue, Nov 15, 2022 at 11:52:40AM +0100, netdev@kapio-technology.com wrote:
> I had a discussion with Jacub, which resulted in me sending a mail to
> maintainers on this. The problem is shown below...
> 
> the phy is marvell/6097/88e3082
> 
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 332 at drivers/net/phy/phy.c:975
> phy_error+0x28/0x54
> Modules linked in:
> CPU: 0 PID: 332 Comm: kworker/0:0 Tainted: G        W          6.0.0 #17
> Hardware name: Freescale i.MX27 (Device Tree Support)
> Workqueue: events_power_efficient phy_state_machine
>   unwind_backtrace from show_stack+0x18/0x1c
>   show_stack from dump_stack_lvl+0x28/0x30
>   dump_stack_lvl from __warn+0xb8/0x114
>   __warn from warn_slowpath_fmt+0x80/0xbc
>   warn_slowpath_fmt from phy_error+0x28/0x54
>   phy_error from phy_state_machine+0xbc/0x218
>   phy_state_machine from process_one_work+0x17c/0x244
>   process_one_work from worker_thread+0x248/0x2cc
>   worker_thread from kthread+0xb0/0xbc
>   kthread from ret_from_fork+0x14/0x2c
> Exception stack(0xc4a71fb0 to 0xc4a71ff8)
> 1fa0:                                     00000000 00000000 00000000 00000000
> 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> ---[ end trace 0000000000000000 ]---

Was that email public on the lists? I don't see it...

The phy_error() is called from phy_state_machine() if one of
phy_check_link_status() or phy_start_aneg() fails.

Could you please print exactly the value of "err", as well as dig deeper
to see which call is failing, all the way into the PHY driver?

Easiest way to do that would probably be something like:

$ trace-cmd record -e mdio sleep 10 &
... do your stuff ...
$ trace-cmd report
    kworker/u4:3-337   [001]    59.054741: mdio_access:          0000:00:00.3 read  phy:0x13 reg:0x01 val:0x7949
    kworker/u4:3-337   [001]    59.054941: mdio_access:          0000:00:00.3 read  phy:0x13 reg:0x09 val:0x0700
    kworker/u4:3-337   [001]    59.055262: mdio_access:          0000:00:00.3 read  phy:0x13 reg:0x0a val:0x4000
    kworker/u4:3-337   [001]    60.075808: mdio_access:          0000:00:00.3 read  phy:0x13 reg:0x1c val:0x3005

"val" will be negative when there is an error. This is to see quicker what fails,
and if some MDIO access ever works.

If you don't want to enable CONFIG_FTRACE or install trace-cmd, you
could also probably do the debugging manually.

Re: [PATCH v8 net-next 0/2] mv88e6xxx: Add MAB offload support

[netdev]

On 2022-11-15 12:10, Vladimir Oltean wrote:
> On Tue, Nov 15, 2022 at 11:52:40AM +0100, netdev@kapio-technology.com 
> wrote:
>> I had a discussion with Jacub, which resulted in me sending a mail to
>> maintainers on this. The problem is shown below...
>> 
>> the phy is marvell/6097/88e3082
>> 
>> ------------[ cut here ]------------
>> WARNING: CPU: 0 PID: 332 at drivers/net/phy/phy.c:975
>> phy_error+0x28/0x54
>> Modules linked in:
>> CPU: 0 PID: 332 Comm: kworker/0:0 Tainted: G        W          6.0.0 
>> #17
>> Hardware name: Freescale i.MX27 (Device Tree Support)
>> Workqueue: events_power_efficient phy_state_machine
>>   unwind_backtrace from show_stack+0x18/0x1c
>>   show_stack from dump_stack_lvl+0x28/0x30
>>   dump_stack_lvl from __warn+0xb8/0x114
>>   __warn from warn_slowpath_fmt+0x80/0xbc
>>   warn_slowpath_fmt from phy_error+0x28/0x54
>>   phy_error from phy_state_machine+0xbc/0x218
>>   phy_state_machine from process_one_work+0x17c/0x244
>>   process_one_work from worker_thread+0x248/0x2cc
>>   worker_thread from kthread+0xb0/0xbc
>>   kthread from ret_from_fork+0x14/0x2c
>> Exception stack(0xc4a71fb0 to 0xc4a71ff8)
>> 1fa0:                                     00000000 00000000 00000000 
>> 00000000
>> 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 
>> 00000000
>> 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
>> ---[ end trace 0000000000000000 ]---
> 
> Was that email public on the lists? I don't see it...

Sorry, yes the public list was not added.

> 
> The phy_error() is called from phy_state_machine() if one of
> phy_check_link_status() or phy_start_aneg() fails.
> 
> Could you please print exactly the value of "err", as well as dig 
> deeper
> to see which call is failing, all the way into the PHY driver?

It happens on upstart, so I would then have to hack the system upstart 
to add trace.

I also have:
mv88e6085 1002b000.ethernet-1:04: switch 0x990 detected: Marvell 
88E6097/88E6097F, revision 2
mv88e6085 1002b000.ethernet-1:04: configuring for fixed/rgmii-id link 
mode
mv88e6085 1002b000.ethernet-1:04: Link is Up - 100Mbps/Full - flow 
control off
mv88e6085 1002b000.ethernet-1:04 eth10 (uninitialized): PHY 
[!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdio:00] driver 
[Generic PHY] (irq=POLL)
mv88e6085 1002b000.ethernet-1:04 eth6 (uninitialized): PHY 
[!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdio:01] driver 
[Generic PHY] (irq=POLL)
mv88e6085 1002b000.ethernet-1:04 eth9 (uninitialized): PHY 
[!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdio:02] driver 
[Generic PHY] (irq=POLL)
mv88e6085 1002b000.ethernet-1:04 eth5 (uninitialized): PHY 
[!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdio:03] driver 
[Generic PHY] (irq=POLL)
mv88e6085 1002b000.ethernet-1:04 eth8 (uninitialized): PHY 
[!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdio:04] driver 
[Generic PHY] (irq=POLL)
mv88e6085 1002b000.ethernet-1:04 eth4 (uninitialized): PHY 
[!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdio:05] driver 
[Generic PHY] (irq=POLL)
mv88e6085 1002b000.ethernet-1:04 eth7 (uninitialized): PHY 
[!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdio:06] driver 
[Generic PHY] (irq=POLL)
mv88e6085 1002b000.ethernet-1:04 eth3 (uninitialized): PHY 
[!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdio:07] driver 
[Generic PHY] (irq=POLL)
mv88e6085 1002b000.ethernet-1:04 eth2 (uninitialized): PHY 
[!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdioe:08] driver 
[Marvell 88E1112] (irq=174)
mv88e6085 1002b000.ethernet-1:04 eth1 (uninitialized): PHY 
[!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdioe:09] driver 
[Marvell 88E1112] (irq=175)

after this and adding the ifaces to the bridge, it continues like:

br0: port 1(eth10) entered blocking state
br0: port 1(eth10) entered disabled state
br0: port 2(eth6) entered blocking state
br0: port 2(eth6) entered disabled state
device eth6 entered promiscuous mode
device eth10 entered promiscuous mode
br0: port 3(eth9) entered blocking state
br0: port 3(eth9) entered disabled state
device eth9 entered promiscuous mode
br0: port 4(eth5) entered blocking state
br0: port 4(eth5) entered disabled state
device eth5 entered promiscuous mode
br0: port 5(eth8) entered blocking state
br0: port 5(eth8) entered disabled state
device eth8 entered promiscuous mode
br0: port 6(eth4) entered blocking state
br0: port 6(eth4) entered disabled state
mv88e6085 1002b000.ethernet-1:04: Timeout while waiting for switch
mv88e6085 1002b000.ethernet-1:04: port 0 failed to add 9a:af:03:f1:bd:0a 
vid 1 to fdb: -110
device eth4 entered promiscuous mode
br0: port 7(eth7) entered blocking state
br0: port 7(eth7) entered disabled state

I don't know if that gives ay clues...?

Otherwise I have to take more time to see what I can dig out. The 
easiest for me is then to
add some printk statements giving targeted information if told what and 
where...

> 
> Easiest way to do that would probably be something like:
> 
> $ trace-cmd record -e mdio sleep 10 &
> ... do your stuff ...
> $ trace-cmd report
>     kworker/u4:3-337   [001]    59.054741: mdio_access:
> 0000:00:00.3 read  phy:0x13 reg:0x01 val:0x7949
>     kworker/u4:3-337   [001]    59.054941: mdio_access:
> 0000:00:00.3 read  phy:0x13 reg:0x09 val:0x0700
>     kworker/u4:3-337   [001]    59.055262: mdio_access:
> 0000:00:00.3 read  phy:0x13 reg:0x0a val:0x4000
>     kworker/u4:3-337   [001]    60.075808: mdio_access:
> 0000:00:00.3 read  phy:0x13 reg:0x1c val:0x3005
> 
> "val" will be negative when there is an error. This is to see quicker
> what fails,
> and if some MDIO access ever works.
> 
> If you don't want to enable CONFIG_FTRACE or install trace-cmd, you
> could also probably do the debugging manually.

Re: [PATCH v8 net-next 0/2] mv88e6xxx: Add MAB offload support

[Vladimir Oltean]

On Tue, Nov 15, 2022 at 12:31:59PM +0100, netdev@kapio-technology.com wrote:
> It happens on upstart, so I would then have to hack the system upstart to
> add trace.

Hack upstart or disable the service that brings the switch ports up, and
bring them up manually...

> I also have:
> mv88e6085 1002b000.ethernet-1:04: switch 0x990 detected: Marvell 88E6097/88E6097F, revision 2
> mv88e6085 1002b000.ethernet-1:04: configuring for fixed/rgmii-id link mode
> mv88e6085 1002b000.ethernet-1:04: Link is Up - 100Mbps/Full - flow control off
> mv88e6085 1002b000.ethernet-1:04 eth10 (uninitialized): PHY [!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdio:00] driver [Generic PHY] (irq=POLL)
> mv88e6085 1002b000.ethernet-1:04 eth6 (uninitialized): PHY [!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdio:01] driver [Generic PHY] (irq=POLL)
> mv88e6085 1002b000.ethernet-1:04 eth9 (uninitialized): PHY [!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdio:02] driver [Generic PHY] (irq=POLL)
> mv88e6085 1002b000.ethernet-1:04 eth5 (uninitialized): PHY [!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdio:03] driver [Generic PHY] (irq=POLL)
> mv88e6085 1002b000.ethernet-1:04 eth8 (uninitialized): PHY [!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdio:04] driver [Generic PHY] (irq=POLL)
> mv88e6085 1002b000.ethernet-1:04 eth4 (uninitialized): PHY [!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdio:05] driver [Generic PHY] (irq=POLL)
> mv88e6085 1002b000.ethernet-1:04 eth7 (uninitialized): PHY [!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdio:06] driver [Generic PHY] (irq=POLL)
> mv88e6085 1002b000.ethernet-1:04 eth3 (uninitialized): PHY [!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdio:07] driver [Generic PHY] (irq=POLL)
> mv88e6085 1002b000.ethernet-1:04 eth2 (uninitialized): PHY [!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdioe:08] driver [Marvell 88E1112] (irq=174)
> mv88e6085 1002b000.ethernet-1:04 eth1 (uninitialized): PHY [!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdioe:09] driver [Marvell 88E1112] (irq=175)
> 
> after this and adding the ifaces to the bridge, it continues like:
> 
> br0: port 1(eth10) entered blocking state
> br0: port 1(eth10) entered disabled state
> br0: port 2(eth6) entered blocking state
> br0: port 2(eth6) entered disabled state
> device eth6 entered promiscuous mode
> device eth10 entered promiscuous mode
> br0: port 3(eth9) entered blocking state
> br0: port 3(eth9) entered disabled state
> device eth9 entered promiscuous mode
> br0: port 4(eth5) entered blocking state
> br0: port 4(eth5) entered disabled state
> device eth5 entered promiscuous mode
> br0: port 5(eth8) entered blocking state
> br0: port 5(eth8) entered disabled state
> device eth8 entered promiscuous mode
> br0: port 6(eth4) entered blocking state
> br0: port 6(eth4) entered disabled state
> mv88e6085 1002b000.ethernet-1:04: Timeout while waiting for switch
> mv88e6085 1002b000.ethernet-1:04: port 0 failed to add 9a:af:03:f1:bd:0a vid 1 to fdb: -110

Dumb question, but if you get errors like this, how can you test anything at all
in the patches that you submit?

> device eth4 entered promiscuous mode
> br0: port 7(eth7) entered blocking state
> br0: port 7(eth7) entered disabled state
> 
> I don't know if that gives ay clues...?

Not really. That error might be related - something indicating a breakage
in the top-level (fec IIUC) MDIO controller, or not. There was "recent"
rework almost everywhere.  For example commit 35da1dfd9484 ("net: dsa:
mv88e6xxx: Improve performance of busy bit polling"). That also hooks
into the mv88e6xxx cascaded MDIO controller (mv88e6xxx_g2_smi_phy_wait),
so there might be something there.

> 
> Otherwise I have to take more time to see what I can dig out. The easiest
> for me is then to add some printk statements giving targeted information if told what and
> where...

Do you have a timeline for when the regression was introduced?
Commit 35da1dfd9484 reverts cleanly, so I suppose giving it a go with
that reverted might be worth a shot. Otherwise, a bisect from a known
working version only takes a couple of hours, and shouldn't require
other changes to the setup.

Re: [PATCH v8 net-next 0/2] mv88e6xxx: Add MAB offload support

[netdev]

On 2022-11-15 13:22, Vladimir Oltean wrote:
> On Tue, Nov 15, 2022 at 12:31:59PM +0100, netdev@kapio-technology.com 
> wrote:
>> It happens on upstart, so I would then have to hack the system upstart 
>> to
>> add trace.
> 
> Hack upstart or disable the service that brings the switch ports up, 
> and
> bring them up manually...
> 
>> I also have:
>> mv88e6085 1002b000.ethernet-1:04: switch 0x990 detected: Marvell 
>> 88E6097/88E6097F, revision 2
>> mv88e6085 1002b000.ethernet-1:04: configuring for fixed/rgmii-id link 
>> mode
>> mv88e6085 1002b000.ethernet-1:04: Link is Up - 100Mbps/Full - flow 
>> control off
>> mv88e6085 1002b000.ethernet-1:04 eth10 (uninitialized): PHY 
>> [!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdio:00] driver 
>> [Generic PHY] (irq=POLL)
>> mv88e6085 1002b000.ethernet-1:04 eth6 (uninitialized): PHY 
>> [!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdio:01] driver 
>> [Generic PHY] (irq=POLL)
>> mv88e6085 1002b000.ethernet-1:04 eth9 (uninitialized): PHY 
>> [!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdio:02] driver 
>> [Generic PHY] (irq=POLL)
>> mv88e6085 1002b000.ethernet-1:04 eth5 (uninitialized): PHY 
>> [!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdio:03] driver 
>> [Generic PHY] (irq=POLL)
>> mv88e6085 1002b000.ethernet-1:04 eth8 (uninitialized): PHY 
>> [!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdio:04] driver 
>> [Generic PHY] (irq=POLL)
>> mv88e6085 1002b000.ethernet-1:04 eth4 (uninitialized): PHY 
>> [!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdio:05] driver 
>> [Generic PHY] (irq=POLL)
>> mv88e6085 1002b000.ethernet-1:04 eth7 (uninitialized): PHY 
>> [!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdio:06] driver 
>> [Generic PHY] (irq=POLL)
>> mv88e6085 1002b000.ethernet-1:04 eth3 (uninitialized): PHY 
>> [!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdio:07] driver 
>> [Generic PHY] (irq=POLL)
>> mv88e6085 1002b000.ethernet-1:04 eth2 (uninitialized): PHY 
>> [!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdioe:08] driver 
>> [Marvell 88E1112] (irq=174)
>> mv88e6085 1002b000.ethernet-1:04 eth1 (uninitialized): PHY 
>> [!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdioe:09] driver 
>> [Marvell 88E1112] (irq=175)
>> 
>> after this and adding the ifaces to the bridge, it continues like:
>> 
>> br0: port 1(eth10) entered blocking state
>> br0: port 1(eth10) entered disabled state
>> br0: port 2(eth6) entered blocking state
>> br0: port 2(eth6) entered disabled state
>> device eth6 entered promiscuous mode
>> device eth10 entered promiscuous mode
>> br0: port 3(eth9) entered blocking state
>> br0: port 3(eth9) entered disabled state
>> device eth9 entered promiscuous mode
>> br0: port 4(eth5) entered blocking state
>> br0: port 4(eth5) entered disabled state
>> device eth5 entered promiscuous mode
>> br0: port 5(eth8) entered blocking state
>> br0: port 5(eth8) entered disabled state
>> device eth8 entered promiscuous mode
>> br0: port 6(eth4) entered blocking state
>> br0: port 6(eth4) entered disabled state
>> mv88e6085 1002b000.ethernet-1:04: Timeout while waiting for switch
>> mv88e6085 1002b000.ethernet-1:04: port 0 failed to add 
>> 9a:af:03:f1:bd:0a vid 1 to fdb: -110
> 
> Dumb question, but if you get errors like this, how can you test 
> anything at all
> in the patches that you submit?

The answer is that I don't always get these errors... once in a while 
(maaany resets) it does
not happen, and all is fine.

The error code is... well of course -110 (timed out).

> 
>> device eth4 entered promiscuous mode
>> br0: port 7(eth7) entered blocking state
>> br0: port 7(eth7) entered disabled state
>> 
>> I don't know if that gives ay clues...?
> 
> Not really. That error might be related - something indicating a 
> breakage
> in the top-level (fec IIUC) MDIO controller, or not. There was "recent"
> rework almost everywhere.  For example commit 35da1dfd9484 ("net: dsa:
> mv88e6xxx: Improve performance of busy bit polling"). That also hooks
> into the mv88e6xxx cascaded MDIO controller 
> (mv88e6xxx_g2_smi_phy_wait),
> so there might be something there.
> 

I can check that out, but I remember that net-next has not worked on 
this device for quite some
time...

>> 
>> Otherwise I have to take more time to see what I can dig out. The 
>> easiest
>> for me is then to add some printk statements giving targeted 
>> information if told what and
>> where...
> 
> Do you have a timeline for when the regression was introduced?
> Commit 35da1dfd9484 reverts cleanly, so I suppose giving it a go with
> that reverted might be worth a shot. Otherwise, a bisect from a known
> working version only takes a couple of hours, and shouldn't require
> other changes to the setup.

I can't say when the regression was introduced as I used modified 
kernels, but something
between 5.16 and 5.17, I know there was something phy related, but it's 
a bit more complicated,
so it is only a guess...

I would have to get the whole locked port patch set etc. on a 5.16 to 
see if that works.

Re: [PATCH v8 net-next 0/2] mv88e6xxx: Add MAB offload support

[Andrew Lunn]

> [!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdio:00] driver [Generic
> PHY] (irq=POLL)

It is interesting it is using the generic PHY driver, not the Marvell
PHY driver. 

> mv88e6085 1002b000.ethernet-1:04 eth6 (uninitialized): PHY
> [!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdio:01] driver [Generic
> PHY] (irq=POLL)
> mv88e6085 1002b000.ethernet-1:04 eth9 (uninitialized): PHY
> [!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdio:02] driver [Generic
> PHY] (irq=POLL)
> mv88e6085 1002b000.ethernet-1:04 eth5 (uninitialized): PHY
> [!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdio:03] driver [Generic
> PHY] (irq=POLL)
> mv88e6085 1002b000.ethernet-1:04 eth8 (uninitialized): PHY
> [!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdio:04] driver [Generic
> PHY] (irq=POLL)
> mv88e6085 1002b000.ethernet-1:04 eth4 (uninitialized): PHY
> [!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdio:05] driver [Generic
> PHY] (irq=POLL)
> mv88e6085 1002b000.ethernet-1:04 eth7 (uninitialized): PHY
> [!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdio:06] driver [Generic
> PHY] (irq=POLL)
> mv88e6085 1002b000.ethernet-1:04 eth3 (uninitialized): PHY
> [!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdio:07] driver [Generic
> PHY] (irq=POLL)
> mv88e6085 1002b000.ethernet-1:04 eth2 (uninitialized): PHY
> [!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdioe:08] driver
> [Marvell 88E1112] (irq=174)
> mv88e6085 1002b000.ethernet-1:04 eth1 (uninitialized): PHY
> [!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdioe:09] driver
> [Marvell 88E1112] (irq=175)

And here it does use the Marvell PHY driver. Are ports 8 and 9
external, where as the others are internal?

    Andrew

Re: [PATCH v8 net-next 0/2] mv88e6xxx: Add MAB offload support

[netdev]

On 2022-11-15 13:22, Vladimir Oltean wrote:

> Do you have a timeline for when the regression was introduced?
> Commit 35da1dfd9484 reverts cleanly, so I suppose giving it a go with
> that reverted might be worth a shot. Otherwise, a bisect from a known
> working version only takes a couple of hours, and shouldn't require
> other changes to the setup.


Wow! Reverting 35da1dfd9484 and the problem has disappeared. :-)

The bridge_locked_port.sh tests all succeeded... as expected... ;-)

Re: [PATCH v8 net-next 0/2] mv88e6xxx: Add MAB offload support

[netdev]

On 2022-11-15 14:21, Andrew Lunn wrote:
>> [!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdio:00] driver 
>> [Generic
>> PHY] (irq=POLL)
> 
> It is interesting it is using the generic PHY driver, not the Marvell
> PHY driver.
> 
>> mv88e6085 1002b000.ethernet-1:04 eth6 (uninitialized): PHY
>> [!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdio:01] driver 
>> [Generic
>> PHY] (irq=POLL)
>> mv88e6085 1002b000.ethernet-1:04 eth9 (uninitialized): PHY
>> [!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdio:02] driver 
>> [Generic
>> PHY] (irq=POLL)
>> mv88e6085 1002b000.ethernet-1:04 eth5 (uninitialized): PHY
>> [!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdio:03] driver 
>> [Generic
>> PHY] (irq=POLL)
>> mv88e6085 1002b000.ethernet-1:04 eth8 (uninitialized): PHY
>> [!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdio:04] driver 
>> [Generic
>> PHY] (irq=POLL)
>> mv88e6085 1002b000.ethernet-1:04 eth4 (uninitialized): PHY
>> [!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdio:05] driver 
>> [Generic
>> PHY] (irq=POLL)
>> mv88e6085 1002b000.ethernet-1:04 eth7 (uninitialized): PHY
>> [!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdio:06] driver 
>> [Generic
>> PHY] (irq=POLL)
>> mv88e6085 1002b000.ethernet-1:04 eth3 (uninitialized): PHY
>> [!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdio:07] driver 
>> [Generic
>> PHY] (irq=POLL)
>> mv88e6085 1002b000.ethernet-1:04 eth2 (uninitialized): PHY
>> [!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdioe:08] driver
>> [Marvell 88E1112] (irq=174)
>> mv88e6085 1002b000.ethernet-1:04 eth1 (uninitialized): PHY
>> [!soc!aipi@10020000!ethernet@1002b000!mdio!switch@4!mdioe:09] driver
>> [Marvell 88E1112] (irq=175)
> 
> And here it does use the Marvell PHY driver. Are ports 8 and 9
> external, where as the others are internal?
> 
>     Andrew

It is an 8 port switch, so the two (8+9) are for internal use, I guess, 
as I
have not had any part in the system design etc of this device.

I have it for test and development purposes, incl. upstreaming.

Re: [PATCH v8 net-next 0/2] mv88e6xxx: Add MAB offload support

[Vladimir Oltean]

On Tue, Nov 15, 2022 at 02:25:13PM +0100, netdev@kapio-technology.com wrote:
> On 2022-11-15 13:22, Vladimir Oltean wrote:
> > Do you have a timeline for when the regression was introduced?
> > Commit 35da1dfd9484 reverts cleanly, so I suppose giving it a go with
> > that reverted might be worth a shot. Otherwise, a bisect from a known
> > working version only takes a couple of hours, and shouldn't require
> > other changes to the setup.
> 
> Wow! Reverting 35da1dfd9484 and the problem has disappeared. :-)

See? That wasn't very painful, was it.

Now, why doesn't that commit work for you? that's the real question.
I'm going to say there's a big assumption made there. The old code used
to poll up to 16 times with sleeps of up to 2 ms in between.
The new code polls until at least 50 ms have elapsed.
I can imagine the thought process being something like "hmm, 16*2=32ms,
let's round that up to 50 just to be sure". But the effective timeout
was not really increased. Rather said, in the old code there was never
really an effective timeout, since the polling code could have been
preempted many times, and these preemptions would not be accounted
against the msleep(2) calls. Whereas the new code really tracks
something approximating 50 ms now.

Could you please add the reverted patch back to your git tree, and see
by how much do you need to increase the timeout for your system to get
reliable results?

> The bridge_locked_port.sh tests all succeeded... as expected... ;-)

Yeah, I confirm this works on a 6390 over here. But I still don't like
the log spam from the IRQ handlers.

[root@mox:~/.../drivers/net/dsa] # ./bridge_locked_port.sh lan1 lan2 lan3 lan4
[ 1298.218224] mv88e6085 d0032004.mdio-mii:10 lan1: configuring for phy/gmii link mode
[ 1299.150249] mv88e6085 d0032004.mdio-mii:10 lan4: configuring for phy/gmii link mode
[ 1299.791778] br0: port 1(lan2) entered blocking state
[ 1299.800824] br0: port 1(lan2) entered disabled state
[ 1300.419596] br0: port 2(lan3) entered blocking state
[ 1300.425016] br0: port 2(lan3) entered disabled state
[ 1300.527959] device lan3 entered promiscuous mode
[ 1300.538124] device lan2 entered promiscuous mode
[ 1300.733679] mv88e6085 d0032004.mdio-mii:10 lan2: configuring for phy/gmii link mode
[ 1300.765642] 8021q: adding VLAN 0 to HW filter on device lan2
[ 1300.818540] mv88e6085 d0032004.mdio-mii:10 lan3: configuring for phy/gmii link mode
[ 1300.855003] 8021q: adding VLAN 0 to HW filter on device lan3
[ 1303.903840] mv88e6085 d0032004.mdio-mii:10 lan3: Link is Up - 1Gbps/Full - flow control rx/tx
[ 1303.912939] IPv6: ADDRCONF(NETDEV_CHANGE): lan3: link becomes ready
[ 1303.928313] br0: port 2(lan3) entered blocking state
[ 1303.933685] br0: port 2(lan3) entered forwarding state
[ 1303.948607] mv88e6085 d0032004.mdio-mii:10 lan4: Link is Up - 1Gbps/Full - flow control rx/tx
[ 1303.985784] IPv6: ADDRCONF(NETDEV_CHANGE): br0: link becomes ready
[ 1303.999962] IPv6: ADDRCONF(NETDEV_CHANGE): lan4: link becomes ready
[ 1304.003780] mv88e6085 d0032004.mdio-mii:10 lan2: Link is Up - 1Gbps/Full - flow control rx/tx
[ 1304.017407] IPv6: ADDRCONF(NETDEV_CHANGE): lan2: link becomes ready
[ 1304.027922] br0: port 1(lan2) entered blocking state
[ 1304.033157] br0: port 1(lan2) entered forwarding state
[ 1304.043508] mv88e6085 d0032004.mdio-mii:10 lan1: Link is Up - 1Gbps/Full - flow control rx/tx
[ 1304.052601] IPv6: ADDRCONF(NETDEV_CHANGE): lan4.100: link becomes ready
[ 1304.158404] IPv6: ADDRCONF(NETDEV_CHANGE): lan1: link becomes ready
[ 1304.167574] IPv6: ADDRCONF(NETDEV_CHANGE): lan1.100: link becomes ready
TEST: Locked port ipv4                                              [ OK ]
TEST: Locked port ipv6                                              [ OK ]
[ 1337.627010] mv88e6xxx_g1_vtu_prob_irq_thread_fn: 1 callbacks suppressed
[ 1337.627042] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 1337.644822] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 1337.727920] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 1337.862053] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 1337.956972] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 1338.065996] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 1338.136905] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 1338.238182] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 1338.339244] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 1338.440106] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 1342.655520] mv88e6xxx_g1_vtu_prob_irq_thread_fn: 33 callbacks suppressed
[ 1342.655568] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 1342.763619] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 1342.835264] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 1342.847464] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 1342.865387] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 1342.971661] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 1343.075774] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 1343.179562] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 1343.283426] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 1343.387409] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 1348.758296] mv88e6xxx_g1_vtu_prob_irq_thread_fn: 24 callbacks suppressed
[ 1348.758328] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 1348.858879] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 1348.990492] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 1349.063977] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 1349.165979] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 1349.268187] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 1349.373827] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 1349.472601] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 1349.573752] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
[ 1349.585837] mv88e6085 d0032004.mdio-mii:10: VTU member violation for vid 100, source port 9
TEST: Locked port vlan                                              [ OK ]
[ 1352.550194] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:01 portvec 4 spid 2
[ 1352.659486] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:01 portvec 4 spid 2
[ 1352.792941] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:01 portvec 4 spid 2
[ 1352.888959] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:01 portvec 4 spid 2
[ 1352.968150] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:01 portvec 4 spid 2
[ 1353.072434] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:01 portvec 4 spid 2
[ 1353.182844] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:01 portvec 4 spid 2
[ 1353.280595] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:01 portvec 4 spid 2
[ 1353.384755] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:01 portvec 4 spid 2
[ 1353.488602] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:01 portvec 4 spid 2
[ 1358.704139] mv88e6xxx_g1_atu_prob_irq_thread_fn: 39 callbacks suppressed
[ 1358.704172] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:01 portvec 4 spid 2
[ 1359.280930] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:01 portvec 4 spid 2
[ 1359.388609] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:01 portvec 4 spid 2
[ 1359.524500] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:01 portvec 4 spid 2
[ 1359.620272] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:01 portvec 4 spid 2
[ 1359.696597] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:01 portvec 4 spid 2
[ 1359.727872] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:01 portvec 4 spid 2
[ 1359.801563] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:01 portvec 4 spid 2
[ 1359.908665] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:01 portvec 4 spid 2
[ 1360.012063] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:01 portvec 4 spid 2
[ 1363.858627] mv88e6xxx_g1_atu_prob_irq_thread_fn: 29 callbacks suppressed
[ 1363.858674] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:01 portvec 4 spid 2
[ 1364.879407] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:01 portvec 4 spid 2
TEST: Locked port MAB                                               [ OK ]
[ 1369.837089] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for a0:b0:c0:c0:b0:a0 portvec 4 spid 2
[ 1369.971489] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for a0:b0:c0:c0:b0:a0 portvec 4 spid 2
[ 1370.070444] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for a0:b0:c0:c0:b0:a0 portvec 4 spid 2
[ 1370.143784] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for a0:b0:c0:c0:b0:a0 portvec 4 spid 2
[ 1370.245843] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for a0:b0:c0:c0:b0:a0 portvec 4 spid 2
[ 1371.465429] mv88e6085 d0032004.mdio-mii:10: ATU member violation for a0:b0:c0:c0:b0:a0 portvec 8 spid 2
[ 1371.599084] mv88e6085 d0032004.mdio-mii:10: ATU member violation for a0:b0:c0:c0:b0:a0 portvec 8 spid 2
[ 1371.703341] mv88e6085 d0032004.mdio-mii:10: ATU member violation for a0:b0:c0:c0:b0:a0 portvec 8 spid 2
[ 1371.794905] mv88e6085 d0032004.mdio-mii:10: ATU member violation for a0:b0:c0:c0:b0:a0 portvec 8 spid 2
[ 1371.873307] mv88e6085 d0032004.mdio-mii:10: ATU member violation for a0:b0:c0:c0:b0:a0 portvec 8 spid 2
[ 1372.022403] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:01 portvec 4 spid 2
TEST: Locked port MAB roam                                          [ OK ]
TEST: Locked port MAB configuration                                 [ OK ]
[ 1373.039089] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:01 portvec 4 spid 2
[ 1373.060995] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:05 portvec 4 spid 2
[ 1373.167964] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:05 portvec 4 spid 2
[ 1373.296506] mv88e6085 d0032004.mdio-mii:10: ATU miss violation for 00:01:02:03:04:05 portvec 4 spid 2
TEST: Locked port MAB FDB flush                                     [ OK ]
[ 1375.330376] mv88e6085 d0032004.mdio-mii:10 lan3: Link is Down
[ 1375.341372] br0: port 2(lan3) entered disabled state
[ 1375.461136] mv88e6085 d0032004.mdio-mii:10 lan2: Link is Down
[ 1375.489476] br0: port 1(lan2) entered disabled state
[ 1375.611159] device lan3 left promiscuous mode
[ 1375.618253] br0: port 2(lan3) entered disabled state
[ 1375.737909] device lan2 left promiscuous mode
[ 1375.745305] br0: port 1(lan2) entered disabled state

Re: [PATCH v8 net-next 2/2] net: dsa: mv88e6xxx: mac-auth/MAB implementation

[Ido Schimmel]

On Tue, Nov 15, 2022 at 11:36:38AM +0100, netdev@kapio-technology.com wrote:
> On 2022-11-15 10:58, Ido Schimmel wrote:
> > On Sat, Nov 12, 2022 at 09:37:48PM +0100, Hans J. Schultz wrote:
> > > diff --git a/drivers/net/dsa/mv88e6xxx/global1_atu.c
> > > b/drivers/net/dsa/mv88e6xxx/global1_atu.c
> > > index 8a874b6fc8e1..0a57f4e7dd46 100644
> > > --- a/drivers/net/dsa/mv88e6xxx/global1_atu.c
> > > +++ b/drivers/net/dsa/mv88e6xxx/global1_atu.c
> > > @@ -12,6 +12,7 @@
> > > 
> > >  #include "chip.h"
> > >  #include "global1.h"
> > > +#include "switchdev.h"
> > > 
> > >  /* Offset 0x01: ATU FID Register */
> > > 
> > > @@ -426,6 +427,8 @@ static irqreturn_t
> > > mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
> > >  	if (err)
> > >  		goto out;
> > > 
> > > +	mv88e6xxx_reg_unlock(chip);
> > 
> > Why? At minimum such a change needs to be explained in the commit
> > message and probably split to a separate preparatory patch, assuming the
> > change is actually required.
> 
> This was a change done long time ago related to that the violation handle
> function takes the NL lock,
> which could lead to a double-lock deadlock afair if the chip lock is taken
> throughout the handler.

Why do you need to take RTNL lock? br_switchdev_event() which receives
the 'SWITCHDEV_FDB_ADD_TO_BRIDGE' event has this comment:
"/* called with RTNL or RCU */"
And it's using br_port_get_rtnl_rcu(), so looks like RCU is enough.

Re: [PATCH v8 net-next 0/2] mv88e6xxx: Add MAB offload support

[netdev]

On 2022-11-15 15:56, Vladimir Oltean wrote:
> On Tue, Nov 15, 2022 at 02:25:13PM +0100, netdev@kapio-technology.com 
> wrote:
>> On 2022-11-15 13:22, Vladimir Oltean wrote:
> 
>> The bridge_locked_port.sh tests all succeeded... as expected... ;-)
> 
> Yeah, I confirm this works on a 6390 over here.

Thanks :-)

> But I still don't like
> the log spam from the IRQ handlers.
> 
> [root@mox:~/.../drivers/net/dsa] # ./bridge_locked_port.sh lan1 lan2 
> lan3 lan4
> [ 1298.218224] mv88e6085 d0032004.mdio-mii:10 lan1: configuring for
> ...

I think the violation log issue should be handled in a seperate 
patch(set)?

Re: [PATCH v8 net-next 2/2] net: dsa: mv88e6xxx: mac-auth/MAB implementation

[netdev]

On 2022-11-15 16:12, Ido Schimmel wrote:
> On Tue, Nov 15, 2022 at 11:36:38AM +0100, netdev@kapio-technology.com 
> wrote:
>> On 2022-11-15 10:58, Ido Schimmel wrote:
>> > On Sat, Nov 12, 2022 at 09:37:48PM +0100, Hans J. Schultz wrote:
>> > > diff --git a/drivers/net/dsa/mv88e6xxx/global1_atu.c
>> > > b/drivers/net/dsa/mv88e6xxx/global1_atu.c
>> > > index 8a874b6fc8e1..0a57f4e7dd46 100644
>> > > --- a/drivers/net/dsa/mv88e6xxx/global1_atu.c
>> > > +++ b/drivers/net/dsa/mv88e6xxx/global1_atu.c
>> > > @@ -12,6 +12,7 @@
>> > >
>> > >  #include "chip.h"
>> > >  #include "global1.h"
>> > > +#include "switchdev.h"
>> > >
>> > >  /* Offset 0x01: ATU FID Register */
>> > >
>> > > @@ -426,6 +427,8 @@ static irqreturn_t
>> > > mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
>> > >  	if (err)
>> > >  		goto out;
>> > >
>> > > +	mv88e6xxx_reg_unlock(chip);
>> >
>> > Why? At minimum such a change needs to be explained in the commit
>> > message and probably split to a separate preparatory patch, assuming the
>> > change is actually required.
>> 
>> This was a change done long time ago related to that the violation 
>> handle
>> function takes the NL lock,
>> which could lead to a double-lock deadlock afair if the chip lock is 
>> taken
>> throughout the handler.
> 
> Why do you need to take RTNL lock? br_switchdev_event() which receives
> the 'SWITCHDEV_FDB_ADD_TO_BRIDGE' event has this comment:
> "/* called with RTNL or RCU */"
> And it's using br_port_get_rtnl_rcu(), so looks like RCU is enough.

As I understand, dsa_port_to_bridge_port() needs to be called with the 
NL lock taken...

Re: [PATCH v8 net-next 0/2] mv88e6xxx: Add MAB offload support

[netdev]

On 2022-11-15 15:56, Vladimir Oltean wrote:
> On Tue, Nov 15, 2022 at 02:25:13PM +0100, netdev@kapio-technology.com 
> wrote:
>> On 2022-11-15 13:22, Vladimir Oltean wrote:
>> > Do you have a timeline for when the regression was introduced?
>> > Commit 35da1dfd9484 reverts cleanly, so I suppose giving it a go with
>> > that reverted might be worth a shot. Otherwise, a bisect from a known
>> > working version only takes a couple of hours, and shouldn't require
>> > other changes to the setup.
>> 
>> Wow! Reverting 35da1dfd9484 and the problem has disappeared. :-)
> 
> See? That wasn't very painful, was it.

Indeed it was not, when you get a good tip. Thanks alot! :-)

> 
> Now, why doesn't that commit work for you? that's the real question.
> I'm going to say there's a big assumption made there. The old code used
> to poll up to 16 times with sleeps of up to 2 ms in between.
> The new code polls until at least 50 ms have elapsed.
> I can imagine the thought process being something like "hmm, 16*2=32ms,
> let's round that up to 50 just to be sure". But the effective timeout
> was not really increased. Rather said, in the old code there was never
> really an effective timeout, since the polling code could have been
> preempted many times, and these preemptions would not be accounted
> against the msleep(2) calls. Whereas the new code really tracks
> something approximating 50 ms now.
> 
> Could you please add the reverted patch back to your git tree, and see
> by how much do you need to increase the timeout for your system to get
> reliable results?
> 

Yes, so you want me to simply increase the 50ms on line 58 in smi.c...

I have now tried to increase it even to 10000ms == 10s and it didn't 
help,
so something else must be needed...

Re: [PATCH v8 net-next 0/2] mv88e6xxx: Add MAB offload support

[Vladimir Oltean]

On Tue, Nov 15, 2022 at 04:14:05PM +0100, netdev@kapio-technology.com wrote:
> I think the violation log issue should be handled in a seperate patch(set)?

idk, what do you plan to do about it?

Re: [PATCH v8 net-next 0/2] mv88e6xxx: Add MAB offload support

[Vladimir Oltean]

On Tue, Nov 15, 2022 at 05:03:01PM +0100, netdev@kapio-technology.com wrote:
> Yes, so you want me to simply increase the 50ms on line 58 in smi.c...
> 
> I have now tried to increase it even to 10000ms == 10s and it didn't help,
> so something else must be needed...

Not only that, but also the one in mv88e6xxx_wait_mask().

Re: [PATCH v8 net-next 0/2] mv88e6xxx: Add MAB offload support

[netdev]

On 2022-11-15 17:15, Vladimir Oltean wrote:
> On Tue, Nov 15, 2022 at 04:14:05PM +0100, netdev@kapio-technology.com 
> wrote:
>> I think the violation log issue should be handled in a seperate 
>> patch(set)?
> 
> idk, what do you plan to do about it?

When I think about it, I think that the messages should be disabled by 
default
and like one enables NO_LL_LEARN (echo 1 >/sys/class...), they can be 
activated
if one needs it. And of course the ethtool stats will still be there 
anyhow...

How does that sound?

Re: [PATCH v8 net-next 0/2] mv88e6xxx: Add MAB offload support

[Vladimir Oltean]

On Tue, Nov 15, 2022 at 06:11:08PM +0100, netdev@kapio-technology.com wrote:
> On 2022-11-15 17:15, Vladimir Oltean wrote:
> > On Tue, Nov 15, 2022 at 04:14:05PM +0100, netdev@kapio-technology.com
> > wrote:
> > > I think the violation log issue should be handled in a seperate
> > > patch(set)?
> > 
> > idk, what do you plan to do about it?
> 
> When I think about it, I think that the messages should be disabled by default
> and like one enables NO_LL_LEARN (echo 1 >/sys/class...), they can be activated
> if one needs it. And of course the ethtool stats will still be there anyhow...
> 
> How does that sound?

Just make them trace points...
If you don't know how to do that, just search for who has "#define TRACE_SYSTEM"
in drivers/net/ethernet/ and steal from them...

Re: [PATCH v8 net-next 0/2] mv88e6xxx: Add MAB offload support

[netdev]

On 2022-11-15 17:18, Vladimir Oltean wrote:
> On Tue, Nov 15, 2022 at 05:03:01PM +0100, netdev@kapio-technology.com 
> wrote:
>> Yes, so you want me to simply increase the 50ms on line 58 in smi.c...
>> 
>> I have now tried to increase it even to 10000ms == 10s and it didn't 
>> help,
>> so something else must be needed...
> 
> Not only that, but also the one in mv88e6xxx_wait_mask().

So, I will not present you with a graph as it is a tedious process 
(probably
it is some descending gaussian curve wrt timeout occurring).

But 100ms fails, 125 I had 1 port fail, at 140, 150  and 180 I saw 
timeouts
resulting in fdb add fails, like (and occasional port fail):

mv88e6085 1002b000.ethernet-1:04: Timeout while waiting for switch
mv88e6085 1002b000.ethernet-1:04: port 0 failed to add be:7c:96:06:9f:09 
vid 1 to fdb: -110

At around 200 ms it looks like it is getting stable (like 5 runs, no 
problems).

So with the gaussian curve tail whipping ones behind (risque of failure) 
it
might need to be like 300 ms in my case... :-)

Re: [PATCH v8 net-next 2/2] net: dsa: mv88e6xxx: mac-auth/MAB implementation

[Vladimir Oltean]

On Sat, Nov 12, 2022 at 09:37:48PM +0100, Hans J. Schultz wrote:
> This implementation for the Marvell mv88e6xxx chip series, is based on
> handling ATU miss violations occurring when packets ingress on a port
> that is locked with learning on. This will trigger a
> SWITCHDEV_FDB_ADD_TO_BRIDGE event, that wil incurr the bridge module,

"wil incurr" wants to be what?

> to add a locked FDB entry. This bridge FDB entry will not age out as
> it has the extern_learn flag set.
> 
> Userspace daemons can listen to these events and either accept or deny
> access for the host, by either replacing the locked FDB entry with a
> simple entry or leave the locked entry.
> 
> If the host MAC address is already present on another port, a ATU
> member violation will occur, but to no real effect. Statistics on these
> violations can be shown with the command and example output of interest:
> 
> NIC statistics:
> ...
>      atu_member_violation: 36
>      atu_miss_violation: 23
> ...
> 
> Where ethX is the interface of the MAB enabled port.
> 
> Signed-off-by: Hans J. Schultz <netdev@kapio-technology.com>
> ---
>  drivers/net/dsa/mv88e6xxx/Makefile      |  1 +
>  drivers/net/dsa/mv88e6xxx/chip.c        | 19 ++++--
>  drivers/net/dsa/mv88e6xxx/chip.h        | 10 ++++
>  drivers/net/dsa/mv88e6xxx/global1_atu.c | 10 +++-
>  drivers/net/dsa/mv88e6xxx/port.h        |  6 ++
>  drivers/net/dsa/mv88e6xxx/switchdev.c   | 77 +++++++++++++++++++++++++
>  drivers/net/dsa/mv88e6xxx/switchdev.h   | 19 ++++++

How much (and what) do you plan to add to switchdev.{c,h} in the future?
It's a bit arbitrary to put only mv88e6xxx_handle_violation() in a file
called switchdev.c.

port_fdb_add(), port_mdb_add(), port_vlan_add(), port_vlan_filtering(),
etc etc, are all switchdev things. Anyway.

>  7 files changed, 135 insertions(+), 7 deletions(-)
>  create mode 100644 drivers/net/dsa/mv88e6xxx/switchdev.c
>  create mode 100644 drivers/net/dsa/mv88e6xxx/switchdev.h
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/Makefile b/drivers/net/dsa/mv88e6xxx/Makefile
> index c8eca2b6f959..be903a983780 100644
> --- a/drivers/net/dsa/mv88e6xxx/Makefile
> +++ b/drivers/net/dsa/mv88e6xxx/Makefile
> @@ -15,3 +15,4 @@ mv88e6xxx-objs += port_hidden.o
>  mv88e6xxx-$(CONFIG_NET_DSA_MV88E6XXX_PTP) += ptp.o
>  mv88e6xxx-objs += serdes.o
>  mv88e6xxx-objs += smi.o
> +mv88e6xxx-objs += switchdev.o
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index ccfa4751d3b7..efc0085a5616 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -1726,11 +1726,11 @@ static int mv88e6xxx_vtu_get(struct mv88e6xxx_chip *chip, u16 vid,
>  	return err;
>  }
>  
> -static int mv88e6xxx_vtu_walk(struct mv88e6xxx_chip *chip,
> -			      int (*cb)(struct mv88e6xxx_chip *chip,
> -					const struct mv88e6xxx_vtu_entry *entry,
> -					void *priv),
> -			      void *priv)
> +int mv88e6xxx_vtu_walk(struct mv88e6xxx_chip *chip,
> +		       int (*cb)(struct mv88e6xxx_chip *chip,
> +				 const struct mv88e6xxx_vtu_entry *entry,
> +				 void *priv),
> +		       void *priv)
>  {
>  	struct mv88e6xxx_vtu_entry entry = {
>  		.vid = mv88e6xxx_max_vid(chip),
> @@ -6524,7 +6524,7 @@ static int mv88e6xxx_port_pre_bridge_flags(struct dsa_switch *ds, int port,
>  	const struct mv88e6xxx_ops *ops;
>  
>  	if (flags.mask & ~(BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD |
> -			   BR_BCAST_FLOOD | BR_PORT_LOCKED))
> +			   BR_BCAST_FLOOD | BR_PORT_LOCKED | BR_PORT_MAB))
>  		return -EINVAL;
>  
>  	ops = chip->info->ops;
> @@ -6582,12 +6582,19 @@ static int mv88e6xxx_port_bridge_flags(struct dsa_switch *ds, int port,
>  			goto out;
>  	}
>  
> +	if (flags.mask & BR_PORT_MAB) {
> +		chip->ports[port].mab = !!(flags.val & BR_PORT_MAB);
> +		err = 0;

Please make a separate patch which replaces "int err = -EOPNOTSUPP;" as the beginning
of mv88e6xxx_port_bridge_flags() with "err = 0". Please explain that the
-EOPNOTSUPP dates since commit 4f85901f0063 ("net: dsa: mv88e6xxx: add
support for bridge flags") as a return code for the "port_egress_floods()"
DSA method, but the DSA API changed in commit a8b659e7ff75 ("net: dsa:
act as passthrough for bridge port flags"). With the API change,
returning -EOPNOTSUPP from port_bridge_flags() is ignored; what's
important is to return -EINVAL for unsupported operations from the
port_pre_bridge_flags() method. Since the mv88e6xxx driver does that,
there will never appear a situation where it must reject making a change
to an unsupported flag in port_bridge_flags().

After you make that change, please create a superficial wrapper function
called "mv88e6xxx_port_set_mab()", similar to the rest of this function's
structure.

> +	}
> +
>  	if (flags.mask & BR_PORT_LOCKED) {
>  		bool locked = !!(flags.val & BR_PORT_LOCKED);
>  
>  		err = mv88e6xxx_port_set_lock(chip, port, locked);
>  		if (err)
>  			goto out;
> +
> +		chip->ports[port].locked = locked;

The driver is not using this flag.

>  	}
>  out:
>  	mv88e6xxx_reg_unlock(chip);
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
> index e693154cf803..3b951cd0e6f8 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.h
> +++ b/drivers/net/dsa/mv88e6xxx/chip.h
> @@ -280,6 +280,10 @@ struct mv88e6xxx_port {
>  	unsigned int serdes_irq;
>  	char serdes_irq_name[64];
>  	struct devlink_region *region;
> +
> +	/* Locked port and MacAuth control flags */

Can you please be consistent and call MAB MAC Authentication Bypass?
I mean, "bypass" is the most important part of what goes on, and you
just omit it.

> +	bool locked;
> +	bool mab;
>  };
>  
>  enum mv88e6xxx_region_id {
> @@ -802,6 +806,12 @@ static inline void mv88e6xxx_reg_unlock(struct mv88e6xxx_chip *chip)
>  	mutex_unlock(&chip->reg_lock);
>  }
>  
> +int mv88e6xxx_vtu_walk(struct mv88e6xxx_chip *chip,
> +		       int (*cb)(struct mv88e6xxx_chip *chip,
> +				 const struct mv88e6xxx_vtu_entry *entry,
> +				 void *priv),
> +		       void *priv);
> +
>  int mv88e6xxx_fid_map(struct mv88e6xxx_chip *chip, unsigned long *bitmap);
>  
>  #endif /* _MV88E6XXX_CHIP_H */
> diff --git a/drivers/net/dsa/mv88e6xxx/global1_atu.c b/drivers/net/dsa/mv88e6xxx/global1_atu.c
> index 8a874b6fc8e1..0a57f4e7dd46 100644
> --- a/drivers/net/dsa/mv88e6xxx/global1_atu.c
> +++ b/drivers/net/dsa/mv88e6xxx/global1_atu.c
> @@ -12,6 +12,7 @@
>  
>  #include "chip.h"
>  #include "global1.h"
> +#include "switchdev.h"
>  
>  /* Offset 0x01: ATU FID Register */
>  
> @@ -426,6 +427,8 @@ static irqreturn_t mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
>  	if (err)
>  		goto out;
>  
> +	mv88e6xxx_reg_unlock(chip);
> +

I concur with Ido's suggestion to split up changes which are only
tangentially related as preparatory patches, with the motivation which
you explained over email as the commit message. Also, the current "out"
label needs to become something like "out_unlock", and a new "out"
created, for the error path jumps below, that don't have the register
lock held.

>  	spid = entry.state;
>  
>  	if (val & MV88E6XXX_G1_ATU_OP_AGE_OUT_VIOLATION) {
> @@ -446,6 +449,12 @@ static irqreturn_t mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
>  				    "ATU miss violation for %pM portvec %x spid %d\n",
>  				    entry.mac, entry.portvec, spid);
>  		chip->ports[spid].atu_miss_violation++;
> +
> +		if (fid && chip->ports[spid].mab)
> +			err = mv88e6xxx_handle_violation(chip, spid, &entry,
> +							 fid, MV88E6XXX_G1_ATU_OP_MISS_VIOLATION);

The check for non-zero FID looks strange until one considers that FID 0
is MV88E6XXX_FID_STANDALONE. But then again, since only standalone ports
use FID 0 and standalone ports cannot have the MAB/locked feature enabled,
I consider the check to be redundant. We should know for sure that the
FID is non-zero.

> +		if (err)
> +			goto out;

Did the "if (err)" test belong to the same code block as the
mv88e6xxx_handle_violation() call above it?

>  	}
>  
>  	if (val & MV88E6XXX_G1_ATU_OP_FULL_VIOLATION) {
> @@ -454,7 +463,6 @@ static irqreturn_t mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
>  				    entry.mac, entry.portvec, spid);
>  		chip->ports[spid].atu_full_violation++;
>  	}
> -	mv88e6xxx_reg_unlock(chip);
>  
>  	return IRQ_HANDLED;
>  
> diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h
> index aec9d4fd20e3..bd90a02865a0 100644
> --- a/drivers/net/dsa/mv88e6xxx/port.h
> +++ b/drivers/net/dsa/mv88e6xxx/port.h
> @@ -377,6 +377,12 @@ int mv88e6xxx_port_set_pvid(struct mv88e6xxx_chip *chip, int port, u16 pvid);
>  int mv88e6xxx_port_set_lock(struct mv88e6xxx_chip *chip, int port,
>  			    bool locked);
>  
> +static inline bool mv88e6xxx_port_is_locked(struct mv88e6xxx_chip *chip,
> +					    int port)

As commented above, it looks like you can delete this and the "locked"
field in the port structure.

> +{
> +	return chip->ports[port].locked;
> +}
> +
>  int mv88e6xxx_port_set_8021q_mode(struct mv88e6xxx_chip *chip, int port,
>  				  u16 mode);
>  int mv88e6095_port_tag_remap(struct mv88e6xxx_chip *chip, int port);
> diff --git a/drivers/net/dsa/mv88e6xxx/switchdev.c b/drivers/net/dsa/mv88e6xxx/switchdev.c
> new file mode 100644
> index 000000000000..000778550532
> --- /dev/null
> +++ b/drivers/net/dsa/mv88e6xxx/switchdev.c
> @@ -0,0 +1,77 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * switchdev.c
> + *
> + *	Authors:
> + *	Hans J. Schultz		<hans.schultz@westermo.com>
> + *
> + */
> +
> +#include <net/switchdev.h>
> +#include "chip.h"
> +#include "global1.h"
> +
> +struct mv88e6xxx_fid_search_ctx {
> +	u16 fid_search;
> +	u16 vid_found;
> +};
> +
> +static int mv88e6xxx_find_vid(struct mv88e6xxx_chip *chip,
> +			      const struct mv88e6xxx_vtu_entry *entry,
> +			      void *priv)
> +{
> +	struct mv88e6xxx_fid_search_ctx *ctx = priv;
> +
> +	if (ctx->fid_search == entry->fid) {
> +		ctx->vid_found = entry->vid;
> +		return 1;
> +	}
> +
> +	return 0;
> +}
> +
> +int mv88e6xxx_handle_violation(struct mv88e6xxx_chip *chip, int port,
> +			       struct mv88e6xxx_atu_entry *entry,
> +			       u16 fid, u16 type)
> +{
> +	struct switchdev_notifier_fdb_info info = {
> +		.addr = entry->mac,
> +		.locked = true,
> +	};
> +	struct mv88e6xxx_fid_search_ctx ctx;
> +	struct net_device *brport;
> +	struct dsa_port *dp;
> +	int err;
> +
> +	if (mv88e6xxx_is_invalid_port(chip, port))

Can it ever happen that the switch reports a violation for an invalid
port, or is it absurdly defensive programming? According to struct
mv88e6xxx_info, invalid_port_mask "is required for example for the
MV88E6220 (which is in general a MV88E6250 with 7 ports) but the ports
2-4 are not routet [sic] to pins." So my understanding is that the
driver does not probe at all if invalid ports are being declared in the
device tree.

> +		return -ENODEV;
> +
> +	ctx.fid_search = fid;
> +	mv88e6xxx_reg_lock(chip);
> +	err = mv88e6xxx_vtu_walk(chip, mv88e6xxx_find_vid, &ctx);
> +	mv88e6xxx_reg_unlock(chip);
> +	if (err < 0)
> +		return err;
> +	if (err == 1)
> +		info.vid = ctx.vid_found;
> +	else
> +		return -ENODATA;

ENOENT maybe?

> +
> +	switch (type) {
> +	case MV88E6XXX_G1_ATU_OP_MISS_VIOLATION:

Is it beneficial in any way to pass the violation type to
mv88e6xxx_handle_violation(), considering that we only call it from the
"miss" code path, and if we were to call it with something else ("member"),
it would return a strange error code (1)?

I don't necessarily see any way in which we'll need to handle the
"member" (migration, right?) violation any different in the future,
except ignore it, either.

> +		dp = dsa_to_port(chip->ds, port);
> +
> +		rtnl_lock();
> +		brport = dsa_port_to_bridge_port(dp);
> +		if (!brport) {
> +			rtnl_unlock();
> +			return -ENODEV;
> +		}
> +		err = call_switchdev_notifiers(SWITCHDEV_FDB_ADD_TO_BRIDGE,
> +					       brport, &info.info, NULL);
> +		rtnl_unlock();
> +		break;
> +	}
> +
> +	return err;
> +}
> diff --git a/drivers/net/dsa/mv88e6xxx/switchdev.h b/drivers/net/dsa/mv88e6xxx/switchdev.h
> new file mode 100644
> index 000000000000..ccc10a9d4072
> --- /dev/null
> +++ b/drivers/net/dsa/mv88e6xxx/switchdev.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * switchdev.h
> + *
> + *	Authors:
> + *	Hans J. Schultz		<hans.schultz@westermo.com>
> + *
> + */
> +
> +#ifndef DRIVERS_NET_DSA_MV88E6XXX_SWITCHDEV_H_
> +#define DRIVERS_NET_DSA_MV88E6XXX_SWITCHDEV_H_

If it's going to turn out that keeping switchdev.{c,h} is worth it,
can you please at least integrate with the coding conventions in the
rest of the driver?

The other header pragmas in the mv88e6xxx are:

#ifndef _MV88E6XXX_CHIP_H
#define _MV88E6XXX_CHIP_H

#ifndef _MV88E6XXX_DEVLINK_H
#define _MV88E6XXX_DEVLINK_H

#ifndef _MV88E6XXX_GLOBAL1_H
#define _MV88E6XXX_GLOBAL1_H

#ifndef _MV88E6XXX_GLOBAL2_H
#define _MV88E6XXX_GLOBAL2_H

#ifndef _MV88E6XXX_HWTSTAMP_H
#define _MV88E6XXX_HWTSTAMP_H

#ifndef _MV88E6XXX_PHY_H
#define _MV88E6XXX_PHY_H

#ifndef _MV88E6XXX_PORT_H
#define _MV88E6XXX_PORT_H

#ifndef _MV88E6XXX_PTP_H
#define _MV88E6XXX_PTP_H

#ifndef _MV88E6XXX_SERDES_H
#define _MV88E6XXX_SERDES_H

#ifndef _MV88E6XXX_SMI_H
#define _MV88E6XXX_SMI_H

Notice a pattern?

> +
> +#include "chip.h"
> +
> +int mv88e6xxx_handle_violation(struct mv88e6xxx_chip *chip, int port,
> +			       struct mv88e6xxx_atu_entry *entry,
> +			       u16 fid, u16 type);
> +
> +#endif /* DRIVERS_NET_DSA_MV88E6XXX_SWITCHDEV_H_ */
> -- 
> 2.34.1
> 

Re: [PATCH v8 net-next 0/2] mv88e6xxx: Add MAB offload support

[Vladimir Oltean]

On Tue, Nov 15, 2022 at 07:40:02PM +0100, netdev@kapio-technology.com wrote:
> So, I will not present you with a graph as it is a tedious process (probably
> it is some descending gaussian curve wrt timeout occurring).
> 
> But 100ms fails, 125 I had 1 port fail, at 140, 150  and 180 I saw timeouts
> resulting in fdb add fails, like (and occasional port fail):
> 
> mv88e6085 1002b000.ethernet-1:04: Timeout while waiting for switch
> mv88e6085 1002b000.ethernet-1:04: port 0 failed to add be:7c:96:06:9f:09 vid
> 1 to fdb: -110
> 
> At around 200 ms it looks like it is getting stable (like 5 runs, no
> problems).
> 
> So with the gaussian curve tail whipping ones behind (risque of failure) it
> might need to be like 300 ms in my case... :-)

Pick a value that is high enough to be reliable and submit a patch to
"net" where you present the evidence for it (top-level MDIO controller,
SoC, switch, kernel). I don't believe there's much to read into. A large
timeout shouldn't have a negative effect on the MDIO performance,
because it just determines how long it takes until the kernel declares
it dead, rather than how long it takes for transactions to actually take
place.

Re: [PATCH v8 net-next 0/2] mv88e6xxx: Add MAB offload support

[Andrew Lunn]

> Pick a value that is high enough to be reliable and submit a patch to
> "net" where you present the evidence for it (top-level MDIO controller,
> SoC, switch, kernel). I don't believe there's much to read into. A large
> timeout shouldn't have a negative effect on the MDIO performance,
> because it just determines how long it takes until the kernel declares
> it dead, rather than how long it takes for transactions to actually take
> place.

Yes, please do that.

It is interesting that you found this. I'm just curious, so no need to
investigate if you don't have time. Is there a pattern, is it the same
register which always times out?

	 Andrew

Re: [PATCH v8 net-next 0/2] mv88e6xxx: Add MAB offload support

[netdev]

On 2022-11-16 11:24, Vladimir Oltean wrote:
> On Tue, Nov 15, 2022 at 07:40:02PM +0100, netdev@kapio-technology.com 
> wrote:
>> So, I will not present you with a graph as it is a tedious process 
>> (probably
>> it is some descending gaussian curve wrt timeout occurring).
>> 
>> But 100ms fails, 125 I had 1 port fail, at 140, 150  and 180 I saw 
>> timeouts
>> resulting in fdb add fails, like (and occasional port fail):
>> 
>> mv88e6085 1002b000.ethernet-1:04: Timeout while waiting for switch
>> mv88e6085 1002b000.ethernet-1:04: port 0 failed to add 
>> be:7c:96:06:9f:09 vid
>> 1 to fdb: -110
>> 
>> At around 200 ms it looks like it is getting stable (like 5 runs, no
>> problems).
>> 
>> So with the gaussian curve tail whipping ones behind (risque of 
>> failure) it
>> might need to be like 300 ms in my case... :-)
> 
> Pick a value that is high enough to be reliable and submit a patch to
> "net" where you present the evidence for it (top-level MDIO controller,
> SoC, switch, kernel). I don't believe there's much to read into. A 
> large
> timeout shouldn't have a negative effect on the MDIO performance,
> because it just determines how long it takes until the kernel declares
> it dead, rather than how long it takes for transactions to actually 
> take
> place.

Would it not be appropriate to have a define that specifies the value 
instead
of the same value two places as it is now?

And in so case, what would be an appropriate name?

Re: [PATCH v8 net-next 0/2] mv88e6xxx: Add MAB offload support

[Andrew Lunn]

> Would it not be appropriate to have a define that specifies the
> value instead of the same value two places as it is now?

Yes.

> And in so case, what would be an appropriate name?

MV88E6XXX_WAIT_TIMEOUT_MS ?

	  Andrew

Re: [PATCH v8 net-next 2/2] net: dsa: mv88e6xxx: mac-auth/MAB implementation

[netdev]

On 2022-11-15 23:23, Vladimir Oltean wrote:
> 
> How much (and what) do you plan to add to switchdev.{c,h} in the 
> future?
> It's a bit arbitrary to put only mv88e6xxx_handle_violation() in a file
> called switchdev.c.
> 
> port_fdb_add(), port_mdb_add(), port_vlan_add(), port_vlan_filtering(),
> etc etc, are all switchdev things. Anyway.
> 

Firstly, those functions you list are ops functions, while what is in
switchdev.{c,h} is not, secondly the functions in switchdev.{c,h} are 
the
first to send switchdev messages like SWITCHDEV_FDB_ADD_TO_BRIDGE 
events.

Furthermore I think that chip.c is bloated, but I also do plan to add 
more
to switchdev.{c,h}, and there can be others adding in the future...

Re: [PATCH v8 net-next 2/2] net: dsa: mv88e6xxx: mac-auth/MAB implementation

[netdev]

On 2022-11-15 23:23, Vladimir Oltean wrote:
> On Sat, Nov 12, 2022 at 09:37:48PM +0100, Hans J. Schultz wrote:
>> diff --git a/drivers/net/dsa/mv88e6xxx/chip.h 
>> b/drivers/net/dsa/mv88e6xxx/chip.h
>> index e693154cf803..3b951cd0e6f8 100644
>> --- a/drivers/net/dsa/mv88e6xxx/chip.h
>> +++ b/drivers/net/dsa/mv88e6xxx/chip.h
>> @@ -280,6 +280,10 @@ struct mv88e6xxx_port {
>>  	unsigned int serdes_irq;
>>  	char serdes_irq_name[64];
>>  	struct devlink_region *region;
>> +
>> +	/* Locked port and MacAuth control flags */
> 
> Can you please be consistent and call MAB MAC Authentication Bypass?
> I mean, "bypass" is the most important part of what goes on, and you
> just omit it.
> 

I must admit that I consider 'MacAuth' and 'Mac Authentication Bypass' 
to be
completely equivalent terms, where the MAB terminology is what is coined 
by
Cisco. Afaik, there is no difference in the core functionality between 
the two.

I do see that Cisco has a more extended concept, when you consider 
non-core
functionality, as how the whole authorization decision process and the
infrastructure that is involved works, and thus is very Cisco centered, 
as I
have had in my cover letter:

"This feature is known as MAC-Auth or MAC Authentication Bypass
(MAB) in Cisco terminology, where the full MAB concept involves
additional Cisco infrastructure for authorization."

I would have preferred the MacAuth terminology as I see it as more 
generic
and open, but 'mab' is short as a flag name... :D

Re: [PATCH v8 net-next 2/2] net: dsa: mv88e6xxx: mac-auth/MAB implementation

[netdev]

On 2022-11-15 23:23, Vladimir Oltean wrote:
>> diff --git a/drivers/net/dsa/mv88e6xxx/global1_atu.c 
>> b/drivers/net/dsa/mv88e6xxx/global1_atu.c
>> index 8a874b6fc8e1..0a57f4e7dd46 100644
>> --- a/drivers/net/dsa/mv88e6xxx/global1_atu.c
>> +++ b/drivers/net/dsa/mv88e6xxx/global1_atu.c
>> @@ -12,6 +12,7 @@
>> 
>>  #include "chip.h"
>>  #include "global1.h"
>> +#include "switchdev.h"
>> 
>>  /* Offset 0x01: ATU FID Register */
>> 
>> @@ -426,6 +427,8 @@ static irqreturn_t 
>> mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
>>  	if (err)
>>  		goto out;
>> 
>> +	mv88e6xxx_reg_unlock(chip);
>> +
> 
> I concur with Ido's suggestion to split up changes which are only
> tangentially related as preparatory patches, with the motivation which
> you explained over email as the commit message. Also, the current "out"
> label needs to become something like "out_unlock", and a new "out"
> created, for the error path jumps below, that don't have the register
> lock held.
> 
>>  	spid = entry.state;
>> 
>>  	if (val & MV88E6XXX_G1_ATU_OP_AGE_OUT_VIOLATION) {
>> @@ -446,6 +449,12 @@ static irqreturn_t 
>> mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
>>  				    "ATU miss violation for %pM portvec %x spid %d\n",
>>  				    entry.mac, entry.portvec, spid);
>>  		chip->ports[spid].atu_miss_violation++;
>> +
>> +		if (fid && chip->ports[spid].mab)
>> +			err = mv88e6xxx_handle_violation(chip, spid, &entry,
>> +							 fid, MV88E6XXX_G1_ATU_OP_MISS_VIOLATION);
> 
> The check for non-zero FID looks strange until one considers that FID 0
> is MV88E6XXX_FID_STANDALONE. But then again, since only standalone 
> ports
> use FID 0 and standalone ports cannot have the MAB/locked feature 
> enabled,
> I consider the check to be redundant. We should know for sure that the
> FID is non-zero.
> 

I have something like this, using 'mvls vtu' from 
https://github.com/wkz/mdio-tools:
  VID   FID  SID  P  Q  F  0  1  2  3  4  5  6  7  8  9  a
    0     0    0  y  -  -  =  =  =  =  =  =  =  =  =  =  =
    1     2    0  -  -  -  u  u  u  u  u  u  u  u  u  u  =
4095     1    0  -  -  -  =  =  =  =  =  =  =  =  =  =  =

as a vtu table. I don't remember exactly the consequences, but I am 
quite sure that fid=0 gave
incorrect handling, but there might be something that I have missed as 
to other setups.

Re: [PATCH v8 net-next 2/2] net: dsa: mv88e6xxx: mac-auth/MAB implementation

[Vladimir Oltean]

On Sun, Nov 20, 2022 at 11:21:08AM +0100, netdev@kapio-technology.com wrote:
> I have something like this, using 'mvls vtu' from
> https://github.com/wkz/mdio-tools:
>  VID   FID  SID  P  Q  F  0  1  2  3  4  5  6  7  8  9  a
>    0     0    0  y  -  -  =  =  =  =  =  =  =  =  =  =  =
>    1     2    0  -  -  -  u  u  u  u  u  u  u  u  u  u  =
> 4095     1    0  -  -  -  =  =  =  =  =  =  =  =  =  =  =
> 
> as a vtu table. I don't remember exactly the consequences, but I am quite
> sure that fid=0 gave
> incorrect handling, but there might be something that I have missed as to
> other setups.

Can you please find out? There needs to be an answer as to why something
which shouldn't happen happens.

Re: [PATCH v8 net-next 2/2] net: dsa: mv88e6xxx: mac-auth/MAB implementation

[netdev]

On 2022-11-20 16:00, Vladimir Oltean wrote:
> On Sun, Nov 20, 2022 at 11:21:08AM +0100, netdev@kapio-technology.com 
> wrote:
>> I have something like this, using 'mvls vtu' from
>> https://github.com/wkz/mdio-tools:
>>  VID   FID  SID  P  Q  F  0  1  2  3  4  5  6  7  8  9  a
>>    0     0    0  y  -  -  =  =  =  =  =  =  =  =  =  =  =
>>    1     2    0  -  -  -  u  u  u  u  u  u  u  u  u  u  =
>> 4095     1    0  -  -  -  =  =  =  =  =  =  =  =  =  =  =
>> 
>> as a vtu table. I don't remember exactly the consequences, but I am 
>> quite
>> sure that fid=0 gave
>> incorrect handling, but there might be something that I have missed as 
>> to
>> other setups.
> 
> Can you please find out? There needs to be an answer as to why 
> something
> which shouldn't happen happens.

Hi Vladimir,
I haven't been able to reproduce the situation with fid=0, and it may be 
superfluous to check if fid has a non-zero value as the case of fid=0 in 
the miss violation handling is not valid on a bridged port, where I 
understand from consultation that the case fid=0 corresponds to a 
non-bridged port.

What I experienced then might have been from some previous bug at a 
time, but I don't know.

Should I remove the check or not?

Re: [PATCH v8 net-next 2/2] net: dsa: mv88e6xxx: mac-auth/MAB implementation

[netdev]

On 2022-11-15 23:23, Vladimir Oltean wrote:
> 
> Is it beneficial in any way to pass the violation type to
> mv88e6xxx_handle_violation(), considering that we only call it from the
> "miss" code path, and if we were to call it with something else 
> ("member"),
> it would return a strange error code (1)?
> 
> I don't necessarily see any way in which we'll need to handle the
> "member" (migration, right?) violation any different in the future,
> except ignore it, either.
> 

MV88E6XXX_G1_ATU_OP_AGE_OUT_VIOLATION will also be handled, and it could 
be
that MV88E6XXX_G1_ATU_OP_FULL_VIOLATION would want handling, though I 
don't
know of plans for that.

The MV88E6XXX_G1_ATU_OP_MEMBER_VIOLATION interrupt can be suppressed if 
we
want.

I think a switch on the type is the most readable code form.


p.s. I have changed it, so that global1_atu.c reads:

         if (val & MV88E6XXX_G1_ATU_OP_MISS_VIOLATION) {
                 dev_err_ratelimited(chip->dev,
                                     "ATU miss violation for %pM portvec 
%x spid %d\n",
                                     entry.mac, entry.portvec, spid);
                 chip->ports[spid].atu_miss_violation++;

                 if (!fid) {
                         err = -EINVAL;
                         goto out;
                 }

                 if (chip->ports[spid].mab)
                         err = mv88e6xxx_handle_violation(chip, spid, 
&entry,
                                                          fid, 
MV88E6XXX_G1_ATU_OP_MISS_VIOLATION);
                 if (err)
                         goto out;
         }

with the use of out_unlock in the chip mutex locked case.

Re: [PATCH v8 net-next 2/2] net: dsa: mv88e6xxx: mac-auth/MAB implementation

[netdev]

On 2022-11-20 16:00, Vladimir Oltean wrote:
> On Sun, Nov 20, 2022 at 11:21:08AM +0100, netdev@kapio-technology.com 
> wrote:
>> I have something like this, using 'mvls vtu' from
>> https://github.com/wkz/mdio-tools:
>>  VID   FID  SID  P  Q  F  0  1  2  3  4  5  6  7  8  9  a
>>    0     0    0  y  -  -  =  =  =  =  =  =  =  =  =  =  =
>>    1     2    0  -  -  -  u  u  u  u  u  u  u  u  u  u  =
>> 4095     1    0  -  -  -  =  =  =  =  =  =  =  =  =  =  =
>> 
>> as a vtu table. I don't remember exactly the consequences, but I am 
>> quite
>> sure that fid=0 gave
>> incorrect handling, but there might be something that I have missed as 
>> to
>> other setups.
> 
> Can you please find out? There needs to be an answer as to why 
> something
> which shouldn't happen happens.

Just an update on this, as when running the selftests now, I have 
experienced
the case where fid=0 in the interrupt handler. The reported mac is the 
same
as the one where the handling was successful in the selftest.

So I don't know what causes this fid=0 event, maybe some timing with the 
chip
op when stressed resulting in erroneous reading of fid?