linux-kernel - Re: [PATCH net-next 10/15] ethtool: provide ring sizes with RINGS

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] [day] [month] [year] [list]

Date:   Thu, 12 Mar 2020 13:29:22 +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 10/15] ethtool: provide ring sizes with
 RINGS_GET request

On Wed, Mar 11, 2020 at 04:16:25PM -0700, Jakub Kicinski wrote:
> On Wed, 11 Mar 2020 22:40:53 +0100 (CET) Michal Kubecek wrote:
> > +static int rings_prepare_data(const struct ethnl_req_info *req_base,
> > +			      struct ethnl_reply_data *reply_base,
> > +			      struct genl_info *info)
> > +{
> > +	struct rings_reply_data *data = RINGS_REPDATA(reply_base);
> > +	struct net_device *dev = reply_base->dev;
> > +	int ret;
> > +
> > +	if (!dev->ethtool_ops->get_ringparam)
> > +		return -EOPNOTSUPP;
> > +	ret = ethnl_ops_begin(dev);
> > +	if (ret < 0)
> > +		return ret;
> > +	dev->ethtool_ops->get_ringparam(dev, &data->ringparam);
> > +	ret = 0;
> > +	ethnl_ops_complete(dev);
> > +
> > +	return ret;
> 
> nit: just return 0 and drop ret = 0 above, there is no goto here

OK

> > +}
> > +
> > +static int rings_reply_size(const struct ethnl_req_info *req_base,
> > +			    const struct ethnl_reply_data *reply_base)
> > +{
> > +	return 8 * nla_total_size(sizeof(u32))
> 
> nit: 8 is a little bit of a magic constant

I'll rewrite this as a sum of 8 entries with comment referring to
attribute types. It's still a compile time computed constant so that
there should be no impact on resulting code.

> > +		+ 0;
> 
> nit: personally not a huge fan

I don't like it either, to be honest. I just thought, based on reading
some earlier discussions, that it's the preferred way as it enforces
a compiler error when someone adds a new attribute and forgets to
replace the semicolon with plus (which IIRC happened in the past).

But as I checked now, reasonably new gcc (at least from version 7)
issues a warning in such case so that it wouldn't go unnoticed with
various kbuild bots around. So I agree to get rid of this trick.

> > +static int rings_fill_reply(struct sk_buff *skb,
> > +			    const struct ethnl_req_info *req_base,
> > +			    const struct ethnl_reply_data *reply_base)
> > +{
> > +	const struct rings_reply_data *data = RINGS_REPDATA(reply_base);
> > +	const struct ethtool_ringparam *ringparam = &data->ringparam;
> > +
> > +	if (nla_put_u32(skb, ETHTOOL_A_RINGS_RX_MAX,
> > +			ringparam->rx_max_pending) ||
> > +	    nla_put_u32(skb, ETHTOOL_A_RINGS_RX_MINI_MAX,
> > +			ringparam->rx_mini_max_pending) ||
> > +	    nla_put_u32(skb, ETHTOOL_A_RINGS_RX_JUMBO_MAX,
> > +			ringparam->rx_jumbo_max_pending) ||
> > +	    nla_put_u32(skb, ETHTOOL_A_RINGS_TX_MAX,
> > +			ringparam->tx_max_pending) ||
> > +	    nla_put_u32(skb, ETHTOOL_A_RINGS_RX,
> > +			ringparam->rx_pending) ||
> > +	    nla_put_u32(skb, ETHTOOL_A_RINGS_RX_MINI,
> > +			ringparam->rx_mini_pending) ||
> > +	    nla_put_u32(skb, ETHTOOL_A_RINGS_RX_JUMBO,
> > +			ringparam->rx_jumbo_pending) ||
> > +	    nla_put_u32(skb, ETHTOOL_A_RINGS_TX,
> > +			ringparam->tx_pending))
> > +		return -EMSGSIZE;
> 
> nit: I wonder if it's necessary to report the zero values..

Good point. I would say that it makes perfect sense to omit the
attributes if the max value is zero (i.e. this type of ring is not
supported) but I would still report zero current size if corresponding
limit is not zero as it means the zero size is meaningful. (Many drivers
do not allow zero size but I found one which does.)

Michal