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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <550D04B2.9000709@cumulusnetworks.com>
Date:	Fri, 20 Mar 2015 22:42:10 -0700
From:	roopa <roopa@...ulusnetworks.com>
To:	Scott Feldman <sfeldma@...il.com>
CC:	"David S. Miller" <davem@...emloft.net>,
	Jiří Pírko <jiri@...nulli.us>,
	Netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v5] switchdev: fix stp update API to work with
 layered netdevices

On 3/20/15, 10:35 PM, Scott Feldman wrote:
> On Fri, Mar 20, 2015 at 10:20 PM,  <roopa@...ulusnetworks.com> wrote:
>> From: Roopa Prabhu <roopa@...ulusnetworks.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@...ulusnetworks.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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html