Re: [PATCH net-next 10/15] ethtool: provide ring sizes with RINGS_GET request

[Michal Kubecek]

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