[PATCH net-next v5] switchdev: fix stp update API to work with layered netdevices

[PATCH net-next v5] switchdev: fix stp update API to work with layered netdevices

[roopa]

From: Roopa Prabhu <roopa@cumulusnetworks.com>

make it same as the netdev_switch_port_bridge_setlink/dellink
api (ie traverse lowerdevs to get to the switch port).

removes "WARN_ON(!ops->ndo_switch_parent_id_get)" because
direct bridge ports can be stacked netdevices (like bonds
and team of switch ports) which may not implement this ndo.

v2 to v3:
	- remove changes to bond and team. Bring back the
	transparently following lowerdevs like i initially
	had for setlink/getlink
	(http://www.spinics.net/lists/netdev/msg313436.html)
	dave and scott feldman also seem to prefer it be that
	way and move to non-transparent way of doing things
	if we see a problem down the lane.

v3 to v4:
	- fix ret initialization

v4 to v5:
	- return err on first failure (scott feldman)

Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
---
 net/switchdev/switchdev.c |   17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
index c9bfa00..7d7b487 100644
--- a/net/switchdev/switchdev.c
+++ b/net/switchdev/switchdev.c
@@ -47,11 +47,20 @@ EXPORT_SYMBOL_GPL(netdev_switch_parent_id_get);
 int netdev_switch_port_stp_update(struct net_device *dev, u8 state)
 {
 	const struct swdev_ops *ops = dev->swdev_ops;
+	struct net_device *lower_dev;
+	struct list_head *iter;
+	int ret;
 
-	if (!ops || !ops->swdev_port_stp_update)
-		return -EOPNOTSUPP;
-	WARN_ON(!ops->swdev_parent_id_get);
-	return ops->swdev_port_stp_update(dev, state);
+	if (ops && ops->swdev_port_stp_update)
+		return ops->swdev_port_stp_update(dev, state);
+
+	netdev_for_each_lower_dev(dev, lower_dev, iter) {
+		ret = netdev_switch_port_stp_update(lower_dev, state);
+		if (ret && ret != -EOPNOTSUPP)
+			return ret;
+	}
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(netdev_switch_port_stp_update);
 
-- 
1.7.10.4

Re: [PATCH net-next v5] switchdev: fix stp update API to work with layered netdevices

[Scott Feldman]

On Fri, Mar 20, 2015 at 10:20 PM,  <roopa@cumulusnetworks.com> wrote:
> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>
> make it same as the netdev_switch_port_bridge_setlink/dellink
> api (ie traverse lowerdevs to get to the switch port).
>
> removes "WARN_ON(!ops->ndo_switch_parent_id_get)" because
> direct bridge ports can be stacked netdevices (like bonds
> and team of switch ports) which may not implement this ndo.
>
> v2 to v3:
>         - remove changes to bond and team. Bring back the
>         transparently following lowerdevs like i initially
>         had for setlink/getlink
>         (http://www.spinics.net/lists/netdev/msg313436.html)
>         dave and scott feldman also seem to prefer it be that
>         way and move to non-transparent way of doing things
>         if we see a problem down the lane.
>
> v3 to v4:
>         - fix ret initialization
>
> v4 to v5:
>         - return err on first failure (scott feldman)
>
> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
> ---
>  net/switchdev/switchdev.c |   17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
> index c9bfa00..7d7b487 100644
> --- a/net/switchdev/switchdev.c
> +++ b/net/switchdev/switchdev.c
> @@ -47,11 +47,20 @@ EXPORT_SYMBOL_GPL(netdev_switch_parent_id_get);
>  int netdev_switch_port_stp_update(struct net_device *dev, u8 state)
>  {
>         const struct swdev_ops *ops = dev->swdev_ops;
> +       struct net_device *lower_dev;
> +       struct list_head *iter;
> +       int ret;

Use "err" rather than "ret"....to be consistent with rest of switchdev code.

>
> -       if (!ops || !ops->swdev_port_stp_update)
> -               return -EOPNOTSUPP;
> -       WARN_ON(!ops->swdev_parent_id_get);
> -       return ops->swdev_port_stp_update(dev, state);
> +       if (ops && ops->swdev_port_stp_update)
> +               return ops->swdev_port_stp_update(dev, state);
> +
> +       netdev_for_each_lower_dev(dev, lower_dev, iter) {
> +               ret = netdev_switch_port_stp_update(lower_dev, state);
> +               if (ret && ret != -EOPNOTSUPP)
> +                       return ret;
> +       }
> +
> +       return 0;

Return err and initialize err to -EOPNOTSUPP.

The way you have it, you're claiming success for the case where there
is no op and no lower devs...be consistent with same func when
CONFIG_NET_SWITCHDEV is not defined.

Sorry for being pedantic, but why not be consistent with the surrounding code?

-scott

Re: [PATCH net-next v5] switchdev: fix stp update API to work with layered netdevices

[roopa]

On 3/20/15, 10:35 PM, Scott Feldman wrote:
> On Fri, Mar 20, 2015 at 10:20 PM,  <roopa@cumulusnetworks.com> wrote:
>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>>
>> make it same as the netdev_switch_port_bridge_setlink/dellink
>> api (ie traverse lowerdevs to get to the switch port).
>>
>> removes "WARN_ON(!ops->ndo_switch_parent_id_get)" because
>> direct bridge ports can be stacked netdevices (like bonds
>> and team of switch ports) which may not implement this ndo.
>>
>> v2 to v3:
>>          - remove changes to bond and team. Bring back the
>>          transparently following lowerdevs like i initially
>>          had for setlink/getlink
>>          (http://www.spinics.net/lists/netdev/msg313436.html)
>>          dave and scott feldman also seem to prefer it be that
>>          way and move to non-transparent way of doing things
>>          if we see a problem down the lane.
>>
>> v3 to v4:
>>          - fix ret initialization
>>
>> v4 to v5:
>>          - return err on first failure (scott feldman)
>>
>> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
>> ---
>>   net/switchdev/switchdev.c |   17 +++++++++++++----
>>   1 file changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
>> index c9bfa00..7d7b487 100644
>> --- a/net/switchdev/switchdev.c
>> +++ b/net/switchdev/switchdev.c
>> @@ -47,11 +47,20 @@ EXPORT_SYMBOL_GPL(netdev_switch_parent_id_get);
>>   int netdev_switch_port_stp_update(struct net_device *dev, u8 state)
>>   {
>>          const struct swdev_ops *ops = dev->swdev_ops;
>> +       struct net_device *lower_dev;
>> +       struct list_head *iter;
>> +       int ret;
> Use "err" rather than "ret"....to be consistent with rest of switchdev code.
>
>> -       if (!ops || !ops->swdev_port_stp_update)
>> -               return -EOPNOTSUPP;
>> -       WARN_ON(!ops->swdev_parent_id_get);
>> -       return ops->swdev_port_stp_update(dev, state);
>> +       if (ops && ops->swdev_port_stp_update)
>> +               return ops->swdev_port_stp_update(dev, state);
>> +
>> +       netdev_for_each_lower_dev(dev, lower_dev, iter) {
>> +               ret = netdev_switch_port_stp_update(lower_dev, state);
>> +               if (ret && ret != -EOPNOTSUPP)
>> +                       return ret;
>> +       }
>> +
>> +       return 0;
> Return err and initialize err to -EOPNOTSUPP.
>
> The way you have it, you're claiming success for the case where there
> is no op and no lower devs...be consistent with same func when
> CONFIG_NET_SWITCHDEV is not defined.

I did it that way initially. But, changed it to the above,  thinking 
that in the case where
the operation succeeded on some lowerdevs and was not supported on some, 
you still would want to return 0 in that case.

If you prefer -EOPNOTSUPP in this case, i have no problem resubmitting.