Re: [PATCH net-next v2 07/11] devlink: Add health diagnose command

[Jiri Pirko <jiri@resnulli.us>]

Thu, Jan 17, 2019 at 10:59:16PM CET, eranbe@mellanox.com wrote:
>Add devlink health diagnose command, in order to run a diagnose
>operation over a specific reporter.
>
>It is expected from driver's callback for diagnose command to fill it
>via the buffer descriptors API. Devlink will parse it and convert it to
>netlink nla API in order to pass it to the user.
>
>Signed-off-by: Eran Ben Elisha <eranbe@mellanox.com>
>Reviewed-by: Moshe Shemesh <moshe@mellanox.com>
>---
> include/uapi/linux/devlink.h |  1 +
> net/core/devlink.c           | 51 ++++++++++++++++++++++++++++++++++++
> 2 files changed, 52 insertions(+)
>
>diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>index 1c186fd77343..51b4d7612cf8 100644
>--- a/include/uapi/linux/devlink.h
>+++ b/include/uapi/linux/devlink.h
>@@ -92,6 +92,7 @@ enum devlink_command {
> 	DEVLINK_CMD_HEALTH_REPORTER_GET,
> 	DEVLINK_CMD_HEALTH_REPORTER_SET,
> 	DEVLINK_CMD_HEALTH_REPORTER_RECOVER,
>+	DEVLINK_CMD_HEALTH_REPORTER_DIAGNOSE,
> 
> 	/* add new commands above here */
> 	__DEVLINK_CMD_MAX,
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index b224d0d31c0c..57252ca31e1e 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -4500,6 +4500,50 @@ static int devlink_nl_cmd_health_reporter_recover_doit(struct sk_buff *skb,
> 	return devlink_health_reporter_recover(reporter, NULL);
> }
> 
>+static int devlink_nl_cmd_health_reporter_diagnose_doit(struct sk_buff *skb,
>+							struct genl_info *info)
>+{
>+	struct devlink *devlink = info->user_ptr[0];
>+	struct devlink_health_reporter *reporter;
>+	u64 num_of_buffers;
>+	int err;
>+
>+	reporter = devlink_health_reporter_get_from_info(devlink, info);
>+	if (!reporter)
>+		return -EINVAL;
>+
>+	if (!reporter->ops->diagnose)
>+		return -EOPNOTSUPP;
>+
>+	num_of_buffers =
>+		DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->diagnose_size);

This is odd. You need to convert to number of "buffers" the magic value
the driver provided. And then couple lines below....


>+
>+	mutex_lock(&reporter->diagnose_lock);
>+	devlink_health_buffers_reset(reporter->diagnose_buffers_array,
>+				     num_of_buffers);
>+
>+	err = reporter->ops->diagnose(reporter,
>+				      reporter->diagnose_buffers_array,
>+				      DEVLINK_HEALTH_BUFFER_SIZE,
>+				      num_of_buffers);

....you call the driver with "buffers array", buffer size and number of
buffers. It should be just:

	struct devlink_msg_ctx msg_ctx;

	err = reporter->ops->diagnose(reporter, &msg_ctx);

and then;

	err = devlink_health_buffer_snd(info,
					devlink_cmd_health_reporter_diagnose,
					0, &msg_ctx);


>+	if (err)
>+		goto out;
>+
>+	err = devlink_health_buffer_snd(info,
>+					devlink_cmd_health_reporter_diagnose,
>+					0, reporter->diagnose_buffers_array,
>+					num_of_buffers);
>+	if (err)
>+		goto out;
>+
>+	mutex_unlock(&reporter->diagnose_lock);
>+	return 0;
>+
>+out:
>+	mutex_unlock(&reporter->diagnose_lock);
>+	return err;
>+}
>+
> static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = {
> 	[DEVLINK_ATTR_BUS_NAME] = { .type = NLA_NUL_STRING },
> 	[DEVLINK_ATTR_DEV_NAME] = { .type = NLA_NUL_STRING },
>@@ -4770,6 +4814,13 @@ static const struct genl_ops devlink_nl_ops[] = {
> 		.flags = GENL_ADMIN_PERM,
> 		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK,
> 	},
>+	{
>+		.cmd = DEVLINK_CMD_HEALTH_REPORTER_DIAGNOSE,
>+		.doit = devlink_nl_cmd_health_reporter_diagnose_doit,
>+		.policy = devlink_nl_policy,
>+		.flags = GENL_ADMIN_PERM,
>+		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK,
>+	},
> };
> 
> static struct genl_family devlink_nl_family __ro_after_init = {
>-- 
>2.17.1
>