Re: [PATCH v20 02/20] netlink: add new family to manage ULP_DDP enablement and stats

[Aurelien Aptel <aaptel@nvidia.com>]

Jiri Pirko <jiri@resnulli.us> writes:
> Overall looks good to me, couple of nits below, take it or leave it.

Glad to hear. We will send v21 with the changes below.
Hopefully we can get a RB or Ack on this patch.

>>+
>>+      ctx->ifindex = nla_get_u32(info->attrs[ULP_DDP_A_CAPS_IFINDEX]);
>
> You don't need to store ifindex. You have ctx->dev->ifindex with the
> same value.

Ok, we will remove it from the context.

>>+      switch (cmd) {
>>+      case ULP_DDP_CMD_CAPS_GET:
>>+      case ULP_DDP_CMD_CAPS_SET:
>>+      case ULP_DDP_CMD_CAPS_SET_NTF:
>
> Hmm, you never call ulp_ddp_prepare_context() with
> ULP_DDP_CMD_CAPS_SET_NTF. Perhaps just warn if this case is hit. IDK.

While that is true it also reflects what the payload is and it mirrors
ulp_ddp_reply_size(). We will keep it as is.

>>+static int ulp_ddp_write_reply(struct sk_buff *rsp,
>>+                             struct ulp_ddp_reply_context *ctx,
>>+                             int cmd,
>>+                             const struct genl_info *info)
>>+{
>>+      ctx->hdr = genlmsg_iput(rsp, info);
>
> Interesting, you use "hdr" just in this fuction. Why isn't it a local
> variable?

Good point, we will make it a local variable.

>>+      ret = ulp_ddp_write_reply(rsp, ctx, ULP_DDP_CMD_CAPS_GET, info);
>>+      if (ret < 0)
>
> You mix "if (ret)" and "if (ret < 0)" in this code, I don't see any of
> the functions return positive value on success, so you can make it
> consistent to "if (ret)".

Ok, we will change the checks to "if (ret)" and change
ulp_ddp_apply_bits() return the notify flag via an input parameter.

>>+
>>+      if (ulp_ddp_write_reply(ntf, ctx, ULP_DDP_CMD_CAPS_SET_NTF, &info)) {
>
> Always use "ret" variable to store return value before you check it.

Ok.

>>+
>>+      rsp = genlmsg_new(ulp_ddp_reply_size(ULP_DDP_CMD_STATS_GET), GFP_KERNEL);
>>+      if (!rsp)
>>+              return -EMSGSIZE;
>>+
>>+      wanted = nla_get_u64(info->attrs[ULP_DDP_A_CAPS_WANTED]);
>
> nla_get_uint()
>
>
>>+      wanted_mask = nla_get_u64(info->attrs[ULP_DDP_A_CAPS_WANTED_MASK]);
>
> nla_get_uint()
>

Ok.

>
>>+
>>+      ret = ulp_ddp_apply_bits(ctx, &wanted, &wanted_mask, info);
>
>         ret = ulp_ddp_apply_bits(ctx, &wanted, &wanted_mask, info,
>                                  &notify);
>
> Would be a bit cleaner to read perhaps.

True, we will change it to that.

Thanks