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

[Aurelien Aptel]

Hi,

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

Jakub Kicinski <kuba at kernel.org> writes:
>>       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.

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

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

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

>> +     /* outermost nest */
>> +     return nla_total_size(len);
>
> nla_total_size(0) is more common for nests.

Ok

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

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

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

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

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

Ok.

Thanks.