linux-kernel - Re: [PATCH net-next 03/15] ethtool: provide netdev features with FEATURES

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200312091943.GK8012@unicorn.suse.cz>
Date:   Thu, 12 Mar 2020 10:19:43 +0100
From:   Michal Kubecek <mkubecek@...e.cz>
To:     netdev@...r.kernel.org
Cc:     Jakub Kicinski <kuba@...nel.org>,
        David Miller <davem@...emloft.net>,
        Jiri Pirko <jiri@...nulli.us>, Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        John Linville <linville@...driver.com>,
        Johannes Berg <johannes@...solutions.net>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 03/15] ethtool: provide netdev features with
 FEATURES_GET request

On Wed, Mar 11, 2020 at 03:49:08PM -0700, Jakub Kicinski wrote:
> On Wed, 11 Mar 2020 22:40:18 +0100 (CET) Michal Kubecek wrote:
> >  
> > +#define ETHTOOL_DEV_FEATURE_WORDS	((NETDEV_FEATURE_COUNT + 31) / 32)
> 
> nit: since this line is touched perhaps worth converting to
> DIV_ROUND_UP()?

Yes.

> > +	for (i = 0; i < ETHTOOL_DEV_FEATURE_WORDS; i++)
> > +		dest[i] = (u32)(src >> (32 * i));
> 
> nit: cast unnecessary

OK.

> > +	all_features = ~(netdev_features_t)0 >>
> > +		       (8 * sizeof(all_features) - NETDEV_FEATURE_COUNT);
> 
> nit: GENMASK_ULL(NETDEV_FEATURE_COUNT % 32 - 1, 0) ?

Nice, I wasn't aware of this macro. But unless I want to fill data->all
directly, it would be without the "% 32" part.

> > +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?

Ah, right. I eliminated "if (ret < 0)" but did not check again.

Michal