| Message ID | 20230130173429.3577450-6-netdev@kapio-technology.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ATU and FDB synchronization on locked ports | expand |
On Mon, Jan 30, 2023 at 06:34:29PM +0100, Hans J. Schultz wrote: > For 802.1X or MAB security authed hosts we want to have these hosts authed > by adding dynamic FDB entries, so that if an authed host goes silent for > a time period it's FDB entry will be removed and it must reauth when > wanting to communicate again. > In the mv88e6xxx offloaded case, we can use the HoldAt1 feature, that > gives an age out interrupt when the FDB entry is about to age out, so > that userspace can be notified of the entry being deleted with the help > of an SWITCHDEV_FDB_DEL_TO_BRIDGE event. > When adding a dynamic entry the bridge must be informed that the driver > takes care of the ageing be sending an SWITCHDEV_FDB_OFFLOADED event, > telling the bridge that this added FDB entry will be handled by the > driver. > With this implementation, trace events for age out interrupts are also > added. > > note: A special case arises with the age out interrupt, as the entry > state/spid (source port id) value read from the registers will be zero. > Thus we need to extract the source port from the port vector instead. > > Signed-off-by: Hans J. Schultz <netdev@kapio-technology.com> > --- > drivers/net/dsa/mv88e6xxx/chip.c | 18 ++++++-- > drivers/net/dsa/mv88e6xxx/global1_atu.c | 21 +++++++++ > drivers/net/dsa/mv88e6xxx/port.c | 6 ++- > drivers/net/dsa/mv88e6xxx/switchdev.c | 61 +++++++++++++++++++++++++ > drivers/net/dsa/mv88e6xxx/switchdev.h | 5 ++ > drivers/net/dsa/mv88e6xxx/trace.h | 5 ++ > 6 files changed, 110 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c > index 61d5dc4680e3..a0007d96b2a3 100644 > --- a/drivers/net/dsa/mv88e6xxx/chip.c > +++ b/drivers/net/dsa/mv88e6xxx/chip.c > @@ -42,6 +42,7 @@ > #include "ptp.h" > #include "serdes.h" > #include "smi.h" > +#include "switchdev.h" > > static void assert_reg_lock(struct mv88e6xxx_chip *chip) > { > @@ -2726,18 +2727,25 @@ static int mv88e6xxx_port_fdb_add(struct dsa_switch *ds, int port, > const unsigned char *addr, u16 vid, > u16 fdb_flags, struct dsa_db db) > { > + bool is_dynamic = !!(fdb_flags & DSA_FDB_FLAG_DYNAMIC); > struct mv88e6xxx_chip *chip = ds->priv; > + u8 state; > int err; > > - /* Ignore entries with flags set */ > - if (fdb_flags) > - return 0; > + state = MV88E6XXX_G1_ATU_DATA_STATE_UC_STATIC; > + if (is_dynamic) > + state = MV88E6XXX_G1_ATU_DATA_STATE_UC_AGE_7_NEWEST; What if flags other than DSA_FDB_FLAG_DYNAMIC are set (in future)? > + else > + if (fdb_flags) nit: else if (fdb_flags) > + return 0; > ...
On 2023-01-31 19:56, Simon Horman wrote: >> --- a/drivers/net/dsa/mv88e6xxx/chip.c >> +++ b/drivers/net/dsa/mv88e6xxx/chip.c >> @@ -42,6 +42,7 @@ >> #include "ptp.h" >> #include "serdes.h" >> #include "smi.h" >> +#include "switchdev.h" >> >> static void assert_reg_lock(struct mv88e6xxx_chip *chip) >> { >> @@ -2726,18 +2727,25 @@ static int mv88e6xxx_port_fdb_add(struct >> dsa_switch *ds, int port, >> const unsigned char *addr, u16 vid, >> u16 fdb_flags, struct dsa_db db) >> { >> + bool is_dynamic = !!(fdb_flags & DSA_FDB_FLAG_DYNAMIC); >> struct mv88e6xxx_chip *chip = ds->priv; >> + u8 state; >> int err; >> >> - /* Ignore entries with flags set */ >> - if (fdb_flags) >> - return 0; >> + state = MV88E6XXX_G1_ATU_DATA_STATE_UC_STATIC; >> + if (is_dynamic) >> + state = MV88E6XXX_G1_ATU_DATA_STATE_UC_AGE_7_NEWEST; > > What if flags other than DSA_FDB_FLAG_DYNAMIC are set (in future)? They will have to be caught and handled here if there is support for it, e.g. something like... else if (someflag) dosomething(); For now only one flag will actually be set and they are mutually exclusive, as they will not make sense together with the potential flags I know, but that can change at some time of course. > >> + else >> + if (fdb_flags) > > nit: else if (fdb_flags) > >> + return 0; >> > > ...
On Thu, Feb 02, 2023 at 06:00:00PM +0100, netdev@kapio-technology.com wrote: > On 2023-01-31 19:56, Simon Horman wrote: > > > --- a/drivers/net/dsa/mv88e6xxx/chip.c > > > +++ b/drivers/net/dsa/mv88e6xxx/chip.c > > > @@ -42,6 +42,7 @@ > > > #include "ptp.h" > > > #include "serdes.h" > > > #include "smi.h" > > > +#include "switchdev.h" > > > > > > static void assert_reg_lock(struct mv88e6xxx_chip *chip) > > > { > > > @@ -2726,18 +2727,25 @@ static int mv88e6xxx_port_fdb_add(struct > > > dsa_switch *ds, int port, > > > const unsigned char *addr, u16 vid, > > > u16 fdb_flags, struct dsa_db db) > > > { > > > + bool is_dynamic = !!(fdb_flags & DSA_FDB_FLAG_DYNAMIC); > > > struct mv88e6xxx_chip *chip = ds->priv; > > > + u8 state; > > > int err; > > > > > > - /* Ignore entries with flags set */ > > > - if (fdb_flags) > > > - return 0; > > > + state = MV88E6XXX_G1_ATU_DATA_STATE_UC_STATIC; > > > + if (is_dynamic) > > > + state = MV88E6XXX_G1_ATU_DATA_STATE_UC_AGE_7_NEWEST; > > > > What if flags other than DSA_FDB_FLAG_DYNAMIC are set (in future)? > > They will have to be caught and handled here if there is support for it, > e.g. something like... > > else if (someflag) > dosomething(); > > For now only one flag will actually be set and they are mutually exclusive, > as they will not make sense together with the potential flags I know, but > that can change at some time of course. Yes, I see that is workable. I do feel that checking for other flags would be a bit more robust. But as you say, there are none. So whichever approach you prefer is fine by me. > > > > > + else > > > + if (fdb_flags) > > > > nit: else if (fdb_flags) > > > > > + return 0; > > > > > > > ...
On Fri, Feb 03, 2023 at 09:20:22AM +0100, Simon Horman wrote: > > else if (someflag) > > dosomething(); > > > > For now only one flag will actually be set and they are mutually exclusive, > > as they will not make sense together with the potential flags I know, but > > that can change at some time of course. > > Yes, I see that is workable. I do feel that checking for other flags would > be a bit more robust. But as you say, there are none. So whichever > approach you prefer is fine by me. The model we have for unsupported bits in the SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS and SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS handlers is essentially this: if (flags & ~(supported_flag_mask)) return -EOPNOTSUPP; if (flags & supported_flag_1) ... if (flags & supported_flag_2) ... I suppose applying this model here would address Simon's extensibility concern.
On Fri, Feb 03, 2023 at 10:44:22PM +0200, Vladimir Oltean wrote: > On Fri, Feb 03, 2023 at 09:20:22AM +0100, Simon Horman wrote: > > > else if (someflag) > > > dosomething(); > > > > > > For now only one flag will actually be set and they are mutually exclusive, > > > as they will not make sense together with the potential flags I know, but > > > that can change at some time of course. > > > > Yes, I see that is workable. I do feel that checking for other flags would > > be a bit more robust. But as you say, there are none. So whichever > > approach you prefer is fine by me. > > The model we have for unsupported bits in the SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS > and SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS handlers is essentially this: > > if (flags & ~(supported_flag_mask)) > return -EOPNOTSUPP; > > if (flags & supported_flag_1) > ... > > if (flags & supported_flag_2) > ... > > I suppose applying this model here would address Simon's extensibility concern. Yes, that is the model I had in mind.
On 2023-02-04 09:12, Simon Horman wrote: > On Fri, Feb 03, 2023 at 10:44:22PM +0200, Vladimir Oltean wrote: >> On Fri, Feb 03, 2023 at 09:20:22AM +0100, Simon Horman wrote: >> > > else if (someflag) >> > > dosomething(); >> > > >> > > For now only one flag will actually be set and they are mutually exclusive, >> > > as they will not make sense together with the potential flags I know, but >> > > that can change at some time of course. >> > >> > Yes, I see that is workable. I do feel that checking for other flags would >> > be a bit more robust. But as you say, there are none. So whichever >> > approach you prefer is fine by me. >> >> The model we have for unsupported bits in the >> SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS >> and SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS handlers is essentially this: >> >> if (flags & ~(supported_flag_mask)) >> return -EOPNOTSUPP; >> >> if (flags & supported_flag_1) >> ... >> >> if (flags & supported_flag_2) >> ... >> >> I suppose applying this model here would address Simon's extensibility >> concern. > > Yes, that is the model I had in mind. The only thing is that we actually need to return both 0 and -EOPNOTSUPP for unsupported flags. The dynamic flag requires 0 when not supported (and supported) AFAICS. Setting a mask as 'supported' for a feature that is not really supported defeats the notion of 'supported' IMHO.
On Sat, Feb 04, 2023 at 09:48:24AM +0100, netdev@kapio-technology.com wrote: > On 2023-02-04 09:12, Simon Horman wrote: > > On Fri, Feb 03, 2023 at 10:44:22PM +0200, Vladimir Oltean wrote: > > > On Fri, Feb 03, 2023 at 09:20:22AM +0100, Simon Horman wrote: > > > > > else if (someflag) > > > > > dosomething(); > > > > > > > > > > For now only one flag will actually be set and they are mutually exclusive, > > > > > as they will not make sense together with the potential flags I know, but > > > > > that can change at some time of course. > > > > > > > > Yes, I see that is workable. I do feel that checking for other flags would > > > > be a bit more robust. But as you say, there are none. So whichever > > > > approach you prefer is fine by me. > > > > > > The model we have for unsupported bits in the > > > SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS > > > and SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS handlers is essentially this: > > > > > > if (flags & ~(supported_flag_mask)) > > > return -EOPNOTSUPP; > > > > > > if (flags & supported_flag_1) > > > ... > > > > > > if (flags & supported_flag_2) > > > ... > > > > > > I suppose applying this model here would address Simon's > > > extensibility concern. > > > > Yes, that is the model I had in mind. > > The only thing is that we actually need to return both 0 and -EOPNOTSUPP for > unsupported flags. The dynamic flag requires 0 when not supported (and > supported) AFAICS. > Setting a mask as 'supported' for a feature that is not really supported > defeats the notion of 'supported' IMHO. Just to clarify my suggestion one last time, it would be along the lines of the following (completely untested!). I feel that it robustly covers all cases for fdb_flags. And as a bonus doesn't need to be modified if other (unsupported) flags are added in future. if (fdb_flags & ~(DSA_FDB_FLAG_DYNAMIC)) return -EOPNOTSUPP; is_dynamic = !!(fdb_flags & DSA_FDB_FLAG_DYNAMIC) if (is_dynamic) state = MV88E6XXX_G1_ATU_DATA_STATE_UC_AGE_7_NEWEST; And perhaps for other drivers: if (fdb_flags & ~(DSA_FDB_FLAG_DYNAMIC)) return -EOPNOTSUPP; if (fdb_flags) return 0; Perhaps a helper would be warranted for the above. But in writing this I think that, perhaps drivers could return -EOPNOTSUPP for the DSA_FDB_FLAG_DYNAMIC case and the caller can handle, rather tha propagate, -EOPNOTSUPP. Returning -EOPNOTSUPP is the normal way to drivers to respond to requests for unsupported hardware offloads. Sticking to that may be clearner in the long run. That said, I do agree your current patch is correct given the flag that is defined (by your patchset).
On Mon, Feb 06, 2023 at 17:02, Simon Horman <simon.horman@corigine.com> wrote: > > Just to clarify my suggestion one last time, it would be along the lines > of the following (completely untested!). I feel that it robustly covers > all cases for fdb_flags. And as a bonus doesn't need to be modified > if other (unsupported) flags are added in future. > > if (fdb_flags & ~(DSA_FDB_FLAG_DYNAMIC)) > return -EOPNOTSUPP; > > is_dynamic = !!(fdb_flags & DSA_FDB_FLAG_DYNAMIC) > if (is_dynamic) > state = MV88E6XXX_G1_ATU_DATA_STATE_UC_AGE_7_NEWEST; > > > And perhaps for other drivers: > > if (fdb_flags & ~(DSA_FDB_FLAG_DYNAMIC)) > return -EOPNOTSUPP; > if (fdb_flags) > return 0; > > Perhaps a helper would be warranted for the above. How would such a helper look? Inline function is not clean. > > But in writing this I think that, perhaps drivers could return -EOPNOTSUPP > for the DSA_FDB_FLAG_DYNAMIC case and the caller can handle, rather tha > propagate, -EOPNOTSUPP. I looked at that, but changing the caller is also a bit ugly. > > Returning -EOPNOTSUPP is the normal way to drivers to respond to requests > for unsupported hardware offloads. Sticking to that may be clearner > in the long run. That said, I do agree your current patch is correct > given the flag that is defined (by your patchset).
On Tue, Feb 14, 2023 at 10:14:55PM +0100, Hans Schultz wrote: > On Mon, Feb 06, 2023 at 17:02, Simon Horman <simon.horman@corigine.com> wrote: > > > > Just to clarify my suggestion one last time, it would be along the lines > > of the following (completely untested!). I feel that it robustly covers > > all cases for fdb_flags. And as a bonus doesn't need to be modified > > if other (unsupported) flags are added in future. > > > > if (fdb_flags & ~(DSA_FDB_FLAG_DYNAMIC)) > > return -EOPNOTSUPP; > > > > is_dynamic = !!(fdb_flags & DSA_FDB_FLAG_DYNAMIC) > > if (is_dynamic) > > state = MV88E6XXX_G1_ATU_DATA_STATE_UC_AGE_7_NEWEST; > > > > > > And perhaps for other drivers: > > > > if (fdb_flags & ~(DSA_FDB_FLAG_DYNAMIC)) > > return -EOPNOTSUPP; > > if (fdb_flags) > > return 0; > > > > Perhaps a helper would be warranted for the above. > > How would such a helper look? Inline function is not clean. > > > > > But in writing this I think that, perhaps drivers could return -EOPNOTSUPP > > for the DSA_FDB_FLAG_DYNAMIC case and the caller can handle, rather tha > > propagate, -EOPNOTSUPP. > > I looked at that, but changing the caller is also a bit ugly. Answering on behalf of Simon, and hoping he will agree. You are missing a big opportunity to make the kernel avoid doing useless work. The dsa_slave_fdb_event() function runs in atomic switchdev notifier context, and schedules a deferred workqueue item - dsa_schedule_work() - to get sleepable context to program hardware. Only that scheduling a deferred work item is not exactly cheap, so we try to avoid doing that unless we know that we'll end up doing something with that FDB entry once the deferred work does get scheduled: /* Check early that we're not doing work in vain. * Host addresses on LAG ports still require regular FDB ops, * since the CPU port isn't in a LAG. */ if (dp->lag && !host_addr) { if (!ds->ops->lag_fdb_add || !ds->ops->lag_fdb_del) return -EOPNOTSUPP; } else { if (!ds->ops->port_fdb_add || !ds->ops->port_fdb_del) return -EOPNOTSUPP; } What you should be doing is you should be using the pahole tool to find a good place for a new unsigned long field in struct dsa_switch, and add a new field ds->supported_fdb_flags. You should extend the early checking from dsa_slave_fdb_event() and exit without doing anything if the (fdb->flags & ~ds->supported_fdb_flags) expression is non-zero. This way you would kill 2 birds with 1 stone, since individual drivers would no longer need to check the flags; DSA would guarantee not calling them with unsupported flags. It would be also very good to reach an agreement with switchdev maintainers regarding the naming of the is_static/is_dyn field. It would also be excellent if you could rename "fdb_flags" to just "flags" within DSA.
On Fri, Feb 17, 2023 at 07:44:31PM +0200, Vladimir Oltean wrote: > On Tue, Feb 14, 2023 at 10:14:55PM +0100, Hans Schultz wrote: > > On Mon, Feb 06, 2023 at 17:02, Simon Horman <simon.horman@corigine.com> wrote: > > > > > > Just to clarify my suggestion one last time, it would be along the lines > > > of the following (completely untested!). I feel that it robustly covers > > > all cases for fdb_flags. And as a bonus doesn't need to be modified > > > if other (unsupported) flags are added in future. > > > > > > if (fdb_flags & ~(DSA_FDB_FLAG_DYNAMIC)) > > > return -EOPNOTSUPP; > > > > > > is_dynamic = !!(fdb_flags & DSA_FDB_FLAG_DYNAMIC) > > > if (is_dynamic) > > > state = MV88E6XXX_G1_ATU_DATA_STATE_UC_AGE_7_NEWEST; > > > > > > > > > And perhaps for other drivers: > > > > > > if (fdb_flags & ~(DSA_FDB_FLAG_DYNAMIC)) > > > return -EOPNOTSUPP; > > > if (fdb_flags) > > > return 0; > > > > > > Perhaps a helper would be warranted for the above. > > > > How would such a helper look? Inline function is not clean. > > > > > > > > But in writing this I think that, perhaps drivers could return -EOPNOTSUPP > > > for the DSA_FDB_FLAG_DYNAMIC case and the caller can handle, rather tha > > > propagate, -EOPNOTSUPP. > > > > I looked at that, but changing the caller is also a bit ugly. > > Answering on behalf of Simon, and hoping he will agree. Sorry for not responding earlier - I was on vacation last week. TBH my idea was not nearly as well developed as the one you describe below. But yes, I agree this is an interesting approach. > You are missing a big opportunity to make the kernel avoid doing useless work. > The dsa_slave_fdb_event() function runs in atomic switchdev notifier context, > and schedules a deferred workqueue item - dsa_schedule_work() - to get sleepable > context to program hardware. > > Only that scheduling a deferred work item is not exactly cheap, so we try to > avoid doing that unless we know that we'll end up doing something with that > FDB entry once the deferred work does get scheduled: > > /* Check early that we're not doing work in vain. > * Host addresses on LAG ports still require regular FDB ops, > * since the CPU port isn't in a LAG. > */ > if (dp->lag && !host_addr) { > if (!ds->ops->lag_fdb_add || !ds->ops->lag_fdb_del) > return -EOPNOTSUPP; > } else { > if (!ds->ops->port_fdb_add || !ds->ops->port_fdb_del) > return -EOPNOTSUPP; > } > > What you should be doing is you should be using the pahole tool to find > a good place for a new unsigned long field in struct dsa_switch, and add > a new field ds->supported_fdb_flags. You should extend the early checking > from dsa_slave_fdb_event() and exit without doing anything if the > (fdb->flags & ~ds->supported_fdb_flags) expression is non-zero. > > This way you would kill 2 birds with 1 stone, since individual drivers > would no longer need to check the flags; DSA would guarantee not calling > them with unsupported flags. > > It would be also very good to reach an agreement with switchdev > maintainers regarding the naming of the is_static/is_dyn field. > > It would also be excellent if you could rename "fdb_flags" to just > "flags" within DSA.
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index 61d5dc4680e3..a0007d96b2a3 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -42,6 +42,7 @@ #include "ptp.h" #include "serdes.h" #include "smi.h" +#include "switchdev.h" static void assert_reg_lock(struct mv88e6xxx_chip *chip) { @@ -2726,18 +2727,25 @@ static int mv88e6xxx_port_fdb_add(struct dsa_switch *ds, int port, const unsigned char *addr, u16 vid, u16 fdb_flags, struct dsa_db db) { + bool is_dynamic = !!(fdb_flags & DSA_FDB_FLAG_DYNAMIC); struct mv88e6xxx_chip *chip = ds->priv; + u8 state; int err; - /* Ignore entries with flags set */ - if (fdb_flags) - return 0; + state = MV88E6XXX_G1_ATU_DATA_STATE_UC_STATIC; + if (is_dynamic) + state = MV88E6XXX_G1_ATU_DATA_STATE_UC_AGE_7_NEWEST; + else + if (fdb_flags) + return 0; mv88e6xxx_reg_lock(chip); - err = mv88e6xxx_port_db_load_purge(chip, port, addr, vid, - MV88E6XXX_G1_ATU_DATA_STATE_UC_STATIC); + err = mv88e6xxx_port_db_load_purge(chip, port, addr, vid, state); mv88e6xxx_reg_unlock(chip); + if (is_dynamic && !err) + mv88e6xxx_set_fdb_offloaded(ds, port, addr, vid); + return err; } diff --git a/drivers/net/dsa/mv88e6xxx/global1_atu.c b/drivers/net/dsa/mv88e6xxx/global1_atu.c index ce3b3690c3c0..c95f8cffba41 100644 --- a/drivers/net/dsa/mv88e6xxx/global1_atu.c +++ b/drivers/net/dsa/mv88e6xxx/global1_atu.c @@ -432,6 +432,27 @@ static irqreturn_t mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id) spid = entry.state; + if (val & MV88E6XXX_G1_ATU_OP_AGE_OUT_VIOLATION) { + unsigned long port = 0; + unsigned long portvec = entry.portvec; + + port = find_first_bit(&portvec, MV88E6XXX_MAX_PVT_PORTS); + if (port >= MV88E6XXX_MAX_PVT_PORTS) { + dev_err(chip->dev, + "ATU err: mac: %pM. Port not in portvec: %x\n", + entry.mac, entry.portvec); + goto out; + } + + spid = port; + trace_mv88e6xxx_atu_age_out_violation(chip->dev, spid, + entry.portvec, entry.mac, + fid); + + err = mv88e6xxx_handle_age_out_violation(chip, spid, + &entry, fid); + } + if (val & MV88E6XXX_G1_ATU_OP_MEMBER_VIOLATION) { trace_mv88e6xxx_atu_member_violation(chip->dev, spid, entry.portvec, entry.mac, diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c index f79cf716c541..5225971b9a33 100644 --- a/drivers/net/dsa/mv88e6xxx/port.c +++ b/drivers/net/dsa/mv88e6xxx/port.c @@ -1255,7 +1255,11 @@ int mv88e6xxx_port_set_lock(struct mv88e6xxx_chip *chip, int port, reg &= ~MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT; if (locked) - reg |= MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT; + reg |= MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT | + MV88E6XXX_PORT_ASSOC_VECTOR_REFRESH_LOCKED | + MV88E6XXX_PORT_ASSOC_VECTOR_IGNORE_WRONG | + MV88E6XXX_PORT_ASSOC_VECTOR_INT_AGE_OUT | + MV88E6XXX_PORT_ASSOC_VECTOR_HOLD_AT_1; return mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_ASSOC_VECTOR, reg); } diff --git a/drivers/net/dsa/mv88e6xxx/switchdev.c b/drivers/net/dsa/mv88e6xxx/switchdev.c index 4c346a884fb2..76f7f8cc1835 100644 --- a/drivers/net/dsa/mv88e6xxx/switchdev.c +++ b/drivers/net/dsa/mv88e6xxx/switchdev.c @@ -12,6 +12,25 @@ #include "global1.h" #include "switchdev.h" +void mv88e6xxx_set_fdb_offloaded(struct dsa_switch *ds, int port, + const unsigned char *addr, u16 vid) +{ + struct switchdev_notifier_fdb_info info = { + .addr = addr, + .vid = vid, + .offloaded = true, + }; + struct net_device *brport; + struct dsa_port *dp; + + dp = dsa_to_port(ds, port); + brport = dsa_port_to_bridge_port(dp); + + if (brport) + call_switchdev_notifiers(SWITCHDEV_FDB_OFFLOADED, + brport, &info.info, NULL); +} + struct mv88e6xxx_fid_search_ctx { u16 fid_search; u16 vid_found; @@ -81,3 +100,45 @@ int mv88e6xxx_handle_miss_violation(struct mv88e6xxx_chip *chip, int port, return err; } + +int mv88e6xxx_handle_age_out_violation(struct mv88e6xxx_chip *chip, int port, + struct mv88e6xxx_atu_entry *entry, + u16 fid) +{ + struct switchdev_notifier_fdb_info info = { + .addr = entry->mac, + }; + 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; + entry->portvec &= ~BIT(port); + entry->state = MV88E6XXX_G1_ATU_DATA_STATE_UC_UNUSED; + entry->trunk = false; + + mv88e6xxx_reg_lock(chip); + err = mv88e6xxx_g1_atu_loadpurge(chip, fid, entry); + mv88e6xxx_reg_unlock(chip); + if (err) + return err; + + 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_DEL_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 index 62214f9d62b0..5af6ac6a490a 100644 --- a/drivers/net/dsa/mv88e6xxx/switchdev.h +++ b/drivers/net/dsa/mv88e6xxx/switchdev.h @@ -12,8 +12,13 @@ #include "chip.h" +void mv88e6xxx_set_fdb_offloaded(struct dsa_switch *ds, int port, + const unsigned char *addr, u16 vid); int mv88e6xxx_handle_miss_violation(struct mv88e6xxx_chip *chip, int port, struct mv88e6xxx_atu_entry *entry, u16 fid); +int mv88e6xxx_handle_age_out_violation(struct mv88e6xxx_chip *chip, int port, + struct mv88e6xxx_atu_entry *entry, + u16 fid); #endif /* _MV88E6XXX_SWITCHDEV_H_ */ diff --git a/drivers/net/dsa/mv88e6xxx/trace.h b/drivers/net/dsa/mv88e6xxx/trace.h index f59ca04768e7..c6b32abf68a5 100644 --- a/drivers/net/dsa/mv88e6xxx/trace.h +++ b/drivers/net/dsa/mv88e6xxx/trace.h @@ -40,6 +40,11 @@ DECLARE_EVENT_CLASS(mv88e6xxx_atu_violation, __entry->addr, __entry->fid) ); +DEFINE_EVENT(mv88e6xxx_atu_violation, mv88e6xxx_atu_age_out_violation, + TP_PROTO(const struct device *dev, int spid, u16 portvec, + const unsigned char *addr, u16 fid), + TP_ARGS(dev, spid, portvec, addr, fid)); + DEFINE_EVENT(mv88e6xxx_atu_violation, mv88e6xxx_atu_member_violation, TP_PROTO(const struct device *dev, int spid, u16 portvec, const unsigned char *addr, u16 fid),
For 802.1X or MAB security authed hosts we want to have these hosts authed by adding dynamic FDB entries, so that if an authed host goes silent for a time period it's FDB entry will be removed and it must reauth when wanting to communicate again. In the mv88e6xxx offloaded case, we can use the HoldAt1 feature, that gives an age out interrupt when the FDB entry is about to age out, so that userspace can be notified of the entry being deleted with the help of an SWITCHDEV_FDB_DEL_TO_BRIDGE event. When adding a dynamic entry the bridge must be informed that the driver takes care of the ageing be sending an SWITCHDEV_FDB_OFFLOADED event, telling the bridge that this added FDB entry will be handled by the driver. With this implementation, trace events for age out interrupts are also added. note: A special case arises with the age out interrupt, as the entry state/spid (source port id) value read from the registers will be zero. Thus we need to extract the source port from the port vector instead. Signed-off-by: Hans J. Schultz <netdev@kapio-technology.com> --- drivers/net/dsa/mv88e6xxx/chip.c | 18 ++++++-- drivers/net/dsa/mv88e6xxx/global1_atu.c | 21 +++++++++ drivers/net/dsa/mv88e6xxx/port.c | 6 ++- drivers/net/dsa/mv88e6xxx/switchdev.c | 61 +++++++++++++++++++++++++ drivers/net/dsa/mv88e6xxx/switchdev.h | 5 ++ drivers/net/dsa/mv88e6xxx/trace.h | 5 ++ 6 files changed, 110 insertions(+), 6 deletions(-)