2023-01-17 21:06:40

by Hans Schultz

[permalink] [raw]
Subject: [RFC PATCH net-next 0/5] ATU and FDB synchronization on locked ports

This patch set makes it possible to have synchronized dynamic ATU and FDB
entries on locked ports. As locked ports are not able to automatically
learn, they depend on userspace added entries, where userspace can add
static or dynamic entries. The lifetime of static entries are completely
dependent on userspace intervention, and thus not of interest here. We
are only concerned with dynamic entries, which can be added with a
command like:

bridge fdb replace ADDR dev <DEV> master dynamic

We choose only to support this feature on locked ports, as it involves
utilizing the CPU to handle ATU related switchcore events (typically
interrupts) and thus can result in significant performance loss if
exposed to heavy traffic.

On locked ports it is important for userspace to know when an authorized
station has become silent, hence not breaking the communication of a
station that has been authorized based on the MAC-Authentication Bypass
(MAB) scheme. Thus if the station keeps being active after authorization,
it will continue to have an open port as long as it is active. Only after
a silent period will it have to be reauthorized. As the ageing process in
the ATU is dependent on incoming traffic to the switchcore port, it is
necessary for the ATU to signal that an entry has aged out, so that the
FDB can be updated at the correct time.

This patch set includes a solution for the Marvell mv88e6xxx driver, where
for this driver we use the Hold-At-One feature so that an age-out
violation interrupt occurs when a station has been silent for the
system-set age time. The age out violation interrupt allows the switchcore
driver to remove both the ATU and the FDB entry at the same time.

It is up to the maintainers of other switchcore drivers to implement the
feature for their specific driver.

Hans J. Schultz (5):
net: bridge: add dynamic flag to switchdev notifier
net: dsa: propagate flags down towards drivers
drivers: net: dsa: add fdb entry flags incoming to switchcore drivers
net: bridge: ensure FDB offloaded flag is handled as needed
net: dsa: mv88e6xxx: implementation of dynamic ATU entries

drivers/net/dsa/b53/b53_common.c | 12 ++++-
drivers/net/dsa/b53/b53_priv.h | 4 +-
drivers/net/dsa/hirschmann/hellcreek.c | 12 ++++-
drivers/net/dsa/lan9303-core.c | 12 ++++-
drivers/net/dsa/lantiq_gswip.c | 12 ++++-
drivers/net/dsa/microchip/ksz9477.c | 8 ++--
drivers/net/dsa/microchip/ksz9477.h | 8 ++--
drivers/net/dsa/microchip/ksz_common.c | 14 ++++--
drivers/net/dsa/mt7530.c | 12 ++++-
drivers/net/dsa/mv88e6xxx/chip.c | 24 ++++++++--
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 ++
drivers/net/dsa/ocelot/felix.c | 12 ++++-
drivers/net/dsa/qca/qca8k-common.c | 12 ++++-
drivers/net/dsa/qca/qca8k.h | 4 +-
drivers/net/dsa/rzn1_a5psw.c | 12 ++++-
drivers/net/dsa/sja1105/sja1105_main.c | 19 ++++++--
include/net/dsa.h | 6 ++-
include/net/switchdev.h | 1 +
net/bridge/br_fdb.c | 5 +-
net/bridge/br_switchdev.c | 1 +
net/dsa/port.c | 28 +++++++-----
net/dsa/port.h | 8 ++--
net/dsa/slave.c | 17 +++++--
net/dsa/switch.c | 30 ++++++++----
net/dsa/switch.h | 1 +
29 files changed, 298 insertions(+), 74 deletions(-)

--
2.34.1


2023-01-17 21:22:43

by Hans Schultz

[permalink] [raw]
Subject: [RFC PATCH net-next 1/5] net: bridge: add dynamic flag to switchdev notifier

To be able to add dynamic FDB entries to drivers from userspace, the
dynamic flag must be added when sending RTM_NEWNEIGH events down.

Signed-off-by: Hans J. Schultz <[email protected]>
---
include/net/switchdev.h | 1 +
net/bridge/br_switchdev.c | 1 +
2 files changed, 2 insertions(+)

diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index ca0312b78294..aaf918d4ba67 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -249,6 +249,7 @@ struct switchdev_notifier_fdb_info {
u8 added_by_user:1,
is_local:1,
locked:1,
+ is_dyn:1,
offloaded:1;
};

diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
index 7eb6fd5bb917..60c05a00a1df 100644
--- a/net/bridge/br_switchdev.c
+++ b/net/bridge/br_switchdev.c
@@ -136,6 +136,7 @@ static void br_switchdev_fdb_populate(struct net_bridge *br,
item->added_by_user = test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags);
item->offloaded = test_bit(BR_FDB_OFFLOADED, &fdb->flags);
item->is_local = test_bit(BR_FDB_LOCAL, &fdb->flags);
+ item->is_dyn = !test_bit(BR_FDB_STATIC, &fdb->flags);
item->locked = false;
item->info.dev = (!p || item->is_local) ? br->dev : p->dev;
item->info.ctx = ctx;
--
2.34.1

2023-01-17 21:23:01

by Hans Schultz

[permalink] [raw]
Subject: [RFC PATCH net-next 5/5] net: dsa: mv88e6xxx: implementation of dynamic ATU entries

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 <[email protected]>
---
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 a9c84b92a10e..e5d64622a629 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),
--
2.34.1

2023-01-17 21:27:48

by Hans Schultz

[permalink] [raw]
Subject: [RFC PATCH net-next 4/5] net: bridge: ensure FDB offloaded flag is handled as needed

Since user added entries in the bridge FDB will get the BR_FDB_OFFLOADED
flag set, we do not want the bridge to age those entries and we want the
entries to be deleted in the bridge upon an SWITCHDEV_FDB_DEL_TO_BRIDGE
event.

Signed-off-by: Hans J. Schultz <[email protected]>
---
net/bridge/br_fdb.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index e69a872bfc1d..b0c23a72bc76 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -537,6 +537,7 @@ void br_fdb_cleanup(struct work_struct *work)
unsigned long this_timer = f->updated + delay;

if (test_bit(BR_FDB_STATIC, &f->flags) ||
+ test_bit(BR_FDB_OFFLOADED, &f->flags) ||
test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &f->flags)) {
if (test_bit(BR_FDB_NOTIFY, &f->flags)) {
if (time_after(this_timer, now))
@@ -1465,7 +1466,9 @@ int br_fdb_external_learn_del(struct net_bridge *br, struct net_bridge_port *p,
spin_lock_bh(&br->hash_lock);

fdb = br_fdb_find(br, addr, vid);
- if (fdb && test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags))
+ if (fdb &&
+ (test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags) ||
+ test_bit(BR_FDB_OFFLOADED, &fdb->flags)))
fdb_delete(br, fdb, swdev_notify);
else
err = -ENOENT;
--
2.34.1

2023-01-18 00:53:20

by Vladimir Oltean

[permalink] [raw]
Subject: Re: [RFC PATCH net-next 1/5] net: bridge: add dynamic flag to switchdev notifier

On Tue, Jan 17, 2023 at 07:57:10PM +0100, Hans J. Schultz wrote:
> To be able to add dynamic FDB entries to drivers from userspace, the
> dynamic flag must be added when sending RTM_NEWNEIGH events down.
>
> Signed-off-by: Hans J. Schultz <[email protected]>
> ---
> include/net/switchdev.h | 1 +
> net/bridge/br_switchdev.c | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/include/net/switchdev.h b/include/net/switchdev.h
> index ca0312b78294..aaf918d4ba67 100644
> --- a/include/net/switchdev.h
> +++ b/include/net/switchdev.h
> @@ -249,6 +249,7 @@ struct switchdev_notifier_fdb_info {
> u8 added_by_user:1,
> is_local:1,
> locked:1,
> + is_dyn:1,
> offloaded:1;
> };
>
> diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
> index 7eb6fd5bb917..60c05a00a1df 100644
> --- a/net/bridge/br_switchdev.c
> +++ b/net/bridge/br_switchdev.c
> @@ -136,6 +136,7 @@ static void br_switchdev_fdb_populate(struct net_bridge *br,
> item->added_by_user = test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags);
> item->offloaded = test_bit(BR_FDB_OFFLOADED, &fdb->flags);
> item->is_local = test_bit(BR_FDB_LOCAL, &fdb->flags);
> + item->is_dyn = !test_bit(BR_FDB_STATIC, &fdb->flags);

Why reverse logic? Why not just name this "is_static" and leave any
further interpretations up to the consumer?

> item->locked = false;
> item->info.dev = (!p || item->is_local) ? br->dev : p->dev;
> item->info.ctx = ctx;
> --
> 2.34.1
>

2023-01-18 22:31:04

by Hans Schultz

[permalink] [raw]
Subject: Re: [RFC PATCH net-next 1/5] net: bridge: add dynamic flag to switchdev notifier

On 2023-01-18 00:08, Vladimir Oltean wrote:
> On Tue, Jan 17, 2023 at 07:57:10PM +0100, Hans J. Schultz wrote:
>> To be able to add dynamic FDB entries to drivers from userspace, the
>> dynamic flag must be added when sending RTM_NEWNEIGH events down.
>>
>> Signed-off-by: Hans J. Schultz <[email protected]>
>> ---
>> include/net/switchdev.h | 1 +
>> net/bridge/br_switchdev.c | 1 +
>> 2 files changed, 2 insertions(+)
>>
>> diff --git a/include/net/switchdev.h b/include/net/switchdev.h
>> index ca0312b78294..aaf918d4ba67 100644
>> --- a/include/net/switchdev.h
>> +++ b/include/net/switchdev.h
>> @@ -249,6 +249,7 @@ struct switchdev_notifier_fdb_info {
>> u8 added_by_user:1,
>> is_local:1,
>> locked:1,
>> + is_dyn:1,
>> offloaded:1;
>> };
>>
>> diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
>> index 7eb6fd5bb917..60c05a00a1df 100644
>> --- a/net/bridge/br_switchdev.c
>> +++ b/net/bridge/br_switchdev.c
>> @@ -136,6 +136,7 @@ static void br_switchdev_fdb_populate(struct
>> net_bridge *br,
>> item->added_by_user = test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags);
>> item->offloaded = test_bit(BR_FDB_OFFLOADED, &fdb->flags);
>> item->is_local = test_bit(BR_FDB_LOCAL, &fdb->flags);
>> + item->is_dyn = !test_bit(BR_FDB_STATIC, &fdb->flags);
>
> Why reverse logic? Why not just name this "is_static" and leave any
> further interpretations up to the consumer?
>

My reasoning for this is that the common case is to have static entries,
thus is_dyn=false, so whenever someone uses a
switchdev_notifier_fdb_info struct the common case does not need to be
entered.
Otherwise it might also break something when someone uses this struct
and if it was 'is_static' and they forget to code is_static=true they
will get dynamic entries without wanting it and it can be hard to find
such an error.

>> item->locked = false;
>> item->info.dev = (!p || item->is_local) ? br->dev : p->dev;
>> item->info.ctx = ctx;
>> --
>> 2.34.1
>>

2023-01-19 09:46:45

by Vladimir Oltean

[permalink] [raw]
Subject: Re: [RFC PATCH net-next 1/5] net: bridge: add dynamic flag to switchdev notifier

On Wed, Jan 18, 2023 at 11:14:00PM +0100, [email protected] wrote:
> > > + item->is_dyn = !test_bit(BR_FDB_STATIC, &fdb->flags);
> >
> > Why reverse logic? Why not just name this "is_static" and leave any
> > further interpretations up to the consumer?
>
> My reasoning for this is that the common case is to have static entries,
> thus is_dyn=false, so whenever someone uses a switchdev_notifier_fdb_info
> struct the common case does not need to be entered.
> Otherwise it might also break something when someone uses this struct and if
> it was 'is_static' and they forget to code is_static=true they will get
> dynamic entries without wanting it and it can be hard to find such an error.

I'll leave it up to bridge maintainers if this is preferable to patching
all callers of SWITCHDEV_FDB_ADD_TO_BRIDGE such that they set is_static=true.

2023-01-19 14:28:35

by Vladimir Oltean

[permalink] [raw]
Subject: Re: [RFC PATCH net-next 1/5] net: bridge: add dynamic flag to switchdev notifier

On Thu, Jan 19, 2023 at 11:33:58AM +0200, Vladimir Oltean wrote:
> On Wed, Jan 18, 2023 at 11:14:00PM +0100, [email protected] wrote:
> > > > + item->is_dyn = !test_bit(BR_FDB_STATIC, &fdb->flags);
> > >
> > > Why reverse logic? Why not just name this "is_static" and leave any
> > > further interpretations up to the consumer?
> >
> > My reasoning for this is that the common case is to have static entries,
> > thus is_dyn=false, so whenever someone uses a switchdev_notifier_fdb_info
> > struct the common case does not need to be entered.
> > Otherwise it might also break something when someone uses this struct and if
> > it was 'is_static' and they forget to code is_static=true they will get
> > dynamic entries without wanting it and it can be hard to find such an error.
>
> I'll leave it up to bridge maintainers if this is preferable to patching
> all callers of SWITCHDEV_FDB_ADD_TO_BRIDGE such that they set is_static=true.

Actually, why would you assume that all users of SWITCHDEV_FDB_ADD_TO_BRIDGE
want to add static FDB entries? You can't avoid inspecting the code and
making sure that the is_dyn/is_static flag is set correctly either way.

2023-01-20 21:24:20

by Hans Schultz

[permalink] [raw]
Subject: Re: [RFC PATCH net-next 1/5] net: bridge: add dynamic flag to switchdev notifier

On 2023-01-19 14:40, Vladimir Oltean wrote:
> On Thu, Jan 19, 2023 at 11:33:58AM +0200, Vladimir Oltean wrote:
>> On Wed, Jan 18, 2023 at 11:14:00PM +0100, [email protected]
>> wrote:
>> > > > + item->is_dyn = !test_bit(BR_FDB_STATIC, &fdb->flags);
>> > >
>> > > Why reverse logic? Why not just name this "is_static" and leave any
>> > > further interpretations up to the consumer?
>> >
>> > My reasoning for this is that the common case is to have static entries,
>> > thus is_dyn=false, so whenever someone uses a switchdev_notifier_fdb_info
>> > struct the common case does not need to be entered.
>> > Otherwise it might also break something when someone uses this struct and if
>> > it was 'is_static' and they forget to code is_static=true they will get
>> > dynamic entries without wanting it and it can be hard to find such an error.
>>
>> I'll leave it up to bridge maintainers if this is preferable to
>> patching
>> all callers of SWITCHDEV_FDB_ADD_TO_BRIDGE such that they set
>> is_static=true.
>
> Actually, why would you assume that all users of
> SWITCHDEV_FDB_ADD_TO_BRIDGE
> want to add static FDB entries? You can't avoid inspecting the code and
> making sure that the is_dyn/is_static flag is set correctly either way.

Well, up until this patch set there is no option, besides entries from
SWITCHDEV_FDB_ADD_TO_BRIDGE events will get the external learned flag
set, so they will not be aged by the bridge, and so dynamic entries that
way don't make much sense I think. Is that not right?