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 16:16:25 -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 10/15] ethtool: provide ring sizes with
 RINGS_GET request
Message-ID: <20200311161625.7292f745@kicinski-fedora-PC1C0HJN>
In-Reply-To: <55a76ca4eecc92c7209775340ff36ba5dd32f713.1583962006.git.mkubecek@suse.cz>
References: <cover.1583962006.git.mkubecek@suse.cz>
        <55a76ca4eecc92c7209775340ff36ba5dd32f713.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: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

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

> +		+ 0;

nit: personally not a huge fan

> +}
> +
> +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..

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