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

Aurelien Aptel
2023-11-27T13:18:15Z

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