Thank you Scott. Please see inline.
> >> -----Original Message-----
> >> From: Scott Feldman [mailto:sfel...@gmail.com]
> >> Sent: Tuesday, August 18, 2015 12:48 PM
> >> To: Premkumar Jonnala
> >> Cc: netdev@vger.kernel.org
> >> Subject: Re: [PATCH] bridge: Enable configuration of ageing interval for
> bridges
> >> and switch devices.
> >>
> >>
> >>
> >> On Fri, 14 Aug 2015, Premkumar Jonnala wrote:
> >>
> >> > Bridge devices have ageing interval used to age out MAC addresses
> >> > from FDB. This ageing interval was not configuratble.
> >> >
> >> > Enable netlink based configuration of ageing interval for bridges and
> >> > switch devices. The ageing interval changes the timer used to purge
> >> > inactive FDB entries in bridges. The ageing interval config is
> >> > propagated to switch devices, so that platform or hardware based
> >> > ageing works according to configuration.
> >> >
> >> > Signed-off-by: Premkumar Jonnala <pjonn...@broadcom.com>
> >>
> >> Hi Premkumar,
> >>
> >> I agree with Roopa that we should use existing IFLA_BR_AGEING_TIME.
> >
> > What is the motivation for using 'ip link' command to configure bridge
> attributes? IMHO,
> > bridge command is better suited for that.
>
> Can you extend bridge command to allow setting/getting these bridge
> attrs? Looks like you construct a RTM_NEWLINK IFLA_INFO_DATA msg. No
> changes needed to the kernel.
>
> bridge link set dev br0 ageing_time 1000
>
> --or--
>
> ip link set dev br0 type bridge ageing_time 1000
I'd prefer to deprecate/remove all the 6 options on the 'ip link' command and
move them to 'bridge' command.
>
> >> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> >> index 0f2408f..01401ea 100644
> >> --- a/net/bridge/br_netlink.c
> >> +++ b/net/bridge/br_netlink.c
> >> @@ -759,9 +759,9 @@ static int br_changelink(struct net_device *brdev,
> struct
> >> nlattr *tb[],
> >> }
> >>
> >> if (data[IFLA_BR_AGEING_TIME]) {
> >> - u32 ageing_time = nla_get_u32(data[IFLA_BR_AGEING_TIME]);
> >
> > Should we do some range checking here to ensure that the value is within a
> certain range.
> > IEEE 802.1d recommends that the ageing time be between 10 sec and 1 million
> seconds.
>
> Sure, but make that a separate patch.
Sure.
>
> >> +int br_set_ageing_time(struct net_bridge *br, u32 ageing_time)
> >> +{
> >> + struct switchdev_attr attr = {
> >> + .id = SWITCHDEV_ATTR_BRIDGE,
> >> + .flags = SWITCHDEV_F_SKIP_EOPNOTSUPP,
> >> + .u.bridge.attr = IFLA_BR_AGEING_TIME,
> >> + .u.bridge.val = ageing_time,
> >> + };
> >> + int err;
> >> +
> >> + err = switchdev_port_attr_set(br->dev, &attr);
> >> + if (err)
> >> + return err;
> >> +
> >> + br->ageing_time = clock_t_to_jiffies(ageing_time);
> >
> > Should we restart the timer here the new time takes effect?
>
> I don't know...I just copied what the original code did. If it does
> need to be restarted, break that out as a separate patch.
There is another comment on this thread where it was suggested that we restart
the timer. I will
make the change in a separate patch.
-Prem
>
> -scott
