Date:   Sat, 2 Feb 2019 16:59:38 +0100
From:   Jiri Pirko <jiri@...nulli.us>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     dsahern@...il.com, stephen@...workplumber.org,
        netdev@...r.kernel.org, oss-drivers@...ronome.com
Subject: Re: [PATCH iproute2-next] devlink: add info subcommand

Sat, Feb 02, 2019 at 01:03:38AM CET, jakub.kicinski@...ronome.com wrote:
>Add support for reading the device serial number and versions
>from the kernel.
>
>RFCv2:
> - make info subcommand of dev.

Please add some examples of inputs and outputs.


>
>Signed-off-by: Jakub Kicinski <jakub.kicinski@...ronome.com>
>---
> devlink/devlink.c      | 207 +++++++++++++++++++++++++++++++++++++++++
> man/man8/devlink-dev.8 |  36 +++++++
> 2 files changed, 243 insertions(+)
>
>diff --git a/devlink/devlink.c b/devlink/devlink.c
>index 3651e90c1159..3ab046ace8f8 100644
>--- a/devlink/devlink.c
>+++ b/devlink/devlink.c
>@@ -199,6 +199,7 @@ static void ifname_map_free(struct ifname_map *ifname_map)
> #define DL_OPT_REGION_SNAPSHOT_ID	BIT(22)
> #define DL_OPT_REGION_ADDRESS		BIT(23)
> #define DL_OPT_REGION_LENGTH		BIT(24)
>+#define DL_OPT_VERSIONS_TYPE		BIT(25)

Why "type"? Confusing.


> 
> struct dl_opts {
> 	uint32_t present; /* flags of present items */
>@@ -230,6 +231,7 @@ struct dl_opts {
> 	uint32_t region_snapshot_id;
> 	uint64_t region_address;
> 	uint64_t region_length;
>+	int versions_attr;
> };
> 
> struct dl {
>@@ -383,6 +385,13 @@ static const enum mnl_attr_data_type devlink_policy[DEVLINK_ATTR_MAX + 1] = {
> 	[DEVLINK_ATTR_REGION_CHUNK_DATA] = MNL_TYPE_BINARY,
> 	[DEVLINK_ATTR_REGION_CHUNK_ADDR] = MNL_TYPE_U64,
> 	[DEVLINK_ATTR_REGION_CHUNK_LEN] = MNL_TYPE_U64,
>+	[DEVLINK_ATTR_INFO_DRIVER_NAME] = MNL_TYPE_STRING,
>+	[DEVLINK_ATTR_INFO_SERIAL_NUMBER] = MNL_TYPE_STRING,
>+	[DEVLINK_ATTR_INFO_VERSION_FIXED] = MNL_TYPE_NESTED,
>+	[DEVLINK_ATTR_INFO_VERSION_RUNNING] = MNL_TYPE_NESTED,
>+	[DEVLINK_ATTR_INFO_VERSION_STORED] = MNL_TYPE_NESTED,
>+	[DEVLINK_ATTR_INFO_VERSION_NAME] = MNL_TYPE_STRING,
>+	[DEVLINK_ATTR_INFO_VERSION_VALUE] = MNL_TYPE_STRING,
> };
> 
> static int attr_cb(const struct nlattr *attr, void *data)
>@@ -943,6 +952,21 @@ static int param_cmode_get(const char *cmodestr,
> 	return 0;
> }
> 
>+static int versions_type_get(const char *typestr, int *p_attr)
>+{
>+	if (strcmp(typestr, "fixed") == 0) {
>+		*p_attr = DEVLINK_ATTR_INFO_VERSION_FIXED;
>+	} else if (strcmp(typestr, "stored") == 0) {
>+		*p_attr = DEVLINK_ATTR_INFO_VERSION_STORED;
>+	} else if (strcmp(typestr, "running") == 0) {
>+		*p_attr = DEVLINK_ATTR_INFO_VERSION_RUNNING;
>+	} else {
>+		pr_err("Unknown versions type \"%s\"\n", typestr);
>+		return -EINVAL;
>+	}
>+	return 0;
>+}
>+
> static int dl_argv_parse(struct dl *dl, uint32_t o_required,
> 			 uint32_t o_optional)
> {
>@@ -1178,6 +1202,19 @@ static int dl_argv_parse(struct dl *dl, uint32_t o_required,
> 			if (err)
> 				return err;
> 			o_found |= DL_OPT_REGION_LENGTH;
>+		} else if (dl_argv_match(dl, "versions") &&
>+			   (o_all & DL_OPT_VERSIONS_TYPE)) {
>+			const char *versionstr;
>+
>+			dl_arg_inc(dl);
>+			err = dl_argv_str(dl, &versionstr);
>+			if (err)
>+				return err;
>+			err = versions_type_get(versionstr,
>+						&opts->versions_attr);
>+			if (err)
>+				return err;
>+			o_found |= DL_OPT_VERSIONS_TYPE;
> 		} else {
> 			pr_err("Unknown option \"%s\"\n", dl_argv(dl));
> 			return -EINVAL;
>@@ -1443,6 +1480,9 @@ static void cmd_dev_help(void)
> 	pr_err("       devlink dev param set DEV name PARAMETER value VALUE cmode { permanent | driverinit | runtime }\n");
> 	pr_err("       devlink dev param show [DEV name PARAMETER]\n");
> 	pr_err("       devlink dev reload DEV\n");
>+	pr_err("       devlink dev info [ DEV [ { versions [ VTYPE ] } ] ]\n");
>+	pr_err("\n");
>+	pr_err("       VTYPE := { fixed | running | stored }\n");

So you would like to filter according to the version type? Why?


> }
> 
> static bool cmp_arr_last_handle(struct dl *dl, const char *bus_name,
>@@ -1775,6 +1815,30 @@ static void pr_out_array_end(struct dl *dl)
> 	}
> }
> 
>+static void pr_out_object_start(struct dl *dl, const char *name)
>+{
>+	if (dl->json_output) {
>+		jsonw_name(dl->jw, name);
>+		jsonw_start_object(dl->jw);
>+	} else {
>+		__pr_out_indent_inc();
>+		__pr_out_newline();
>+		pr_out("%s:", name);
>+		__pr_out_indent_inc();
>+		__pr_out_newline();
>+	}
>+}
>+
>+static void pr_out_object_end(struct dl *dl)
>+{
>+	if (dl->json_output) {
>+		jsonw_end_object(dl->jw);
>+	} else {
>+		__pr_out_indent_dec();
>+		__pr_out_indent_dec();
>+	}
>+}
>+
> static void pr_out_entry_start(struct dl *dl)
> {
> 	if (dl->json_output)
>@@ -2415,6 +2479,146 @@ static int cmd_dev_reload(struct dl *dl)
> 	return _mnlg_socket_sndrcv(dl->nlg, nlh, NULL, NULL);
> }
> 
>+static void pr_out_versions_single(struct dl *dl, const struct nlmsghdr *nlh,
>+				   const char *name, int type)
>+{
>+	struct nlattr *version;
>+
>+	if (dl->opts.versions_attr && dl->opts.versions_attr != type)
>+		return;
>+
>+	mnl_attr_for_each(version, nlh, sizeof(struct genlmsghdr)) {
>+		struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {};
>+		const char *ver_value;
>+		const char *ver_name;
>+		int err;
>+
>+		if (mnl_attr_get_type(version) != type)
>+			continue;
>+
>+		err = mnl_attr_parse_nested(version, attr_cb, tb);
>+		if (err != MNL_CB_OK)
>+			continue;
>+
>+		if (!tb[DEVLINK_ATTR_INFO_VERSION_NAME] ||
>+		    !tb[DEVLINK_ATTR_INFO_VERSION_VALUE])
>+			continue;
>+
>+		if (name) {
>+			pr_out_object_start(dl, name);
>+			name = NULL;
>+		}
>+
>+		ver_name = mnl_attr_get_str(tb[DEVLINK_ATTR_INFO_VERSION_NAME]);
>+		ver_value = mnl_attr_get_str(tb[DEVLINK_ATTR_INFO_VERSION_VALUE]);
>+
>+		pr_out_str(dl, ver_name, ver_value);
>+		if (!dl->json_output)
>+			__pr_out_newline();
>+	}
>+
>+	if (!name)
>+		pr_out_object_end(dl);
>+}
>+
>+static void pr_out_info(struct dl *dl, const struct nlmsghdr *nlh,
>+			struct nlattr **tb, bool has_versions)
>+{
>+	__pr_out_handle_start(dl, tb, true, false);
>+
>+	__pr_out_indent_inc();
>+	if (!dl->opts.versions_attr && tb[DEVLINK_ATTR_INFO_DRIVER_NAME]) {
>+		struct nlattr *nla_drv = tb[DEVLINK_ATTR_INFO_DRIVER_NAME];
>+
>+		__pr_out_newline();
>+		pr_out_str(dl, "driver", mnl_attr_get_str(nla_drv));
>+	}
>+
>+	if (!dl->opts.versions_attr && tb[DEVLINK_ATTR_INFO_SERIAL_NUMBER]) {
>+		struct nlattr *nla_sn = tb[DEVLINK_ATTR_INFO_SERIAL_NUMBER];
>+
>+		__pr_out_newline();
>+		pr_out_str(dl, "serial_number", mnl_attr_get_str(nla_sn));
>+	}
>+	__pr_out_indent_dec();
>+
>+	if (has_versions) {
>+		pr_out_object_start(dl, "versions");
>+
>+		pr_out_versions_single(dl, nlh, "fixed",
>+				       DEVLINK_ATTR_INFO_VERSION_FIXED);
>+		pr_out_versions_single(dl, nlh, "running",
>+				       DEVLINK_ATTR_INFO_VERSION_RUNNING);
>+		pr_out_versions_single(dl, nlh, "stored",
>+				       DEVLINK_ATTR_INFO_VERSION_STORED);
>+
>+		pr_out_object_end(dl);
>+	}
>+
>+	pr_out_handle_end(dl);
>+}
>+
>+static int cmd_versions_show_cb(const struct nlmsghdr *nlh, void *data)
>+{
>+	struct genlmsghdr *genl = mnl_nlmsg_get_payload(nlh);
>+	struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {};
>+	bool has_versions, has_info;
>+	struct dl *dl = data;
>+
>+	mnl_attr_parse(nlh, sizeof(*genl), attr_cb, tb);
>+
>+	if (!tb[DEVLINK_ATTR_BUS_NAME] || !tb[DEVLINK_ATTR_DEV_NAME])
>+		return MNL_CB_ERROR;
>+
>+	has_versions = tb[DEVLINK_ATTR_INFO_VERSION_FIXED] ||
>+		tb[DEVLINK_ATTR_INFO_VERSION_RUNNING] ||
>+		tb[DEVLINK_ATTR_INFO_VERSION_STORED];
>+	has_info = tb[DEVLINK_ATTR_INFO_DRIVER_NAME] ||
>+		tb[DEVLINK_ATTR_INFO_SERIAL_NUMBER] ||
>+		has_versions;
>+
>+	if (has_info)
>+		pr_out_info(dl, nlh, tb, has_versions);
>+
>+	return MNL_CB_OK;
>+}
>+
>+static void cmd_dev_info_help(void)
>+{
>+	pr_err("Usage: devlink dev info [ DEV [ { versions [ VTYPE ] } ] ]\n");
>+	pr_err("\n");
>+	pr_err("       VTYPE := { fixed | running | stored }\n");
>+}
>+
>+static int cmd_dev_info(struct dl *dl)
>+{
>+	struct nlmsghdr *nlh;
>+	uint16_t flags = NLM_F_REQUEST | NLM_F_ACK;
>+	int err;
>+
>+	if (dl_argv_match(dl, "help")) {
>+		cmd_dev_info_help();
>+		return 0;
>+	}
>+
>+	if (dl_argc(dl) == 0)
>+		flags |= NLM_F_DUMP;
>+
>+	nlh = mnlg_msg_prepare(dl->nlg, DEVLINK_CMD_INFO_GET, flags);
>+
>+	if (dl_argc(dl) > 0) {
>+		err = dl_argv_parse_put(nlh, dl, DL_OPT_HANDLE,
>+					DL_OPT_VERSIONS_TYPE);
>+		if (err)
>+			return err;
>+	}
>+
>+	pr_out_section_start(dl, "info");
>+	err = _mnlg_socket_sndrcv(dl->nlg, nlh, cmd_versions_show_cb, dl);
>+	pr_out_section_end(dl);
>+	return err;
>+}
>+
> static int cmd_dev(struct dl *dl)
> {
> 	if (dl_argv_match(dl, "help")) {
>@@ -2433,6 +2637,9 @@ static int cmd_dev(struct dl *dl)
> 	} else if (dl_argv_match(dl, "param")) {
> 		dl_arg_inc(dl);
> 		return cmd_dev_param(dl);
>+	} else if (dl_argv_match(dl, "info")) {
>+		dl_arg_inc(dl);
>+		return cmd_dev_info(dl);
> 	}
> 	pr_err("Command \"%s\" not found\n", dl_argv(dl));
> 	return -ENOENT;
>diff --git a/man/man8/devlink-dev.8 b/man/man8/devlink-dev.8
>index d985da172aa0..5b05298f88bf 100644
>--- a/man/man8/devlink-dev.8
>+++ b/man/man8/devlink-dev.8
>@@ -63,6 +63,17 @@ devlink-dev \- devlink device configuration
> .BR "devlink dev reload"
> .IR DEV
> 
>+.ti -8
>+.BR "devlink dev info"
>+.RI "[ " DEV
>+.RI "["
>+.BR versions
>+.RI "{ "
>+.BR fixed " | " running " | " stored
>+.RI "} "
>+.RI "]"
>+.RI "]"
>+
> .SH "DESCRIPTION"
> .SS devlink dev show - display devlink device attributes
> 
>@@ -151,6 +162,31 @@ If this argument is omitted all parameters supported by devlink devices are list
> .I "DEV"
> - Specifies the devlink device to reload.
> 
>+.SS devlink dev info - display device information.
>+Display device information provided by the driver. This command can be used
>+to query versions of the hardware components or device components which
>+can't be updated (
>+.I fixed
>+) as well as device firmware which can be updated. For firmware components
>+.I running
>+displays the versions of firmware currently loaded into the device, while
>+.I stored
>+reports the versions in device's flash.
>+.I Running
>+and
>+.I stored
>+versions may differ after flash has been updated, but before reboot.
>+
>+.PP
>+.I "DEV"
>+- specifies the devlink device to show.
>+If this argument is omitted all devices are listed.
>+
>+.PP
>+.BR versions " { " fixed " | " running " | " stored " } "
>+- specifies the versions category to show.
>+If this argument is omitted all categories are listed.
>+
> .SH "EXAMPLES"
> .PP
> devlink dev show
>-- 
>2.19.2
>

Powered by blists - more mailing lists