From: Marco Varlese <[email protected]>
Switch hardware offers a list of attributes that are configurable on a per port
basis.
This patch provides a mechanism to configure switch ports by adding an NDO
for setting specific values to specific attributes.
There will be a separate patch that adds the "get" functionality via another
NDO and another patch that extends iproute2 to call the two new NDOs.
Signed-off-by: Marco Varlese <[email protected]>
---
include/linux/netdevice.h | 5 +++
include/uapi/linux/if_link.h | 15 +++++++++
net/core/rtnetlink.c | 73 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 93 insertions(+)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index c31f74d..4881c7b 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1027,6 +1027,9 @@ typedef u16 (*select_queue_fallback_t)(struct net_device *dev,
* int (*ndo_switch_port_stp_update)(struct net_device *dev, u8 state);
* Called to notify switch device port of bridge port STP
* state change.
+ * int (*ndo_switch_port_set_cfg)(struct net_device *dev,
+ * u32 attr, u64 value);
+ * Called to set specific switch ports attributes.
*/
struct net_device_ops {
int (*ndo_init)(struct net_device *dev);
@@ -1185,6 +1188,8 @@ struct net_device_ops {
struct netdev_phys_item_id *psid);
int (*ndo_switch_port_stp_update)(struct net_device *dev,
u8 state);
+ int (*ndo_switch_port_set_cfg)(struct net_device *dev,
+ u32 attr, u64 value);
#endif
};
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index f7d0d2d..6ad9b91 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -146,6 +146,7 @@ enum {
IFLA_PHYS_PORT_ID,
IFLA_CARRIER_CHANGES,
IFLA_PHYS_SWITCH_ID,
+ IFLA_SWITCH_PORT_CFG,
__IFLA_MAX
};
@@ -603,4 +604,18 @@ enum {
#define IFLA_HSR_MAX (__IFLA_HSR_MAX - 1)
+/* Switch Port Attributes section */
+
+enum {
+ IFLA_SW_UNSPEC,
+ IFLA_SW_LEARNING,
+ IFLA_SW_LOOPBACK,
+ IFLA_SW_BCAST_FLOODING,
+ IFLA_SW_UCAST_FLOODING,
+ IFLA_SW_MCAST_FLOODING,
+ __IFLA_SW_ATTR_MAX
+};
+
+#define IFLA_SW_ATTR_MAX (__IFLA_SW_ATTR_MAX - 1)
+
#endif /* _UAPI_LINUX_IF_LINK_H */
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index eaa057f..d50ca71 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1223,6 +1223,7 @@ static const struct nla_policy ifla_policy[IFLA_MAX+1] = {
[IFLA_PHYS_PORT_ID] = { .type = NLA_BINARY, .len = MAX_PHYS_ITEM_ID_LEN },
[IFLA_CARRIER_CHANGES] = { .type = NLA_U32 }, /* ignored */
[IFLA_PHYS_SWITCH_ID] = { .type = NLA_BINARY, .len = MAX_PHYS_ITEM_ID_LEN },
+ [IFLA_SWITCH_PORT_CFG] = { .type = NLA_NESTED },
};
static const struct nla_policy ifla_info_policy[IFLA_INFO_MAX+1] = {
@@ -1265,6 +1266,14 @@ static const struct nla_policy ifla_port_policy[IFLA_PORT_MAX+1] = {
[IFLA_PORT_RESPONSE] = { .type = NLA_U16, },
};
+static const struct nla_policy ifla_sw_attr_policy[IFLA_SW_ATTR_MAX+1] = {
+ [IFLA_SW_LEARNING] = { .type = NLA_U64 },
+ [IFLA_SW_LOOPBACK] = { .type = NLA_U64 },
+ [IFLA_SW_BCAST_FLOODING] = { .type = NLA_U64 },
+ [IFLA_SW_UCAST_FLOODING] = { .type = NLA_U64 },
+ [IFLA_SW_MCAST_FLOODING] = { .type = NLA_U64 },
+};
+
static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb)
{
struct net *net = sock_net(skb->sk);
@@ -1389,6 +1398,41 @@ static int validate_linkmsg(struct net_device *dev, struct nlattr *tb[])
return 0;
}
+#ifdef CONFIG_NET_SWITCHDEV
+static int do_setswcfg(struct net_device *dev, struct nlattr *attr)
+{
+ int rem, err = -EINVAL;
+ struct nlattr *v;
+ const struct net_device_ops *ops = dev->netdev_ops;
+
+ nla_for_each_nested(v, attr, rem) {
+ u32 op = nla_type(v);
+ u64 value = 0;
+
+ switch (op) {
+ case IFLA_SW_LEARNING:
+ case IFLA_SW_LOOPBACK:
+ case IFLA_SW_BCAST_FLOODING:
+ case IFLA_SW_UCAST_FLOODING:
+ case IFLA_SW_MCAST_FLOODING: {
+ value = nla_get_u64(v);
+ err = ops->ndo_switch_port_set_cfg(dev,
+ op,
+ value);
+ break;
+ }
+ default:
+ err = -EINVAL;
+ break;
+ }
+ if (err)
+ break;
+ }
+ return err;
+}
+
+#endif
+
static int do_setvfinfo(struct net_device *dev, struct nlattr *attr)
{
int rem, err = -EINVAL;
@@ -1740,6 +1784,35 @@ static int do_setlink(const struct sk_buff *skb,
status |= DO_SETLINK_NOTIFY;
}
}
+#ifdef CONFIG_NET_SWITCHDEV
+ if (tb[IFLA_SWITCH_PORT_CFG]) {
+ struct nlattr *attrs[IFLA_SW_ATTR_MAX+1];
+
+ err = -EOPNOTSUPP;
+ if (!ops->ndo_switch_port_set_cfg)
+ goto errout;
+ if (!ops->ndo_switch_parent_id_get)
+ goto errout;
+
+ err = nla_parse_nested(attrs, IFLA_SW_ATTR_MAX,
+ tb[IFLA_SWITCH_PORT_CFG],
+ ifla_sw_attr_policy);
+ if (err < 0)
+ return err;
+
+ err = do_setswcfg(dev, tb[IFLA_SWITCH_PORT_CFG]);
+ if (err < 0)
+ goto errout;
+
+ status |= DO_SETLINK_NOTIFY;
+ }
+#else
+ if (tb[IFLA_SWITCH_PORT_CFG]) {
+ err = -EOPNOTSUPP;
+ goto errout;
+ }
+#endif
+
err = 0;
errout:
--
1.8.5.3
On 12/18/2014 07:30 AM, Varlese, Marco wrote:
> From: Marco Varlese <[email protected]>
>
> Switch hardware offers a list of attributes that are configurable on a per port
> basis.
> This patch provides a mechanism to configure switch ports by adding an NDO
> for setting specific values to specific attributes.
> There will be a separate patch that adds the "get" functionality via another
> NDO and another patch that extends iproute2 to call the two new NDOs.
>
> Signed-off-by: Marco Varlese <[email protected]>
> ---
> include/linux/netdevice.h | 5 +++
> include/uapi/linux/if_link.h | 15 +++++++++
> net/core/rtnetlink.c | 73 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 93 insertions(+)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index c31f74d..4881c7b 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1027,6 +1027,9 @@ typedef u16 (*select_queue_fallback_t)(struct net_device *dev,
> * int (*ndo_switch_port_stp_update)(struct net_device *dev, u8 state);
> * Called to notify switch device port of bridge port STP
> * state change.
> + * int (*ndo_switch_port_set_cfg)(struct net_device *dev,
> + * u32 attr, u64 value);
> + * Called to set specific switch ports attributes.
> */
> struct net_device_ops {
> int (*ndo_init)(struct net_device *dev);
> @@ -1185,6 +1188,8 @@ struct net_device_ops {
> struct netdev_phys_item_id *psid);
> int (*ndo_switch_port_stp_update)(struct net_device *dev,
> u8 state);
> + int (*ndo_switch_port_set_cfg)(struct net_device *dev,
> + u32 attr, u64 value);
> #endif
> };
>
> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
> index f7d0d2d..6ad9b91 100644
> --- a/include/uapi/linux/if_link.h
> +++ b/include/uapi/linux/if_link.h
> @@ -146,6 +146,7 @@ enum {
> IFLA_PHYS_PORT_ID,
> IFLA_CARRIER_CHANGES,
> IFLA_PHYS_SWITCH_ID,
> + IFLA_SWITCH_PORT_CFG,
> __IFLA_MAX
> };
>
> @@ -603,4 +604,18 @@ enum {
>
> #define IFLA_HSR_MAX (__IFLA_HSR_MAX - 1)
>
> +/* Switch Port Attributes section */
Could you also document the attributes. I think they are mostly
clear but what is IFLA_SW_LOOPBACK. It will help later when we
try to read the code in 6months and implement drivers.
I am thinking something like
/* Switch Port Attributes section
* IFLA_SW_LEARNING - turns learning on in the bridge
* IFLA_SW_LOOPBACK - does something interesting
[...]
*/
> +
> +enum {
> + IFLA_SW_UNSPEC,
> + IFLA_SW_LEARNING,
Can you address Roopa's feedback. I'm also a bit confused by the
duplication.
> + IFLA_SW_LOOPBACK,
> + IFLA_SW_BCAST_FLOODING,
> + IFLA_SW_UCAST_FLOODING,
> + IFLA_SW_MCAST_FLOODING,
> + __IFLA_SW_ATTR_MAX
> +};
> +
> +#define IFLA_SW_ATTR_MAX (__IFLA_SW_ATTR_MAX - 1)
> +
> #endif /* _UAPI_LINUX_IF_LINK_H */
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index eaa057f..d50ca71 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -1223,6 +1223,7 @@ static const struct nla_policy ifla_policy[IFLA_MAX+1] = {
> [IFLA_PHYS_PORT_ID] = { .type = NLA_BINARY, .len = MAX_PHYS_ITEM_ID_LEN },
> [IFLA_CARRIER_CHANGES] = { .type = NLA_U32 }, /* ignored */
> [IFLA_PHYS_SWITCH_ID] = { .type = NLA_BINARY, .len = MAX_PHYS_ITEM_ID_LEN },
> + [IFLA_SWITCH_PORT_CFG] = { .type = NLA_NESTED },
> };
>
> static const struct nla_policy ifla_info_policy[IFLA_INFO_MAX+1] = {
> @@ -1265,6 +1266,14 @@ static const struct nla_policy ifla_port_policy[IFLA_PORT_MAX+1] = {
> [IFLA_PORT_RESPONSE] = { .type = NLA_U16, },
> };
>
> +static const struct nla_policy ifla_sw_attr_policy[IFLA_SW_ATTR_MAX+1] = {
> + [IFLA_SW_LEARNING] = { .type = NLA_U64 },
> + [IFLA_SW_LOOPBACK] = { .type = NLA_U64 },
> + [IFLA_SW_BCAST_FLOODING] = { .type = NLA_U64 },
> + [IFLA_SW_UCAST_FLOODING] = { .type = NLA_U64 },
> + [IFLA_SW_MCAST_FLOODING] = { .type = NLA_U64 },
> +};
Why U64 values? What would we pass in these? Are these just boolean
bits? Maybe the annotation above will help me understand this.
> +
> static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb)
> {
> struct net *net = sock_net(skb->sk);
> @@ -1389,6 +1398,41 @@ static int validate_linkmsg(struct net_device *dev, struct nlattr *tb[])
> return 0;
> }
>
> +#ifdef CONFIG_NET_SWITCHDEV
> +static int do_setswcfg(struct net_device *dev, struct nlattr *attr)
> +{
> + int rem, err = -EINVAL;
> + struct nlattr *v;
> + const struct net_device_ops *ops = dev->netdev_ops;
> +
style nit again since its an RFC after all... Its preferred to arrange
arguments like this as a loosly followed convention,
const struct net_device_ops *ops = dev->netdev_ops;
int rem, err = -EINVAL;
struct nlattr *v;
> + nla_for_each_nested(v, attr, rem) {
> + u32 op = nla_type(v);
> + u64 value = 0;
> +
> + switch (op) {
> + case IFLA_SW_LEARNING:
> + case IFLA_SW_LOOPBACK:
> + case IFLA_SW_BCAST_FLOODING:
> + case IFLA_SW_UCAST_FLOODING:
> + case IFLA_SW_MCAST_FLOODING: {
> + value = nla_get_u64(v);
should we validate the get_u64 'value'? Are there valid ranges or
something?
> + err = ops->ndo_switch_port_set_cfg(dev,
> + op,
> + value);
> + break;
> + }
> + default:
> + err = -EINVAL;
> + break;
> + }
> + if (err)
> + break;
> + }
> + return err;
> +}
> +
> +#endif
> +
> static int do_setvfinfo(struct net_device *dev, struct nlattr *attr)
> {
> int rem, err = -EINVAL;
> @@ -1740,6 +1784,35 @@ static int do_setlink(const struct sk_buff *skb,
> status |= DO_SETLINK_NOTIFY;
> }
> }
> +#ifdef CONFIG_NET_SWITCHDEV
> + if (tb[IFLA_SWITCH_PORT_CFG]) {
> + struct nlattr *attrs[IFLA_SW_ATTR_MAX+1];
> +
> + err = -EOPNOTSUPP;
> + if (!ops->ndo_switch_port_set_cfg)
> + goto errout;
> + if (!ops->ndo_switch_parent_id_get)
> + goto errout;
> +
style nit (take it for what its worth) but I would compress the above to
if (!ops->ndo_switch_port_set_cfg ||
!ops->ndo_switch_parent_id_get)
goto errout;
I'm also wondering if we really need to check for parent_id_get() here.
I'm not sure I see why a driver would implement port_set_cfg() and not
parent_id_get() though so its mostly harmless.
> + err = nla_parse_nested(attrs, IFLA_SW_ATTR_MAX,
> + tb[IFLA_SWITCH_PORT_CFG],
> + ifla_sw_attr_policy);
> + if (err < 0)
> + return err;
hmm everywhere else you use goto errout but not here?
> +
> + err = do_setswcfg(dev, tb[IFLA_SWITCH_PORT_CFG]);
> + if (err < 0)
> + goto errout;
> +
> + status |= DO_SETLINK_NOTIFY;
hmm another question if you modify the hardware from do_setswcfg()
for an attribute but then fail on a subsequent attribute shouldn't
we have DO_SETLINK_MODIFY set? Say IFLA_SW_LEARNING is set but then
the device fails on IFLA_SW_LOOPBACK.
> + }
> +#else
> + if (tb[IFLA_SWITCH_PORT_CFG]) {
> + err = -EOPNOTSUPP;
> + goto errout;
> + }
> +#endif
> +
> err = 0;
>
> errout:
>
--
John Fastabend Intel Corporation
On 12/18/2014 7:30 AM, Varlese, Marco wrote:
> From: Marco Varlese <[email protected]>
>
> Switch hardware offers a list of attributes that are configurable on a per port
> basis.
> This patch provides a mechanism to configure switch ports by adding an NDO
> for setting specific values to specific attributes.
> There will be a separate patch that adds the "get" functionality via another
> NDO and another patch that extends iproute2 to call the two new NDOs.
>
> Signed-off-by: Marco Varlese <[email protected]>
> ---
> include/linux/netdevice.h | 5 +++
> include/uapi/linux/if_link.h | 15 +++++++++
> net/core/rtnetlink.c | 73 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 93 insertions(+)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index c31f74d..4881c7b 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1027,6 +1027,9 @@ typedef u16 (*select_queue_fallback_t)(struct net_device *dev,
> * int (*ndo_switch_port_stp_update)(struct net_device *dev, u8 state);
> * Called to notify switch device port of bridge port STP
> * state change.
> + * int (*ndo_switch_port_set_cfg)(struct net_device *dev,
> + * u32 attr, u64 value);
> + * Called to set specific switch ports attributes.
> */
> struct net_device_ops {
> int (*ndo_init)(struct net_device *dev);
> @@ -1185,6 +1188,8 @@ struct net_device_ops {
> struct netdev_phys_item_id *psid);
> int (*ndo_switch_port_stp_update)(struct net_device *dev,
> u8 state);
> + int (*ndo_switch_port_set_cfg)(struct net_device *dev,
> + u32 attr, u64 value);
Is it OK to have a requirement that all the attributes are 64 bit values?
Isn't it more flexible to pass an explicit type of port attributes?
(*ndo_switch_port_set_cfg(struct net_device *dev, u32 attr, u8
attr_type, void *value)
> #endif
> };
>
> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
> index f7d0d2d..6ad9b91 100644
> --- a/include/uapi/linux/if_link.h
> +++ b/include/uapi/linux/if_link.h
> @@ -146,6 +146,7 @@ enum {
> IFLA_PHYS_PORT_ID,
> IFLA_CARRIER_CHANGES,
> IFLA_PHYS_SWITCH_ID,
> + IFLA_SWITCH_PORT_CFG,
> __IFLA_MAX
> };
>
> @@ -603,4 +604,18 @@ enum {
>
> #define IFLA_HSR_MAX (__IFLA_HSR_MAX - 1)
>
> +/* Switch Port Attributes section */
> +
> +enum {
> + IFLA_SW_UNSPEC,
> + IFLA_SW_LEARNING,
> + IFLA_SW_LOOPBACK,
> + IFLA_SW_BCAST_FLOODING,
> + IFLA_SW_UCAST_FLOODING,
> + IFLA_SW_MCAST_FLOODING,
> + __IFLA_SW_ATTR_MAX
> +};
> +
> +#define IFLA_SW_ATTR_MAX (__IFLA_SW_ATTR_MAX - 1)
> +
> #endif /* _UAPI_LINUX_IF_LINK_H */
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index eaa057f..d50ca71 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -1223,6 +1223,7 @@ static const struct nla_policy ifla_policy[IFLA_MAX+1] = {
> [IFLA_PHYS_PORT_ID] = { .type = NLA_BINARY, .len = MAX_PHYS_ITEM_ID_LEN },
> [IFLA_CARRIER_CHANGES] = { .type = NLA_U32 }, /* ignored */
> [IFLA_PHYS_SWITCH_ID] = { .type = NLA_BINARY, .len = MAX_PHYS_ITEM_ID_LEN },
> + [IFLA_SWITCH_PORT_CFG] = { .type = NLA_NESTED },
> };
>
> static const struct nla_policy ifla_info_policy[IFLA_INFO_MAX+1] = {
> @@ -1265,6 +1266,14 @@ static const struct nla_policy ifla_port_policy[IFLA_PORT_MAX+1] = {
> [IFLA_PORT_RESPONSE] = { .type = NLA_U16, },
> };
>
> +static const struct nla_policy ifla_sw_attr_policy[IFLA_SW_ATTR_MAX+1] = {
> + [IFLA_SW_LEARNING] = { .type = NLA_U64 },
> + [IFLA_SW_LOOPBACK] = { .type = NLA_U64 },
> + [IFLA_SW_BCAST_FLOODING] = { .type = NLA_U64 },
> + [IFLA_SW_UCAST_FLOODING] = { .type = NLA_U64 },
> + [IFLA_SW_MCAST_FLOODING] = { .type = NLA_U64 },
> +};
> +
> static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb)
> {
> struct net *net = sock_net(skb->sk);
> @@ -1389,6 +1398,41 @@ static int validate_linkmsg(struct net_device *dev, struct nlattr *tb[])
> return 0;
> }
>
> +#ifdef CONFIG_NET_SWITCHDEV
> +static int do_setswcfg(struct net_device *dev, struct nlattr *attr)
> +{
> + int rem, err = -EINVAL;
> + struct nlattr *v;
> + const struct net_device_ops *ops = dev->netdev_ops;
> +
> + nla_for_each_nested(v, attr, rem) {
> + u32 op = nla_type(v);
> + u64 value = 0;
> +
> + switch (op) {
> + case IFLA_SW_LEARNING:
> + case IFLA_SW_LOOPBACK:
> + case IFLA_SW_BCAST_FLOODING:
> + case IFLA_SW_UCAST_FLOODING:
> + case IFLA_SW_MCAST_FLOODING: {
> + value = nla_get_u64(v);
> + err = ops->ndo_switch_port_set_cfg(dev,
> + op,
> + value);
> + break;
> + }
> + default:
> + err = -EINVAL;
> + break;
> + }
> + if (err)
> + break;
> + }
> + return err;
> +}
> +
> +#endif
> +
> static int do_setvfinfo(struct net_device *dev, struct nlattr *attr)
> {
> int rem, err = -EINVAL;
> @@ -1740,6 +1784,35 @@ static int do_setlink(const struct sk_buff *skb,
> status |= DO_SETLINK_NOTIFY;
> }
> }
> +#ifdef CONFIG_NET_SWITCHDEV
> + if (tb[IFLA_SWITCH_PORT_CFG]) {
> + struct nlattr *attrs[IFLA_SW_ATTR_MAX+1];
> +
> + err = -EOPNOTSUPP;
> + if (!ops->ndo_switch_port_set_cfg)
> + goto errout;
> + if (!ops->ndo_switch_parent_id_get)
> + goto errout;
> +
> + err = nla_parse_nested(attrs, IFLA_SW_ATTR_MAX,
> + tb[IFLA_SWITCH_PORT_CFG],
> + ifla_sw_attr_policy);
> + if (err < 0)
> + return err;
> +
> + err = do_setswcfg(dev, tb[IFLA_SWITCH_PORT_CFG]);
> + if (err < 0)
> + goto errout;
> +
> + status |= DO_SETLINK_NOTIFY;
> + }
> +#else
> + if (tb[IFLA_SWITCH_PORT_CFG]) {
> + err = -EOPNOTSUPP;
> + goto errout;
> + }
> +#endif
> +
> err = 0;
>
> errout:
Thu, Dec 18, 2014 at 04:30:00PM CET, [email protected] wrote:
>From: Marco Varlese <[email protected]>
>
>Switch hardware offers a list of attributes that are configurable on a per port
>basis.
>This patch provides a mechanism to configure switch ports by adding an NDO
>for setting specific values to specific attributes.
>There will be a separate patch that adds the "get" functionality via another
>NDO and another patch that extends iproute2 to call the two new NDOs.
>
>Signed-off-by: Marco Varlese <[email protected]>
>---
> include/linux/netdevice.h | 5 +++
> include/uapi/linux/if_link.h | 15 +++++++++
> net/core/rtnetlink.c | 73 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 93 insertions(+)
>
>diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>index c31f74d..4881c7b 100644
>--- a/include/linux/netdevice.h
>+++ b/include/linux/netdevice.h
>@@ -1027,6 +1027,9 @@ typedef u16 (*select_queue_fallback_t)(struct net_device *dev,
> * int (*ndo_switch_port_stp_update)(struct net_device *dev, u8 state);
> * Called to notify switch device port of bridge port STP
> * state change.
>+ * int (*ndo_switch_port_set_cfg)(struct net_device *dev,
>+ * u32 attr, u64 value);
>+ * Called to set specific switch ports attributes.
> */
> struct net_device_ops {
> int (*ndo_init)(struct net_device *dev);
>@@ -1185,6 +1188,8 @@ struct net_device_ops {
> struct netdev_phys_item_id *psid);
> int (*ndo_switch_port_stp_update)(struct net_device *dev,
> u8 state);
>+ int (*ndo_switch_port_set_cfg)(struct net_device *dev,
>+ u32 attr, u64 value);
How about get? Userspace should be able to read the values as well.
> #endif
> };
>
>diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
>index f7d0d2d..6ad9b91 100644
>--- a/include/uapi/linux/if_link.h
>+++ b/include/uapi/linux/if_link.h
>@@ -146,6 +146,7 @@ enum {
> IFLA_PHYS_PORT_ID,
> IFLA_CARRIER_CHANGES,
> IFLA_PHYS_SWITCH_ID,
>+ IFLA_SWITCH_PORT_CFG,
> __IFLA_MAX
> };
>
>@@ -603,4 +604,18 @@ enum {
>
> #define IFLA_HSR_MAX (__IFLA_HSR_MAX - 1)
>
>+/* Switch Port Attributes section */
>+
>+enum {
>+ IFLA_SW_UNSPEC,
>+ IFLA_SW_LEARNING,
>+ IFLA_SW_LOOPBACK,
>+ IFLA_SW_BCAST_FLOODING,
>+ IFLA_SW_UCAST_FLOODING,
>+ IFLA_SW_MCAST_FLOODING,
>+ __IFLA_SW_ATTR_MAX
>+};
>+
>+#define IFLA_SW_ATTR_MAX (__IFLA_SW_ATTR_MAX - 1)
>+
> #endif /* _UAPI_LINUX_IF_LINK_H */
>diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
>index eaa057f..d50ca71 100644
>--- a/net/core/rtnetlink.c
>+++ b/net/core/rtnetlink.c
>@@ -1223,6 +1223,7 @@ static const struct nla_policy ifla_policy[IFLA_MAX+1] = {
> [IFLA_PHYS_PORT_ID] = { .type = NLA_BINARY, .len = MAX_PHYS_ITEM_ID_LEN },
> [IFLA_CARRIER_CHANGES] = { .type = NLA_U32 }, /* ignored */
> [IFLA_PHYS_SWITCH_ID] = { .type = NLA_BINARY, .len = MAX_PHYS_ITEM_ID_LEN },
>+ [IFLA_SWITCH_PORT_CFG] = { .type = NLA_NESTED },
> };
>
> static const struct nla_policy ifla_info_policy[IFLA_INFO_MAX+1] = {
>@@ -1265,6 +1266,14 @@ static const struct nla_policy ifla_port_policy[IFLA_PORT_MAX+1] = {
> [IFLA_PORT_RESPONSE] = { .type = NLA_U16, },
> };
>
>+static const struct nla_policy ifla_sw_attr_policy[IFLA_SW_ATTR_MAX+1] = {
>+ [IFLA_SW_LEARNING] = { .type = NLA_U64 },
>+ [IFLA_SW_LOOPBACK] = { .type = NLA_U64 },
>+ [IFLA_SW_BCAST_FLOODING] = { .type = NLA_U64 },
>+ [IFLA_SW_UCAST_FLOODING] = { .type = NLA_U64 },
>+ [IFLA_SW_MCAST_FLOODING] = { .type = NLA_U64 },
Please maintain namespaces. Use IFLA_SWITCH_PORT_LEARNING for example.
That gives us place for possible future whole-switch-cfg.
>+};
>+
> static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb)
> {
> struct net *net = sock_net(skb->sk);
>@@ -1389,6 +1398,41 @@ static int validate_linkmsg(struct net_device *dev, struct nlattr *tb[])
> return 0;
> }
>
>+#ifdef CONFIG_NET_SWITCHDEV
>+static int do_setswcfg(struct net_device *dev, struct nlattr *attr)
>+{
>+ int rem, err = -EINVAL;
>+ struct nlattr *v;
>+ const struct net_device_ops *ops = dev->netdev_ops;
>+
>+ nla_for_each_nested(v, attr, rem) {
>+ u32 op = nla_type(v);
>+ u64 value = 0;
>+
>+ switch (op) {
>+ case IFLA_SW_LEARNING:
>+ case IFLA_SW_LOOPBACK:
>+ case IFLA_SW_BCAST_FLOODING:
>+ case IFLA_SW_UCAST_FLOODING:
>+ case IFLA_SW_MCAST_FLOODING: {
>+ value = nla_get_u64(v);
>+ err = ops->ndo_switch_port_set_cfg(dev,
>+ op,
>+ value);
>+ break;
>+ }
>+ default:
>+ err = -EINVAL;
>+ break;
>+ }
>+ if (err)
>+ break;
>+ }
>+ return err;
>+}
>+
>+#endif
>+
> static int do_setvfinfo(struct net_device *dev, struct nlattr *attr)
> {
> int rem, err = -EINVAL;
>@@ -1740,6 +1784,35 @@ static int do_setlink(const struct sk_buff *skb,
> status |= DO_SETLINK_NOTIFY;
> }
> }
>+#ifdef CONFIG_NET_SWITCHDEV
>+ if (tb[IFLA_SWITCH_PORT_CFG]) {
>+ struct nlattr *attrs[IFLA_SW_ATTR_MAX+1];
>+
>+ err = -EOPNOTSUPP;
>+ if (!ops->ndo_switch_port_set_cfg)
>+ goto errout;
>+ if (!ops->ndo_switch_parent_id_get)
>+ goto errout;
>+
>+ err = nla_parse_nested(attrs, IFLA_SW_ATTR_MAX,
>+ tb[IFLA_SWITCH_PORT_CFG],
>+ ifla_sw_attr_policy);
>+ if (err < 0)
>+ return err;
>+
>+ err = do_setswcfg(dev, tb[IFLA_SWITCH_PORT_CFG]);
>+ if (err < 0)
>+ goto errout;
>+
Would make sense to me to move this including do_setswcfg into
net/switchdev/switchdev.c
>+ status |= DO_SETLINK_NOTIFY;
>+ }
>+#else
>+ if (tb[IFLA_SWITCH_PORT_CFG]) {
>+ err = -EOPNOTSUPP;
>+ goto errout;
>+ }
>+#endif
>+
> err = 0;
>
> errout:
>--
>1.8.5.3
>
On 12/18/14 at 08:03am, John Fastabend wrote:
> On 12/18/2014 07:30 AM, Varlese, Marco wrote:
> Could you also document the attributes. I think they are mostly
> clear but what is IFLA_SW_LOOPBACK. It will help later when we
> try to read the code in 6months and implement drivers.
>
> I am thinking something like
>
> /* Switch Port Attributes section
> * IFLA_SW_LEARNING - turns learning on in the bridge
> * IFLA_SW_LOOPBACK - does something interesting
>
> [...]
> */
+1. I would even ask for more than that. While clear in the bridge
context, "learning" for this API targetting multi layer switches
is ambigious. The expectation towards the driver must be crystical
clear.
> >+
> >+enum {
> >+ IFLA_SW_UNSPEC,
> >+ IFLA_SW_LEARNING,
>
> Can you address Roopa's feedback. I'm also a bit confused by the
> duplication.
Agreed. Can we decide on the ndo first and then build the APIs on
top of that? While I agree that we should have a non-bridge based
Netlink API, the underlying ndo should be the same.
> >+static const struct nla_policy ifla_sw_attr_policy[IFLA_SW_ATTR_MAX+1] = {
> >+ [IFLA_SW_LEARNING] = { .type = NLA_U64 },
> >+ [IFLA_SW_LOOPBACK] = { .type = NLA_U64 },
> >+ [IFLA_SW_BCAST_FLOODING] = { .type = NLA_U64 },
> >+ [IFLA_SW_UCAST_FLOODING] = { .type = NLA_U64 },
> >+ [IFLA_SW_MCAST_FLOODING] = { .type = NLA_U64 },
> >+};
>
> Why U64 values? What would we pass in these? Are these just boolean
> bits? Maybe the annotation above will help me understand this.
I think the intent is to keep the ndo API as simple as possible
but I agree that this is wasteful. I gave this feedback on v2 already.
Fri, Dec 19, 2014 at 01:35:10AM CET, [email protected] wrote:
>On 12/18/14 at 08:03am, John Fastabend wrote:
>> On 12/18/2014 07:30 AM, Varlese, Marco wrote:
>> Could you also document the attributes. I think they are mostly
>> clear but what is IFLA_SW_LOOPBACK. It will help later when we
>> try to read the code in 6months and implement drivers.
>>
>> I am thinking something like
>>
>> /* Switch Port Attributes section
>> * IFLA_SW_LEARNING - turns learning on in the bridge
>> * IFLA_SW_LOOPBACK - does something interesting
>>
>> [...]
>> */
>
>+1. I would even ask for more than that. While clear in the bridge
>context, "learning" for this API targetting multi layer switches
>is ambigious. The expectation towards the driver must be crystical
>clear.
>
>> >+
>> >+enum {
>> >+ IFLA_SW_UNSPEC,
>> >+ IFLA_SW_LEARNING,
>>
>> Can you address Roopa's feedback. I'm also a bit confused by the
>> duplication.
>
>Agreed. Can we decide on the ndo first and then build the APIs on
>top of that? While I agree that we should have a non-bridge based
>Netlink API, the underlying ndo should be the same.
Maybe we can use this kind of ndos (proposed by this patch) and call it
for switchdevs instead of bridge_get/setlink. Would make sense to me.
single set of ndos, many possible users (userspace/in-kernel).
bridge_get/setlink would be just a wrapper for this.
>
>> >+static const struct nla_policy ifla_sw_attr_policy[IFLA_SW_ATTR_MAX+1] = {
>> >+ [IFLA_SW_LEARNING] = { .type = NLA_U64 },
>> >+ [IFLA_SW_LOOPBACK] = { .type = NLA_U64 },
>> >+ [IFLA_SW_BCAST_FLOODING] = { .type = NLA_U64 },
>> >+ [IFLA_SW_UCAST_FLOODING] = { .type = NLA_U64 },
>> >+ [IFLA_SW_MCAST_FLOODING] = { .type = NLA_U64 },
>> >+};
>>
>> Why U64 values? What would we pass in these? Are these just boolean
>> bits? Maybe the annotation above will help me understand this.
>
>I think the intent is to keep the ndo API as simple as possible
>but I agree that this is wasteful. I gave this feedback on v2 already.