| Message ID | 20221205185908.217520-4-netdev@kapio-technology.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | mv88e6xxx: Add MAB offload support | expand |
On Mon, Dec 05, 2022 at 07:59:08PM +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, which will result in the bridge module > adding 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. And the packet will be dropped in hardware, right? > Statistics on these violations can be shown with the command and > example output of interest: > > ethtool -S ethX > NIC statistics: > ... > atu_member_violation: 5 > atu_miss_violation: 23 > ... > > Where ethX is the interface of the MAB enabled port. > > An anomaly has been observed, where the ATU op to read the FID reports > FID=0 even though it is not a valid read. An -EINVAL error will be logged > in this case. This was seen on a mv88e6097. > > Signed-off-by: Hans J. Schultz <netdev@kapio-technology.com> > --- The changelog from previous versions is missing. > drivers/net/dsa/mv88e6xxx/Makefile | 1 + > drivers/net/dsa/mv88e6xxx/chip.c | 18 ++++-- > drivers/net/dsa/mv88e6xxx/chip.h | 15 +++++ > drivers/net/dsa/mv88e6xxx/global1_atu.c | 29 ++++++--- > drivers/net/dsa/mv88e6xxx/switchdev.c | 83 +++++++++++++++++++++++++ > drivers/net/dsa/mv88e6xxx/switchdev.h | 19 ++++++ > 6 files changed, 152 insertions(+), 13 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 66d7eae24ce0..732d7a2e2a07 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,6 +6582,12 @@ static int mv88e6xxx_port_bridge_flags(struct dsa_switch *ds, int port, > goto out; > } > > + if (flags.mask & BR_PORT_MAB) { > + bool mab = !!(flags.val & BR_PORT_MAB); > + > + mv88e6xxx_port_set_mab(chip, port, mab); > + } > + > if (flags.mask & BR_PORT_LOCKED) { > bool locked = !!(flags.val & BR_PORT_LOCKED); > > diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h > index e693154cf803..f635a5bb47ce 100644 > --- a/drivers/net/dsa/mv88e6xxx/chip.h > +++ b/drivers/net/dsa/mv88e6xxx/chip.h > @@ -280,6 +280,9 @@ struct mv88e6xxx_port { > unsigned int serdes_irq; > char serdes_irq_name[64]; > struct devlink_region *region; > + > + /* MacAuth Bypass control flag */ > + bool mab; > }; > > enum mv88e6xxx_region_id { > @@ -784,6 +787,12 @@ static inline bool mv88e6xxx_is_invalid_port(struct mv88e6xxx_chip *chip, int po > return (chip->info->invalid_port_mask & BIT(port)) != 0; > } > > +static inline void mv88e6xxx_port_set_mab(struct mv88e6xxx_chip *chip, > + int port, bool mab) > +{ > + chip->ports[port].mab = mab; > +} > + > int mv88e6xxx_read(struct mv88e6xxx_chip *chip, int addr, int reg, u16 *val); > int mv88e6xxx_write(struct mv88e6xxx_chip *chip, int addr, int reg, u16 val); > int mv88e6xxx_wait_mask(struct mv88e6xxx_chip *chip, int addr, int reg, > @@ -802,6 +811,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..bb004df517b2 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 */ > > @@ -408,23 +409,25 @@ static irqreturn_t mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id) > > err = mv88e6xxx_g1_read_atu_violation(chip); > if (err) > - goto out; > + goto out_unlock; > > err = mv88e6xxx_g1_read(chip, MV88E6XXX_G1_ATU_OP, &val); > if (err) > - goto out; > + goto out_unlock; > > err = mv88e6xxx_g1_atu_fid_read(chip, &fid); > if (err) > - goto out; > + goto out_unlock; > > err = mv88e6xxx_g1_atu_data_read(chip, &entry); > if (err) > - goto out; > + goto out_unlock; > > err = mv88e6xxx_g1_atu_mac_read(chip, &entry); > if (err) > - goto out; > + goto out_unlock; > + > + mv88e6xxx_reg_unlock(chip); I was under the impression that we agreed that the locking change will be split to a separate patch. > > spid = entry.state;
On 2022-12-06 13:53, Ido Schimmel wrote: > On Mon, Dec 05, 2022 at 07:59:08PM +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, which will result in the bridge >> module >> adding 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. > > And the packet will be dropped in hardware, right? Every packet that enters a locked port and does not have a matching ATU entry on the port will be dropped (in HW) afaik. >> --- > > The changelog from previous versions is missing. > I am afraid because I made a mistake with the version string last, this should be regarded as a first. Therefore no changelog. >> err = mv88e6xxx_g1_atu_mac_read(chip, &entry); >> if (err) >> - goto out; >> + goto out_unlock; >> + >> + mv88e6xxx_reg_unlock(chip); > > I was under the impression that we agreed that the locking change will > be split to a separate patch. > Sorry, I guess that because of the quite long time that has passed as I needed to get this FID=0 issue sorted out, and had many other different changes to attend, I forgot. I see an updated version is needed anyhow, so I will do it there. >> >> spid = entry.state;
On Tue, Dec 06, 2022 at 05:36:42PM +0100, netdev@kapio-technology.com wrote: > > I was under the impression that we agreed that the locking change will > > be split to a separate patch. > > Sorry, I guess that because of the quite long time that has passed as I > needed to get this FID=0 issue sorted out, and had many other different > changes to attend, I forgot. Well, at least you got the FID=0 issue sorted out... right? What was the cause, what is the solution?
On 2022-12-07 21:29, Vladimir Oltean wrote: > On Tue, Dec 06, 2022 at 05:36:42PM +0100, netdev@kapio-technology.com > wrote: >> > I was under the impression that we agreed that the locking change will >> > be split to a separate patch. >> >> Sorry, I guess that because of the quite long time that has passed as >> I >> needed to get this FID=0 issue sorted out, and had many other >> different >> changes to attend, I forgot. > > Well, at least you got the FID=0 issue sorted out... right? > What was the cause, what is the solution? Well I got it sorted out in the way that I have identified that it is the ATU op that fails some times. I don't think there is anything that can be done about that, other than what I do and let the interrupt routing return an error.
On Thu, Dec 08, 2022 at 01:28:27PM +0100, netdev@kapio-technology.com wrote: > On 2022-12-07 21:29, Vladimir Oltean wrote: > > On Tue, Dec 06, 2022 at 05:36:42PM +0100, netdev@kapio-technology.com wrote: > > > > I was under the impression that we agreed that the locking change will > > > > be split to a separate patch. > > > > > > Sorry, I guess that because of the quite long time that has passed as I > > > needed to get this FID=0 issue sorted out, and had many other different > > > changes to attend, I forgot. > > > > Well, at least you got the FID=0 issue sorted out... right? > > What was the cause, what is the solution? > > Well I got it sorted out in the way that I have identified that it is the > ATU op that fails some times. I don't think there is anything that can be > done about that, other than what I do and let the interrupt routing return > an error. Yikes. But why would you call that "sorted out", though? Just to make it appear as though you really spent some time on it, and use it as an excuse for something else? > it is the ATU op that fails some times. Let's start with the assumption that this is correct. A person with critical thinking will ask "can I prove that it is?". If the ATU operation fails sometimes, I would expect that it always fails in the same way, by returning FID 0, where 0 is some kind of "invalid" value. But if FID 0 is actually FID_STANDALONE, then you'd read FID_STANDALONE even if you change the value of FID_STANDALONE in the driver to something else, like 1. Something ultra hackish like this will install VLAN 3050 as first VID in the switch, and that will gain FID 0. Then, MV886XXX_VID_STANDALONE will gain FID 1. So we need to adjust the definitions. diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index ccfa4751d3b7..5923cbb172f9 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -3410,9 +3410,15 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port) if (err) return err; + err = mv88e6xxx_port_vlan_join(chip, port, 3050, + MV88E6XXX_G1_VTU_DATA_MEMBER_TAG_UNMODIFIED, + false); + if (err) + return err; + /* Bind MV88E6XXX_VID_STANDALONE to MV88E6XXX_FID_STANDALONE by * virtue of the fact that mv88e6xxx_atu_new() will pick it as - * the first free FID. This will be used as the private PVID for + * the second free FID. This will be used as the private PVID for * unbridged ports. Shared (DSA and CPU) ports must also be * members of this VID, in order to trap all frames assigned to * it to the CPU. diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h index e693154cf803..48d4db4f2adf 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.h +++ b/drivers/net/dsa/mv88e6xxx/chip.h @@ -22,8 +22,8 @@ #define MV88E6XXX_N_FID 4096 #define MV88E6XXX_N_SID 64 -#define MV88E6XXX_FID_STANDALONE 0 -#define MV88E6XXX_FID_BRIDGED 1 +#define MV88E6XXX_FID_STANDALONE 1 +#define MV88E6XXX_FID_BRIDGED 2 /* PVT limits for 4-bit port and 5-bit switch */ #define MV88E6XXX_MAX_PVT_SWITCHES 32 Now back to running the ./bridge_locked_port.sh selftest. One can now see that whereas before, the errors we got from time to time with FID 0 are now reported with FID 1. So it is MV88E6XXX_FID_STANDALONE, after all, not just 0. So why, then? And most importantly, when in the selftest does this problem occur, and as a result of which traffic? If you actually open the selftest and put some prints in the areas of the failure, you might be tempted to think that it's the "ping_do $h1 192.0.2.2" command that causes it. locked_port_mab() { RET=0 check_port_mab_support || return 0 ping_do $h1 192.0.2.2 check_err $? "Ping did not work before locking port" bridge link set dev $swp1 learning on locked on ping_do $h1 192.0.2.2 check_fail $? "Ping worked on a locked port without an FDB entry" bridge fdb get `mac_get $h1` br br0 vlan 1 &> /dev/null check_fail $? "FDB entry created before enabling MAB" bridge link set dev $swp1 learning on locked on mab on set -x ping_do $h1 192.0.2.2 check_fail $? "Ping worked on MAB enabled port without an FDB entry" set +x bash bridge fdb get `mac_get $h1` br br0 vlan 1 | grep "dev $swp1" | grep -q "locked" check_err $? "Locked FDB entry not created" bridge fdb replace `mac_get $h1` dev $swp1 master static ping_do $h1 192.0.2.2 check_err $? "Ping did not work after replacing FDB entry" bridge fdb get `mac_get $h1` br br0 vlan 1 | grep "dev $swp1" | grep -q "locked" check_fail $? "FDB entry marked as locked after replacement" bridge fdb del `mac_get $h1` dev $swp1 master bridge link set dev $swp1 learning off locked off mab off log_test "Locked port MAB" } "Interesting", you might say. So stop the selftest execution there, and run that ping again, and again, and again. But the packets from $h1 consistently get classified to the correct FID (the FID of the bridge port's VLAN-aware PVID). But from time to time, those ATU errors with FID_STANDALONE keep popping up. Strangely, it doesn't seem to be related to the ping command, at all, in that the errors appear even while there's no pinging going on. And then you realize, but hey, there's also a VLAN interface in the network, created by "vlan_create $h1 100 v$h1 198.51.100.1/24". And VID 100, at the time of the locked_port_mab() selftest, is not present in the bridge VLAN database of port $h2. So instead of ping_do $h1 192.0.2.2 why don't we try to do ping_do $h1.100 198.51.100.2 and actually ping over that VLAN interface? And surprise surprise, now every packet with VID 100 gets classified to FID_STANDALONE, and the problem is 100% reproducible. The reverse is also true. You delete the "vlan_create" commands and you skip the selftests that use the $h1.100 interface, and the problem goes away. Then, the next step is opening the documentation. If you look at Figure 23 "Relationship between VTU, STU and ATU", it will say that "the port's Default FID is used if 802.1Q is disabled on the port or if the VID assigned to the frame points to an empty VTU entry". VID 100 indeed points to an empty VTU entry. The port Default FID is set with this call: /* Port based VLAN map: give each port the same default address * database, and allow bidirectional communication between the * CPU and DSA port(s), and the other ports. */ err = mv88e6xxx_port_set_fid(chip, port, MV88E6XXX_FID_STANDALONE); if (err) return err; So it appears that frames which get a VTU miss will still also cause an ATU miss, and that's what you're seeing. The solution would be to acknowledge this fact, and not print any error message from the ATU IRQ handler for unknown FID/VID, which would just alarm the user. What I wanted to say with this is that it doesn't take a mad scientist to figure this stuff out, just somebody who is motivated not to throw half assed stuff over the fence to the mainline kernel, and just call it a day when his project is over. I don't even interact with Marvell hardware on a day to day basis, what I know is just information gathered during patch review, and I could still figure out what's going on. There's actually a deeper problem which concerns me more. I'm extremely fed up with seeing this patch set progress so slowly, to the point where I've considered a few times to just say fsck it, it's good enough, it's not going to get better in your hands, let it be merged, and I'll take a second look when I'll have some time to spare and I'll clean up what I can. Now with Ido's help for the software bridge and MAB offload part, the patch set is really so small and so close to getting accepted, that I don't see what's holding you up, really. The review comments are absolutely actionable. My dilemma is that I don't think it's okay that there exists this "merge patch set through reviewer exhaustion" loophole. But on the other hand, if as a reviewer I don't want that to happen, I have to waste my time with people who simply don't want to use their critical thinking, test on actual hardware what they've done, find problems that they didn't want to tackle. This is also the reason why I sent the tracepoints patch set, which I really wanted *you* to do, not later than the MAB/locked port support itself. I find the current state of your patch set fairly unusable from the perspective of a user who uses a serial console. But I can't waste an unlimited amount of time, I have other stuff to do, too. I hope we can find a compromise where you can be more responsive to what you're being told during review. Thanks for listening.
On 2022-12-08 14:35, Vladimir Oltean wrote: > On Thu, Dec 08, 2022 at 01:28:27PM +0100, netdev@kapio-technology.com > wrote: >> On 2022-12-07 21:29, Vladimir Oltean wrote: >> > On Tue, Dec 06, 2022 at 05:36:42PM +0100, netdev@kapio-technology.com wrote: >> > > > I was under the impression that we agreed that the locking change will >> > > > be split to a separate patch. >> > > >> > > Sorry, I guess that because of the quite long time that has passed as I >> > > needed to get this FID=0 issue sorted out, and had many other different >> > > changes to attend, I forgot. >> > >> > Well, at least you got the FID=0 issue sorted out... right? >> > What was the cause, what is the solution? >> >> Well I got it sorted out in the way that I have identified that it is >> the >> ATU op that fails some times. I don't think there is anything that can >> be >> done about that, other than what I do and let the interrupt routing >> return >> an error. > > Yikes. But why would you call that "sorted out", though? Just to make > it > appear as though you really spent some time on it, and use it as an > excuse for something else? > >> it is the ATU op that fails some times. > > Let's start with the assumption that this is correct. A person with > critical thinking will ask "can I prove that it is?". > > If the ATU operation fails sometimes, I would expect that it always > fails in the same way, by returning FID 0, where 0 is some kind of > "invalid" value. > > But if FID 0 is actually FID_STANDALONE, then you'd read FID_STANDALONE > even if you change the value of FID_STANDALONE in the driver to > something else, like 1. > > Something ultra hackish like this will install VLAN 3050 as first VID > in > the switch, and that will gain FID 0. Then, MV886XXX_VID_STANDALONE > will > gain FID 1. So we need to adjust the definitions. > Here is an example of the output I have when running the locked_port_mab() under the selftests... mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU problem: error -22 while handling interrupt mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU problem: error -22 while handling interrupt mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU problem: error -22 while handling interrupt mv88e6085 1002b000.ethernet-1:04: ATU problem: error -22 while handling interrupt mv88e6085 1002b000.ethernet-1:04: ATU problem: error -22 while handling interrupt mv88e6xxx_g1_atu_prob_irq_thread_fn: 13 callbacks suppressed mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU problem: error -22 while handling interrupt mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU problem: error -22 while handling interrupt mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 the -22 errors are all when it returns FID=0, and it is the same mac all the way. I have other logs, where the -22 occurs at random other times, f.ex. same test: mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU problem: error -22 while handling interrupt mv88e6xxx_g1_atu_prob_irq_thread_fn: 4 callbacks suppressed mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU problem: error -22 while handling interrupt mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU miss violation for 8e:a9:fc:14:58:06 portvec 0 spid 2 mv88e6085 1002b000.ethernet-1:04: ATU problem: error -22 while handling interrupt What else conclusion than it is the ATU op that fails?
On Thu, Dec 08, 2022 at 03:41:24PM +0100, netdev@kapio-technology.com wrote:
> What else conclusion than it is the ATU op that fails?
I don't have any more time than to say "read the rest of my email", sorry.
On 2022-12-08 14:35, Vladimir Oltean wrote: > > So it appears that frames which get a VTU miss will still also cause an > ATU miss, and that's what you're seeing. > > The solution would be to acknowledge this fact, and not print any error > message from the ATU IRQ handler for unknown FID/VID, which would just > alarm the user. Thanks for clearing that up! At leisure, do you have an idea why it will encounter a VTU miss violation at random? I guess I must check if FID != FID_STANDALONE instead then...
On Thu, Dec 08, 2022 at 05:03:59PM +0100, netdev@kapio-technology.com wrote: > At leisure, do you have an idea why it will encounter a VTU miss violation > at random? Do you understand that any packets with a VID that isn't present in the VTU will trigger a VTU miss, and also an ATU miss if the port is locked and the ATU doesn't have an entry with that FID? Your selftest creates a VLAN interface on top of $h1 with a VID that isn't present in the VTU of the switch ("bridge vlan add .. vid 100" is run elsewhere; we run "bridge vlan del ... vid 100" when we no longer need it). But the $h1.100 interface is persistent across the selftest. And it's not silent. Linux does all sorts of crap by default, like IPv6 neighbor discovery, even if you don't use the interface. So it will send packets from time to time. And that's when you get those ATU and VTU violations. The MAC address of $h1.100 is the same as the MAC address of $h1, of course.
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 66d7eae24ce0..732d7a2e2a07 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,6 +6582,12 @@ static int mv88e6xxx_port_bridge_flags(struct dsa_switch *ds, int port, goto out; } + if (flags.mask & BR_PORT_MAB) { + bool mab = !!(flags.val & BR_PORT_MAB); + + mv88e6xxx_port_set_mab(chip, port, mab); + } + if (flags.mask & BR_PORT_LOCKED) { bool locked = !!(flags.val & BR_PORT_LOCKED); diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h index e693154cf803..f635a5bb47ce 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.h +++ b/drivers/net/dsa/mv88e6xxx/chip.h @@ -280,6 +280,9 @@ struct mv88e6xxx_port { unsigned int serdes_irq; char serdes_irq_name[64]; struct devlink_region *region; + + /* MacAuth Bypass control flag */ + bool mab; }; enum mv88e6xxx_region_id { @@ -784,6 +787,12 @@ static inline bool mv88e6xxx_is_invalid_port(struct mv88e6xxx_chip *chip, int po return (chip->info->invalid_port_mask & BIT(port)) != 0; } +static inline void mv88e6xxx_port_set_mab(struct mv88e6xxx_chip *chip, + int port, bool mab) +{ + chip->ports[port].mab = mab; +} + int mv88e6xxx_read(struct mv88e6xxx_chip *chip, int addr, int reg, u16 *val); int mv88e6xxx_write(struct mv88e6xxx_chip *chip, int addr, int reg, u16 val); int mv88e6xxx_wait_mask(struct mv88e6xxx_chip *chip, int addr, int reg, @@ -802,6 +811,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..bb004df517b2 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 */ @@ -408,23 +409,25 @@ static irqreturn_t mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id) err = mv88e6xxx_g1_read_atu_violation(chip); if (err) - goto out; + goto out_unlock; err = mv88e6xxx_g1_read(chip, MV88E6XXX_G1_ATU_OP, &val); if (err) - goto out; + goto out_unlock; err = mv88e6xxx_g1_atu_fid_read(chip, &fid); if (err) - goto out; + goto out_unlock; err = mv88e6xxx_g1_atu_data_read(chip, &entry); if (err) - goto out; + goto out_unlock; err = mv88e6xxx_g1_atu_mac_read(chip, &entry); if (err) - goto out; + goto out_unlock; + + mv88e6xxx_reg_unlock(chip); spid = entry.state; @@ -446,6 +449,18 @@ 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) { + err = -EINVAL; + goto out; + } + + if (chip->ports[spid].mab) { + err = mv88e6xxx_handle_miss_violation(chip, spid, + &entry, fid); + if (err) + goto out; + } } if (val & MV88E6XXX_G1_ATU_OP_FULL_VIOLATION) { @@ -454,13 +469,13 @@ 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; -out: +out_unlock: mv88e6xxx_reg_unlock(chip); +out: dev_err(chip->dev, "ATU problem: error %d while handling interrupt\n", err); return IRQ_HANDLED; diff --git a/drivers/net/dsa/mv88e6xxx/switchdev.c b/drivers/net/dsa/mv88e6xxx/switchdev.c new file mode 100644 index 000000000000..4c346a884fb2 --- /dev/null +++ b/drivers/net/dsa/mv88e6xxx/switchdev.c @@ -0,0 +1,83 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * switchdev.c + * + * Authors: + * Hans J. Schultz <netdev@kapio-technology.com> + * + */ + +#include <net/switchdev.h> +#include "chip.h" +#include "global1.h" +#include "switchdev.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; +} + +static int mv88e6xxx_find_vid(struct mv88e6xxx_chip *chip, u16 fid, u16 *vid) +{ + struct mv88e6xxx_fid_search_ctx ctx; + int err; + + 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) + *vid = ctx.vid_found; + else + return -ENOENT; + + return 0; +} + +int mv88e6xxx_handle_miss_violation(struct mv88e6xxx_chip *chip, int port, + struct mv88e6xxx_atu_entry *entry, u16 fid) +{ + struct switchdev_notifier_fdb_info info = { + .addr = entry->mac, + .locked = true, + }; + struct net_device *brport; + struct dsa_port *dp; + u16 vid; + int err; + + err = mv88e6xxx_find_vid(chip, fid, &vid); + if (err) + return err; + + info.vid = vid; + 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(); + + return err; +} diff --git a/drivers/net/dsa/mv88e6xxx/switchdev.h b/drivers/net/dsa/mv88e6xxx/switchdev.h new file mode 100644 index 000000000000..62214f9d62b0 --- /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 <netdev@kapio-technology.com> + * + */ + +#ifndef _MV88E6XXX_SWITCHDEV_H_ +#define _MV88E6XXX_SWITCHDEV_H_ + +#include "chip.h" + +int mv88e6xxx_handle_miss_violation(struct mv88e6xxx_chip *chip, int port, + struct mv88e6xxx_atu_entry *entry, + u16 fid); + +#endif /* _MV88E6XXX_SWITCHDEV_H_ */
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, which will result in the bridge module adding 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: ethtool -S ethX NIC statistics: ... atu_member_violation: 5 atu_miss_violation: 23 ... Where ethX is the interface of the MAB enabled port. An anomaly has been observed, where the ATU op to read the FID reports FID=0 even though it is not a valid read. An -EINVAL error will be logged in this case. This was seen on a mv88e6097. Signed-off-by: Hans J. Schultz <netdev@kapio-technology.com> --- drivers/net/dsa/mv88e6xxx/Makefile | 1 + drivers/net/dsa/mv88e6xxx/chip.c | 18 ++++-- drivers/net/dsa/mv88e6xxx/chip.h | 15 +++++ drivers/net/dsa/mv88e6xxx/global1_atu.c | 29 ++++++--- drivers/net/dsa/mv88e6xxx/switchdev.c | 83 +++++++++++++++++++++++++ drivers/net/dsa/mv88e6xxx/switchdev.h | 19 ++++++ 6 files changed, 152 insertions(+), 13 deletions(-) create mode 100644 drivers/net/dsa/mv88e6xxx/switchdev.c create mode 100644 drivers/net/dsa/mv88e6xxx/switchdev.h