diff mbox

[net-next,v3,3/4] bridge: push bridge setting ageing_time down to switchdev

Message ID 1444357400-37078-4-git-send-email-sfeldma@gmail.com
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Scott Feldman Oct. 9, 2015, 2:23 a.m. UTC 
From: Scott Feldman <sfeldma@gmail.com>

Use SWITCHDEV_F_SKIP_EOPNOTSUPP to skip over ports in bridge that don't
support setting ageing_time (or setting bridge attrs in general).

If push fails, don't update ageing_time in bridge and return err to user.

If push succeeds, update ageing_time in bridge and run gc_timer now to
recalabrate when to run gc_timer next, based on new ageing_time.

Signed-off-by: Scott Feldman <sfeldma@gmail.com>
Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
 net/bridge/br_ioctl.c    |    3 +--
 net/bridge/br_netlink.c  |    6 +++---
 net/bridge/br_private.h  |    1 +
 net/bridge/br_stp.c      |   23 +++++++++++++++++++++++
 net/bridge/br_sysfs_br.c |    3 +--
 5 files changed, 29 insertions(+), 7 deletions(-)

Comments

Florian Fainelli Oct. 9, 2015, 2:40 a.m. UTC | #1 
2015-10-08 19:23 GMT-07:00  <sfeldma@gmail.com>:
> From: Scott Feldman <sfeldma@gmail.com>
>
> Use SWITCHDEV_F_SKIP_EOPNOTSUPP to skip over ports in bridge that don't
> support setting ageing_time (or setting bridge attrs in general).
>
> If push fails, don't update ageing_time in bridge and return err to user.
>
> If push succeeds, update ageing_time in bridge and run gc_timer now to
> recalabrate when to run gc_timer next, based on new ageing_time.
>
> Signed-off-by: Scott Feldman <sfeldma@gmail.com>
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> ---
>  net/bridge/br_ioctl.c    |    3 +--
>  net/bridge/br_netlink.c  |    6 +++---
>  net/bridge/br_private.h  |    1 +
>  net/bridge/br_stp.c      |   23 +++++++++++++++++++++++
>  net/bridge/br_sysfs_br.c |    3 +--
>  5 files changed, 29 insertions(+), 7 deletions(-)
>
> diff --git a/net/bridge/br_ioctl.c b/net/bridge/br_ioctl.c
> index 8d423bc..263b4de 100644
> --- a/net/bridge/br_ioctl.c
> +++ b/net/bridge/br_ioctl.c
> @@ -200,8 +200,7 @@ static int old_dev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
>                 if (!ns_capable(dev_net(dev)->user_ns, CAP_NET_ADMIN))
>                         return -EPERM;
>
> -               br->ageing_time = clock_t_to_jiffies(args[1]);
> -               return 0;
> +               return br_set_ageing_time(br, args[1]);
>
>         case BRCTL_GET_PORT_INFO:
>         {
> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> index d78b442..544ab96 100644
> --- a/net/bridge/br_netlink.c
> +++ b/net/bridge/br_netlink.c
> @@ -870,9 +870,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]);
> -
> -               br->ageing_time = clock_t_to_jiffies(ageing_time);
> +               err = br_set_ageing_time(br, nla_get_u32(data[IFLA_BR_AGEING_TIME]));
> +               if (err)
> +                       return err;
>         }
>
>         if (data[IFLA_BR_STP_STATE]) {
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 09d3ecb..ba0c67b 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -882,6 +882,7 @@ void __br_set_forward_delay(struct net_bridge *br, unsigned long t);
>  int br_set_forward_delay(struct net_bridge *br, unsigned long x);
>  int br_set_hello_time(struct net_bridge *br, unsigned long x);
>  int br_set_max_age(struct net_bridge *br, unsigned long x);
> +int br_set_ageing_time(struct net_bridge *br, u32 ageing_time);
>
>
>  /* br_stp_if.c */
> diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
> index 3a982c0..db6d243de 100644
> --- a/net/bridge/br_stp.c
> +++ b/net/bridge/br_stp.c
> @@ -566,6 +566,29 @@ int br_set_max_age(struct net_bridge *br, unsigned long val)
>
>  }
>
> +int br_set_ageing_time(struct net_bridge *br, u32 ageing_time)
> +{
> +       struct switchdev_attr attr = {
> +               .id = SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME,
> +               .flags = SWITCHDEV_F_SKIP_EOPNOTSUPP,
> +               .u.ageing_time = ageing_time,
> +       };
> +       unsigned long t = clock_t_to_jiffies(ageing_time);
> +       int err;
> +
> +       if (t < BR_MIN_AGEING_TIME || t > BR_MAX_AGEING_TIME)
> +               return -ERANGE;
> +
> +       err = switchdev_port_attr_set(br->dev, &attr);
> +       if (err)
> +               return err;
> +
> +       br->ageing_time = t;
> +       mod_timer(&br->gc_timer, jiffies);

If the switch driver/HW supports ageing, does it still make sense to
have this software timer ticking?
--
Florian
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Scott Feldman Oct. 9, 2015, 3:31 a.m. UTC | #2 
On Thu, Oct 8, 2015 at 7:40 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> 2015-10-08 19:23 GMT-07:00  <sfeldma@gmail.com>:
>> From: Scott Feldman <sfeldma@gmail.com>
>>
>> Use SWITCHDEV_F_SKIP_EOPNOTSUPP to skip over ports in bridge that don't
>> support setting ageing_time (or setting bridge attrs in general).
>>
>> If push fails, don't update ageing_time in bridge and return err to user.
>>
>> If push succeeds, update ageing_time in bridge and run gc_timer now to
>> recalabrate when to run gc_timer next, based on new ageing_time.
>>
>> Signed-off-by: Scott Feldman <sfeldma@gmail.com>
>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>> ---
>>  net/bridge/br_ioctl.c    |    3 +--
>>  net/bridge/br_netlink.c  |    6 +++---
>>  net/bridge/br_private.h  |    1 +
>>  net/bridge/br_stp.c      |   23 +++++++++++++++++++++++
>>  net/bridge/br_sysfs_br.c |    3 +--
>>  5 files changed, 29 insertions(+), 7 deletions(-)
>>
>> diff --git a/net/bridge/br_ioctl.c b/net/bridge/br_ioctl.c
>> index 8d423bc..263b4de 100644
>> --- a/net/bridge/br_ioctl.c
>> +++ b/net/bridge/br_ioctl.c
>> @@ -200,8 +200,7 @@ static int old_dev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
>>                 if (!ns_capable(dev_net(dev)->user_ns, CAP_NET_ADMIN))
>>                         return -EPERM;
>>
>> -               br->ageing_time = clock_t_to_jiffies(args[1]);
>> -               return 0;
>> +               return br_set_ageing_time(br, args[1]);
>>
>>         case BRCTL_GET_PORT_INFO:
>>         {
>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
>> index d78b442..544ab96 100644
>> --- a/net/bridge/br_netlink.c
>> +++ b/net/bridge/br_netlink.c
>> @@ -870,9 +870,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]);
>> -
>> -               br->ageing_time = clock_t_to_jiffies(ageing_time);
>> +               err = br_set_ageing_time(br, nla_get_u32(data[IFLA_BR_AGEING_TIME]));
>> +               if (err)
>> +                       return err;
>>         }
>>
>>         if (data[IFLA_BR_STP_STATE]) {
>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>> index 09d3ecb..ba0c67b 100644
>> --- a/net/bridge/br_private.h
>> +++ b/net/bridge/br_private.h
>> @@ -882,6 +882,7 @@ void __br_set_forward_delay(struct net_bridge *br, unsigned long t);
>>  int br_set_forward_delay(struct net_bridge *br, unsigned long x);
>>  int br_set_hello_time(struct net_bridge *br, unsigned long x);
>>  int br_set_max_age(struct net_bridge *br, unsigned long x);
>> +int br_set_ageing_time(struct net_bridge *br, u32 ageing_time);
>>
>>
>>  /* br_stp_if.c */
>> diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
>> index 3a982c0..db6d243de 100644
>> --- a/net/bridge/br_stp.c
>> +++ b/net/bridge/br_stp.c
>> @@ -566,6 +566,29 @@ int br_set_max_age(struct net_bridge *br, unsigned long val)
>>
>>  }
>>
>> +int br_set_ageing_time(struct net_bridge *br, u32 ageing_time)
>> +{
>> +       struct switchdev_attr attr = {
>> +               .id = SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME,
>> +               .flags = SWITCHDEV_F_SKIP_EOPNOTSUPP,
>> +               .u.ageing_time = ageing_time,
>> +       };
>> +       unsigned long t = clock_t_to_jiffies(ageing_time);
>> +       int err;
>> +
>> +       if (t < BR_MIN_AGEING_TIME || t > BR_MAX_AGEING_TIME)
>> +               return -ERANGE;
>> +
>> +       err = switchdev_port_attr_set(br->dev, &attr);
>> +       if (err)
>> +               return err;
>> +
>> +       br->ageing_time = t;
>> +       mod_timer(&br->gc_timer, jiffies);
>
> If the switch driver/HW supports ageing, does it still make sense to
> have this software timer ticking?

Yes, because the bridge still needs to age out entries it has learned
(those not marked with added_by_external_learn), for example entries
learned on non-offloaded ports.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jiri Pirko Oct. 9, 2015, 4:28 a.m. UTC | #3 
Fri, Oct 09, 2015 at 04:23:19AM CEST, sfeldma@gmail.com wrote:
>From: Scott Feldman <sfeldma@gmail.com>
>
>Use SWITCHDEV_F_SKIP_EOPNOTSUPP to skip over ports in bridge that don't
>support setting ageing_time (or setting bridge attrs in general).
>
>If push fails, don't update ageing_time in bridge and return err to user.
>
>If push succeeds, update ageing_time in bridge and run gc_timer now to
>recalabrate when to run gc_timer next, based on new ageing_time.
>
>Signed-off-by: Scott Feldman <sfeldma@gmail.com>
>Signed-off-by: Jiri Pirko <jiri@resnulli.us>

Acked-by: Jiri Pirko <jiri@mellanox.com>
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Premkumar Jonnala Oct. 9, 2015, 4:38 a.m. UTC | #4 
> -----Original Message-----
> From: sfeldma@gmail.com [mailto:sfeldma@gmail.com]
> Sent: Friday, October 09, 2015 7:53 AM
> To: netdev@vger.kernel.org
> Cc: davem@davemloft.net; jiri@resnulli.us; siva.mannem.lnx@gmail.com;
> Premkumar Jonnala; stephen@networkplumber.org;
> roopa@cumulusnetworks.com; andrew@lunn.ch; f.fainelli@gmail.com;
> vivien.didelot@savoirfairelinux.com
> Subject: [PATCH net-next v3 3/4] bridge: push bridge setting ageing_time down
> to switchdev
> 
> From: Scott Feldman <sfeldma@gmail.com>
> 
> Use SWITCHDEV_F_SKIP_EOPNOTSUPP to skip over ports in bridge that don't
> support setting ageing_time (or setting bridge attrs in general).
> 
> If push fails, don't update ageing_time in bridge and return err to user.
> 
> If push succeeds, update ageing_time in bridge and run gc_timer now to
> recalabrate when to run gc_timer next, based on new ageing_time.
> 
> Signed-off-by: Scott Feldman <sfeldma@gmail.com>
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> ---
>  net/bridge/br_ioctl.c    |    3 +--
>  net/bridge/br_netlink.c  |    6 +++---
>  net/bridge/br_private.h  |    1 +
>  net/bridge/br_stp.c      |   23 +++++++++++++++++++++++
>  net/bridge/br_sysfs_br.c |    3 +--
>  5 files changed, 29 insertions(+), 7 deletions(-)
> 
> diff --git a/net/bridge/br_ioctl.c b/net/bridge/br_ioctl.c
> index 8d423bc..263b4de 100644
> --- a/net/bridge/br_ioctl.c
> +++ b/net/bridge/br_ioctl.c
> @@ -200,8 +200,7 @@ static int old_dev_ioctl(struct net_device *dev, struct
> ifreq *rq, int cmd)
>  		if (!ns_capable(dev_net(dev)->user_ns, CAP_NET_ADMIN))
>  			return -EPERM;
> 
> -		br->ageing_time = clock_t_to_jiffies(args[1]);
> -		return 0;
> +		return br_set_ageing_time(br, args[1]);
> 
>  	case BRCTL_GET_PORT_INFO:
>  	{
> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> index d78b442..544ab96 100644
> --- a/net/bridge/br_netlink.c
> +++ b/net/bridge/br_netlink.c
> @@ -870,9 +870,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]);
> -
> -		br->ageing_time = clock_t_to_jiffies(ageing_time);
> +		err = br_set_ageing_time(br,
> nla_get_u32(data[IFLA_BR_AGEING_TIME]));
> +		if (err)
> +			return err;
>  	}
> 
>  	if (data[IFLA_BR_STP_STATE]) {
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 09d3ecb..ba0c67b 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -882,6 +882,7 @@ void __br_set_forward_delay(struct net_bridge *br,
> unsigned long t);
>  int br_set_forward_delay(struct net_bridge *br, unsigned long x);
>  int br_set_hello_time(struct net_bridge *br, unsigned long x);
>  int br_set_max_age(struct net_bridge *br, unsigned long x);
> +int br_set_ageing_time(struct net_bridge *br, u32 ageing_time);
> 
> 
>  /* br_stp_if.c */
> diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
> index 3a982c0..db6d243de 100644
> --- a/net/bridge/br_stp.c
> +++ b/net/bridge/br_stp.c
> @@ -566,6 +566,29 @@ int br_set_max_age(struct net_bridge *br, unsigned
> long val)
> 
>  }
> 
> +int br_set_ageing_time(struct net_bridge *br, u32 ageing_time)
> +{
> +	struct switchdev_attr attr = {
> +		.id = SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME,
> +		.flags = SWITCHDEV_F_SKIP_EOPNOTSUPP,
> +		.u.ageing_time = ageing_time,
> +	};
> +	unsigned long t = clock_t_to_jiffies(ageing_time);
> +	int err;
> +
> +	if (t < BR_MIN_AGEING_TIME || t > BR_MAX_AGEING_TIME)
> +		return -ERANGE;
> +
> +	err = switchdev_port_attr_set(br->dev, &attr);

A thought - given that the ageing time is not a per-bridge-port attr, why are we using a "port based api"
to pass the attribute down?  May be I'm missing something here?

-Prem


> +	if (err)
> +		return err;
> +
> +	br->ageing_time = t;
> +	mod_timer(&br->gc_timer, jiffies);
> +
> +	return 0;
> +}
> +
>  void __br_set_forward_delay(struct net_bridge *br, unsigned long t)
>  {
>  	br->bridge_forward_delay = t;
> diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
> index 4c97fc5..04ef192 100644
> --- a/net/bridge/br_sysfs_br.c
> +++ b/net/bridge/br_sysfs_br.c
> @@ -102,8 +102,7 @@ static ssize_t ageing_time_show(struct device *d,
> 
>  static int set_ageing_time(struct net_bridge *br, unsigned long val)
>  {
> -	br->ageing_time = clock_t_to_jiffies(val);
> -	return 0;
> +	return br_set_ageing_time(br, val);
>  }
> 
>  static ssize_t ageing_time_store(struct device *d,
> --
> 1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jiri Pirko Oct. 9, 2015, 6:41 a.m. UTC | #5 
Fri, Oct 09, 2015 at 06:38:10AM CEST, pjonnala@broadcom.com wrote:
>
>
>> -----Original Message-----
>> From: sfeldma@gmail.com [mailto:sfeldma@gmail.com]
>> Sent: Friday, October 09, 2015 7:53 AM
>> To: netdev@vger.kernel.org
>> Cc: davem@davemloft.net; jiri@resnulli.us; siva.mannem.lnx@gmail.com;
>> Premkumar Jonnala; stephen@networkplumber.org;
>> roopa@cumulusnetworks.com; andrew@lunn.ch; f.fainelli@gmail.com;
>> vivien.didelot@savoirfairelinux.com
>> Subject: [PATCH net-next v3 3/4] bridge: push bridge setting ageing_time down
>> to switchdev
>> 
>> From: Scott Feldman <sfeldma@gmail.com>
>> 
>> Use SWITCHDEV_F_SKIP_EOPNOTSUPP to skip over ports in bridge that don't
>> support setting ageing_time (or setting bridge attrs in general).
>> 
>> If push fails, don't update ageing_time in bridge and return err to user.
>> 
>> If push succeeds, update ageing_time in bridge and run gc_timer now to
>> recalabrate when to run gc_timer next, based on new ageing_time.
>> 
>> Signed-off-by: Scott Feldman <sfeldma@gmail.com>
>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>> ---
>>  net/bridge/br_ioctl.c    |    3 +--
>>  net/bridge/br_netlink.c  |    6 +++---
>>  net/bridge/br_private.h  |    1 +
>>  net/bridge/br_stp.c      |   23 +++++++++++++++++++++++
>>  net/bridge/br_sysfs_br.c |    3 +--
>>  5 files changed, 29 insertions(+), 7 deletions(-)
>> 
>> diff --git a/net/bridge/br_ioctl.c b/net/bridge/br_ioctl.c
>> index 8d423bc..263b4de 100644
>> --- a/net/bridge/br_ioctl.c
>> +++ b/net/bridge/br_ioctl.c
>> @@ -200,8 +200,7 @@ static int old_dev_ioctl(struct net_device *dev, struct
>> ifreq *rq, int cmd)
>>  		if (!ns_capable(dev_net(dev)->user_ns, CAP_NET_ADMIN))
>>  			return -EPERM;
>> 
>> -		br->ageing_time = clock_t_to_jiffies(args[1]);
>> -		return 0;
>> +		return br_set_ageing_time(br, args[1]);
>> 
>>  	case BRCTL_GET_PORT_INFO:
>>  	{
>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
>> index d78b442..544ab96 100644
>> --- a/net/bridge/br_netlink.c
>> +++ b/net/bridge/br_netlink.c
>> @@ -870,9 +870,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]);
>> -
>> -		br->ageing_time = clock_t_to_jiffies(ageing_time);
>> +		err = br_set_ageing_time(br,
>> nla_get_u32(data[IFLA_BR_AGEING_TIME]));
>> +		if (err)
>> +			return err;
>>  	}
>> 
>>  	if (data[IFLA_BR_STP_STATE]) {
>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>> index 09d3ecb..ba0c67b 100644
>> --- a/net/bridge/br_private.h
>> +++ b/net/bridge/br_private.h
>> @@ -882,6 +882,7 @@ void __br_set_forward_delay(struct net_bridge *br,
>> unsigned long t);
>>  int br_set_forward_delay(struct net_bridge *br, unsigned long x);
>>  int br_set_hello_time(struct net_bridge *br, unsigned long x);
>>  int br_set_max_age(struct net_bridge *br, unsigned long x);
>> +int br_set_ageing_time(struct net_bridge *br, u32 ageing_time);
>> 
>> 
>>  /* br_stp_if.c */
>> diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
>> index 3a982c0..db6d243de 100644
>> --- a/net/bridge/br_stp.c
>> +++ b/net/bridge/br_stp.c
>> @@ -566,6 +566,29 @@ int br_set_max_age(struct net_bridge *br, unsigned
>> long val)
>> 
>>  }
>> 
>> +int br_set_ageing_time(struct net_bridge *br, u32 ageing_time)
>> +{
>> +	struct switchdev_attr attr = {
>> +		.id = SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME,
>> +		.flags = SWITCHDEV_F_SKIP_EOPNOTSUPP,
>> +		.u.ageing_time = ageing_time,
>> +	};
>> +	unsigned long t = clock_t_to_jiffies(ageing_time);
>> +	int err;
>> +
>> +	if (t < BR_MIN_AGEING_TIME || t > BR_MAX_AGEING_TIME)
>> +		return -ERANGE;
>> +
>> +	err = switchdev_port_attr_set(br->dev, &attr);
>
>A thought - given that the ageing time is not a per-bridge-port attr, why are we using a "port based api"
>to pass the attribute down?  May be I'm missing something here?

I general, it can be. And in case of rocker, it is.
Other drivers will just use port handler to set the ageing time on the
appropriate bridge.



>
>-Prem
>
>
>> +	if (err)
>> +		return err;
>> +
>> +	br->ageing_time = t;
>> +	mod_timer(&br->gc_timer, jiffies);
>> +
>> +	return 0;
>> +}
>> +
>>  void __br_set_forward_delay(struct net_bridge *br, unsigned long t)
>>  {
>>  	br->bridge_forward_delay = t;
>> diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
>> index 4c97fc5..04ef192 100644
>> --- a/net/bridge/br_sysfs_br.c
>> +++ b/net/bridge/br_sysfs_br.c
>> @@ -102,8 +102,7 @@ static ssize_t ageing_time_show(struct device *d,
>> 
>>  static int set_ageing_time(struct net_bridge *br, unsigned long val)
>>  {
>> -	br->ageing_time = clock_t_to_jiffies(val);
>> -	return 0;
>> +	return br_set_ageing_time(br, val);
>>  }
>> 
>>  static ssize_t ageing_time_store(struct device *d,
>> --
>> 1.7.10.4
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Scott Feldman Oct. 10, 2015, 2:53 a.m. UTC | #6 
On Thu, Oct 8, 2015 at 9:38 PM, Premkumar Jonnala <pjonnala@broadcom.com> wrote:
>
>
>> -----Original Message-----
>> From: sfeldma@gmail.com [mailto:sfeldma@gmail.com]
>> Sent: Friday, October 09, 2015 7:53 AM
>> To: netdev@vger.kernel.org
>> Cc: davem@davemloft.net; jiri@resnulli.us; siva.mannem.lnx@gmail.com;
>> Premkumar Jonnala; stephen@networkplumber.org;
>> roopa@cumulusnetworks.com; andrew@lunn.ch; f.fainelli@gmail.com;
>> vivien.didelot@savoirfairelinux.com
>> Subject: [PATCH net-next v3 3/4] bridge: push bridge setting ageing_time down
>> to switchdev
>>
>> From: Scott Feldman <sfeldma@gmail.com>
>>
>> Use SWITCHDEV_F_SKIP_EOPNOTSUPP to skip over ports in bridge that don't
>> support setting ageing_time (or setting bridge attrs in general).
>>
>> If push fails, don't update ageing_time in bridge and return err to user.
>>
>> If push succeeds, update ageing_time in bridge and run gc_timer now to
>> recalabrate when to run gc_timer next, based on new ageing_time.
>>
>> Signed-off-by: Scott Feldman <sfeldma@gmail.com>
>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>

<snip>

>> +int br_set_ageing_time(struct net_bridge *br, u32 ageing_time)
>> +{
>> +     struct switchdev_attr attr = {
>> +             .id = SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME,
>> +             .flags = SWITCHDEV_F_SKIP_EOPNOTSUPP,
>> +             .u.ageing_time = ageing_time,
>> +     };
>> +     unsigned long t = clock_t_to_jiffies(ageing_time);
>> +     int err;
>> +
>> +     if (t < BR_MIN_AGEING_TIME || t > BR_MAX_AGEING_TIME)
>> +             return -ERANGE;
>> +
>> +     err = switchdev_port_attr_set(br->dev, &attr);
>
> A thought - given that the ageing time is not a per-bridge-port attr, why are we using a "port based api"
> to pass the attribute down?  May be I'm missing something here?

I think Florian raised the same point earlier.  Sigh, I think this
should be addressed....v4 coming soon...thanks guys for keeping the
standard high.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jiri Pirko Oct. 10, 2015, 7:04 a.m. UTC | #7 
Sat, Oct 10, 2015 at 04:53:52AM CEST, sfeldma@gmail.com wrote:
>On Thu, Oct 8, 2015 at 9:38 PM, Premkumar Jonnala <pjonnala@broadcom.com> wrote:
>>
>>
>>> -----Original Message-----
>>> From: sfeldma@gmail.com [mailto:sfeldma@gmail.com]
>>> Sent: Friday, October 09, 2015 7:53 AM
>>> To: netdev@vger.kernel.org
>>> Cc: davem@davemloft.net; jiri@resnulli.us; siva.mannem.lnx@gmail.com;
>>> Premkumar Jonnala; stephen@networkplumber.org;
>>> roopa@cumulusnetworks.com; andrew@lunn.ch; f.fainelli@gmail.com;
>>> vivien.didelot@savoirfairelinux.com
>>> Subject: [PATCH net-next v3 3/4] bridge: push bridge setting ageing_time down
>>> to switchdev
>>>
>>> From: Scott Feldman <sfeldma@gmail.com>
>>>
>>> Use SWITCHDEV_F_SKIP_EOPNOTSUPP to skip over ports in bridge that don't
>>> support setting ageing_time (or setting bridge attrs in general).
>>>
>>> If push fails, don't update ageing_time in bridge and return err to user.
>>>
>>> If push succeeds, update ageing_time in bridge and run gc_timer now to
>>> recalabrate when to run gc_timer next, based on new ageing_time.
>>>
>>> Signed-off-by: Scott Feldman <sfeldma@gmail.com>
>>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>
><snip>
>
>>> +int br_set_ageing_time(struct net_bridge *br, u32 ageing_time)
>>> +{
>>> +     struct switchdev_attr attr = {
>>> +             .id = SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME,
>>> +             .flags = SWITCHDEV_F_SKIP_EOPNOTSUPP,
>>> +             .u.ageing_time = ageing_time,
>>> +     };
>>> +     unsigned long t = clock_t_to_jiffies(ageing_time);
>>> +     int err;
>>> +
>>> +     if (t < BR_MIN_AGEING_TIME || t > BR_MAX_AGEING_TIME)
>>> +             return -ERANGE;
>>> +
>>> +     err = switchdev_port_attr_set(br->dev, &attr);
>>
>> A thought - given that the ageing time is not a per-bridge-port attr, why are we using a "port based api"
>> to pass the attribute down?  May be I'm missing something here?
>
>I think Florian raised the same point earlier.  Sigh, I think this
>should be addressed....v4 coming soon...thanks guys for keeping the
>standard high.

Scott, can you tell us how do you want to address this? I like the
current implementation.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vivien Didelot Oct. 10, 2015, 3:56 p.m. UTC | #8 
On Oct. Saturday 10 (41) 09:04 AM, Jiri Pirko wrote:
> Sat, Oct 10, 2015 at 04:53:52AM CEST, sfeldma@gmail.com wrote:
> >On Thu, Oct 8, 2015 at 9:38 PM, Premkumar Jonnala <pjonnala@broadcom.com> wrote:
> >>
> >>
> >>> -----Original Message-----
> >>> From: sfeldma@gmail.com [mailto:sfeldma@gmail.com]
> >>> Sent: Friday, October 09, 2015 7:53 AM
> >>> To: netdev@vger.kernel.org
> >>> Cc: davem@davemloft.net; jiri@resnulli.us; siva.mannem.lnx@gmail.com;
> >>> Premkumar Jonnala; stephen@networkplumber.org;
> >>> roopa@cumulusnetworks.com; andrew@lunn.ch; f.fainelli@gmail.com;
> >>> vivien.didelot@savoirfairelinux.com
> >>> Subject: [PATCH net-next v3 3/4] bridge: push bridge setting ageing_time down
> >>> to switchdev
> >>>
> >>> From: Scott Feldman <sfeldma@gmail.com>
> >>>
> >>> Use SWITCHDEV_F_SKIP_EOPNOTSUPP to skip over ports in bridge that don't
> >>> support setting ageing_time (or setting bridge attrs in general).
> >>>
> >>> If push fails, don't update ageing_time in bridge and return err to user.
> >>>
> >>> If push succeeds, update ageing_time in bridge and run gc_timer now to
> >>> recalabrate when to run gc_timer next, based on new ageing_time.
> >>>
> >>> Signed-off-by: Scott Feldman <sfeldma@gmail.com>
> >>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> >
> ><snip>
> >
> >>> +int br_set_ageing_time(struct net_bridge *br, u32 ageing_time)
> >>> +{
> >>> +     struct switchdev_attr attr = {
> >>> +             .id = SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME,
> >>> +             .flags = SWITCHDEV_F_SKIP_EOPNOTSUPP,
> >>> +             .u.ageing_time = ageing_time,
> >>> +     };
> >>> +     unsigned long t = clock_t_to_jiffies(ageing_time);
> >>> +     int err;
> >>> +
> >>> +     if (t < BR_MIN_AGEING_TIME || t > BR_MAX_AGEING_TIME)
> >>> +             return -ERANGE;
> >>> +
> >>> +     err = switchdev_port_attr_set(br->dev, &attr);
> >>
> >> A thought - given that the ageing time is not a per-bridge-port attr, why are we using a "port based api"
> >> to pass the attribute down?  May be I'm missing something here?
> >
> >I think Florian raised the same point earlier.  Sigh, I think this
> >should be addressed....v4 coming soon...thanks guys for keeping the
> >standard high.
> 
> Scott, can you tell us how do you want to address this? I like the
> current implementation.

Scott, didn't you have a plan to add a struct device for the parent of
switchdev ports?

I think it would be good to introduce such device with an helper to
retrieve this upper parent, and move the switchdev ops to this guy.

So switchdev drivers may implement such API calls:

    .obj_add(struct device *swdev, struct switchdev_obj *obj);

    .port_obj_add(struct device *swdev, struct net_device *port,
                  struct switchdev_obj *obj);

Then switchdev code may have a parent API and the current port API may
look like this:

    int switchdev_port_obj_add(struct net_device *dev,
                               struct switchdev_obj *obj)
    {
        struct device *swdev = switchdev_get_parent(dev);
        int err = -EOPNOTSUPP;

        if (swdev && swdev->switchdev_ops &&
            swdev->switchdev_ops->port_obj_add)
            err = swdev->switchdev_ops->port_obj_add(swdev, dev, obj);

        return err;
    }

Thanks,
-v
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jiri Pirko Oct. 10, 2015, 9:09 p.m. UTC | #9 
Sat, Oct 10, 2015 at 05:56:19PM CEST, vivien.didelot@savoirfairelinux.com wrote:
>On Oct. Saturday 10 (41) 09:04 AM, Jiri Pirko wrote:
>> Sat, Oct 10, 2015 at 04:53:52AM CEST, sfeldma@gmail.com wrote:
>> >On Thu, Oct 8, 2015 at 9:38 PM, Premkumar Jonnala <pjonnala@broadcom.com> wrote:
>> >>
>> >>
>> >>> -----Original Message-----
>> >>> From: sfeldma@gmail.com [mailto:sfeldma@gmail.com]
>> >>> Sent: Friday, October 09, 2015 7:53 AM
>> >>> To: netdev@vger.kernel.org
>> >>> Cc: davem@davemloft.net; jiri@resnulli.us; siva.mannem.lnx@gmail.com;
>> >>> Premkumar Jonnala; stephen@networkplumber.org;
>> >>> roopa@cumulusnetworks.com; andrew@lunn.ch; f.fainelli@gmail.com;
>> >>> vivien.didelot@savoirfairelinux.com
>> >>> Subject: [PATCH net-next v3 3/4] bridge: push bridge setting ageing_time down
>> >>> to switchdev
>> >>>
>> >>> From: Scott Feldman <sfeldma@gmail.com>
>> >>>
>> >>> Use SWITCHDEV_F_SKIP_EOPNOTSUPP to skip over ports in bridge that don't
>> >>> support setting ageing_time (or setting bridge attrs in general).
>> >>>
>> >>> If push fails, don't update ageing_time in bridge and return err to user.
>> >>>
>> >>> If push succeeds, update ageing_time in bridge and run gc_timer now to
>> >>> recalabrate when to run gc_timer next, based on new ageing_time.
>> >>>
>> >>> Signed-off-by: Scott Feldman <sfeldma@gmail.com>
>> >>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>> >
>> ><snip>
>> >
>> >>> +int br_set_ageing_time(struct net_bridge *br, u32 ageing_time)
>> >>> +{
>> >>> +     struct switchdev_attr attr = {
>> >>> +             .id = SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME,
>> >>> +             .flags = SWITCHDEV_F_SKIP_EOPNOTSUPP,
>> >>> +             .u.ageing_time = ageing_time,
>> >>> +     };
>> >>> +     unsigned long t = clock_t_to_jiffies(ageing_time);
>> >>> +     int err;
>> >>> +
>> >>> +     if (t < BR_MIN_AGEING_TIME || t > BR_MAX_AGEING_TIME)
>> >>> +             return -ERANGE;
>> >>> +
>> >>> +     err = switchdev_port_attr_set(br->dev, &attr);
>> >>
>> >> A thought - given that the ageing time is not a per-bridge-port attr, why are we using a "port based api"
>> >> to pass the attribute down?  May be I'm missing something here?
>> >
>> >I think Florian raised the same point earlier.  Sigh, I think this
>> >should be addressed....v4 coming soon...thanks guys for keeping the
>> >standard high.
>> 
>> Scott, can you tell us how do you want to address this? I like the
>> current implementation.
>
>Scott, didn't you have a plan to add a struct device for the parent of
>switchdev ports?
>
>I think it would be good to introduce such device with an helper to
>retrieve this upper parent, and move the switchdev ops to this guy.
>
>So switchdev drivers may implement such API calls:
>
>    .obj_add(struct device *swdev, struct switchdev_obj *obj);
>
>    .port_obj_add(struct device *swdev, struct net_device *port,
>                  struct switchdev_obj *obj);
>
>Then switchdev code may have a parent API and the current port API may
>look like this:
>
>    int switchdev_port_obj_add(struct net_device *dev,
>                               struct switchdev_obj *obj)
>    {
>        struct device *swdev = switchdev_get_parent(dev);
>        int err = -EOPNOTSUPP;
>
>        if (swdev && swdev->switchdev_ops &&
>            swdev->switchdev_ops->port_obj_add)
>            err = swdev->switchdev_ops->port_obj_add(swdev, dev, obj);
>
>        return err;
>    }

Fro the record, I don't see any reason for this "device". It would just
introduce unnecessary complexicity. So far, we are fine without it.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vivien Didelot Oct. 10, 2015, 10:41 p.m. UTC | #10 
On Oct. Saturday 10 (41) 11:09 PM, Jiri Pirko wrote:
> Sat, Oct 10, 2015 at 05:56:19PM CEST, vivien.didelot@savoirfairelinux.com wrote:
> >On Oct. Saturday 10 (41) 09:04 AM, Jiri Pirko wrote:
> >> Sat, Oct 10, 2015 at 04:53:52AM CEST, sfeldma@gmail.com wrote:
> >> >On Thu, Oct 8, 2015 at 9:38 PM, Premkumar Jonnala <pjonnala@broadcom.com> wrote:
> >> >>
> >> >>
> >> >>> -----Original Message-----
> >> >>> From: sfeldma@gmail.com [mailto:sfeldma@gmail.com]
> >> >>> Sent: Friday, October 09, 2015 7:53 AM
> >> >>> To: netdev@vger.kernel.org
> >> >>> Cc: davem@davemloft.net; jiri@resnulli.us; siva.mannem.lnx@gmail.com;
> >> >>> Premkumar Jonnala; stephen@networkplumber.org;
> >> >>> roopa@cumulusnetworks.com; andrew@lunn.ch; f.fainelli@gmail.com;
> >> >>> vivien.didelot@savoirfairelinux.com
> >> >>> Subject: [PATCH net-next v3 3/4] bridge: push bridge setting ageing_time down
> >> >>> to switchdev
> >> >>>
> >> >>> From: Scott Feldman <sfeldma@gmail.com>
> >> >>>
> >> >>> Use SWITCHDEV_F_SKIP_EOPNOTSUPP to skip over ports in bridge that don't
> >> >>> support setting ageing_time (or setting bridge attrs in general).
> >> >>>
> >> >>> If push fails, don't update ageing_time in bridge and return err to user.
> >> >>>
> >> >>> If push succeeds, update ageing_time in bridge and run gc_timer now to
> >> >>> recalabrate when to run gc_timer next, based on new ageing_time.
> >> >>>
> >> >>> Signed-off-by: Scott Feldman <sfeldma@gmail.com>
> >> >>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> >> >
> >> ><snip>
> >> >
> >> >>> +int br_set_ageing_time(struct net_bridge *br, u32 ageing_time)
> >> >>> +{
> >> >>> +     struct switchdev_attr attr = {
> >> >>> +             .id = SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME,
> >> >>> +             .flags = SWITCHDEV_F_SKIP_EOPNOTSUPP,
> >> >>> +             .u.ageing_time = ageing_time,
> >> >>> +     };
> >> >>> +     unsigned long t = clock_t_to_jiffies(ageing_time);
> >> >>> +     int err;
> >> >>> +
> >> >>> +     if (t < BR_MIN_AGEING_TIME || t > BR_MAX_AGEING_TIME)
> >> >>> +             return -ERANGE;
> >> >>> +
> >> >>> +     err = switchdev_port_attr_set(br->dev, &attr);
> >> >>
> >> >> A thought - given that the ageing time is not a per-bridge-port attr, why are we using a "port based api"
> >> >> to pass the attribute down?  May be I'm missing something here?
> >> >
> >> >I think Florian raised the same point earlier.  Sigh, I think this
> >> >should be addressed....v4 coming soon...thanks guys for keeping the
> >> >standard high.
> >> 
> >> Scott, can you tell us how do you want to address this? I like the
> >> current implementation.
> >
> >Scott, didn't you have a plan to add a struct device for the parent of
> >switchdev ports?
> >
> >I think it would be good to introduce such device with an helper to
> >retrieve this upper parent, and move the switchdev ops to this guy.
> >
> >So switchdev drivers may implement such API calls:
> >
> >    .obj_add(struct device *swdev, struct switchdev_obj *obj);
> >
> >    .port_obj_add(struct device *swdev, struct net_device *port,
> >                  struct switchdev_obj *obj);
> >
> >Then switchdev code may have a parent API and the current port API may
> >look like this:
> >
> >    int switchdev_port_obj_add(struct net_device *dev,
> >                               struct switchdev_obj *obj)
> >    {
> >        struct device *swdev = switchdev_get_parent(dev);
> >        int err = -EOPNOTSUPP;
> >
> >        if (swdev && swdev->switchdev_ops &&
> >            swdev->switchdev_ops->port_obj_add)
> >            err = swdev->switchdev_ops->port_obj_add(swdev, dev, obj);
> >
> >        return err;
> >    }
> 
> Fro the record, I don't see any reason for this "device". It would just
> introduce unnecessary complexicity. So far, we are fine without it.

I wouldn't say that. I beleive that an Ethernet switch deserves its
struct device in the tree, since it is a physical chip, like any other.

Configuring it through one of its port (net_device) is fine, since you
want to change the port behavior, and Linux config is on per-port basis.

But the complexity is already introduced in the struct net_device with
the switchdev_ops. These ops really belong to the parent device, not to
all of its ports.

Ideally a switch device would be registered with this set of operations,
creates its net_devices, and will be accessible from a port net_device
through a netdev helper function.

Thanks,
-v
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Florian Fainelli Oct. 11, 2015, 12:07 a.m. UTC | #11 
2015-10-10 15:41 GMT-07:00 Vivien Didelot <vivien.didelot@savoirfairelinux.com>:
> On Oct. Saturday 10 (41) 11:09 PM, Jiri Pirko wrote:
>> Sat, Oct 10, 2015 at 05:56:19PM CEST, vivien.didelot@savoirfairelinux.com wrote:
>> >On Oct. Saturday 10 (41) 09:04 AM, Jiri Pirko wrote:
>> >> Sat, Oct 10, 2015 at 04:53:52AM CEST, sfeldma@gmail.com wrote:
>> >> >On Thu, Oct 8, 2015 at 9:38 PM, Premkumar Jonnala <pjonnala@broadcom.com> wrote:
>> >> >>
>> >> >>
>> >> >>> -----Original Message-----
>> >> >>> From: sfeldma@gmail.com [mailto:sfeldma@gmail.com]
>> >> >>> Sent: Friday, October 09, 2015 7:53 AM
>> >> >>> To: netdev@vger.kernel.org
>> >> >>> Cc: davem@davemloft.net; jiri@resnulli.us; siva.mannem.lnx@gmail.com;
>> >> >>> Premkumar Jonnala; stephen@networkplumber.org;
>> >> >>> roopa@cumulusnetworks.com; andrew@lunn.ch; f.fainelli@gmail.com;
>> >> >>> vivien.didelot@savoirfairelinux.com
>> >> >>> Subject: [PATCH net-next v3 3/4] bridge: push bridge setting ageing_time down
>> >> >>> to switchdev
>> >> >>>
>> >> >>> From: Scott Feldman <sfeldma@gmail.com>
>> >> >>>
>> >> >>> Use SWITCHDEV_F_SKIP_EOPNOTSUPP to skip over ports in bridge that don't
>> >> >>> support setting ageing_time (or setting bridge attrs in general).
>> >> >>>
>> >> >>> If push fails, don't update ageing_time in bridge and return err to user.
>> >> >>>
>> >> >>> If push succeeds, update ageing_time in bridge and run gc_timer now to
>> >> >>> recalabrate when to run gc_timer next, based on new ageing_time.
>> >> >>>
>> >> >>> Signed-off-by: Scott Feldman <sfeldma@gmail.com>
>> >> >>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>> >> >
>> >> ><snip>
>> >> >
>> >> >>> +int br_set_ageing_time(struct net_bridge *br, u32 ageing_time)
>> >> >>> +{
>> >> >>> +     struct switchdev_attr attr = {
>> >> >>> +             .id = SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME,
>> >> >>> +             .flags = SWITCHDEV_F_SKIP_EOPNOTSUPP,
>> >> >>> +             .u.ageing_time = ageing_time,
>> >> >>> +     };
>> >> >>> +     unsigned long t = clock_t_to_jiffies(ageing_time);
>> >> >>> +     int err;
>> >> >>> +
>> >> >>> +     if (t < BR_MIN_AGEING_TIME || t > BR_MAX_AGEING_TIME)
>> >> >>> +             return -ERANGE;
>> >> >>> +
>> >> >>> +     err = switchdev_port_attr_set(br->dev, &attr);
>> >> >>
>> >> >> A thought - given that the ageing time is not a per-bridge-port attr, why are we using a "port based api"
>> >> >> to pass the attribute down?  May be I'm missing something here?
>> >> >
>> >> >I think Florian raised the same point earlier.  Sigh, I think this
>> >> >should be addressed....v4 coming soon...thanks guys for keeping the
>> >> >standard high.
>> >>
>> >> Scott, can you tell us how do you want to address this? I like the
>> >> current implementation.
>> >
>> >Scott, didn't you have a plan to add a struct device for the parent of
>> >switchdev ports?
>> >
>> >I think it would be good to introduce such device with an helper to
>> >retrieve this upper parent, and move the switchdev ops to this guy.
>> >
>> >So switchdev drivers may implement such API calls:
>> >
>> >    .obj_add(struct device *swdev, struct switchdev_obj *obj);
>> >
>> >    .port_obj_add(struct device *swdev, struct net_device *port,
>> >                  struct switchdev_obj *obj);
>> >
>> >Then switchdev code may have a parent API and the current port API may
>> >look like this:
>> >
>> >    int switchdev_port_obj_add(struct net_device *dev,
>> >                               struct switchdev_obj *obj)
>> >    {
>> >        struct device *swdev = switchdev_get_parent(dev);
>> >        int err = -EOPNOTSUPP;
>> >
>> >        if (swdev && swdev->switchdev_ops &&
>> >            swdev->switchdev_ops->port_obj_add)
>> >            err = swdev->switchdev_ops->port_obj_add(swdev, dev, obj);
>> >
>> >        return err;
>> >    }
>>
>> Fro the record, I don't see any reason for this "device". It would just
>> introduce unnecessary complexicity. So far, we are fine without it.
>
> I wouldn't say that. I beleive that an Ethernet switch deserves its
> struct device in the tree, since it is a physical chip, like any other.

Agreed, but gating these patches because we do not yet have a device
driver model for an Ethernet switch outside of its individual ports
does not seem like it hurts the current patch series, nor the existing
model (and future).

>
> Configuring it through one of its port (net_device) is fine, since you
> want to change the port behavior, and Linux config is on per-port basis.
>
> But the complexity is already introduced in the struct net_device with
> the switchdev_ops. These ops really belong to the parent device, not to
> all of its ports.

I am not sure if complexity is the correct term here, bloat (to some
extent) maybe, since with what you are suggesting we could save one
set of function pointers per-port, and instead move that to a
global/switch-wide device implementing these operations. In essence,
there will be per-port switchdev_ops, bridge-specific, and maybe in
the future switch device specific.

>
> Ideally a switch device would be registered with this set of operations,
> creates its net_devices, and will be accessible from a port net_device
> through a netdev helper function.

I think the core of the discussion for a proper Ethernet switch device
model is precisely whether we want to have a special network device to
configure the switch as a whole. It sure would represent one facet of
the switch device, but not everything else for which we are still
trying to find out what that is.
Vivien Didelot Oct. 11, 2015, 10:38 p.m. UTC | #12 
On Oct. Saturday 10 (41) 05:07 PM, Florian Fainelli wrote:
> 2015-10-10 15:41 GMT-07:00 Vivien Didelot <vivien.didelot@savoirfairelinux.com>:
> > On Oct. Saturday 10 (41) 11:09 PM, Jiri Pirko wrote:
> >> Sat, Oct 10, 2015 at 05:56:19PM CEST, vivien.didelot@savoirfairelinux.com wrote:
> >> >On Oct. Saturday 10 (41) 09:04 AM, Jiri Pirko wrote:
> >> >> Sat, Oct 10, 2015 at 04:53:52AM CEST, sfeldma@gmail.com wrote:
> >> >> >On Thu, Oct 8, 2015 at 9:38 PM, Premkumar Jonnala <pjonnala@broadcom.com> wrote:
> >> >> >>
> >> >> >>
> >> >> >>> -----Original Message-----
> >> >> >>> From: sfeldma@gmail.com [mailto:sfeldma@gmail.com]
> >> >> >>> Sent: Friday, October 09, 2015 7:53 AM
> >> >> >>> To: netdev@vger.kernel.org
> >> >> >>> Cc: davem@davemloft.net; jiri@resnulli.us; siva.mannem.lnx@gmail.com;
> >> >> >>> Premkumar Jonnala; stephen@networkplumber.org;
> >> >> >>> roopa@cumulusnetworks.com; andrew@lunn.ch; f.fainelli@gmail.com;
> >> >> >>> vivien.didelot@savoirfairelinux.com
> >> >> >>> Subject: [PATCH net-next v3 3/4] bridge: push bridge setting ageing_time down
> >> >> >>> to switchdev
> >> >> >>>
> >> >> >>> From: Scott Feldman <sfeldma@gmail.com>
> >> >> >>>
> >> >> >>> Use SWITCHDEV_F_SKIP_EOPNOTSUPP to skip over ports in bridge that don't
> >> >> >>> support setting ageing_time (or setting bridge attrs in general).
> >> >> >>>
> >> >> >>> If push fails, don't update ageing_time in bridge and return err to user.
> >> >> >>>
> >> >> >>> If push succeeds, update ageing_time in bridge and run gc_timer now to
> >> >> >>> recalabrate when to run gc_timer next, based on new ageing_time.
> >> >> >>>
> >> >> >>> Signed-off-by: Scott Feldman <sfeldma@gmail.com>
> >> >> >>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> >> >> >
> >> >> ><snip>
> >> >> >
> >> >> >>> +int br_set_ageing_time(struct net_bridge *br, u32 ageing_time)
> >> >> >>> +{
> >> >> >>> +     struct switchdev_attr attr = {
> >> >> >>> +             .id = SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME,
> >> >> >>> +             .flags = SWITCHDEV_F_SKIP_EOPNOTSUPP,
> >> >> >>> +             .u.ageing_time = ageing_time,
> >> >> >>> +     };
> >> >> >>> +     unsigned long t = clock_t_to_jiffies(ageing_time);
> >> >> >>> +     int err;
> >> >> >>> +
> >> >> >>> +     if (t < BR_MIN_AGEING_TIME || t > BR_MAX_AGEING_TIME)
> >> >> >>> +             return -ERANGE;
> >> >> >>> +
> >> >> >>> +     err = switchdev_port_attr_set(br->dev, &attr);
> >> >> >>
> >> >> >> A thought - given that the ageing time is not a per-bridge-port attr, why are we using a "port based api"
> >> >> >> to pass the attribute down?  May be I'm missing something here?
> >> >> >
> >> >> >I think Florian raised the same point earlier.  Sigh, I think this
> >> >> >should be addressed....v4 coming soon...thanks guys for keeping the
> >> >> >standard high.
> >> >>
> >> >> Scott, can you tell us how do you want to address this? I like the
> >> >> current implementation.
> >> >
> >> >Scott, didn't you have a plan to add a struct device for the parent of
> >> >switchdev ports?
> >> >
> >> >I think it would be good to introduce such device with an helper to
> >> >retrieve this upper parent, and move the switchdev ops to this guy.
> >> >
> >> >So switchdev drivers may implement such API calls:
> >> >
> >> >    .obj_add(struct device *swdev, struct switchdev_obj *obj);
> >> >
> >> >    .port_obj_add(struct device *swdev, struct net_device *port,
> >> >                  struct switchdev_obj *obj);
> >> >
> >> >Then switchdev code may have a parent API and the current port API may
> >> >look like this:
> >> >
> >> >    int switchdev_port_obj_add(struct net_device *dev,
> >> >                               struct switchdev_obj *obj)
> >> >    {
> >> >        struct device *swdev = switchdev_get_parent(dev);
> >> >        int err = -EOPNOTSUPP;
> >> >
> >> >        if (swdev && swdev->switchdev_ops &&
> >> >            swdev->switchdev_ops->port_obj_add)
> >> >            err = swdev->switchdev_ops->port_obj_add(swdev, dev, obj);
> >> >
> >> >        return err;
> >> >    }
> >>
> >> Fro the record, I don't see any reason for this "device". It would just
> >> introduce unnecessary complexicity. So far, we are fine without it.
> >
> > I wouldn't say that. I beleive that an Ethernet switch deserves its
> > struct device in the tree, since it is a physical chip, like any other.
> 
> Agreed, but gating these patches because we do not yet have a device
> driver model for an Ethernet switch outside of its individual ports
> does not seem like it hurts the current patch series, nor the existing
> model (and future).

Sure, I didn't mean to NAK the patch with this comment, I just wrote it
because we raised a concern about an API higher than the port level.

> >
> > Configuring it through one of its port (net_device) is fine, since you
> > want to change the port behavior, and Linux config is on per-port basis.
> >
> > But the complexity is already introduced in the struct net_device with
> > the switchdev_ops. These ops really belong to the parent device, not to
> > all of its ports.
> 
> I am not sure if complexity is the correct term here, bloat (to some
> extent) maybe, since with what you are suggesting we could save one
> set of function pointers per-port, and instead move that to a
> global/switch-wide device implementing these operations. In essence,
> there will be per-port switchdev_ops, bridge-specific, and maybe in
> the future switch device specific.

Exact. I think that a switch net_device port should just have a pointer
or something to its parent device (the switch) to query its operations.

> >
> > Ideally a switch device would be registered with this set of operations,
> > creates its net_devices, and will be accessible from a port net_device
> > through a netdev helper function.
> 
> I think the core of the discussion for a proper Ethernet switch device
> model is precisely whether we want to have a special network device to
> configure the switch as a whole. It sure would represent one facet of
> the switch device, but not everything else for which we are still
> trying to find out what that is.

I am not even convinced that a switch chip must be represented in the
Linux tree by a net_device. That's basically a chip exposing a bench of
registers to configure and expose not only net interfaces, but also
temperature sensors, and even GPIO sometimes.

Thanks,
-v
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Premkumar Jonnala Oct. 12, 2015, 4:39 a.m. UTC | #13 
> 2015-10-10 15:41 GMT-07:00 Vivien Didelot

> <vivien.didelot@savoirfairelinux.com>:

> > On Oct. Saturday 10 (41) 11:09 PM, Jiri Pirko wrote:

> >> Sat, Oct 10, 2015 at 05:56:19PM CEST, vivien.didelot@savoirfairelinux.com

> wrote:

> >> >On Oct. Saturday 10 (41) 09:04 AM, Jiri Pirko wrote:

> >> >> Sat, Oct 10, 2015 at 04:53:52AM CEST, sfeldma@gmail.com wrote:

> >> >> >On Thu, Oct 8, 2015 at 9:38 PM, Premkumar Jonnala

> <pjonnala@broadcom.com> wrote:

> >> >> >>

> >> >> >>

> >> >> >>> -----Original Message-----

> >> >> >>> From: sfeldma@gmail.com [mailto:sfeldma@gmail.com]

> >> >> >>> Sent: Friday, October 09, 2015 7:53 AM

> >> >> >>> To: netdev@vger.kernel.org

> >> >> >>> Cc: davem@davemloft.net; jiri@resnulli.us;

> siva.mannem.lnx@gmail.com;

> >> >> >>> Premkumar Jonnala; stephen@networkplumber.org;

> >> >> >>> roopa@cumulusnetworks.com; andrew@lunn.ch;

> f.fainelli@gmail.com;

> >> >> >>> vivien.didelot@savoirfairelinux.com

> >> >> >>> Subject: [PATCH net-next v3 3/4] bridge: push bridge setting

> ageing_time down

> >> >> >>> to switchdev

> >> >> >>>

> >> >> >>> From: Scott Feldman <sfeldma@gmail.com>

> >> >> >>>

> >> >> >>> Use SWITCHDEV_F_SKIP_EOPNOTSUPP to skip over ports in bridge

> that don't

> >> >> >>> support setting ageing_time (or setting bridge attrs in general).

> >> >> >>>

> >> >> >>> If push fails, don't update ageing_time in bridge and return err to user.

> >> >> >>>

> >> >> >>> If push succeeds, update ageing_time in bridge and run gc_timer now

> to

> >> >> >>> recalabrate when to run gc_timer next, based on new ageing_time.

> >> >> >>>

> >> >> >>> Signed-off-by: Scott Feldman <sfeldma@gmail.com>

> >> >> >>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>

> >> >> >

> >> >> ><snip>

> >> >> >

> >> >> >>> +int br_set_ageing_time(struct net_bridge *br, u32 ageing_time)

> >> >> >>> +{

> >> >> >>> +     struct switchdev_attr attr = {

> >> >> >>> +             .id = SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME,

> >> >> >>> +             .flags = SWITCHDEV_F_SKIP_EOPNOTSUPP,

> >> >> >>> +             .u.ageing_time = ageing_time,

> >> >> >>> +     };

> >> >> >>> +     unsigned long t = clock_t_to_jiffies(ageing_time);

> >> >> >>> +     int err;

> >> >> >>> +

> >> >> >>> +     if (t < BR_MIN_AGEING_TIME || t > BR_MAX_AGEING_TIME)

> >> >> >>> +             return -ERANGE;

> >> >> >>> +

> >> >> >>> +     err = switchdev_port_attr_set(br->dev, &attr);

> >> >> >>

> >> >> >> A thought - given that the ageing time is not a per-bridge-port attr, why

> are we using a "port based api"

> >> >> >> to pass the attribute down?  May be I'm missing something here?

> >> >> >

> >> >> >I think Florian raised the same point earlier.  Sigh, I think this

> >> >> >should be addressed....v4 coming soon...thanks guys for keeping the

> >> >> >standard high.

> >> >>

> >> >> Scott, can you tell us how do you want to address this? I like the

> >> >> current implementation.

> >> >

> >> >Scott, didn't you have a plan to add a struct device for the parent of

> >> >switchdev ports?

> >> >

> >> >I think it would be good to introduce such device with an helper to

> >> >retrieve this upper parent, and move the switchdev ops to this guy.

> >> >

> >> >So switchdev drivers may implement such API calls:

> >> >

> >> >    .obj_add(struct device *swdev, struct switchdev_obj *obj);

> >> >

> >> >    .port_obj_add(struct device *swdev, struct net_device *port,

> >> >                  struct switchdev_obj *obj);

> >> >

> >> >Then switchdev code may have a parent API and the current port API may

> >> >look like this:

> >> >

> >> >    int switchdev_port_obj_add(struct net_device *dev,

> >> >                               struct switchdev_obj *obj)

> >> >    {

> >> >        struct device *swdev = switchdev_get_parent(dev);

> >> >        int err = -EOPNOTSUPP;

> >> >

> >> >        if (swdev && swdev->switchdev_ops &&

> >> >            swdev->switchdev_ops->port_obj_add)

> >> >            err = swdev->switchdev_ops->port_obj_add(swdev, dev, obj);

> >> >

> >> >        return err;

> >> >    }

> >>

> >> Fro the record, I don't see any reason for this "device". It would just

> >> introduce unnecessary complexicity. So far, we are fine without it.

> >

> > I wouldn't say that. I beleive that an Ethernet switch deserves its

> > struct device in the tree, since it is a physical chip, like any other.

> 

> Agreed, but gating these patches because we do not yet have a device

> driver model for an Ethernet switch outside of its individual ports

> does not seem like it hurts the current patch series, nor the existing

> model (and future).


I don't think we have to gate this patch because of this change, but I wanted to
raise this issue because it didn't seem right to use "port level" API when what we
want to configure is really a "bridge level" attribute.

> 

> >

> > Configuring it through one of its port (net_device) is fine, since you

> > want to change the port behavior, and Linux config is on per-port basis.

> >

> > But the complexity is already introduced in the struct net_device with

> > the switchdev_ops. These ops really belong to the parent device, not to

> > all of its ports.

> 

> I am not sure if complexity is the correct term here, bloat (to some

> extent) maybe, since with what you are suggesting we could save one

> set of function pointers per-port, and instead move that to a

> global/switch-wide device implementing these operations. In essence,

> there will be per-port switchdev_ops, bridge-specific, and maybe in

> the future switch device specific.


This is right.  Going forward, we will have methods/attributes that are bridge specific,
router specific, etc.  I think having a right model at the start will go a long way in
helping keep the code-base extensible in the future.

> 

> >

> > Ideally a switch device would be registered with this set of operations,

> > creates its net_devices, and will be accessible from a port net_device

> > through a netdev helper function.

> 

> I think the core of the discussion for a proper Ethernet switch device

> model is precisely whether we want to have a special network device to

> configure the switch as a whole. It sure would represent one facet of

> the switch device, but not everything else for which we are still

> trying to find out what that is.


Agree.  We will have models other than switch device, but given that we are running
into switch-level attributes, it would be great if we can introduce that concept now,
and add more abstractions/models as we come across new attributes.

-Prem

> --

> Florian
Scott Feldman Oct. 12, 2015, 5:43 a.m. UTC | #14 
On Sat, Oct 10, 2015 at 12:04 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Sat, Oct 10, 2015 at 04:53:52AM CEST, sfeldma@gmail.com wrote:
>>On Thu, Oct 8, 2015 at 9:38 PM, Premkumar Jonnala <pjonnala@broadcom.com> wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: sfeldma@gmail.com [mailto:sfeldma@gmail.com]
>>>> Sent: Friday, October 09, 2015 7:53 AM
>>>> To: netdev@vger.kernel.org
>>>> Cc: davem@davemloft.net; jiri@resnulli.us; siva.mannem.lnx@gmail.com;
>>>> Premkumar Jonnala; stephen@networkplumber.org;
>>>> roopa@cumulusnetworks.com; andrew@lunn.ch; f.fainelli@gmail.com;
>>>> vivien.didelot@savoirfairelinux.com
>>>> Subject: [PATCH net-next v3 3/4] bridge: push bridge setting ageing_time down
>>>> to switchdev
>>>>
>>>> From: Scott Feldman <sfeldma@gmail.com>
>>>>
>>>> Use SWITCHDEV_F_SKIP_EOPNOTSUPP to skip over ports in bridge that don't
>>>> support setting ageing_time (or setting bridge attrs in general).
>>>>
>>>> If push fails, don't update ageing_time in bridge and return err to user.
>>>>
>>>> If push succeeds, update ageing_time in bridge and run gc_timer now to
>>>> recalabrate when to run gc_timer next, based on new ageing_time.
>>>>
>>>> Signed-off-by: Scott Feldman <sfeldma@gmail.com>
>>>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>>
>><snip>
>>
>>>> +int br_set_ageing_time(struct net_bridge *br, u32 ageing_time)
>>>> +{
>>>> +     struct switchdev_attr attr = {
>>>> +             .id = SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME,
>>>> +             .flags = SWITCHDEV_F_SKIP_EOPNOTSUPP,
>>>> +             .u.ageing_time = ageing_time,
>>>> +     };
>>>> +     unsigned long t = clock_t_to_jiffies(ageing_time);
>>>> +     int err;
>>>> +
>>>> +     if (t < BR_MIN_AGEING_TIME || t > BR_MAX_AGEING_TIME)
>>>> +             return -ERANGE;
>>>> +
>>>> +     err = switchdev_port_attr_set(br->dev, &attr);
>>>
>>> A thought - given that the ageing time is not a per-bridge-port attr, why are we using a "port based api"
>>> to pass the attribute down?  May be I'm missing something here?
>>
>>I think Florian raised the same point earlier.  Sigh, I think this
>>should be addressed....v4 coming soon...thanks guys for keeping the
>>standard high.
>
> Scott, can you tell us how do you want to address this? I like the
> current implementation.

I like it also, but there is some awkwardness in calling
switchdev_port_attr_set() with the first argument being the bridge
netdev, not a port netdev.  But, the basic algorithm to recurse from
_this_ netdev down to its lower netdevs works great in this case; it's
just the name "switchdev_port_attr_set" implies a port netdev for
top-level netdev.  So I was thinking about adding another call,
something like "switchdev_master_attr_set", which basically just does
the same thing as switchdev_port_attr_set, except maybe skips the
check to call the ops->switchdev_port_attr_add on the top-level
netdev.  But now I don't like that idea so much as "master" would be
confusing when your passing a bond netdev (which is also a master),
but the bond _is_ the port netdev this time, a port on the bridge.

So let's scrap v4 and go with v3.  I think I can live with this naming
awkwardness, given that we got something for essentially free by using
switchdev_port_attr_set() in a new way.

Davem, I think we're OK going with v3.

(There is a follow-on discussion about a switch device, which we'll
continue but it shouldn't block this v3 version).
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Scott Feldman Oct. 13, 2015, 5:39 a.m. UTC | #15 
On Sat, Oct 10, 2015 at 8:56 AM, Vivien Didelot
<vivien.didelot@savoirfairelinux.com> wrote:

> Scott, didn't you have a plan to add a struct device for the parent of
> switchdev ports?

I had sent out a rough RFC for a switch device in the last window.  I
have continued working on it, and I plan to send it very soon,
probably again as an RFC.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/net/bridge/br_ioctl.c b/net/bridge/br_ioctl.c
index 8d423bc..263b4de 100644
--- a/net/bridge/br_ioctl.c
+++ b/net/bridge/br_ioctl.c
@@ -200,8 +200,7 @@  static int old_dev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
 		if (!ns_capable(dev_net(dev)->user_ns, CAP_NET_ADMIN))
 			return -EPERM;
 
-		br->ageing_time = clock_t_to_jiffies(args[1]);
-		return 0;
+		return br_set_ageing_time(br, args[1]);
 
 	case BRCTL_GET_PORT_INFO:
 	{
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index d78b442..544ab96 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -870,9 +870,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]);
-
-		br->ageing_time = clock_t_to_jiffies(ageing_time);
+		err = br_set_ageing_time(br, nla_get_u32(data[IFLA_BR_AGEING_TIME]));
+		if (err)
+			return err;
 	}
 
 	if (data[IFLA_BR_STP_STATE]) {
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 09d3ecb..ba0c67b 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -882,6 +882,7 @@  void __br_set_forward_delay(struct net_bridge *br, unsigned long t);
 int br_set_forward_delay(struct net_bridge *br, unsigned long x);
 int br_set_hello_time(struct net_bridge *br, unsigned long x);
 int br_set_max_age(struct net_bridge *br, unsigned long x);
+int br_set_ageing_time(struct net_bridge *br, u32 ageing_time);
 
 
 /* br_stp_if.c */
diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
index 3a982c0..db6d243de 100644
--- a/net/bridge/br_stp.c
+++ b/net/bridge/br_stp.c
@@ -566,6 +566,29 @@  int br_set_max_age(struct net_bridge *br, unsigned long val)
 
 }
 
+int br_set_ageing_time(struct net_bridge *br, u32 ageing_time)
+{
+	struct switchdev_attr attr = {
+		.id = SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME,
+		.flags = SWITCHDEV_F_SKIP_EOPNOTSUPP,
+		.u.ageing_time = ageing_time,
+	};
+	unsigned long t = clock_t_to_jiffies(ageing_time);
+	int err;
+
+	if (t < BR_MIN_AGEING_TIME || t > BR_MAX_AGEING_TIME)
+		return -ERANGE;
+
+	err = switchdev_port_attr_set(br->dev, &attr);
+	if (err)
+		return err;
+
+	br->ageing_time = t;
+	mod_timer(&br->gc_timer, jiffies);
+
+	return 0;
+}
+
 void __br_set_forward_delay(struct net_bridge *br, unsigned long t)
 {
 	br->bridge_forward_delay = t;
diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
index 4c97fc5..04ef192 100644
--- a/net/bridge/br_sysfs_br.c
+++ b/net/bridge/br_sysfs_br.c
@@ -102,8 +102,7 @@  static ssize_t ageing_time_show(struct device *d,
 
 static int set_ageing_time(struct net_bridge *br, unsigned long val)
 {
-	br->ageing_time = clock_t_to_jiffies(val);
-	return 0;
+	return br_set_ageing_time(br, val);
 }
 
 static ssize_t ageing_time_store(struct device *d,