[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <253cyvvjpxd.fsf@nvidia.com>
Date: Mon, 27 Nov 2023 15:17:50 +0200
From: Aurelien Aptel <aaptel@...dia.com>
To: Jiri Pirko <jiri@...nulli.us>
Cc: linux-nvme@...ts.infradead.org, netdev@...r.kernel.org,
sagi@...mberg.me, hch@....de, kbusch@...nel.org, axboe@...com,
chaitanyak@...dia.com, davem@...emloft.net, kuba@...nel.org,
aurelien.aptel@...il.com, smalin@...dia.com, malin1024@...il.com,
ogerlitz@...dia.com, yorayz@...dia.com, borisp@...dia.com,
galshalom@...dia.com, mgurtovoy@...dia.com, edumazet@...gle.com,
pabeni@...hat.com
Subject: Re: [PATCH v20 02/20] netlink: add new family to manage ULP_DDP
enablement and stats
Jiri Pirko <jiri@...nulli.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,
> ¬ify);
>
> Would be a bit cleaner to read perhaps.
True, we will change it to that.
Thanks
Powered by blists - more mailing lists