Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751443AbaLRPWh (ORCPT ); Thu, 18 Dec 2014 10:22:37 -0500 Received: from mga11.intel.com ([192.55.52.93]:56328 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751096AbaLRPWg convert rfc822-to-8bit (ORCPT ); Thu, 18 Dec 2014 10:22:36 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.07,601,1413270000"; d="scan'208";a="649917331" From: "Varlese, Marco" To: Thomas Graf CC: "netdev@vger.kernel.org" , "Fastabend, John R" , Jiri Pirko , "roopa@cumulusnetworks.com" , "sfeldma@gmail.com" , "linux-kernel@vger.kernel.org" Subject: RE: [RFC PATCH net-next v2 1/1] net: Support for switch port configuration Thread-Topic: [RFC PATCH net-next v2 1/1] net: Support for switch port configuration Thread-Index: AdAaqPMciB9+ZS56Q6O2uHwvQZyp6AADqZeAAAeWSGA= Date: Thu, 18 Dec 2014 15:20:26 +0000 Message-ID: References: <20141218114145.GH28766@casper.infradead.org> In-Reply-To: <20141218114145.GH28766@casper.infradead.org> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.181] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > -----Original Message----- > From: Thomas Graf [mailto:tgr@infradead.org] On Behalf Of Thomas Graf > Sent: Thursday, December 18, 2014 11:42 AM > To: Varlese, Marco > Cc: netdev@vger.kernel.org; Fastabend, John R; Jiri Pirko; > roopa@cumulusnetworks.com; 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 at 11:29am, Varlese, Marco wrote: > > 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 > > }; > > Needs an entry in ifla_policy[] > > [IFLA_SWITCH_PORT_CFG] = { .type = NLA_NESTED }, > > > @@ -603,4 +604,18 @@ enum { > > > > #define IFLA_HSR_MAX (__IFLA_HSR_MAX - 1) > > > > +/* Switch Port Attributes section */ > > + > > +enum { > > + IFLA_ATTR_UNSPEC, > > + IFLA_ATTR_LEARNING, > > + 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) > > Change the prefix to IFLA_SW_* since it's switch specific? > > > > > +#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; > > + } > > You should validate the message before you start applying the changes. > Otherwise if the 3rd attribute is too short you've already applied some > changes and the user has not idea how much has been applied. > > nla_parse_nested() can help here. > > > > + > > + value = nla_get_u64(v); > > + err = ops->ndo_switch_port_set_cfg(dev, > > + op, > > + value); > > This avoids having individual ndos but wastes a lot of space in the Netlink > message. Not a problem when setting configuration but you likely want to > dump these attributes as well and we need 12 bytes for each attribute even > though some are merely flags which could fit in 4 bytes. > > > 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 > > Should return -EOPNOTSUPP if IFLA_SWITCH_PORT_CFG is provided but > CONFIG_NET_SWITCHDEV is not set. > I think I've addressed your comments Thomas. Thanks for your suggestions. I'm going to resubmit the patch now. Thanks, Marco -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/