[PATCH net-next v3 2/5] swdevice: add new api to set and del bridge port attributes

[PATCH net-next v3 2/5] swdevice: add new api to set and del bridge port attributes

[roopa]

From: Roopa Prabhu <roopa@cumulusnetworks.com>

This patch adds two new api's netdev_switch_port_bridge_setlink
and netdev_switch_port_bridge_dellink to offload bridge port attributes
to switch asic

(The names of the apis look odd with 'switch_port_bridge',
but am more inclined to change the prefix of the api to something else.
Will take any suggestions).

The api's look at the NETIF_F_HW_NETFUNC_OFFLOAD feature flag to
pass bridge port attributes to the port device.

If the device has the NETIF_F_HW_NETFUNC_OFFLOAD, but does not support
the bridge port attribute offload ndo, call bridge port attribute ndo's on
the lowerdevs if supported. This is one way to pass bridge port attributes
through stacked netdevs (example when bridge port is a bond and bond slaves
are switch ports).

Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
---
 include/net/switchdev.h   |   17 ++++++++++-
 net/switchdev/switchdev.c |   70 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 86 insertions(+), 1 deletion(-)

diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index 8a6d164..362f53a 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -17,7 +17,10 @@
 int netdev_switch_parent_id_get(struct net_device *dev,
 				struct netdev_phys_item_id *psid);
 int netdev_switch_port_stp_update(struct net_device *dev, u8 state);
-
+int netdev_switch_port_bridge_setlink(struct net_device *dev,
+				struct nlmsghdr *nlh, u16 flags);
+int netdev_switch_port_bridge_dellink(struct net_device *dev,
+				struct nlmsghdr *nlh, u16 flags);
 #else
 
 static inline int netdev_switch_parent_id_get(struct net_device *dev,
@@ -32,6 +35,18 @@ static inline int netdev_switch_port_stp_update(struct net_device *dev,
 	return -EOPNOTSUPP;
 }
 
+int netdev_switch_port_bridge_setlink(struct net_device *dev,
+				      struct nlmsghdr *nlh, u16 flags)
+{
+	return -EOPNOTSUPP;
+}
+
+int netdev_switch_port_bridge_dellink(struct net_device *dev,
+				      struct nlmsghdr *nlh, u16 flags)
+{
+	return -EOPNOTSUPP;
+}
+
 #endif
 
 #endif /* _LINUX_SWITCHDEV_H_ */
diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
index d162b21..bf0be98 100644
--- a/net/switchdev/switchdev.c
+++ b/net/switchdev/switchdev.c
@@ -50,3 +50,73 @@ int netdev_switch_port_stp_update(struct net_device *dev, u8 state)
 	return ops->ndo_switch_port_stp_update(dev, state);
 }
 EXPORT_SYMBOL(netdev_switch_port_stp_update);
+
+/**
+ *	netdev_switch_port_bridge_setlink - Notify switch device port of bridge
+ *	port attributes
+ *
+ *	@dev: port device
+ *	@nlh: netlink msg with bridge port attributes
+ *
+ *	Notify switch device port of bridge port attributes
+ */
+int netdev_switch_port_bridge_setlink(struct net_device *dev,
+				      struct nlmsghdr *nlh, u16 flags)
+{
+	const struct net_device_ops *ops = dev->netdev_ops;
+	struct net_device *lower_dev;
+	struct list_head *iter;
+	int ret = 0, err = 0;
+
+	if (!(dev->features & NETIF_F_HW_NETFUNC_OFFLOAD))
+		return err;
+
+	if (ops->ndo_bridge_setlink) {
+		WARN_ON(!ops->ndo_switch_parent_id_get);
+		return ops->ndo_bridge_setlink(dev, nlh, flags);
+	}
+
+	netdev_for_each_lower_dev(dev, lower_dev, iter) {
+		err = netdev_switch_port_bridge_setlink(lower_dev, nlh, flags);
+		if (err)
+			ret = err;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL(netdev_switch_port_bridge_setlink);
+
+/**
+ *	netdev_switch_port_bridge_dellink - Notify switch device port of bridge
+ *	attribute delete
+ *
+ *	@dev: port device
+ *	@nlh: netlink msg with bridge port attributes
+ *
+ *	Notify switch device port of bridge port attribute delete
+ */
+int netdev_switch_port_bridge_dellink(struct net_device *dev,
+				      struct nlmsghdr *nlh, u16 flags)
+{
+	const struct net_device_ops *ops = dev->netdev_ops;
+	struct net_device *lower_dev;
+	struct list_head *iter;
+	int ret = 0, err = 0;
+
+	if (!(dev->features & NETIF_F_HW_NETFUNC_OFFLOAD))
+		return err;
+
+	if (ops->ndo_bridge_dellink) {
+		WARN_ON(!ops->ndo_switch_parent_id_get);
+		return ops->ndo_bridge_dellink(dev, nlh, flags);
+	}
+
+	netdev_for_each_lower_dev(dev, lower_dev, iter) {
+		err = netdev_switch_port_bridge_dellink(lower_dev, nlh, flags);
+		if (err)
+			ret = err;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL(netdev_switch_port_bridge_dellink);
-- 
1.7.10.4

Re: [PATCH net-next v3 2/5] swdevice: add new api to set and del bridge port attributes

[Jiri Pirko]

Fri, Jan 23, 2015 at 05:33:23AM CET, roopa@cumulusnetworks.com wrote:
>From: Roopa Prabhu <roopa@cumulusnetworks.com>
>
>This patch adds two new api's netdev_switch_port_bridge_setlink
>and netdev_switch_port_bridge_dellink to offload bridge port attributes
>to switch asic
>
>(The names of the apis look odd with 'switch_port_bridge',
>but am more inclined to change the prefix of the api to something else.
>Will take any suggestions).
>
>The api's look at the NETIF_F_HW_NETFUNC_OFFLOAD feature flag to
>pass bridge port attributes to the port device.
>
>If the device has the NETIF_F_HW_NETFUNC_OFFLOAD, but does not support
>the bridge port attribute offload ndo, call bridge port attribute ndo's on
>the lowerdevs if supported. This is one way to pass bridge port attributes
>through stacked netdevs (example when bridge port is a bond and bond slaves
>are switch ports).
>
>Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
>---
> include/net/switchdev.h   |   17 ++++++++++-
> net/switchdev/switchdev.c |   70 +++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 86 insertions(+), 1 deletion(-)
>
>diff --git a/include/net/switchdev.h b/include/net/switchdev.h
>index 8a6d164..362f53a 100644
>--- a/include/net/switchdev.h
>+++ b/include/net/switchdev.h
>@@ -17,7 +17,10 @@
> int netdev_switch_parent_id_get(struct net_device *dev,
> 				struct netdev_phys_item_id *psid);
> int netdev_switch_port_stp_update(struct net_device *dev, u8 state);
>-
>+int netdev_switch_port_bridge_setlink(struct net_device *dev,
>+				struct nlmsghdr *nlh, u16 flags);
>+int netdev_switch_port_bridge_dellink(struct net_device *dev,
>+				struct nlmsghdr *nlh, u16 flags);
> #else
> 
> static inline int netdev_switch_parent_id_get(struct net_device *dev,
>@@ -32,6 +35,18 @@ static inline int netdev_switch_port_stp_update(struct net_device *dev,
> 	return -EOPNOTSUPP;
> }
> 
>+int netdev_switch_port_bridge_setlink(struct net_device *dev,
>+				      struct nlmsghdr *nlh, u16 flags)
>+{
>+	return -EOPNOTSUPP;
>+}
>+
>+int netdev_switch_port_bridge_dellink(struct net_device *dev,
>+				      struct nlmsghdr *nlh, u16 flags)
>+{
>+	return -EOPNOTSUPP;
>+}
>+
> #endif
> 
> #endif /* _LINUX_SWITCHDEV_H_ */
>diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
>index d162b21..bf0be98 100644
>--- a/net/switchdev/switchdev.c
>+++ b/net/switchdev/switchdev.c
>@@ -50,3 +50,73 @@ int netdev_switch_port_stp_update(struct net_device *dev, u8 state)
> 	return ops->ndo_switch_port_stp_update(dev, state);
> }
> EXPORT_SYMBOL(netdev_switch_port_stp_update);
>+
>+/**
>+ *	netdev_switch_port_bridge_setlink - Notify switch device port of bridge
>+ *	port attributes
>+ *
>+ *	@dev: port device
>+ *	@nlh: netlink msg with bridge port attributes
>+ *
>+ *	Notify switch device port of bridge port attributes
>+ */
>+int netdev_switch_port_bridge_setlink(struct net_device *dev,
>+				      struct nlmsghdr *nlh, u16 flags)
>+{
>+	const struct net_device_ops *ops = dev->netdev_ops;
>+	struct net_device *lower_dev;
>+	struct list_head *iter;
>+	int ret = 0, err = 0;
>+
>+	if (!(dev->features & NETIF_F_HW_NETFUNC_OFFLOAD))
>+		return err;
>+
>+	if (ops->ndo_bridge_setlink) {
>+		WARN_ON(!ops->ndo_switch_parent_id_get);
>+		return ops->ndo_bridge_setlink(dev, nlh, flags);
>+	}
>+
>+	netdev_for_each_lower_dev(dev, lower_dev, iter) {
>+		err = netdev_switch_port_bridge_setlink(lower_dev, nlh, flags);
>+		if (err)
>+			ret = err;
>+	}
>+
>+	return ret;
>+}
>+EXPORT_SYMBOL(netdev_switch_port_bridge_setlink);
>+
>+/**
>+ *	netdev_switch_port_bridge_dellink - Notify switch device port of bridge
>+ *	attribute delete
>+ *
>+ *	@dev: port device
>+ *	@nlh: netlink msg with bridge port attributes
>+ *
>+ *	Notify switch device port of bridge port attribute delete
>+ */
>+int netdev_switch_port_bridge_dellink(struct net_device *dev,
>+				      struct nlmsghdr *nlh, u16 flags)
>+{
>+	const struct net_device_ops *ops = dev->netdev_ops;
>+	struct net_device *lower_dev;
>+	struct list_head *iter;
>+	int ret = 0, err = 0;
>+
>+	if (!(dev->features & NETIF_F_HW_NETFUNC_OFFLOAD))
>+		return err;
>+
>+	if (ops->ndo_bridge_dellink) {
>+		WARN_ON(!ops->ndo_switch_parent_id_get);
>+		return ops->ndo_bridge_dellink(dev, nlh, flags);
>+	}
>+
>+	netdev_for_each_lower_dev(dev, lower_dev, iter) {
>+		err = netdev_switch_port_bridge_dellink(lower_dev, nlh, flags);
>+		if (err)
>+			ret = err;
>+	}
>+
>+	return ret;
>+}
>+EXPORT_SYMBOL(netdev_switch_port_bridge_dellink);
>-- 
>1.7.10.4
>

Is there any other place, other than bridge code, this functions are
suppored to be called from? If not, which I consider likely, it would
make more sense to me to:

- move netdev_for_each_lower_dev iterations directly to bridge code
- let the masters (bond, team, ..) implement ndo_bridge_*link and do
  the traversing there (can be in a form of pre-prepared default
  ndo callback (ndo_dflt_netdev_switch_port_bridge_*link)

Re: [PATCH net-next v3 2/5] swdevice: add new api to set and del bridge port attributes

[roopa]

On 1/23/15, 2:41 AM, Jiri Pirko wrote:

<snip..>
>
> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
> index d162b21..bf0be98 100644
> --- a/net/switchdev/switchdev.c
> +++ b/net/switchdev/switchdev.c
> @@ -50,3 +50,73 @@ int netdev_switch_port_stp_update(struct net_device *dev, u8 state)
> 	return ops->ndo_switch_port_stp_update(dev, state);
> }
> EXPORT_SYMBOL(netdev_switch_port_stp_update);
> +
> +/**
> + *	netdev_switch_port_bridge_setlink - Notify switch device port of bridge
> + *	port attributes
> + *
> + *	@dev: port device
> + *	@nlh: netlink msg with bridge port attributes
> + *
> + *	Notify switch device port of bridge port attributes
> + */
> +int netdev_switch_port_bridge_setlink(struct net_device *dev,
> +				      struct nlmsghdr *nlh, u16 flags)
> +{
> +	const struct net_device_ops *ops = dev->netdev_ops;
> +	struct net_device *lower_dev;
> +	struct list_head *iter;
> +	int ret = 0, err = 0;
> +
> +	if (!(dev->features & NETIF_F_HW_NETFUNC_OFFLOAD))
> +		return err;
> +
> +	if (ops->ndo_bridge_setlink) {
> +		WARN_ON(!ops->ndo_switch_parent_id_get);
> +		return ops->ndo_bridge_setlink(dev, nlh, flags);
> +	}
> +
> +	netdev_for_each_lower_dev(dev, lower_dev, iter) {
> +		err = netdev_switch_port_bridge_setlink(lower_dev, nlh, flags);
> +		if (err)
> +			ret = err;
> +	}
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(netdev_switch_port_bridge_setlink);
> +
> +/**
> + *	netdev_switch_port_bridge_dellink - Notify switch device port of bridge
> + *	attribute delete
> + *
> + *	@dev: port device
> + *	@nlh: netlink msg with bridge port attributes
> + *
> + *	Notify switch device port of bridge port attribute delete
> + */
> +int netdev_switch_port_bridge_dellink(struct net_device *dev,
> +				      struct nlmsghdr *nlh, u16 flags)
> +{
> +	const struct net_device_ops *ops = dev->netdev_ops;
> +	struct net_device *lower_dev;
> +	struct list_head *iter;
> +	int ret = 0, err = 0;
> +
> +	if (!(dev->features & NETIF_F_HW_NETFUNC_OFFLOAD))
> +		return err;
> +
> +	if (ops->ndo_bridge_dellink) {
> +		WARN_ON(!ops->ndo_switch_parent_id_get);
> +		return ops->ndo_bridge_dellink(dev, nlh, flags);
> +	}
> +
> +	netdev_for_each_lower_dev(dev, lower_dev, iter) {
> +		err = netdev_switch_port_bridge_dellink(lower_dev, nlh, flags);
> +		if (err)
> +			ret = err;
> +	}
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(netdev_switch_port_bridge_dellink);
> -- 
> 1.7.10.4
>
> Is there any other place, other than bridge code, this functions are
> suppored to be called from?
No other place today. Its usually the master that implements 
ndo_bridge_setlink/dellink.

> If not, which I consider likely, it would
> make more sense to me to:
>
> - move netdev_for_each_lower_dev iterations directly to bridge code
> - let the masters (bond, team, ..) implement ndo_bridge_*link and do
>    the traversing there (can be in a form of pre-prepared default
>    ndo callback (ndo_dflt_netdev_switch_port_bridge_*link)
But, i am still not understanding why i would modify bond, team and 
other slaves
to offload bridge attributes. They could independently do so in the 
future if they
want to do something more than what this default api provides.
The api's setlink/dellink provided by this patch are the default api's.
The drivers can override them if needed in the future (which aligns with 
what you want as well).

Thanks,
Roopa

Re: [PATCH net-next v3 2/5] swdevice: add new api to set and del bridge port attributes

[Jiri Pirko]

Fri, Jan 23, 2015 at 04:58:57PM CET, roopa@cumulusnetworks.com wrote:
>On 1/23/15, 2:41 AM, Jiri Pirko wrote:
>
><snip..>
>>
>>diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
>>index d162b21..bf0be98 100644
>>--- a/net/switchdev/switchdev.c
>>+++ b/net/switchdev/switchdev.c
>>@@ -50,3 +50,73 @@ int netdev_switch_port_stp_update(struct net_device *dev, u8 state)
>>	return ops->ndo_switch_port_stp_update(dev, state);
>>}
>>EXPORT_SYMBOL(netdev_switch_port_stp_update);
>>+
>>+/**
>>+ *	netdev_switch_port_bridge_setlink - Notify switch device port of bridge
>>+ *	port attributes
>>+ *
>>+ *	@dev: port device
>>+ *	@nlh: netlink msg with bridge port attributes
>>+ *
>>+ *	Notify switch device port of bridge port attributes
>>+ */
>>+int netdev_switch_port_bridge_setlink(struct net_device *dev,
>>+				      struct nlmsghdr *nlh, u16 flags)
>>+{
>>+	const struct net_device_ops *ops = dev->netdev_ops;
>>+	struct net_device *lower_dev;
>>+	struct list_head *iter;
>>+	int ret = 0, err = 0;
>>+
>>+	if (!(dev->features & NETIF_F_HW_NETFUNC_OFFLOAD))
>>+		return err;
>>+
>>+	if (ops->ndo_bridge_setlink) {
>>+		WARN_ON(!ops->ndo_switch_parent_id_get);
>>+		return ops->ndo_bridge_setlink(dev, nlh, flags);
>>+	}
>>+
>>+	netdev_for_each_lower_dev(dev, lower_dev, iter) {
>>+		err = netdev_switch_port_bridge_setlink(lower_dev, nlh, flags);
>>+		if (err)
>>+			ret = err;
>>+	}
>>+
>>+	return ret;
>>+}
>>+EXPORT_SYMBOL(netdev_switch_port_bridge_setlink);
>>+
>>+/**
>>+ *	netdev_switch_port_bridge_dellink - Notify switch device port of bridge
>>+ *	attribute delete
>>+ *
>>+ *	@dev: port device
>>+ *	@nlh: netlink msg with bridge port attributes
>>+ *
>>+ *	Notify switch device port of bridge port attribute delete
>>+ */
>>+int netdev_switch_port_bridge_dellink(struct net_device *dev,
>>+				      struct nlmsghdr *nlh, u16 flags)
>>+{
>>+	const struct net_device_ops *ops = dev->netdev_ops;
>>+	struct net_device *lower_dev;
>>+	struct list_head *iter;
>>+	int ret = 0, err = 0;
>>+
>>+	if (!(dev->features & NETIF_F_HW_NETFUNC_OFFLOAD))
>>+		return err;
>>+
>>+	if (ops->ndo_bridge_dellink) {
>>+		WARN_ON(!ops->ndo_switch_parent_id_get);
>>+		return ops->ndo_bridge_dellink(dev, nlh, flags);
>>+	}
>>+
>>+	netdev_for_each_lower_dev(dev, lower_dev, iter) {
>>+		err = netdev_switch_port_bridge_dellink(lower_dev, nlh, flags);
>>+		if (err)
>>+			ret = err;
>>+	}
>>+
>>+	return ret;
>>+}
>>+EXPORT_SYMBOL(netdev_switch_port_bridge_dellink);
>>-- 
>>1.7.10.4
>>
>>Is there any other place, other than bridge code, this functions are
>>suppored to be called from?
>No other place today. Its usually the master that implements
>ndo_bridge_setlink/dellink.
>
>>If not, which I consider likely, it would
>>make more sense to me to:
>>
>>- move netdev_for_each_lower_dev iterations directly to bridge code
>>- let the masters (bond, team, ..) implement ndo_bridge_*link and do
>>   the traversing there (can be in a form of pre-prepared default
>>   ndo callback (ndo_dflt_netdev_switch_port_bridge_*link)
>But, i am still not understanding why i would modify bond, team and other
>slaves

Well, that is the usual way to propagate ndo calls. People are used to
this. It is visible right away in bonding/other code that is propagated
some ndo call to slaves. With your code, that is somehow hidden and only
dependent on NETIF_F_HW_NETFUNC_OFFLOAD flag.

Note that there are only couple of "master drivers" (for this, most likely
only bond and team modifications are needed).
 

>to offload bridge attributes. They could independently do so in the future if
>they
>want to do something more than what this default api provides.
>The api's setlink/dellink provided by this patch are the default api's.
>The drivers can override them if needed in the future (which aligns with what
>you want as well).
>
>Thanks,
>Roopa
>
>
>
>

Re: [PATCH net-next v3 2/5] swdevice: add new api to set and del bridge port attributes

[roopa]

On 1/23/15, 8:06 AM, Jiri Pirko wrote:
> Fri, Jan 23, 2015 at 04:58:57PM CET, roopa@cumulusnetworks.com wrote:
>> On 1/23/15, 2:41 AM, Jiri Pirko wrote:
>>
>> <snip..>
>>> +
>>> +/**
>>> + *	netdev_switch_port_bridge_dellink - Notify switch device port of bridge
>>> + *	attribute delete
>>> + *
>>> + *	@dev: port device
>>> + *	@nlh: netlink msg with bridge port attributes
>>> + *
>>> + *	Notify switch device port of bridge port attribute delete
>>> + */
>>> +int netdev_switch_port_bridge_dellink(struct net_device *dev,
>>> +				      struct nlmsghdr *nlh, u16 flags)
>>> +{
>>> +	const struct net_device_ops *ops = dev->netdev_ops;
>>> +	struct net_device *lower_dev;
>>> +	struct list_head *iter;
>>> +	int ret = 0, err = 0;
>>> +
>>> +	if (!(dev->features & NETIF_F_HW_NETFUNC_OFFLOAD))
>>> +		return err;
>>> +
>>> +	if (ops->ndo_bridge_dellink) {
>>> +		WARN_ON(!ops->ndo_switch_parent_id_get);
>>> +		return ops->ndo_bridge_dellink(dev, nlh, flags);
>>> +	}
>>> +
>>> +	netdev_for_each_lower_dev(dev, lower_dev, iter) {
>>> +		err = netdev_switch_port_bridge_dellink(lower_dev, nlh, flags);
>>> +		if (err)
>>> +			ret = err;
>>> +	}
>>> +
>>> +	return ret;
>>> +}
>>> +EXPORT_SYMBOL(netdev_switch_port_bridge_dellink);
>>> -- 
>>> 1.7.10.4
>>>
>>> Is there any other place, other than bridge code, this functions are
>>> suppored to be called from?
>> No other place today. Its usually the master that implements
>> ndo_bridge_setlink/dellink.
>>
>>> If not, which I consider likely, it would
>>> make more sense to me to:
>>>
>>> - move netdev_for_each_lower_dev iterations directly to bridge code
>>> - let the masters (bond, team, ..) implement ndo_bridge_*link and do
>>>    the traversing there (can be in a form of pre-prepared default
>>>    ndo callback (ndo_dflt_netdev_switch_port_bridge_*link)
>> But, i am still not understanding why i would modify bond, team and other
>> slaves
> Well, that is the usual way to propagate ndo calls. People are used to
> this. It is visible right away in bonding/other code that is propagated
> some ndo call to slaves. With your code, that is somehow hidden and only
> dependent on NETIF_F_HW_NETFUNC_OFFLOAD flag.
>
> Note that there are only couple of "master drivers" (for this, most likely
> only bond and team modifications are needed).
>   
ndo_bridge_setlink today is only implemented by drivers that implement 
bridging function.
So, having the bond and team driver implement it...seems odd.

But if you insist, i am going to do just that.

Re: [PATCH net-next v3 2/5] swdevice: add new api to set and del bridge port attributes

[roopa]

On 1/23/15, 2:45 PM, roopa wrote:
> On 1/23/15, 8:06 AM, Jiri Pirko wrote:
>> Fri, Jan 23, 2015 at 04:58:57PM CET, roopa@cumulusnetworks.com wrote:
>>> On 1/23/15, 2:41 AM, Jiri Pirko wrote:
>>>
>>> <snip..>
>>>> +
>>>> +/**
>>>> + *    netdev_switch_port_bridge_dellink - Notify switch device 
>>>> port of bridge
>>>> + *    attribute delete
>>>> + *
>>>> + *    @dev: port device
>>>> + *    @nlh: netlink msg with bridge port attributes
>>>> + *
>>>> + *    Notify switch device port of bridge port attribute delete
>>>> + */
>>>> +int netdev_switch_port_bridge_dellink(struct net_device *dev,
>>>> +                      struct nlmsghdr *nlh, u16 flags)
>>>> +{
>>>> +    const struct net_device_ops *ops = dev->netdev_ops;
>>>> +    struct net_device *lower_dev;
>>>> +    struct list_head *iter;
>>>> +    int ret = 0, err = 0;
>>>> +
>>>> +    if (!(dev->features & NETIF_F_HW_NETFUNC_OFFLOAD))
>>>> +        return err;
>>>> +
>>>> +    if (ops->ndo_bridge_dellink) {
>>>> +        WARN_ON(!ops->ndo_switch_parent_id_get);
>>>> +        return ops->ndo_bridge_dellink(dev, nlh, flags);
>>>> +    }
>>>> +
>>>> +    netdev_for_each_lower_dev(dev, lower_dev, iter) {
>>>> +        err = netdev_switch_port_bridge_dellink(lower_dev, nlh, 
>>>> flags);
>>>> +        if (err)
>>>> +            ret = err;
>>>> +    }
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +EXPORT_SYMBOL(netdev_switch_port_bridge_dellink);
>>>> -- 
>>>> 1.7.10.4
>>>>
>>>> Is there any other place, other than bridge code, this functions are
>>>> suppored to be called from?
>>> No other place today. Its usually the master that implements
>>> ndo_bridge_setlink/dellink.
>>>
>>>> If not, which I consider likely, it would
>>>> make more sense to me to:
>>>>
>>>> - move netdev_for_each_lower_dev iterations directly to bridge code
>>>> - let the masters (bond, team, ..) implement ndo_bridge_*link and do
>>>>    the traversing there (can be in a form of pre-prepared default
>>>>    ndo callback (ndo_dflt_netdev_switch_port_bridge_*link)
>>> But, i am still not understanding why i would modify bond, team and 
>>> other
>>> slaves
>> Well, that is the usual way to propagate ndo calls. People are used to
>> this. It is visible right away in bonding/other code that is propagated
>> some ndo call to slaves. With your code, that is somehow hidden and only
>> dependent on NETIF_F_HW_NETFUNC_OFFLOAD flag.
>>
>> Note that there are only couple of "master drivers" (for this, most 
>> likely
>> only bond and team modifications are needed).
> ndo_bridge_setlink today is only implemented by drivers that implement 
> bridging function.
> So, having the bond and team driver implement it...seems odd.
>
> But if you insist, i am going to do just that.
A side note, I dont see any reason for ndo_bridge_setlink to be renamed 
to ndo_setlink. Because it seems to take the whole netlink msg anyways. 
It can be used to offload other link attributes besides bridging (vxlan 
and so on).
Any thoughts on that ?.

Re: [PATCH net-next v3 2/5] swdevice: add new api to set and del bridge port attributes

[Jiri Pirko]

Sat, Jan 24, 2015 at 12:10:01AM CET, roopa@cumulusnetworks.com wrote:
>On 1/23/15, 2:45 PM, roopa wrote:
>>On 1/23/15, 8:06 AM, Jiri Pirko wrote:
>>>Fri, Jan 23, 2015 at 04:58:57PM CET, roopa@cumulusnetworks.com wrote:
>>>>On 1/23/15, 2:41 AM, Jiri Pirko wrote:
>>>>
>>>><snip..>
>>>>>+
>>>>>+/**
>>>>>+ *    netdev_switch_port_bridge_dellink - Notify switch device port
>>>>>of bridge
>>>>>+ *    attribute delete
>>>>>+ *
>>>>>+ *    @dev: port device
>>>>>+ *    @nlh: netlink msg with bridge port attributes
>>>>>+ *
>>>>>+ *    Notify switch device port of bridge port attribute delete
>>>>>+ */
>>>>>+int netdev_switch_port_bridge_dellink(struct net_device *dev,
>>>>>+                      struct nlmsghdr *nlh, u16 flags)
>>>>>+{
>>>>>+    const struct net_device_ops *ops = dev->netdev_ops;
>>>>>+    struct net_device *lower_dev;
>>>>>+    struct list_head *iter;
>>>>>+    int ret = 0, err = 0;
>>>>>+
>>>>>+    if (!(dev->features & NETIF_F_HW_NETFUNC_OFFLOAD))
>>>>>+        return err;
>>>>>+
>>>>>+    if (ops->ndo_bridge_dellink) {
>>>>>+        WARN_ON(!ops->ndo_switch_parent_id_get);
>>>>>+        return ops->ndo_bridge_dellink(dev, nlh, flags);
>>>>>+    }
>>>>>+
>>>>>+    netdev_for_each_lower_dev(dev, lower_dev, iter) {
>>>>>+        err = netdev_switch_port_bridge_dellink(lower_dev, nlh,
>>>>>flags);
>>>>>+        if (err)
>>>>>+            ret = err;
>>>>>+    }
>>>>>+
>>>>>+    return ret;
>>>>>+}
>>>>>+EXPORT_SYMBOL(netdev_switch_port_bridge_dellink);
>>>>>-- 
>>>>>1.7.10.4
>>>>>
>>>>>Is there any other place, other than bridge code, this functions are
>>>>>suppored to be called from?
>>>>No other place today. Its usually the master that implements
>>>>ndo_bridge_setlink/dellink.
>>>>
>>>>>If not, which I consider likely, it would
>>>>>make more sense to me to:
>>>>>
>>>>>- move netdev_for_each_lower_dev iterations directly to bridge code
>>>>>- let the masters (bond, team, ..) implement ndo_bridge_*link and do
>>>>>   the traversing there (can be in a form of pre-prepared default
>>>>>   ndo callback (ndo_dflt_netdev_switch_port_bridge_*link)
>>>>But, i am still not understanding why i would modify bond, team and
>>>>other
>>>>slaves
>>>Well, that is the usual way to propagate ndo calls. People are used to
>>>this. It is visible right away in bonding/other code that is propagated
>>>some ndo call to slaves. With your code, that is somehow hidden and only
>>>dependent on NETIF_F_HW_NETFUNC_OFFLOAD flag.
>>>
>>>Note that there are only couple of "master drivers" (for this, most
>>>likely
>>>only bond and team modifications are needed).
>>ndo_bridge_setlink today is only implemented by drivers that implement
>>bridging function.
>>So, having the bond and team driver implement it...seems odd.

Well, it is not odd. It is ok I believe. Same as for example:
.ndo_set_mac_address
.ndo_change_mtu
.ndo_vlan_rx_add_vid
.ndo_vlan_rx_kill_vid
.ndo_poll_controller
.ndo_netpoll_setup
.ndo_netpoll_cleanup

All take care of propagating the ndo to slaves.

>>
>>But if you insist, i am going to do just that.
>A side note, I dont see any reason for ndo_bridge_setlink to be renamed to
>ndo_setlink. Because it seems to take the whole netlink msg anyways. It can
>be used to offload other link attributes besides bridging (vxlan and so on).
>Any thoughts on that ?.

Well, it is PF_BRIDGE so I think that the name is accurate.

>
>
>
>

Re: [PATCH net-next v3 2/5] swdevice: add new api to set and del bridge port attributes

[roopa]

On 1/24/15, 3:26 AM, Jiri Pirko wrote:
> Sat, Jan 24, 2015 at 12:10:01AM CET, roopa@cumulusnetworks.com wrote:
>> On 1/23/15, 2:45 PM, roopa wrote:
>>> On 1/23/15, 8:06 AM, Jiri Pirko wrote:
>>>> Fri, Jan 23, 2015 at 04:58:57PM CET, roopa@cumulusnetworks.com wrote:
>>>>> On 1/23/15, 2:41 AM, Jiri Pirko wrote:
>>>>>
>>>>> <snip..>
>>>>>> +
>>>>>> +/**
>>>>>> + *    netdev_switch_port_bridge_dellink - Notify switch device port
>>>>>> of bridge
>>>>>> + *    attribute delete
>>>>>> + *
>>>>>> + *    @dev: port device
>>>>>> + *    @nlh: netlink msg with bridge port attributes
>>>>>> + *
>>>>>> + *    Notify switch device port of bridge port attribute delete
>>>>>> + */
>>>>>> +int netdev_switch_port_bridge_dellink(struct net_device *dev,
>>>>>> +                      struct nlmsghdr *nlh, u16 flags)
>>>>>> +{
>>>>>> +    const struct net_device_ops *ops = dev->netdev_ops;
>>>>>> +    struct net_device *lower_dev;
>>>>>> +    struct list_head *iter;
>>>>>> +    int ret = 0, err = 0;
>>>>>> +
>>>>>> +    if (!(dev->features & NETIF_F_HW_NETFUNC_OFFLOAD))
>>>>>> +        return err;
>>>>>> +
>>>>>> +    if (ops->ndo_bridge_dellink) {
>>>>>> +        WARN_ON(!ops->ndo_switch_parent_id_get);
>>>>>> +        return ops->ndo_bridge_dellink(dev, nlh, flags);
>>>>>> +    }
>>>>>> +
>>>>>> +    netdev_for_each_lower_dev(dev, lower_dev, iter) {
>>>>>> +        err = netdev_switch_port_bridge_dellink(lower_dev, nlh,
>>>>>> flags);
>>>>>> +        if (err)
>>>>>> +            ret = err;
>>>>>> +    }
>>>>>> +
>>>>>> +    return ret;
>>>>>> +}
>>>>>> +EXPORT_SYMBOL(netdev_switch_port_bridge_dellink);
>>>>>> -- 
>>>>>> 1.7.10.4
>>>>>>
>>>>>> Is there any other place, other than bridge code, this functions are
>>>>>> suppored to be called from?
>>>>> No other place today. Its usually the master that implements
>>>>> ndo_bridge_setlink/dellink.
>>>>>
>>>>>> If not, which I consider likely, it would
>>>>>> make more sense to me to:
>>>>>>
>>>>>> - move netdev_for_each_lower_dev iterations directly to bridge code
>>>>>> - let the masters (bond, team, ..) implement ndo_bridge_*link and do
>>>>>>    the traversing there (can be in a form of pre-prepared default
>>>>>>    ndo callback (ndo_dflt_netdev_switch_port_bridge_*link)
>>>>> But, i am still not understanding why i would modify bond, team and
>>>>> other
>>>>> slaves
>>>> Well, that is the usual way to propagate ndo calls. People are used to
>>>> this. It is visible right away in bonding/other code that is propagated
>>>> some ndo call to slaves. With your code, that is somehow hidden and only
>>>> dependent on NETIF_F_HW_NETFUNC_OFFLOAD flag.
>>>>
>>>> Note that there are only couple of "master drivers" (for this, most
>>>> likely
>>>> only bond and team modifications are needed).
>>> ndo_bridge_setlink today is only implemented by drivers that implement
>>> bridging function.
>>> So, having the bond and team driver implement it...seems odd.
> Well, it is not odd. It is ok I believe. Same as for example:
> .ndo_set_mac_address
> .ndo_change_mtu
> .ndo_vlan_rx_add_vid
> .ndo_vlan_rx_kill_vid
> .ndo_poll_controller
> .ndo_netpoll_setup
> .ndo_netpoll_cleanup
>
> All take care of propagating the ndo to slaves.
These mostly operate on netdevs directly.
I do have some hesitation, But, I will take your suggestion and add it 
to the bond and team drivers,
>
>>> But if you insist, i am going to do just that.
>> A side note, I dont see any reason for ndo_bridge_setlink to be renamed to
>> ndo_setlink. Because it seems to take the whole netlink msg anyways. It can
>> be used to offload other link attributes besides bridging (vxlan and so on).
>> Any thoughts on that ?.
> Well, it is PF_BRIDGE so I think that the name is accurate.
The ndo op does not necessarily have to be tied to the netlink msg family.
I don't plan to rename it now. But, in future if i see more value in 
reusing it for other offloads,
i will come back to this.

Thanks for the review.

RE: [PATCH net-next v3 2/5] swdevice: add new api to set and del bridge port attributes

[Rosen, Rami]

Hi,

Some trivial comments:
+
+/**
+ *	netdev_switch_port_bridge_setlink - Notify switch device port of bridge
+ *	port attributes
+ *
+ *	@dev: port device
+ *	@nlh: netlink msg with bridge port attributes
+ *
+ *	Notify switch device port of bridge port attributes
+ */
+int netdev_switch_port_bridge_setlink(struct net_device *dev,
+				      struct nlmsghdr *nlh, u16 flags) {

- Missing @flags after @nlh above.
- Wouldn't "Notify switch device port of setting bridge port attributes"  or "Notify switch device port of bridge port attributes set" be 
better here (also in terms of being consistent with the description of the netdev_switch_port_bridge_dellink() method later in this patch)? 


+/**
+ *	netdev_switch_port_bridge_dellink - Notify switch device port of bridge
+ *	attribute delete
+ *
+ *	@dev: port device
+ *	@nlh: netlink msg with bridge port attributes
+ *
+ *	Notify switch device port of bridge port attribute delete
+ */
+int netdev_switch_port_bridge_dellink(struct net_device *dev,
+				      struct nlmsghdr *nlh, u16 flags) {

- Again, missing @flags after @nlh for this method also.

Rami Rosen
Intel Corporation