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

Hi,

We noticed your recent changes and we will adapt the patch to use them.

Jakub Kicinski [off-list ref] writes:

quoted

      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

Ok.

quoted

      /* 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?

Ok. We will add it and use the return value introduced in your latest
refactor commits.

quoted

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

alphabetic order?

bitset.h depends on netlink.h but it doesn't include it: it doesn't
compile if you change the include order. We will keep it as is.

quoted

+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

Ok

quoted

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

nla_total_size(0) is more common for nests.

Ok

quoted

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

Ok

quoted

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

bitset_policy is not exported by bitset.c (static).
We will rename it ethnl_bitset_policy and export it.

quoted

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

Ok.

quoted

+     if (!tb[ETHTOOL_A_ULP_DDP_WANTED])

GENL_REQ_ATTR_CHECK()

Ok. We will use this macro and move it to a .set_validate() callback.

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

Ok.

quoted

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

and drop this

Ok.

Thanks.