Re: [PATCH net-next v6 10/15] ethtool: provide string sets with STRSET_GET request

From: Jiri Pirko <jiri@resnulli.us>
To: Michal Kubecek <mkubecek@suse.cz>
Cc: David Miller <davem@davemloft.net>,
	netdev@vger.kernel.org,
	Jakub Kicinski <jakub.kicinski@netronome.com>,
	Andrew Lunn <andrew@lunn.ch>,
	Florian Fainelli <f.fainelli@gmail.com>,
	John Linville <linville@tuxdriver.com>,
	Stephen Hemminger <stephen@networkplumber.org>,
	Johannes Berg <johannes@sipsolutions.net>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next v6 10/15] ethtool: provide string sets with STRSET_GET request
Date: Thu, 4 Jul 2019 10:17:35 +0200	[thread overview]
Message-ID: <20190704081735.GI2250@nanopsycho> (raw)
In-Reply-To: <3c30527bef64c030078a1e305613080bb372cbe6.1562067622.git.mkubecek@suse.cz>

Tue, Jul 02, 2019 at 01:50:29PM CEST, mkubecek@suse.cz wrote:

[...]

>@@ -87,6 +89,64 @@ enum {
> 	ETHTOOL_A_BITSET_MAX = (__ETHTOOL_A_BITSET_CNT - 1)

You don't need "()". Same for the others below.

> };
> 
>+/* string sets */
>+
>+enum {
>+	ETHTOOL_A_STRING_UNSPEC,
>+	ETHTOOL_A_STRING_INDEX,			/* u32 */
>+	ETHTOOL_A_STRING_VALUE,			/* string */
>+
>+	/* add new constants above here */
>+	__ETHTOOL_A_STRING_CNT,
>+	ETHTOOL_A_STRING_MAX = (__ETHTOOL_A_STRING_CNT - 1)
>+};
>+
>+enum {
>+	ETHTOOL_A_STRINGS_UNSPEC,
>+	ETHTOOL_A_STRINGS_STRING,		/* nest - _A_STRINGS_* */
>+
>+	/* add new constants above here */
>+	__ETHTOOL_A_STRINGS_CNT,
>+	ETHTOOL_A_STRINGS_MAX = (__ETHTOOL_A_STRINGS_CNT - 1)
>+};
>+
>+enum {
>+	ETHTOOL_A_STRINGSET_UNSPEC,
>+	ETHTOOL_A_STRINGSET_ID,			/* u32 */
>+	ETHTOOL_A_STRINGSET_COUNT,		/* u32 */
>+	ETHTOOL_A_STRINGSET_STRINGS,		/* nest - _A_STRINGS_* */
>+
>+	/* add new constants above here */
>+	__ETHTOOL_A_STRINGSET_CNT,
>+	ETHTOOL_A_STRINGSET_MAX = (__ETHTOOL_A_STRINGSET_CNT - 1)
>+};
>+
>+/* STRSET */
>+
>+enum {
>+	ETHTOOL_A_STRSET_UNSPEC,
>+	ETHTOOL_A_STRSET_HEADER,		/* nest - _A_HEADER_* */
>+	ETHTOOL_A_STRSET_STRINGSETS,		/* nest - _A_STRINGSETS_* */
>+
>+	/* add new constants above here */
>+	__ETHTOOL_A_STRSET_CNT,
>+	ETHTOOL_A_STRSET_MAX = (__ETHTOOL_A_STRSET_CNT - 1)
>+};
>+
>+enum {
>+	ETHTOOL_A_STRINGSETS_UNSPEC,
>+	ETHTOOL_A_STRINGSETS_STRINGSET,		/* nest - _A_STRINGSET_* */
>+
>+	/* add new constants above here */
>+	__ETHTOOL_A_STRINGSETS_CNT,
>+	ETHTOOL_A_STRINGSETS_MAX = (__ETHTOOL_A_STRINGSETS_CNT - 1)
>+};
>+

[...]	

>+	nla_for_each_nested(attr, nest, rem) {
>+		u32 id;
>+
>+		if (WARN_ONCE(nla_type(attr) != ETHTOOL_A_STRINGSETS_STRINGSET,
>+			      "unexpected attrtype %u in ETHTOOL_A_STRSET_STRINGSETS\n",
>+			      nla_type(attr)))
>+			return -EINVAL;
>+
>+		ret = strset_get_id(attr, &id, extack);
>+		if (ret < 0)
>+			return ret;
>+		if (ret >= ETH_SS_COUNT) {
>+			NL_SET_ERR_MSG_ATTR(extack, attr,
>+					    "unknown string set id");
>+			return -EOPNOTSUPP;
>+		}
>+
>+		data->req_ids |= (1U << id);

You don't need "()" here either.

[...]