Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 11 Mar 2020 15:49:08 -0700
From:   Jakub Kicinski <kuba@kernel.org>
To:     Michal Kubecek <mkubecek@suse.cz>
Cc:     David Miller <davem@davemloft.net>, netdev@vger.kernel.org,
        Jiri Pirko <jiri@resnulli.us>, Andrew Lunn <andrew@lunn.ch>,
        Florian Fainelli <f.fainelli@gmail.com>,
        John Linville <linville@tuxdriver.com>,
        Johannes Berg <johannes@sipsolutions.net>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next 03/15] ethtool: provide netdev features with
 FEATURES_GET request
Message-ID: <20200311154908.7fd7047d@kicinski-fedora-PC1C0HJN>
In-Reply-To: <a3ed11c43f5ac0a68d3b3dfdf62de755ad240e59.1583962006.git.mkubecek@suse.cz>
References: <cover.1583962006.git.mkubecek@suse.cz>
        <a3ed11c43f5ac0a68d3b3dfdf62de755ad240e59.1583962006.git.mkubecek@suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, 11 Mar 2020 22:40:18 +0100 (CET) Michal Kubecek wrote:
> diff --git a/net/ethtool/common.h b/net/ethtool/common.h
> index 40ba74e0b9bb..82211430d3db 100644
> --- a/net/ethtool/common.h
> +++ b/net/ethtool/common.h
> @@ -6,6 +6,8 @@
>  #include <linux/netdevice.h>
>  #include <linux/ethtool.h>
>  
> +#define ETHTOOL_DEV_FEATURE_WORDS	((NETDEV_FEATURE_COUNT + 31) / 32)

nit: since this line is touched perhaps worth converting to
DIV_ROUND_UP()?

>  /* compose link mode index from speed, type and duplex */
>  #define ETHTOOL_LINK_MODE(speed, type, duplex) \
>  	ETHTOOL_LINK_MODE_ ## speed ## base ## type ## _ ## duplex ## _BIT

> +static void ethnl_features_to_bitmap32(u32 *dest, netdev_features_t src)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < ETHTOOL_DEV_FEATURE_WORDS; i++)
> +		dest[i] = (u32)(src >> (32 * i));

nit: cast unnecessary

> +}
> +
> +static int features_prepare_data(const struct ethnl_req_info *req_base,
> +				 struct ethnl_reply_data *reply_base,
> +				 struct genl_info *info)
> +{
> +	struct features_reply_data *data = FEATURES_REPDATA(reply_base);
> +	struct net_device *dev = reply_base->dev;
> +	netdev_features_t all_features;
> +
> +	ethnl_features_to_bitmap32(data->hw, dev->hw_features);
> +	ethnl_features_to_bitmap32(data->wanted, dev->wanted_features);
> +	ethnl_features_to_bitmap32(data->active, dev->features);
> +	ethnl_features_to_bitmap32(data->nochange, NETIF_F_NEVER_CHANGE);
> +	all_features = ~(netdev_features_t)0 >>
> +		       (8 * sizeof(all_features) - NETDEV_FEATURE_COUNT);

nit: GENMASK_ULL(NETDEV_FEATURE_COUNT % 32 - 1, 0) ?

> +	ethnl_features_to_bitmap32(data->all, all_features);
> +
> +	return 0;
> +}

> +static int features_fill_reply(struct sk_buff *skb,
> +			       const struct ethnl_req_info *req_base,
> +			       const struct ethnl_reply_data *reply_base)
> +{
> +	const struct features_reply_data *data = FEATURES_REPDATA(reply_base);
> +	bool compact = req_base->flags & ETHTOOL_FLAG_COMPACT_BITSETS;
> +	int ret;
> +
> +	ret = ethnl_put_bitset32(skb, ETHTOOL_A_FEATURES_HW, data->hw,
> +				 data->all, NETDEV_FEATURE_COUNT,
> +				 netdev_features_strings, compact);
> +	if (ret < 0)
> +		return ret;
> +	ret = ethnl_put_bitset32(skb, ETHTOOL_A_FEATURES_WANTED, data->wanted,
> +				 NULL, NETDEV_FEATURE_COUNT,
> +				 netdev_features_strings, compact);
> +	if (ret < 0)
> +		return ret;
> +	ret = ethnl_put_bitset32(skb, ETHTOOL_A_FEATURES_ACTIVE, data->active,
> +				 NULL, NETDEV_FEATURE_COUNT,
> +				 netdev_features_strings, compact);
> +	if (ret < 0)
> +		return ret;
> +	ret = ethnl_put_bitset32(skb, ETHTOOL_A_FEATURES_NOCHANGE,
> +				 data->nochange, NULL, NETDEV_FEATURE_COUNT,
> +				 netdev_features_strings, compact);
> +
> +	return ret;

nit: return directly?

> +}

Probably not worth respinning for just those nits:

Reviewed-by: Jakub Kicinski <kuba@kernel.org>