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

[John Fastabend <john.r.fastabend@intel.com>]

On 12/18/2014 07:47 AM, Arad, Ronen wrote:
> 
> 
>> -----Original Message-----
>> From: netdev-owner@vger.kernel.org [mailto:netdev-
>> owner@vger.kernel.org] On Behalf Of Varlese, Marco
>> Sent: Thursday, December 18, 2014 4:56 PM
>> To: Roopa Prabhu
>> 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
>>
>>> -----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.
> 
> Some bridge attributes or more accurately bridge port attributes overlap with port attributes.
> IFLA_BRPORT_LEARNING from IFLA_PROTINFO and BRIDGE_VLAN_INFO_PVID
> flag of IFLA_BRIDGE_VLAN_INFO from IFLA_AF_SPEC are two examples
> where bridge port properties might have to be mapped to a common or
> driver specific port attribute.

You've lost me a bit. 

> Switch port driver that works without and explicit bridge device has
> to implement the bridge NDO's ndo_bridge_setlink/ndo_bridge_dellink. 
> It could take care of mapping such attributes from the bridge netlink
> message to either driver-specific port attribute of to an explicit
> one (assuming IFLA_ATTR_LEARNING would be accepted). For example the
> driver could process the bridge learning information from the
> protinfo as such:

still lost unfortunately.

So you want to enable learning on a port by port basis? Then we also
need to define how the two bits interact.

	switch_learning + port_learning = port in learning mode
	!switch_learning + port_learning = port not in learning mode?
	etc.

Do bridges and users actually enable learning on a per port basis?

> int sw_driver_br_setlink(struct net_device *dev, struct nlmsghdr *nlh)
> {
>         struct nlattr *protinfo;
>         protinfo = nlmsg_find_attr(nlh, sizeof(struct ifinfomsg),
>                                    IFLA_PROTINFO);
>         if (protinfo) {
>                 struct nlattr *attr;
>                 attr = nla_find_nested(protinfo, IFLA_BRPORT_LEARNING);
>                 if (attr) {
>                         if (nla_len(attr) >= sizeof(u8)) {
>                                 if (nla_get_u8(attr))
>                                         brport_flags |= BR_LEARNING;
>                                 else
>                                         brport_flags &= ~BR_LEARNING;
>                         }
>                 }
>                 /*
>                  * Map bridge port attributes to driver-specific port attributes
>                  */
>         }
>         return 0;
> }
> 
>>
>>>> +	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