Re: [RFC PATCH net-next v2 1/1] net: Support for switch port configuration

Roopa Prabhu
2014-12-18T15:16:27Z

On 12/18/14, 6:55 AM, Varlese, Marco wrote:
>> -----Original Message-----
>> From: Roopa Prabhu [mailto:roopa@cumulusnetworks.com]
>> Sent: Thursday, December 18, 2014 2:45 PM
>> To: Varlese, Marco
>> Cc: netdev@vger.kernel.org; Fastabend, John R; Thomas Graf; Jiri Pirko;
>> sfeldma@gmail.com; linux-kernel@vger.kernel.org
>> Subject: Re: [RFC PATCH net-next v2 1/1] net: Support for switch port
>> configuration
>>
>> On 12/18/14, 3:29 AM, Varlese, Marco wrote:
>>> From: Marco Varlese <marco.varlese@intel.com>
>>>
>>> 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 <marco.varlese@intel.com>
>>> ---
>>> include/linux/netdevice.h | 5 ++++
>>> include/uapi/linux/if_link.h | 15 ++++++++++++
>>> net/core/rtnetlink.c | 54
>> ++++++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 74 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..19cb51a 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_ATTR_UNSPEC,
>>> + IFLA_ATTR_LEARNING,
>> Any reason you want learning here ?. This is covered as part of the bridge
>> setlink attributes.
>>
> Yes, because the user may _not_ want to go through a bridge interface necessarily.
But, the bridge setlink/getlink interface was changed to accommodate
'self' for exactly such cases.
I kind of understand your case for the other attributes (these are per
port settings that switch asics provide).

However, i don't understand the reason to pull in bridge attributes here.

>
>>> + IFLA_ATTR_LOOPBACK,
>>> + IFLA_ATTR_BCAST_FLOODING,
>>> + IFLA_ATTR_UCAST_FLOODING,
>>> + IFLA_ATTR_MCAST_FLOODING,
>>> + __IFLA_ATTR_MAX
>>> +};
>>> +
>>> +#define IFLA_ATTR_MAX (__IFLA_ATTR_MAX - 1)
>>> +
>>> #endif /* _UAPI_LINUX_IF_LINK_H */
>>> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index
>>> eaa057f..c0d1c45 100644
>>> --- a/net/core/rtnetlink.c
>>> +++ b/net/core/rtnetlink.c
>>> @@ -1389,6 +1389,46 @@ 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_ATTR_LEARNING:
>>> + case IFLA_ATTR_LOOPBACK:
>>> + case IFLA_ATTR_BCAST_FLOODING:
>>> + case IFLA_ATTR_UCAST_FLOODING:
>>> + case IFLA_ATTR_MCAST_FLOODING: {
>>> + if (nla_len(v) < sizeof(value)) {
>>> + err = -EINVAL;
>>> + break;
>>> + }
>>> +
>>> + 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 +1780,20 @@ static int do_setlink(const struct sk_buff *skb,
>>> status |= DO_SETLINK_NOTIFY;
>>> }
>>> }
>>> +#ifdef CONFIG_NET_SWITCHDEV
>>> + if (tb[IFLA_SWITCH_PORT_CFG]) {
>>> + err = -EOPNOTSUPP;
>>> + if (!ops->ndo_switch_port_set_cfg)
>>> + goto errout;
>>> + if (!ops->ndo_switch_parent_id_get)
>>> + goto errout;
>>> + err = do_setswcfg(dev, tb[IFLA_SWITCH_PORT_CFG]);
>>> + if (err < 0)
>>> + goto errout;
>>> +
>>> + status |= DO_SETLINK_NOTIFY;
>>> + }
>>> +#endif
>>> err = 0;
>>>
>>> errout:
> --
> 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