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 <[email protected]>
---
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 <[email protected]>
+ *
+ */
+
+#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 <[email protected]>
+ *
+ */
+
+#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
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;
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.
On Tue, Nov 15, 2022 at 11:36:38AM +0100, [email protected] 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.
On 2022-11-15 16:12, Ido Schimmel wrote:
> On Tue, Nov 15, 2022 at 11:36:38AM +0100, [email protected]
> 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...
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 <[email protected]>
> ---
> 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 <[email protected]>
> + *
> + */
> +
> +#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 <[email protected]>
> + *
> + */
> +
> +#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
>
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...
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
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.
On Sun, Nov 20, 2022 at 11:21:08AM +0100, [email protected] 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.
On 2022-11-20 16:00, Vladimir Oltean wrote:
> On Sun, Nov 20, 2022 at 11:21:08AM +0100, [email protected]
> 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?
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.
On 2022-11-20 16:00, Vladimir Oltean wrote:
> On Sun, Nov 20, 2022 at 11:21:08AM +0100, [email protected]
> 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?