| Message ID | 20210318231829.3892920-8-olteanv@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Better support for sandwiched LAGs with bridge and DSA | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | fail | Series longer than 15 patches |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 1 maintainers not CCed: bridge@lists.linux-foundation.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | fail | Errors and warnings before: 61 this patch: 61 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 65 lines checked |
| netdev/build_allmodconfig_warn | fail | Errors and warnings before: 59 this patch: 59 |
| netdev/header_inline | success | Link |
On 3/18/2021 4:18 PM, Vladimir Oltean wrote: > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > The SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME attribute is only emitted from: > > sysfs/ioctl/netlink > -> br_set_ageing_time > -> __set_ageing_time > > therefore not at bridge port creation time, so: > (a) drivers had to hardcode the initial value for the address ageing time, > because they didn't get any notification > (b) that hardcoded value can be out of sync, if the user changes the > ageing time before enslaving the port to the bridge > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > --- > include/linux/if_bridge.h | 6 ++++++ > net/bridge/br_stp.c | 13 +++++++++++++ > net/dsa/port.c | 10 ++++++++++ > 3 files changed, 29 insertions(+) > > diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h > index 920d3a02cc68..ebd16495459c 100644 > --- a/include/linux/if_bridge.h > +++ b/include/linux/if_bridge.h > @@ -137,6 +137,7 @@ struct net_device *br_fdb_find_port(const struct net_device *br_dev, > void br_fdb_clear_offload(const struct net_device *dev, u16 vid); > bool br_port_flag_is_set(const struct net_device *dev, unsigned long flag); > u8 br_port_get_stp_state(const struct net_device *dev); > +clock_t br_get_ageing_time(struct net_device *br_dev); > #else > static inline struct net_device * > br_fdb_find_port(const struct net_device *br_dev, > @@ -160,6 +161,11 @@ static inline u8 br_port_get_stp_state(const struct net_device *dev) > { > return BR_STATE_DISABLED; > } > + > +static inline clock_t br_get_ageing_time(struct net_device *br_dev) > +{ > + return 0; > +} > #endif > > #endif > diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c > index 86b5e05d3f21..3dafb6143cff 100644 > --- a/net/bridge/br_stp.c > +++ b/net/bridge/br_stp.c > @@ -639,6 +639,19 @@ int br_set_ageing_time(struct net_bridge *br, clock_t ageing_time) > return 0; > } > > +clock_t br_get_ageing_time(struct net_device *br_dev) > +{ > + struct net_bridge *br; > + > + if (!netif_is_bridge_master(br_dev)) > + return 0; > + > + br = netdev_priv(br_dev); > + > + return jiffies_to_clock_t(br->ageing_time); Don't you want an ASSERT_RTNL() in this function as well?
On Fri, Mar 19, 2021 at 03:13:03PM -0700, Florian Fainelli wrote: > > diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c > > index 86b5e05d3f21..3dafb6143cff 100644 > > --- a/net/bridge/br_stp.c > > +++ b/net/bridge/br_stp.c > > @@ -639,6 +639,19 @@ int br_set_ageing_time(struct net_bridge *br, clock_t ageing_time) > > return 0; > > } > > > > +clock_t br_get_ageing_time(struct net_device *br_dev) > > +{ > > + struct net_bridge *br; > > + > > + if (!netif_is_bridge_master(br_dev)) > > + return 0; > > + > > + br = netdev_priv(br_dev); > > + > > + return jiffies_to_clock_t(br->ageing_time); > > Don't you want an ASSERT_RTNL() in this function as well? Hmm, I'm not sure. I don't think I'm accessing anything that is under the protection of the rtnl_mutex. If anything, the ageing time is protected by the "bridge lock", but I don't think there's much of an issue if I read an unsigned int while not holding it.
On Fri, Mar 19, 2021 at 01:18, Vladimir Oltean <olteanv@gmail.com> wrote: > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > The SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME attribute is only emitted from: > > sysfs/ioctl/netlink > -> br_set_ageing_time > -> __set_ageing_time > > therefore not at bridge port creation time, so: > (a) drivers had to hardcode the initial value for the address ageing time, > because they didn't get any notification > (b) that hardcoded value can be out of sync, if the user changes the > ageing time before enslaving the port to the bridge > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > --- Reviewed-by: Tobias Waldekranz <tobias@waldekranz.com>
diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h index 920d3a02cc68..ebd16495459c 100644 --- a/include/linux/if_bridge.h +++ b/include/linux/if_bridge.h @@ -137,6 +137,7 @@ struct net_device *br_fdb_find_port(const struct net_device *br_dev, void br_fdb_clear_offload(const struct net_device *dev, u16 vid); bool br_port_flag_is_set(const struct net_device *dev, unsigned long flag); u8 br_port_get_stp_state(const struct net_device *dev); +clock_t br_get_ageing_time(struct net_device *br_dev); #else static inline struct net_device * br_fdb_find_port(const struct net_device *br_dev, @@ -160,6 +161,11 @@ static inline u8 br_port_get_stp_state(const struct net_device *dev) { return BR_STATE_DISABLED; } + +static inline clock_t br_get_ageing_time(struct net_device *br_dev) +{ + return 0; +} #endif #endif diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c index 86b5e05d3f21..3dafb6143cff 100644 --- a/net/bridge/br_stp.c +++ b/net/bridge/br_stp.c @@ -639,6 +639,19 @@ int br_set_ageing_time(struct net_bridge *br, clock_t ageing_time) return 0; } +clock_t br_get_ageing_time(struct net_device *br_dev) +{ + struct net_bridge *br; + + if (!netif_is_bridge_master(br_dev)) + return 0; + + br = netdev_priv(br_dev); + + return jiffies_to_clock_t(br->ageing_time); +} +EXPORT_SYMBOL_GPL(br_get_ageing_time); + /* called under bridge lock */ void __br_set_topology_change(struct net_bridge *br, unsigned char val) { diff --git a/net/dsa/port.c b/net/dsa/port.c index 8380509ee47c..9fde2371e1bc 100644 --- a/net/dsa/port.c +++ b/net/dsa/port.c @@ -173,6 +173,7 @@ static int dsa_port_switchdev_sync(struct dsa_port *dp, { struct net_device *brport_dev = dsa_port_to_bridge_port(dp); struct net_device *br = dp->bridge_dev; + clock_t ageing_time; u8 stp_state; int err; @@ -193,6 +194,11 @@ static int dsa_port_switchdev_sync(struct dsa_port *dp, if (err && err != -EOPNOTSUPP) return err; + ageing_time = br_get_ageing_time(br); + err = dsa_port_ageing_time(dp, ageing_time); + if (err && err != -EOPNOTSUPP) + return err; + return 0; } @@ -222,6 +228,10 @@ static void dsa_port_switchdev_unsync(struct dsa_port *dp) * allow this in standalone mode too. */ dsa_port_mrouter(dp->cpu_dp, true, NULL); + + /* Ageing time may be global to the switch chip, so don't change it + * here because we have no good reason (or value) to change it to. + */ } int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br,