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

[Varlese, Marco]

> -----Original Message-----
> From: netdev-owner@xxxxxxxxxxxxxxx [mailto:netdev-
> owner@xxxxxxxxxxxxxxx] On Behalf Of Roopa Prabhu
> Sent: Thursday, December 18, 2014 3:16 PM
> To: Varlese, Marco
> Cc: netdev@xxxxxxxxxxxxxxx; Fastabend, John R; Thomas Graf; Jiri Pirko;
> sfeldma@xxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [RFC PATCH net-next v2 1/1] net: Support for switch port
> configuration
> 
> On 12/18/14, 6:55 AM, Varlese, Marco wrote:
> >> -----Original Message-----
> >> From: Roopa Prabhu [mailto:roopa@xxxxxxxxxxxxxxxxxxx]
> >> Sent: Thursday, December 18, 2014 2:45 PM
> >> To: Varlese, Marco
> >> Cc: netdev@xxxxxxxxxxxxxxx; Fastabend, John R; Thomas Graf; Jiri
> >> Pirko; sfeldma@xxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> >> 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@xxxxxxxxx>
> >>>
> >>> 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@xxxxxxxxx>
> >>> ---
> >>>    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.
> 

Maybe, I am missing something so you might help. The learning attribute - in my case - it is like all other attributes: a port attribute (as you said, port settings that the switch provides per port). 
So, what I was saying is "why the user shall go through a bridge to configure the learning attribute"? From my perspective, it is as any other attribute and as such configurable on the port.

> >
> >>> +	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@xxxxxxxxxxxxxxx More majordomo
> info
> > at  http://vger.kernel.org/majordomo-info.html
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in the body
> of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at
> http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/