Re: [RFC PATCH v2 net-next 07/16] net: dsa: sync ageing time when joining the bridge

[Tobias Waldekranz]

On Fri, Mar 19, 2021 at 01:18, Vladimir Oltean  wrote:
> From: Vladimir Oltean 
>
> 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 
> ---

Reviewed-by: Tobias Waldekranz 

Re: [RFC PATCH v2 net-next 07/16] net: dsa: sync ageing time when joining the bridge

[Vladimir Oltean]

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.

Re: [RFC PATCH v2 net-next 07/16] net: dsa: sync ageing time when joining the bridge

[Florian Fainelli]

On 3/18/2021 4:18 PM, Vladimir Oltean wrote:
> From: Vladimir Oltean 
> 
> 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 
> ---
>  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?
-- 
Florian