| Message ID | 20230318141010.513424-3-netdev@kapio-technology.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ATU and FDB synchronization on locked ports | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 37 this patch: 37 |
| netdev/cc_maintainers | success | CCed 12 of 12 maintainers |
| netdev/build_clang | success | Errors and warnings before: 24 this patch: 24 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 37 this patch: 37 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 277 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Sat, Mar 18, 2023 at 03:10:06PM +0100, Hans J. Schultz wrote: > diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c > index e5f156940c67..c07a2e225ae5 100644 > --- a/net/dsa/dsa.c > +++ b/net/dsa/dsa.c > @@ -626,6 +626,12 @@ static int dsa_switch_setup(struct dsa_switch *ds) > > ds->configure_vlan_while_not_filtering = true; > > + /* Since dynamic FDB entries are legacy, all switch drivers should > + * support the flag at least by just installing a static entry and > + * letting the bridge age it. > + */ > + ds->supported_fdb_flags = DSA_FDB_FLAG_DYNAMIC; I believe that switchdev has a structural problem in the fact that FDB entries with flags that aren't interpreted by drivers (so they don't know if those flags are set or unset) are still passed to the switchdev notifier chains by default. I don't believe that anybody used 'bridge fdb add <mac> <dev> master dynamic" while relying on a static FDB entry in the DSA offloaded data path. Just like commit 6ab4c3117aec ("net: bridge: don't notify switchdev for local FDB addresses"), we could deny that for stable kernels, and add the correct interpretation of the flag in net-next. Ido, Nikolay, Roopa, Jiri, thoughts? > + > err = ds->ops->setup(ds); > if (err < 0) > goto unregister_notifier; By the way, there is a behavior change here. Before: $ ip link add br0 type bridge && ip link set br0 up $ ip link set swp0 master br0 && ip link set swp0 up $ bridge fdb add dev swp0 00:01:02:03:04:05 master dynamic [ 70.010181] mscc_felix 0000:00:00.5: felix_fdb_add: port 0 addr 00:01:02:03:04:05 vid 0 [ 70.019105] mscc_felix 0000:00:00.5: felix_fdb_add: port 0 addr 00:01:02:03:04:05 vid 1 .... 5 minutes later [ 371.686935] mscc_felix 0000:00:00.5: felix_fdb_del: port 0 addr 00:01:02:03:04:05 vid 1 [ 371.695449] mscc_felix 0000:00:00.5: felix_fdb_del: port 0 addr 00:01:02:03:04:05 vid 0 $ bridge fdb | grep 00:01:02:03:04:05 After: $ ip link add br0 type bridge && ip link set br0 up $ ip link set swp0 master br0 && ip link set swp0 up $ bridge fdb add dev swp0 00:01:02:03:04:05 master dynamic [ 222.071492] mscc_felix 0000:00:00.5: felix_fdb_add: port 0 addr 00:01:02:03:04:05 vid 0 flags 0x1 [ 222.081154] mscc_felix 0000:00:00.5: felix_fdb_add: port 0 addr 00:01:02:03:04:05 vid 1 flags 0x1 .... 5 minutes later $ bridge fdb | grep 00:01:02:03:04:05 00:01:02:03:04:05 dev swp0 vlan 1 offload master br0 stale 00:01:02:03:04:05 dev swp0 offload master br0 stale 00:01:02:03:04:05 dev swp0 vlan 1 self 00:01:02:03:04:05 dev swp0 self As you can see, the behavior is not identical, and it made more sense before.
On Mon, Mar 27, 2023 at 14:52, Vladimir Oltean <olteanv@gmail.com> wrote: > > By the way, there is a behavior change here. > > Before: > > $ ip link add br0 type bridge && ip link set br0 up > $ ip link set swp0 master br0 && ip link set swp0 up > $ bridge fdb add dev swp0 00:01:02:03:04:05 master dynamic > [ 70.010181] mscc_felix 0000:00:00.5: felix_fdb_add: port 0 addr 00:01:02:03:04:05 vid 0 > [ 70.019105] mscc_felix 0000:00:00.5: felix_fdb_add: port 0 addr 00:01:02:03:04:05 vid 1 > .... 5 minutes later > [ 371.686935] mscc_felix 0000:00:00.5: felix_fdb_del: port 0 addr 00:01:02:03:04:05 vid 1 > [ 371.695449] mscc_felix 0000:00:00.5: felix_fdb_del: port 0 addr 00:01:02:03:04:05 vid 0 > $ bridge fdb | grep 00:01:02:03:04:05 > > After: > > $ ip link add br0 type bridge && ip link set br0 up > $ ip link set swp0 master br0 && ip link set swp0 up > $ bridge fdb add dev swp0 00:01:02:03:04:05 master dynamic > [ 222.071492] mscc_felix 0000:00:00.5: felix_fdb_add: port 0 addr 00:01:02:03:04:05 vid 0 flags 0x1 > [ 222.081154] mscc_felix 0000:00:00.5: felix_fdb_add: port 0 addr 00:01:02:03:04:05 vid 1 flags 0x1 > .... 5 minutes later > $ bridge fdb | grep 00:01:02:03:04:05 > 00:01:02:03:04:05 dev swp0 vlan 1 offload master br0 stale > 00:01:02:03:04:05 dev swp0 offload master br0 stale > 00:01:02:03:04:05 dev swp0 vlan 1 self > 00:01:02:03:04:05 dev swp0 self > > As you can see, the behavior is not identical, and it made more sense > before. I see this is Felix Ocelot and there is no changes in this patchset that affects Felix Ocelot. Thus I am quite sure the results will be the same without this patchset, ergo it must be because of another patch. All that is done here in the DSA layer is to pass on an extra field and add an extra check that will always pass in the case of this flag.
On Mon, Mar 27, 2023 at 05:31:26PM +0200, Hans Schultz wrote: > On Mon, Mar 27, 2023 at 14:52, Vladimir Oltean <olteanv@gmail.com> wrote: > > > > By the way, there is a behavior change here. > > > > Before: > > > > $ ip link add br0 type bridge && ip link set br0 up > > $ ip link set swp0 master br0 && ip link set swp0 up > > $ bridge fdb add dev swp0 00:01:02:03:04:05 master dynamic > > [ 70.010181] mscc_felix 0000:00:00.5: felix_fdb_add: port 0 addr 00:01:02:03:04:05 vid 0 > > [ 70.019105] mscc_felix 0000:00:00.5: felix_fdb_add: port 0 addr 00:01:02:03:04:05 vid 1 > > .... 5 minutes later > > [ 371.686935] mscc_felix 0000:00:00.5: felix_fdb_del: port 0 addr 00:01:02:03:04:05 vid 1 > > [ 371.695449] mscc_felix 0000:00:00.5: felix_fdb_del: port 0 addr 00:01:02:03:04:05 vid 0 > > $ bridge fdb | grep 00:01:02:03:04:05 > > > > After: > > > > $ ip link add br0 type bridge && ip link set br0 up > > $ ip link set swp0 master br0 && ip link set swp0 up > > $ bridge fdb add dev swp0 00:01:02:03:04:05 master dynamic > > [ 222.071492] mscc_felix 0000:00:00.5: felix_fdb_add: port 0 addr 00:01:02:03:04:05 vid 0 flags 0x1 > > [ 222.081154] mscc_felix 0000:00:00.5: felix_fdb_add: port 0 addr 00:01:02:03:04:05 vid 1 flags 0x1 > > .... 5 minutes later > > $ bridge fdb | grep 00:01:02:03:04:05 > > 00:01:02:03:04:05 dev swp0 vlan 1 offload master br0 stale > > 00:01:02:03:04:05 dev swp0 offload master br0 stale > > 00:01:02:03:04:05 dev swp0 vlan 1 self > > 00:01:02:03:04:05 dev swp0 self > > > > As you can see, the behavior is not identical, and it made more sense > > before. > > I see this is Felix Ocelot and there is no changes in this patchset that > affects Felix Ocelot. Thus I am quite sure the results will be the same > without this patchset, ergo it must be because of another patch. All > that is done here in the DSA layer is to pass on an extra field and add > an extra check that will always pass in the case of this flag. If mv88e6xxx is all you have, you can still sanity-check the equivalent effect of your patch set to other drivers by simply not acting upon the "flags" argument in mv88e6xxx_port_fdb_add()/mv88e6xxx_port_fdb_del(), and disabling the logic to treat Age Out interrupts. Then you should be able to notice exactly the behavior change I am talking about. In your own commit message, it says: Author: Hans J. Schultz <netdev@kapio-technology.com> 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 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ existing drivers do not emit this event. Signed-off-by: Hans J. Schultz <netdev@kapio-technology.com> 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; A reasonable question you could ask yourself is: why do my BR_FDB_OFFLOADED entries have this flag in the software bridge in the first place? Did I add code for it? Is it because there is some difference between mv88e6xxx and ocelot/felix, or is it because dsa_fdb_offload_notify() gets called in both cases from generic code just the same? And if dsa_fdb_offload_notify() gets called in both cases just the same, but no other driver except for mv88e6xxx emits the SWITCHDEV_FDB_DEL_TO_BRIDGE which you've patched the bridge to expect in this series, then what exactly is surprising in the fact that offloaded and dynamic FDB entries now become stale, but are not removed from the software bridge as they were before?
On Mon, Mar 27, 2023 at 19:00, Vladimir Oltean <olteanv@gmail.com> wrote: > A reasonable question you could ask yourself is: why do my BR_FDB_OFFLOADED > entries have this flag in the software bridge in the first place? > Did I add code for it? Is it because there is some difference between > mv88e6xxx and ocelot/felix, or is it because dsa_fdb_offload_notify() > gets called in both cases from generic code just the same? > > And if dsa_fdb_offload_notify() gets called in both cases just the same, > but no other driver except for mv88e6xxx emits the SWITCHDEV_FDB_DEL_TO_BRIDGE > which you've patched the bridge to expect in this series, then what exactly > is surprising in the fact that offloaded and dynamic FDB entries now become > stale, but are not removed from the software bridge as they were before? Yes, I see I have missed that the dsa layer already adds the offloaded flag in dsa_slave_switchdev_event_work() in slave.c. My first approach was to use the SWITCHDEV_FDB_ADD_TO_BRIDGE event and not the SWITCHDEV_FDB_OFFLOADED event as the first would set the external learned flag which is not aged out by the bridge. I have at some point earlier asked why there would be two quite equivalent flags and what the difference between them are, but I didn't get a response. Now I see the difference and that I cannot use the offloaded flag without changing the behaviour of the system as I actually change the behaviour of the offloaded flag in this version of the patch-set. So if the idea of a 'synthetically' learned fdb entry from the driver using the SWITCHDEV_FDB_ADD_TO_BRIDGE event from the driver towards the bridge instead is accepted, I can go with that? (thus removing all the changes in the patch-set regarding the offloaded flag ofcourse)
On Mon, Mar 27, 2023 at 11:49:58PM +0200, Hans Schultz wrote: > My first approach was to use the SWITCHDEV_FDB_ADD_TO_BRIDGE event > and not the SWITCHDEV_FDB_OFFLOADED event as the first would set the > external learned flag which is not aged out by the bridge. Link to patch? I don't see any SWITCHDEV_FDB_ADD_TO_BRIDGE call in either the v1: https://lore.kernel.org/netdev/20230130173429.3577450-6-netdev@kapio-technology.com/ or the RFC: https://lore.kernel.org/netdev/20230117185714.3058453-6-netdev@kapio-technology.com/ and the change log does not mention it either. > I have at some point earlier asked why there would be two quite > equivalent flags and what the difference between them are, but I didn't > get a response. Actually, the part which you are now posing as a question (what is the difference?) was part of the premise of your earlier question (there is no difference => why do we have both?). https://lore.kernel.org/netdev/d972e76bed896b229d9df4da81ad8eb4@kapio-technology.com/ I believe that no one answered because the question was confused and it wasn't really clear what you were asking. > > Now I see the difference and that I cannot use the offloaded flag > without changing the behaviour of the system as I actually change the > behaviour of the offloaded flag in this version of the patch-set. > > So if the idea of a 'synthetically' learned fdb entry from the driver > using the SWITCHDEV_FDB_ADD_TO_BRIDGE event from the driver towards the > bridge instead is accepted, I can go with that? > (thus removing all the changes in the patch-set regarding the offloaded > flag ofcourse) which idea is that, again?
On Tue, Mar 28, 2023 at 01:59, Vladimir Oltean <olteanv@gmail.com> wrote: > > which idea is that, again? So I cannot us the offloaded flag as it is added by DSA in the common case when using 'bridge fdb replace ... dynamic'. The idea is then to use the ext_learn flag instead, which is not aged by the bridge. To do this the driver (mv88e6xxx) will send a SWITCHDEV_FDB_ADD_TO_BRIDGE switchdev event when the new dynamic flag is true. The function sending this event will then be named mv88e6xxx_add_fdb_synth_learned() in drivers/net/dsa/mv88e6xxx/switchdev.c, replacing the mv88e6xxx_set_fdb_offloaded() function but in most part the same content, just another event type.
On Tue, Mar 28, 2023 at 01:04:23PM +0200, Hans Schultz wrote: > On Tue, Mar 28, 2023 at 01:59, Vladimir Oltean <olteanv@gmail.com> wrote: > > > > which idea is that, again? > > So I cannot us the offloaded flag as it is added by DSA in the common > case when using 'bridge fdb replace ... dynamic'. Why not? I find it reasonable that the software bridge does not age out a dynamic FDB entry that is offloaded to hardware... the hardware should do that ("dynamic" being the key). At least, I find it more reasonable than the current behavior, where the bridge notifies dynamic FDB entries to switchdev, but doesn't say they're dynamic, and switchdev treats them as static, so they don't roam from one bridge port to another until software sees a packet with that MAC DA, and they have the potential of blocking traffic because of that. If for some reason you do think that behavior is useful and still want to keep it (I'm not sure I would), I would consider extending struct switchdev_notifier_fdb_info with a "bool pls_dont_age_out", and I would make dsa_fdb_offload_notify() set this to true if the driver did actually install the dynamic FDB entry as dynamic in the ATU. > > The idea is then to use the ext_learn flag instead, which is not aged by > the bridge. To do this the driver (mv88e6xxx) will send a > SWITCHDEV_FDB_ADD_TO_BRIDGE switchdev event when the new dynamic flag is > true. The function sending this event will then be named > mv88e6xxx_add_fdb_synth_learned() in > drivers/net/dsa/mv88e6xxx/switchdev.c, replacing the > mv88e6xxx_set_fdb_offloaded() function but in most part the same > content, just another event type. Basically you're suggesting that the hardware driver, after receiving a SWITCHDEV_FDB_ADD_TO_DEVICE and responding to it with SWITCHDEV_FDB_OFFLOADED, emits a SWITCHDEV_FDB_ADD_TO_BRIDGE which takes over that software bridge FDB entry, with the advantage that the new one already has the semantics of not being aged out by the software bridge. hmmm... I'd say that the flow should work even with a single notifier emitted from the driver side, which would be SWITCHDEV_FDB_OFFLOADED, perhaps annotated with some qualifiers that would inform the bridge a certain behavior is required. Although, as mentioned, I think that in principle, "pls_dont_age_out" should be unnecessary, because it just papers over the issue that switchdev drivers treat static and dynamic FDB entries just the same, and "pls_dont_age_out" would be the differentiator for an issue that should have been solved elsewhere, as it could lead to other problems of its own. Basically we're designing around a workaround to a problem to which we're turning a blind eye. These are my 2c.
On Tue, Mar 28, 2023 at 14:49, Vladimir Oltean <olteanv@gmail.com> wrote: > On Tue, Mar 28, 2023 at 01:04:23PM +0200, Hans Schultz wrote: >> On Tue, Mar 28, 2023 at 01:59, Vladimir Oltean <olteanv@gmail.com> wrote: >> > >> > which idea is that, again? >> >> So I cannot us the offloaded flag as it is added by DSA in the common >> case when using 'bridge fdb replace ... dynamic'. > > Why not? I find it reasonable that the software bridge does not age out > a dynamic FDB entry that is offloaded to hardware... the hardware should > do that ("dynamic" being the key). So the solution would be to not let the DSA layer send the SWITCHDEV_FDB_OFFLOADED event in the case when the new dynamic flag is set? Thus other drivers that don't support the flag yet will install a static entry in HW and the bridge will age it out as there is no offloaded flag on. For the mv88e6xxx it will set the offloaded flag and HW will age it. > At least, I find it more reasonable > than the current behavior, where the bridge notifies dynamic FDB entries > to switchdev, but doesn't say they're dynamic, and switchdev treats them > as static, so they don't roam from one bridge port to another until > software sees a packet with that MAC DA, and they have the potential of > blocking traffic because of that. >
On Tue, Mar 28, 2023 at 09:45:26PM +0200, Hans Schultz wrote: > So the solution would be to not let the DSA layer send the > SWITCHDEV_FDB_OFFLOADED event in the case when the new dynamic flag is > set? I have never said that.
On Thu, Mar 30, 2023 at 15:43, Vladimir Oltean <olteanv@gmail.com> wrote: > On Tue, Mar 28, 2023 at 09:45:26PM +0200, Hans Schultz wrote: >> So the solution would be to not let the DSA layer send the >> SWITCHDEV_FDB_OFFLOADED event in the case when the new dynamic flag is >> set? > > I have never said that. No, I was just thinking of a solution based on your previous comment that dynamic fdb entries with the offloaded flag set should not be aged out by the bridge as they are now.
On Thu, Mar 30, 2023 at 02:59:04PM +0200, Hans Schultz wrote: > On Thu, Mar 30, 2023 at 15:43, Vladimir Oltean <olteanv@gmail.com> wrote: > > On Tue, Mar 28, 2023 at 09:45:26PM +0200, Hans Schultz wrote: > >> So the solution would be to not let the DSA layer send the > >> SWITCHDEV_FDB_OFFLOADED event in the case when the new dynamic flag is > >> set? > > > > I have never said that. > > No, I was just thinking of a solution based on your previous comment > that dynamic fdb entries with the offloaded flag set should not be aged > out by the bridge as they are now. If you were a user of those other drivers, and you ran the command: "bridge fdb add ... master dynamic" would you be ok with the behavior: "I don't have dynamic FDB entries, but here's a static one for you"?
On Thu, Mar 30, 2023 at 16:09, Vladimir Oltean <olteanv@gmail.com> wrote: > On Thu, Mar 30, 2023 at 02:59:04PM +0200, Hans Schultz wrote: >> On Thu, Mar 30, 2023 at 15:43, Vladimir Oltean <olteanv@gmail.com> wrote: >> > On Tue, Mar 28, 2023 at 09:45:26PM +0200, Hans Schultz wrote: >> >> So the solution would be to not let the DSA layer send the >> >> SWITCHDEV_FDB_OFFLOADED event in the case when the new dynamic flag is >> >> set? >> > >> > I have never said that. >> >> No, I was just thinking of a solution based on your previous comment >> that dynamic fdb entries with the offloaded flag set should not be aged >> out by the bridge as they are now. > > If you were a user of those other drivers, and you ran the command: > "bridge fdb add ... master dynamic" > would you be ok with the behavior: "I don't have dynamic FDB entries, > but here's a static one for you"? I don't know if you have a solution in mind wrt the behaviour of the offloaded flag if it is not to do as it does now and let the bridge age out dynamic entries. That led me to conclude that this patch-set cannot use the offloaded flag, but you seem to suggest otherwise. If you have a suggestion, feel free.
On Thu, Mar 30, 2023 at 04:54:19PM +0200, Hans Schultz wrote: > I don't know if you have a solution in mind wrt the behaviour of the > offloaded flag if it is not to do as it does now and let the bridge age > out dynamic entries. That led me to conclude that this patch-set cannot > use the offloaded flag, but you seem to suggest otherwise. > > If you have a suggestion, feel free. Didn't I explain what I would do from the first reply on this thread? https://patchwork.kernel.org/project/netdevbpf/patch/20230318141010.513424-3-netdev@kapio-technology.com/#25270613 As a bug fix, stop reporting to switchdev those FDB entries with BR_FDB_ADDED_BY_USER && !BR_FDB_STATIC. Then, after "net" is merged into "net-next" next Thursday (the ship has sailed for today), add "bool static" to the switchdev notifier info, and make all switchdev drivers (everywhere where a SWITCHDEV_FDB_ADD_TO_DEVICE handler appears) ignore the "added_by_user && !is_static" combination, but by their own choice and not by switchdev's choice. Then, make DSA decide whether to handle the "added_by_user && !is_static" combination or not, based on the presence of the DSA_FDB_FLAG_DYNAMIC flag, which will be set in ds->supported_fdb_flags only for the mv88e6xxx driver. When DSA_FDB_FLAG_DYNAMIC is not supported, DSA will not offload the FDB entry: neither will it call port_fdb_add(), nor will it emit SWITCHDEV_FDB_OFFLOADED. Ideally, it would also inform user space that it can't offload this flag by returning an error, but the lack of an error propagation mechanism from switchdev to the bridge FDB is a known limitation which is hard to overcome, and is outside the scope of your patchset I believe. To see whether DSA has acted upon the "master dynamic" flag or not, it would be good enough for the user to see something adequate in "bridge fdb show | grep offloaded", I believe.
On Thu, Mar 30, 2023 at 18:07, Vladimir Oltean <olteanv@gmail.com> wrote: > > Then, make DSA decide whether to handle the "added_by_user && !is_static" > combination or not, based on the presence of the DSA_FDB_FLAG_DYNAMIC > flag, which will be set in ds->supported_fdb_flags only for the mv88e6xxx > driver. Okay, so this will require a new function in the DSA layer that sets which flags are supported and that the driver will call on initialization. Where (in the DSA layer) should such a function be placed and what should it be called?
On Thu, Mar 30, 2023 at 05:34:44PM +0200, Hans Schultz wrote: > On Thu, Mar 30, 2023 at 18:07, Vladimir Oltean <olteanv@gmail.com> wrote: > > > > Then, make DSA decide whether to handle the "added_by_user && !is_static" > > combination or not, based on the presence of the DSA_FDB_FLAG_DYNAMIC > > flag, which will be set in ds->supported_fdb_flags only for the mv88e6xxx > > driver. > > Okay, so this will require a new function in the DSA layer that sets > which flags are supported and that the driver will call on > initialization. > > Where (in the DSA layer) should such a function be placed and what > should it be called? Don't overthink it, no new function. It's okay to just set ds->supported_fdb_flags = DSA_FDB_FLAG_DYNAMIC in mv88e6xxx_register_switch(), near the place where it currently sets ds->num_lag_ids. Either before dsa_register_switch(), or within the ds->ops->setup(). Both are fine, since the user network interfaces haven't been allocated just yet by dsa_slave_create() and so, the switchdev code path is inaccessible. Existing drivers will have ds->supported_fdb_flags = 0 by default, since they allocate the struct dsa_switch with kzalloc(), and DSA will have to do something sane with that.
On Thu, Mar 30, 2023 at 18:07, Vladimir Oltean <olteanv@gmail.com> wrote: > As a bug fix, stop reporting to switchdev those FDB entries with > BR_FDB_ADDED_BY_USER && !BR_FDB_STATIC. Then, after "net" is merged into > "net-next" next Thursday (the ship has sailed for today), add "bool static" It is probably too late today (now I have a Debian based VM that can do the selftests), but with this bug fix I have 1) not submitted bug fixes before and 2) it probably needs an appropriate explanation, where I don't know the problem well enough for general switchcores to submit with a suitable text.
On Thu, Apr 06, 2023 at 05:17:46PM +0200, Hans Schultz wrote: > On Thu, Mar 30, 2023 at 18:07, Vladimir Oltean <olteanv@gmail.com> wrote: > > As a bug fix, stop reporting to switchdev those FDB entries with > > BR_FDB_ADDED_BY_USER && !BR_FDB_STATIC. Then, after "net" is merged into > > "net-next" next Thursday (the ship has sailed for today), add "bool static" > > It is probably too late today (now I have a Debian based VM that can do > the selftests), but with this bug fix I have 1) not submitted bug fixes > before and 2) it probably needs an appropriate explanation, where I > don't know the problem well enough for general switchcores to submit > with a suitable text. Do you want me to try to submit this change as a bug fix?
On Thu, Apr 06, 2023 at 18:21, Vladimir Oltean <olteanv@gmail.com> wrote: > On Thu, Apr 06, 2023 at 05:17:46PM +0200, Hans Schultz wrote: >> On Thu, Mar 30, 2023 at 18:07, Vladimir Oltean <olteanv@gmail.com> wrote: >> > As a bug fix, stop reporting to switchdev those FDB entries with >> > BR_FDB_ADDED_BY_USER && !BR_FDB_STATIC. Then, after "net" is merged into >> > "net-next" next Thursday (the ship has sailed for today), add "bool static" >> >> It is probably too late today (now I have a Debian based VM that can do >> the selftests), but with this bug fix I have 1) not submitted bug fixes >> before and 2) it probably needs an appropriate explanation, where I >> don't know the problem well enough for general switchcores to submit >> with a suitable text. > > Do you want me to try to submit this change as a bug fix? I think that would be fine as you would know the matter best.
diff --git a/include/net/dsa.h b/include/net/dsa.h index a15f17a38eca..9e98c4fe1520 100644 --- a/include/net/dsa.h +++ b/include/net/dsa.h @@ -437,6 +437,9 @@ struct dsa_switch { */ u32 fdb_isolation:1; + /* Supported fdb flags going from the bridge to drivers */ + u16 supported_fdb_flags; + /* Listener for switch fabric events */ struct notifier_block nb; @@ -818,6 +821,8 @@ static inline bool dsa_port_tree_same(const struct dsa_port *a, return a->ds->dst == b->ds->dst; } +#define DSA_FDB_FLAG_DYNAMIC BIT(0) + typedef int dsa_fdb_dump_cb_t(const unsigned char *addr, u16 vid, bool is_static, void *data); struct dsa_switch_ops { diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c index e5f156940c67..c07a2e225ae5 100644 --- a/net/dsa/dsa.c +++ b/net/dsa/dsa.c @@ -626,6 +626,12 @@ static int dsa_switch_setup(struct dsa_switch *ds) ds->configure_vlan_while_not_filtering = true; + /* Since dynamic FDB entries are legacy, all switch drivers should + * support the flag at least by just installing a static entry and + * letting the bridge age it. + */ + ds->supported_fdb_flags = DSA_FDB_FLAG_DYNAMIC; + err = ds->ops->setup(ds); if (err < 0) goto unregister_notifier; diff --git a/net/dsa/port.c b/net/dsa/port.c index 67ad1adec2a2..9a7c1265546d 100644 --- a/net/dsa/port.c +++ b/net/dsa/port.c @@ -976,12 +976,13 @@ int dsa_port_mtu_change(struct dsa_port *dp, int new_mtu) } int dsa_port_fdb_add(struct dsa_port *dp, const unsigned char *addr, - u16 vid) + u16 vid, u16 flags) { struct dsa_notifier_fdb_info info = { .dp = dp, .addr = addr, .vid = vid, + .flags = flags, .db = { .type = DSA_DB_BRIDGE, .bridge = *dp->bridge, @@ -999,12 +1000,13 @@ int dsa_port_fdb_add(struct dsa_port *dp, const unsigned char *addr, } int dsa_port_fdb_del(struct dsa_port *dp, const unsigned char *addr, - u16 vid) + u16 vid, u16 flags) { struct dsa_notifier_fdb_info info = { .dp = dp, .addr = addr, .vid = vid, + .flags = flags, .db = { .type = DSA_DB_BRIDGE, .bridge = *dp->bridge, @@ -1019,12 +1021,13 @@ int dsa_port_fdb_del(struct dsa_port *dp, const unsigned char *addr, static int dsa_port_host_fdb_add(struct dsa_port *dp, const unsigned char *addr, u16 vid, - struct dsa_db db) + u16 flags, struct dsa_db db) { struct dsa_notifier_fdb_info info = { .dp = dp, .addr = addr, .vid = vid, + .flags = flags, .db = db, }; @@ -1042,11 +1045,11 @@ int dsa_port_standalone_host_fdb_add(struct dsa_port *dp, .dp = dp, }; - return dsa_port_host_fdb_add(dp, addr, vid, db); + return dsa_port_host_fdb_add(dp, addr, vid, 0, db); } -int dsa_port_bridge_host_fdb_add(struct dsa_port *dp, - const unsigned char *addr, u16 vid) +int dsa_port_bridge_host_fdb_add(struct dsa_port *dp, const unsigned char *addr, + u16 vid, u16 flags) { struct net_device *master = dsa_port_to_master(dp); struct dsa_db db = { @@ -1065,17 +1068,18 @@ int dsa_port_bridge_host_fdb_add(struct dsa_port *dp, return err; } - return dsa_port_host_fdb_add(dp, addr, vid, db); + return dsa_port_host_fdb_add(dp, addr, vid, flags, db); } static int dsa_port_host_fdb_del(struct dsa_port *dp, const unsigned char *addr, u16 vid, - struct dsa_db db) + u16 flags, struct dsa_db db) { struct dsa_notifier_fdb_info info = { .dp = dp, .addr = addr, .vid = vid, + .flags = flags, .db = db, }; @@ -1093,11 +1097,11 @@ int dsa_port_standalone_host_fdb_del(struct dsa_port *dp, .dp = dp, }; - return dsa_port_host_fdb_del(dp, addr, vid, db); + return dsa_port_host_fdb_del(dp, addr, vid, 0, db); } -int dsa_port_bridge_host_fdb_del(struct dsa_port *dp, - const unsigned char *addr, u16 vid) +int dsa_port_bridge_host_fdb_del(struct dsa_port *dp, const unsigned char *addr, + u16 vid, u16 flags) { struct net_device *master = dsa_port_to_master(dp); struct dsa_db db = { @@ -1112,7 +1116,7 @@ int dsa_port_bridge_host_fdb_del(struct dsa_port *dp, return err; } - return dsa_port_host_fdb_del(dp, addr, vid, db); + return dsa_port_host_fdb_del(dp, addr, vid, flags, db); } int dsa_port_lag_fdb_add(struct dsa_port *dp, const unsigned char *addr, diff --git a/net/dsa/port.h b/net/dsa/port.h index 9c218660d223..88a9dfed3bce 100644 --- a/net/dsa/port.h +++ b/net/dsa/port.h @@ -47,17 +47,17 @@ int dsa_port_vlan_msti(struct dsa_port *dp, const struct switchdev_vlan_msti *msti); int dsa_port_mtu_change(struct dsa_port *dp, int new_mtu); int dsa_port_fdb_add(struct dsa_port *dp, const unsigned char *addr, - u16 vid); + u16 vid, u16 flags); int dsa_port_fdb_del(struct dsa_port *dp, const unsigned char *addr, - u16 vid); + u16 vid, u16 flags); int dsa_port_standalone_host_fdb_add(struct dsa_port *dp, const unsigned char *addr, u16 vid); int dsa_port_standalone_host_fdb_del(struct dsa_port *dp, const unsigned char *addr, u16 vid); int dsa_port_bridge_host_fdb_add(struct dsa_port *dp, const unsigned char *addr, - u16 vid); + u16 vid, u16 flags); int dsa_port_bridge_host_fdb_del(struct dsa_port *dp, const unsigned char *addr, - u16 vid); + u16 vid, u16 flags); int dsa_port_lag_fdb_add(struct dsa_port *dp, const unsigned char *addr, u16 vid); int dsa_port_lag_fdb_del(struct dsa_port *dp, const unsigned char *addr, diff --git a/net/dsa/slave.c b/net/dsa/slave.c index 6957971c2db2..20d2d1c000ea 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -39,6 +39,7 @@ struct dsa_switchdev_event_work { */ unsigned char addr[ETH_ALEN]; u16 vid; + u16 fdb_flags; bool host_addr; }; @@ -3331,6 +3332,7 @@ static void dsa_slave_switchdev_event_work(struct work_struct *work) container_of(work, struct dsa_switchdev_event_work, work); const unsigned char *addr = switchdev_work->addr; struct net_device *dev = switchdev_work->dev; + u16 flags = switchdev_work->fdb_flags; u16 vid = switchdev_work->vid; struct dsa_switch *ds; struct dsa_port *dp; @@ -3342,11 +3344,12 @@ static void dsa_slave_switchdev_event_work(struct work_struct *work) switch (switchdev_work->event) { case SWITCHDEV_FDB_ADD_TO_DEVICE: if (switchdev_work->host_addr) - err = dsa_port_bridge_host_fdb_add(dp, addr, vid); + err = dsa_port_bridge_host_fdb_add(dp, addr, + vid, flags); else if (dp->lag) err = dsa_port_lag_fdb_add(dp, addr, vid); else - err = dsa_port_fdb_add(dp, addr, vid); + err = dsa_port_fdb_add(dp, addr, vid, flags); if (err) { dev_err(ds->dev, "port %d failed to add %pM vid %d to fdb: %d\n", @@ -3358,11 +3361,12 @@ static void dsa_slave_switchdev_event_work(struct work_struct *work) case SWITCHDEV_FDB_DEL_TO_DEVICE: if (switchdev_work->host_addr) - err = dsa_port_bridge_host_fdb_del(dp, addr, vid); + err = dsa_port_bridge_host_fdb_del(dp, addr, + vid, flags); else if (dp->lag) err = dsa_port_lag_fdb_del(dp, addr, vid); else - err = dsa_port_fdb_del(dp, addr, vid); + err = dsa_port_fdb_del(dp, addr, vid, flags); if (err) { dev_err(ds->dev, "port %d failed to delete %pM vid %d from fdb: %d\n", @@ -3400,6 +3404,7 @@ static int dsa_slave_fdb_event(struct net_device *dev, struct dsa_port *dp = dsa_slave_to_port(dev); bool host_addr = fdb_info->is_local; struct dsa_switch *ds = dp->ds; + u16 flags = 0; if (ctx && ctx != dp) return 0; @@ -3437,6 +3442,12 @@ static int dsa_slave_fdb_event(struct net_device *dev, return -EOPNOTSUPP; } + if (fdb_info->is_dyn && fdb_info->added_by_user) + flags |= DSA_FDB_FLAG_DYNAMIC; + + if (flags & ~ds->supported_fdb_flags) + return 0; + switchdev_work = kzalloc(sizeof(*switchdev_work), GFP_ATOMIC); if (!switchdev_work) return -ENOMEM; @@ -3454,6 +3465,7 @@ static int dsa_slave_fdb_event(struct net_device *dev, ether_addr_copy(switchdev_work->addr, fdb_info->addr); switchdev_work->vid = fdb_info->vid; switchdev_work->host_addr = host_addr; + switchdev_work->fdb_flags = flags; dsa_schedule_work(&switchdev_work->work); diff --git a/net/dsa/switch.c b/net/dsa/switch.c index d5bc4bb7310d..0f5626a425b6 100644 --- a/net/dsa/switch.c +++ b/net/dsa/switch.c @@ -239,7 +239,7 @@ static int dsa_port_do_mdb_del(struct dsa_port *dp, } static int dsa_port_do_fdb_add(struct dsa_port *dp, const unsigned char *addr, - u16 vid, struct dsa_db db) + u16 vid, u16 flags, struct dsa_db db) { struct dsa_switch *ds = dp->ds; struct dsa_mac_addr *a; @@ -283,7 +283,7 @@ static int dsa_port_do_fdb_add(struct dsa_port *dp, const unsigned char *addr, } static int dsa_port_do_fdb_del(struct dsa_port *dp, const unsigned char *addr, - u16 vid, struct dsa_db db) + u16 vid, u16 flags, struct dsa_db db) { struct dsa_switch *ds = dp->ds; struct dsa_mac_addr *a; @@ -410,7 +410,9 @@ static int dsa_switch_host_fdb_add(struct dsa_switch *ds, info->db); } else { err = dsa_port_do_fdb_add(dp, info->addr, - info->vid, info->db); + info->vid, + info->flags, + info->db); } if (err) break; @@ -438,7 +440,9 @@ static int dsa_switch_host_fdb_del(struct dsa_switch *ds, info->db); } else { err = dsa_port_do_fdb_del(dp, info->addr, - info->vid, info->db); + info->vid, + info->flags, + info->db); } if (err) break; @@ -457,7 +461,8 @@ static int dsa_switch_fdb_add(struct dsa_switch *ds, if (!ds->ops->port_fdb_add) return -EOPNOTSUPP; - return dsa_port_do_fdb_add(dp, info->addr, info->vid, info->db); + return dsa_port_do_fdb_add(dp, info->addr, info->vid, + info->flags, info->db); } static int dsa_switch_fdb_del(struct dsa_switch *ds, @@ -469,7 +474,8 @@ static int dsa_switch_fdb_del(struct dsa_switch *ds, if (!ds->ops->port_fdb_del) return -EOPNOTSUPP; - return dsa_port_do_fdb_del(dp, info->addr, info->vid, info->db); + return dsa_port_do_fdb_del(dp, info->addr, info->vid, + info->flags, info->db); } static int dsa_switch_lag_fdb_add(struct dsa_switch *ds, diff --git a/net/dsa/switch.h b/net/dsa/switch.h index 15e67b95eb6e..2c3bf4020158 100644 --- a/net/dsa/switch.h +++ b/net/dsa/switch.h @@ -55,6 +55,7 @@ struct dsa_notifier_fdb_info { const struct dsa_port *dp; const unsigned char *addr; u16 vid; + u16 flags; struct dsa_db db; };
Dynamic FDB flag needs to be propagated through the DSA layer to be added to drivers. Use a u16 for fdb flags for future use, so that other flags can also be sent the same way without having to change function interfaces. If we have unsupported flags in the new flags, we do not do unnecessary work and so we return at once. This ensures that the drivers are not called with unsupported flags, so that the drivers do not need to check the new flags. As the dynamic flag is a legacy flag, all drivers support it by default at least as they have done hitherto. Ensure that the dynamic FDB flag is only set when added from userspace, as the feature it supports is to be handled from userspace. Signed-off-by: Hans J. Schultz <netdev@kapio-technology.com> --- include/net/dsa.h | 5 +++++ net/dsa/dsa.c | 6 ++++++ net/dsa/port.c | 28 ++++++++++++++++------------ net/dsa/port.h | 8 ++++---- net/dsa/slave.c | 20 ++++++++++++++++---- net/dsa/switch.c | 18 ++++++++++++------ net/dsa/switch.h | 1 + 7 files changed, 60 insertions(+), 26 deletions(-)