Re: [PATCH v10 03/25] net/ethtool: add ULP_DDP_{GET,SET} operations for caps and stats

[Jakub Kicinski <kuba@kernel.org>]

On Thu, 26 Jan 2023 18:21:14 +0200 Aurelien Aptel wrote:
> diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
> index 14a1858fd106..35805d8d12a3 100644
> --- a/include/uapi/linux/ethtool_netlink.h
> +++ b/include/uapi/linux/ethtool_netlink.h
> @@ -57,6 +57,8 @@ enum {
>  	ETHTOOL_MSG_PLCA_GET_STATUS,
>  	ETHTOOL_MSG_MM_GET,
>  	ETHTOOL_MSG_MM_SET,
> +	ETHTOOL_MSG_ULP_DDP_GET,
> +	ETHTOOL_MSG_ULP_DDP_SET,

Please add the definition of the command to
Documentation/netlink/specs/ethtool.yaml

>  	/* add new constants above here */
>  	__ETHTOOL_MSG_USER_CNT,
> @@ -109,6 +111,8 @@ enum {
>  	ETHTOOL_MSG_PLCA_NTF,
>  	ETHTOOL_MSG_MM_GET_REPLY,
>  	ETHTOOL_MSG_MM_NTF,
> +	ETHTOOL_MSG_ULP_DDP_GET_REPLY,
> +	ETHTOOL_MSG_ULP_DDP_SET_REPLY,

What about notifications?

> +#include "netlink.h"
> +#include "common.h"
> +#include "bitset.h"

alphabetic order?

> +static int ulp_ddp_stats64_size(unsigned int count)
> +{
> +	unsigned int len = 0;
> +	unsigned int i;
> +
> +	for (i = 0; i < count; i++)
> +		len += nla_total_size(sizeof(u64));

len = nla_total_size(sizeof(u64)) * count
?
but it's not correct. You need nla_total_size_64bit() here

> +	/* outermost nest */
> +	return nla_total_size(len);

nla_total_size(0) is more common for nests.

> +}
> +
> +static int ulp_ddp_put_stats64(struct sk_buff *skb, int attrtype, const u64 *val,
> +			       unsigned int count)
> +{
> +	struct nlattr *nest;
> +	unsigned int i;
> +
> +	nest = nla_nest_start(skb, attrtype);
> +	if (!nest)
> +		return -EMSGSIZE;
> +
> +	/* skip attribute 0 (unspec) */
> +	for (i = 0 ; i < count; i++)
> +		if (nla_put_64bit(skb, i+1, sizeof(u64), &val[i], 0))

nla_put_u64_64bit()
And you'll need to add an attr for padding.

> +			goto nla_put_failure;
> +
> +	nla_nest_end(skb, nest);
> +	return 0;
> +
> +nla_put_failure:
> +	nla_nest_cancel(skb, nest);
> +	return -EMSGSIZE;
> +}

> +const struct nla_policy ethnl_ulp_ddp_set_policy[] = {
> +	[ETHTOOL_A_ULP_DDP_HEADER]	=
> +		NLA_POLICY_NESTED(ethnl_header_policy),
> +	[ETHTOOL_A_ULP_DDP_WANTED]	= { .type = NLA_NESTED },
> +};

Let's link the policy here: NLA_POLICY_NESTED(bitset_policy).

> +static int ulp_ddp_send_reply(struct net_device *dev, struct genl_info *info,
> +			      const unsigned long *wanted,
> +			      const unsigned long *wanted_mask,
> +			      const unsigned long *active,
> +			      const unsigned long *active_mask, bool compact)
> +{
> +	struct sk_buff *rskb;
> +	void *reply_payload;
> +	int reply_len = 0;
> +	int ret;
> +
> +	reply_len = ethnl_reply_header_size();
> +	ret = ethnl_bitset_size(wanted, wanted_mask, ULP_DDP_C_COUNT,
> +				ulp_ddp_caps_names, compact);
> +	if (ret < 0)
> +		goto err;
> +	reply_len += ret;
> +	ret = ethnl_bitset_size(active, active_mask, ULP_DDP_C_COUNT,
> +				ulp_ddp_caps_names, compact);
> +	if (ret < 0)
> +		goto err;
> +	reply_len += ret;
> +
> +	ret = -ENOMEM;
> +	rskb = ethnl_reply_init(reply_len, dev, ETHTOOL_MSG_ULP_DDP_SET_REPLY,
> +				ETHTOOL_A_ULP_DDP_HEADER, info,
> +				&reply_payload);
> +	if (!rskb)
> +		goto err;
> +
> +	ret = ethnl_put_bitset(rskb, ETHTOOL_A_ULP_DDP_WANTED, wanted,
> +			       wanted_mask, ULP_DDP_C_COUNT,
> +			       ulp_ddp_caps_names, compact);
> +	if (ret < 0)
> +		goto nla_put_failure;
> +	ret = ethnl_put_bitset(rskb, ETHTOOL_A_ULP_DDP_ACTIVE, active,
> +			       active_mask, ULP_DDP_C_COUNT,
> +			       ulp_ddp_caps_names, compact);
> +	if (ret < 0)
> +		goto nla_put_failure;
> +
> +	genlmsg_end(rskb, reply_payload);
> +	ret = genlmsg_reply(rskb, info);
> +	return ret;
> +
> +nla_put_failure:
> +	nlmsg_free(rskb);
> +	WARN_ONCE(1, "calculated message payload length (%d) not sufficient\n",
> +		  reply_len);
> +err:
> +	GENL_SET_ERR_MSG(info, "failed to send reply message");

Don't overwrite the message, the message should be set close to 
the error, if needed.

> +	return ret;
> +}
> +
> +int ethnl_set_ulp_ddp(struct sk_buff *skb, struct genl_info *info)
> +{
> +	DECLARE_BITMAP(old_active, ULP_DDP_C_COUNT);
> +	DECLARE_BITMAP(new_active, ULP_DDP_C_COUNT);
> +	DECLARE_BITMAP(req_wanted, ULP_DDP_C_COUNT);
> +	DECLARE_BITMAP(req_mask, ULP_DDP_C_COUNT);
> +	DECLARE_BITMAP(all_bits, ULP_DDP_C_COUNT);
> +	DECLARE_BITMAP(tmp, ULP_DDP_C_COUNT);
> +	struct ethnl_req_info req_info = {};
> +	const struct ulp_ddp_dev_ops *ops;
> +	struct nlattr **tb = info->attrs;
> +	struct ulp_ddp_netdev_caps *caps;
> +	struct net_device *dev;
> +	int ret;
> +
> +	if (!tb[ETHTOOL_A_ULP_DDP_WANTED])

GENL_REQ_ATTR_CHECK()

> +		return -EINVAL;
> +	ret = ethnl_parse_header_dev_get(&req_info,
> +					 tb[ETHTOOL_A_ULP_DDP_HEADER],
> +					 genl_info_net(info), info->extack,
> +					 true);
> +	if (ret < 0)
> +		return ret;
> +
> +	dev = req_info.dev;
> +	rtnl_lock();
> +	caps = netdev_ulp_ddp_caps(dev);
> +	ops = netdev_ulp_ddp_ops(dev);
> +	if (!caps || !ops || !ops->set_caps) {
> +		ret = -EOPNOTSUPP;
> +		goto out_rtnl;
> +	}
> +
> +	ret = ethnl_parse_bitset(req_wanted, req_mask, ULP_DDP_C_COUNT,
> +				 tb[ETHTOOL_A_ULP_DDP_WANTED],
> +				 ulp_ddp_caps_names, info->extack);
> +	if (ret < 0)
> +		goto out_rtnl;
> +
> +	/* if (req_mask & ~all_bits) */
> +	bitmap_fill(all_bits, ULP_DDP_C_COUNT);
> +	bitmap_andnot(tmp, req_mask, all_bits, ULP_DDP_C_COUNT);
> +	if (!bitmap_empty(tmp, ULP_DDP_C_COUNT)) {
> +		ret = -EINVAL;
> +		goto out_rtnl;
> +	}
> +
> +	/* new_active = (old_active & ~req_mask) | (wanted & req_mask)
> +	 * new_active &= caps_hw
> +	 */
> +	bitmap_copy(old_active, caps->active, ULP_DDP_C_COUNT);
> +	bitmap_and(req_wanted, req_wanted, req_mask, ULP_DDP_C_COUNT);
> +	bitmap_andnot(new_active, old_active, req_mask, ULP_DDP_C_COUNT);
> +	bitmap_or(new_active, new_active, req_wanted, ULP_DDP_C_COUNT);
> +	bitmap_and(new_active, new_active, caps->hw, ULP_DDP_C_COUNT);
> +	if (!bitmap_equal(old_active, new_active, ULP_DDP_C_COUNT)) {
> +		ret = ops->set_caps(dev, new_active);

We should pass extack to the driver, so that the driver can report a
meaningful error

> +		if (ret)
> +			netdev_err(dev, "set_ulp_ddp_capabilities() returned error %d\n", ret);

and drop this

> +		bitmap_copy(new_active, caps->active, ULP_DDP_C_COUNT);
> +	}
> +
> +	ret = 0;
> +	if (!(req_info.flags & ETHTOOL_FLAG_OMIT_REPLY)) {
> +		DECLARE_BITMAP(wanted_diff_mask, ULP_DDP_C_COUNT);
> +		DECLARE_BITMAP(active_diff_mask, ULP_DDP_C_COUNT);
> +		bool compact = req_info.flags & ETHTOOL_FLAG_COMPACT_BITSETS;
> +
> +		/* wanted_diff_mask = req_wanted ^ new_active
> +		 * active_diff_mask = old_active ^ new_active -> mask of bits that have changed
> +		 * wanted_diff_mask &= req_mask    -> mask of bits that have diff value than wanted
> +		 * req_wanted &= wanted_diff_mask  -> bits that have diff value than wanted
> +		 * new_active &= active_diff_mask  -> bits that have changed
> +		 */
> +		bitmap_xor(wanted_diff_mask, req_wanted, new_active, ULP_DDP_C_COUNT);
> +		bitmap_xor(active_diff_mask, old_active, new_active, ULP_DDP_C_COUNT);
> +		bitmap_and(wanted_diff_mask, wanted_diff_mask, req_mask, ULP_DDP_C_COUNT);
> +		bitmap_and(req_wanted, req_wanted, wanted_diff_mask,  ULP_DDP_C_COUNT);
> +		bitmap_and(new_active, new_active, active_diff_mask,  ULP_DDP_C_COUNT);
> +		ret = ulp_ddp_send_reply(dev, info,
> +					 req_wanted, wanted_diff_mask,
> +					 new_active, active_diff_mask,
> +					 compact);
> +	}
> +
> +out_rtnl:
> +	rtnl_unlock();
> +	ethnl_parse_header_dev_put(&req_info);
> +	return ret;
> +}