| Message ID | 20220315002543.190587-10-tobias@waldekranz.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: bridge: Multiple Spanning Trees | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 7 of 7 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 9 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Tue, Mar 15, 2022 at 01:25:37AM +0100, Tobias Waldekranz wrote: > If a port joins a bridge that it can't offload, it will fallback to > standalone mode and software bridging. In this case, we never want to > offload any FDB entries to hardware either. > > Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> > --- When you resend, please send this patch separately, unless something breaks really ugly with your MST series in place. > net/dsa/slave.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/net/dsa/slave.c b/net/dsa/slave.c > index a61a7c54af20..647adee97f7f 100644 > --- a/net/dsa/slave.c > +++ b/net/dsa/slave.c > @@ -2624,6 +2624,9 @@ static int dsa_slave_fdb_event(struct net_device *dev, > if (ctx && ctx != dp) > return 0; > > + if (!dp->bridge) > + return 0; > + > if (switchdev_fdb_is_dynamically_learned(fdb_info)) { > if (dsa_port_offloads_bridge_port(dp, orig_dev)) > return 0; > -- > 2.25.1 >
On Tue, Mar 15, 2022 at 18:33, Vladimir Oltean <olteanv@gmail.com> wrote: > On Tue, Mar 15, 2022 at 01:25:37AM +0100, Tobias Waldekranz wrote: >> If a port joins a bridge that it can't offload, it will fallback to >> standalone mode and software bridging. In this case, we never want to >> offload any FDB entries to hardware either. >> >> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> >> --- > > When you resend, please send this patch separately, unless something > breaks really ugly with your MST series in place. Sure. I found this while testing the software fallback. It prevents a segfault in dsa_port_bridge_host_fdb_add, which (rightly, I think) assumes that dp->bridge is valid. I feel like this should have a Fixes: tag, but I'm not sure which commit to blame. Any suggestions? >> net/dsa/slave.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/net/dsa/slave.c b/net/dsa/slave.c >> index a61a7c54af20..647adee97f7f 100644 >> --- a/net/dsa/slave.c >> +++ b/net/dsa/slave.c >> @@ -2624,6 +2624,9 @@ static int dsa_slave_fdb_event(struct net_device *dev, >> if (ctx && ctx != dp) >> return 0; >> >> + if (!dp->bridge) >> + return 0; >> + >> if (switchdev_fdb_is_dynamically_learned(fdb_info)) { >> if (dsa_port_offloads_bridge_port(dp, orig_dev)) >> return 0; >> -- >> 2.25.1 >>
On Tue, Mar 15, 2022 at 11:26:59PM +0100, Tobias Waldekranz wrote: > On Tue, Mar 15, 2022 at 18:33, Vladimir Oltean <olteanv@gmail.com> wrote: > > On Tue, Mar 15, 2022 at 01:25:37AM +0100, Tobias Waldekranz wrote: > >> If a port joins a bridge that it can't offload, it will fallback to > >> standalone mode and software bridging. In this case, we never want to > >> offload any FDB entries to hardware either. > >> > >> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> > >> --- > > > > When you resend, please send this patch separately, unless something > > breaks really ugly with your MST series in place. > > Sure. I found this while testing the software fallback. It prevents a > segfault in dsa_port_bridge_host_fdb_add, which (rightly, I think) > assumes that dp->bridge is valid. I feel like this should have a Fixes: > tag, but I'm not sure which commit to blame. Any suggestions? Ok, makes sense. So far, unoffloaded bridge ports meant that the DSA switch driver didn't have a ->port_bridge_join() implementation. Presumably that also came along with a missing ->port_fdb_add() implementation. So probably no NPD for the existing code paths, it is just your unoffloaded MST support that opens up new possibilities. Anyway, the dereference of dp->bridge first appeared in commit c26933639b54 ("net: dsa: request drivers to perform FDB isolation") which is still just in net-next. > >> net/dsa/slave.c | 3 +++ > >> 1 file changed, 3 insertions(+) > >> > >> diff --git a/net/dsa/slave.c b/net/dsa/slave.c > >> index a61a7c54af20..647adee97f7f 100644 > >> --- a/net/dsa/slave.c > >> +++ b/net/dsa/slave.c > >> @@ -2624,6 +2624,9 @@ static int dsa_slave_fdb_event(struct net_device *dev, > >> if (ctx && ctx != dp) > >> return 0; > >> > >> + if (!dp->bridge) > >> + return 0; > >> + > >> if (switchdev_fdb_is_dynamically_learned(fdb_info)) { > >> if (dsa_port_offloads_bridge_port(dp, orig_dev)) > >> return 0; > >> -- > >> 2.25.1 > >>
On Wed, Mar 16, 2022 at 00:42, Vladimir Oltean <olteanv@gmail.com> wrote: > On Tue, Mar 15, 2022 at 11:26:59PM +0100, Tobias Waldekranz wrote: >> On Tue, Mar 15, 2022 at 18:33, Vladimir Oltean <olteanv@gmail.com> wrote: >> > On Tue, Mar 15, 2022 at 01:25:37AM +0100, Tobias Waldekranz wrote: >> >> If a port joins a bridge that it can't offload, it will fallback to >> >> standalone mode and software bridging. In this case, we never want to >> >> offload any FDB entries to hardware either. >> >> >> >> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> >> >> --- >> > >> > When you resend, please send this patch separately, unless something >> > breaks really ugly with your MST series in place. >> >> Sure. I found this while testing the software fallback. It prevents a >> segfault in dsa_port_bridge_host_fdb_add, which (rightly, I think) >> assumes that dp->bridge is valid. I feel like this should have a Fixes: >> tag, but I'm not sure which commit to blame. Any suggestions? > > Ok, makes sense. So far, unoffloaded bridge ports meant that the DSA > switch driver didn't have a ->port_bridge_join() implementation. > Presumably that also came along with a missing ->port_fdb_add() > implementation. So probably no NPD for the existing code paths, it is > just your unoffloaded MST support that opens up new possibilities. > > Anyway, the dereference of dp->bridge first appeared in commit > c26933639b54 ("net: dsa: request drivers to perform FDB isolation") > which is still just in net-next. Thanks, I just sent it separately: https://lore.kernel.org/netdev/20220315225018.1399269-1-tobias@waldekranz.com >> >> net/dsa/slave.c | 3 +++ >> >> 1 file changed, 3 insertions(+) >> >> >> >> diff --git a/net/dsa/slave.c b/net/dsa/slave.c >> >> index a61a7c54af20..647adee97f7f 100644 >> >> --- a/net/dsa/slave.c >> >> +++ b/net/dsa/slave.c >> >> @@ -2624,6 +2624,9 @@ static int dsa_slave_fdb_event(struct net_device *dev, >> >> if (ctx && ctx != dp) >> >> return 0; >> >> >> >> + if (!dp->bridge) >> >> + return 0; >> >> + >> >> if (switchdev_fdb_is_dynamically_learned(fdb_info)) { >> >> if (dsa_port_offloads_bridge_port(dp, orig_dev)) >> >> return 0; >> >> -- >> >> 2.25.1 >> >>
diff --git a/net/dsa/slave.c b/net/dsa/slave.c index a61a7c54af20..647adee97f7f 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -2624,6 +2624,9 @@ static int dsa_slave_fdb_event(struct net_device *dev, if (ctx && ctx != dp) return 0; + if (!dp->bridge) + return 0; + if (switchdev_fdb_is_dynamically_learned(fdb_info)) { if (dsa_port_offloads_bridge_port(dp, orig_dev)) return 0;
If a port joins a bridge that it can't offload, it will fallback to standalone mode and software bridging. In this case, we never want to offload any FDB entries to hardware either. Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> --- net/dsa/slave.c | 3 +++ 1 file changed, 3 insertions(+)