diff mbox series

[RFC,iproute2-next] devlink: add info subcommand

Message ID 20190115005009.16025-8-jakub.kicinski@netronome.com
State RFC
Delegated to: David Ahern
Headers show
Series [RFC,iproute2-next] devlink: add info subcommand | expand

Commit Message

Jakub Kicinski Jan. 15, 2019, 12:50 a.m. UTC 
Add support for reading the device serial number and versions
from the kernel.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 devlink/devlink.c       | 223 +++++++++++++++++++++++++++++++++++++++-
 man/man8/devlink-info.8 |  75 ++++++++++++++
 man/man8/devlink.8      |   5 +
 3 files changed, 302 insertions(+), 1 deletion(-)
 create mode 100644 man/man8/devlink-info.8

Comments

Jiri Pirko Jan. 15, 2019, 8:20 a.m. UTC | #1 
[...]

> static void help(void)
> {
> 	pr_err("Usage: devlink [ OPTIONS ] OBJECT { COMMAND | help }\n"
> 	       "       devlink [ -f[orce] ] -b[atch] filename\n"
>-	       "where  OBJECT := { dev | port | sb | monitor | dpipe | resource | region }\n"
>+	       "where  OBJECT := { dev | port | sb | monitor | dpipe | resource | region | info }\n"

I think that "info" should be nested under "dev". It is related to dev.
Maybe it even does not have to be a separate command and can be a nested
attribute to existing DEVLINK_CMD_GET cmd.



> 	       "       OPTIONS := { -V[ersion] | -n[o-nice-names] | -j[son] | -p[retty] | -v[erbose] }\n");
> }
Andrew Lunn Jan. 15, 2019, 2 p.m. UTC | #2 
On Tue, Jan 15, 2019 at 09:20:11AM +0100, Jiri Pirko wrote:
> [...]
> 
> > static void help(void)
> > {
> > 	pr_err("Usage: devlink [ OPTIONS ] OBJECT { COMMAND | help }\n"
> > 	       "       devlink [ -f[orce] ] -b[atch] filename\n"
> >-	       "where  OBJECT := { dev | port | sb | monitor | dpipe | resource | region }\n"
> >+	       "where  OBJECT := { dev | port | sb | monitor | dpipe | resource | region | info }\n"
> 
> I think that "info" should be nested under "dev". It is related to dev.
> Maybe it even does not have to be a separate command and can be a nested
> attribute to existing DEVLINK_CMD_GET cmd.

Hi Jiri

Not that i agree devlink is the right place for this, but i think it
probably needs to be nested both under dev and port. I could have a
line card implementing a port which has version information, as well
as version information for the backplane which would be under dev.

   Andrew
Jiri Pirko Jan. 15, 2019, 2:07 p.m. UTC | #3 
Tue, Jan 15, 2019 at 03:00:46PM CET, andrew@lunn.ch wrote:
>On Tue, Jan 15, 2019 at 09:20:11AM +0100, Jiri Pirko wrote:
>> [...]
>> 
>> > static void help(void)
>> > {
>> > 	pr_err("Usage: devlink [ OPTIONS ] OBJECT { COMMAND | help }\n"
>> > 	       "       devlink [ -f[orce] ] -b[atch] filename\n"
>> >-	       "where  OBJECT := { dev | port | sb | monitor | dpipe | resource | region }\n"
>> >+	       "where  OBJECT := { dev | port | sb | monitor | dpipe | resource | region | info }\n"
>> 
>> I think that "info" should be nested under "dev". It is related to dev.
>> Maybe it even does not have to be a separate command and can be a nested
>> attribute to existing DEVLINK_CMD_GET cmd.
>
>Hi Jiri
>
>Not that i agree devlink is the right place for this, but i think it

Where else to put it?

>probably needs to be nested both under dev and port. I could have a
>line card implementing a port which has version information, as well
>as version information for the backplane which would be under dev.

Okay. Sounds good to me.


>
>   Andrew
Jakub Kicinski Jan. 15, 2019, 5:53 p.m. UTC | #4 
On Tue, 15 Jan 2019 09:20:11 +0100, Jiri Pirko wrote:
> > static void help(void)
> > {
> > 	pr_err("Usage: devlink [ OPTIONS ] OBJECT { COMMAND | help }\n"
> > 	       "       devlink [ -f[orce] ] -b[atch] filename\n"
> >-	       "where  OBJECT := { dev | port | sb | monitor | dpipe | resource | region }\n"
> >+	       "where  OBJECT := { dev | port | sb | monitor | dpipe | resource | region | info }\n"  
> 
> I think that "info" should be nested under "dev". It is related to dev.

Ack.

> Maybe it even does not have to be a separate command and can be a nested
> attribute to existing DEVLINK_CMD_GET cmd.

I thought about that, but I'd rather keep it as a separate command.  
I think it'd be good to keep DEVLINK_CMD_GET nice and lean.  

For versions there may be FW communication required, and reading stuff
out of flash.  I bit of overhead for users who just want the list of
devlink instances.

Having in under dev but as a separate command seems quite nice indeed.
Especially given that there can only be a show subcommand..  So:

For dump:
$ devlink dev info

But for get:
$ devlink dev pci/0000:82:00.0 info

or

$ devlink dev info pci/0000:82:00.0

?
Jakub Kicinski Jan. 15, 2019, 5:58 p.m. UTC | #5 
On Tue, 15 Jan 2019 15:00:46 +0100, Andrew Lunn wrote:
> I could have a line card implementing a port which has version
> information, as well as version information for the backplane which
> would be under dev.

I'd argue a line card is not a port, so adding port info for line cards
is, again, taking a step backward towards unclear semantics.  Line cards
are a very well understood concept, and they deserve their own
handling.  And ports may _belong_ to line cards.
Jiri Pirko Jan. 15, 2019, 6:05 p.m. UTC | #6 
Tue, Jan 15, 2019 at 06:53:52PM CET, jakub.kicinski@netronome.com wrote:
>On Tue, 15 Jan 2019 09:20:11 +0100, Jiri Pirko wrote:
>> > static void help(void)
>> > {
>> > 	pr_err("Usage: devlink [ OPTIONS ] OBJECT { COMMAND | help }\n"
>> > 	       "       devlink [ -f[orce] ] -b[atch] filename\n"
>> >-	       "where  OBJECT := { dev | port | sb | monitor | dpipe | resource | region }\n"
>> >+	       "where  OBJECT := { dev | port | sb | monitor | dpipe | resource | region | info }\n"  
>> 
>> I think that "info" should be nested under "dev". It is related to dev.
>
>Ack.
>
>> Maybe it even does not have to be a separate command and can be a nested
>> attribute to existing DEVLINK_CMD_GET cmd.
>
>I thought about that, but I'd rather keep it as a separate command.  
>I think it'd be good to keep DEVLINK_CMD_GET nice and lean.  

Okay. Fair enough.


>
>For versions there may be FW communication required, and reading stuff
>out of flash.  I bit of overhead for users who just want the list of
>devlink instances.
>
>Having in under dev but as a separate command seems quite nice indeed.
>Especially given that there can only be a show subcommand..  So:
>
>For dump:
>$ devlink dev info
>
>But for get:
>$ devlink dev pci/0000:82:00.0 info
>
>or
>
>$ devlink dev info pci/0000:82:00.0

This is aligned with the rest.


>
>?
Jakub Kicinski Jan. 15, 2019, 6:32 p.m. UTC | #7 
On Tue, 15 Jan 2019 19:05:01 +0100, Jiri Pirko wrote:
> > For versions there may be FW communication required, and reading stuff
> > out of flash.  I bit of overhead for users who just want the list of
> > devlink instances.
> >
> > Having in under dev but as a separate command seems quite nice indeed.
> > Especially given that there can only be a show subcommand..  So:
> >
> > For dump:
> > $ devlink dev info
> >
> > But for get:
> > $ devlink dev pci/0000:82:00.0 info
> >
> > or
> >
> > $ devlink dev info pci/0000:82:00.0
>
> This is aligned with the rest.

Cool, thanks for all the comments!
diff mbox series

Patch

diff --git a/devlink/devlink.c b/devlink/devlink.c
index 3651e90c1159..a8ba6e8d3d9d 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)
 
 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,12 @@  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_VERSIONS] = MNL_TYPE_NESTED,
+	[DEVLINK_ATTR_INFO_VERSIONS_FIXED] = MNL_TYPE_NESTED,
+	[DEVLINK_ATTR_INFO_VERSIONS_RUNNING] = MNL_TYPE_NESTED,
+	[DEVLINK_ATTR_INFO_VERSIONS_STORED] = MNL_TYPE_NESTED,
+	[DEVLINK_ATTR_INFO_VERSIONS_NAME] = MNL_TYPE_STRING,
+	[DEVLINK_ATTR_INFO_VERSIONS_VALUE] = MNL_TYPE_STRING,
 };
 
 static int attr_cb(const struct nlattr *attr, void *data)
@@ -943,6 +951,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_VERSIONS_FIXED;
+	} else if (strcmp(typestr, "stored") == 0) {
+		*p_attr = DEVLINK_ATTR_INFO_VERSIONS_STORED;
+	} else if (strcmp(typestr, "running") == 0) {
+		*p_attr = DEVLINK_ATTR_INFO_VERSIONS_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 +1201,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;
@@ -1684,6 +1720,28 @@  static void pr_out_u64(struct dl *dl, const char *name, uint64_t val)
 	return pr_out_uint(dl, name, val);
 }
 
+static void pr_out_bytes(struct dl *dl, const char *name,
+			 uint8_t *data, uint32_t len)
+{
+	unsigned int i;
+
+	if (dl->json_output) {
+		jsonw_name(dl->jw, name);
+		jsonw_start_array(dl->jw);
+		for (i = 0; i < len; i++)
+			jsonw_printf(dl->jw, "%d", data[i]);
+		jsonw_end_array(dl->jw);
+	} else {
+		__pr_out_indent_inc();
+		__pr_out_newline();
+		pr_out("%s: %02hhx", name, data[0]);
+		for (i = 1; i < len; i++)
+			pr_out(":%02hhx", data[i]);
+		__pr_out_newline();
+		__pr_out_indent_dec();
+	}
+}
+
 static void pr_out_region_chunk_start(struct dl *dl, uint64_t addr)
 {
 	if (dl->json_output) {
@@ -1775,6 +1833,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)
@@ -5557,11 +5639,147 @@  static int cmd_region(struct dl *dl)
 	return -ENOENT;
 }
 
+static void pr_out_versions_single(struct dl *dl, struct nlattr *versions_attr,
+				   const char *name, int type)
+{
+	struct nlattr *version;
+
+	if (dl->opts.versions_attr && dl->opts.versions_attr != type)
+		return;
+
+	mnl_attr_for_each_nested(version, versions_attr) {
+		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_VERSIONS_NAME] ||
+		    !tb[DEVLINK_ATTR_INFO_VERSIONS_VALUE])
+			continue;
+
+		if (name) {
+			pr_out_object_start(dl, name);
+			name = NULL;
+		}
+
+		ver_name = mnl_attr_get_str(tb[DEVLINK_ATTR_INFO_VERSIONS_NAME]);
+		ver_value = mnl_attr_get_str(tb[DEVLINK_ATTR_INFO_VERSIONS_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, struct nlattr **tb)
+{
+	__pr_out_handle_start(dl, tb, true, false);
+
+	if (!dl->opts.versions_attr && tb[DEVLINK_ATTR_INFO_SERIAL_NUMBER]) {
+		struct nlattr *nla_sn = tb[DEVLINK_ATTR_INFO_SERIAL_NUMBER];
+
+		pr_out_bytes(dl, "serial_number",
+			     mnl_attr_get_payload(nla_sn),
+			     mnl_attr_get_payload_len(nla_sn));
+	}
+
+	if (tb[DEVLINK_ATTR_INFO_VERSIONS]) {
+		pr_out_object_start(dl, "versions");
+
+		pr_out_versions_single(dl, tb[DEVLINK_ATTR_INFO_VERSIONS],
+				       "fixed",
+				       DEVLINK_ATTR_INFO_VERSIONS_FIXED);
+		pr_out_versions_single(dl, tb[DEVLINK_ATTR_INFO_VERSIONS],
+				       "running",
+				       DEVLINK_ATTR_INFO_VERSIONS_RUNNING);
+		pr_out_versions_single(dl, tb[DEVLINK_ATTR_INFO_VERSIONS],
+				       "stored",
+				       DEVLINK_ATTR_INFO_VERSIONS_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] = {};
+	struct dl *dl = data;
+
+	mnl_attr_parse(nlh, sizeof(*genl), attr_cb, tb);
+
+	if (!tb[DEVLINK_ATTR_BUS_NAME] || !tb[DEVLINK_ATTR_DEV_NAME] ||
+	    (!tb[DEVLINK_ATTR_INFO_VERSIONS] &&
+	     !tb[DEVLINK_ATTR_INFO_SERIAL_NUMBER]))
+		return MNL_CB_ERROR;
+
+	pr_out_info(dl, tb);
+
+	return MNL_CB_OK;
+}
+
+static int cmd_info_show(struct dl *dl)
+{
+	struct nlmsghdr *nlh;
+	uint16_t flags = NLM_F_REQUEST | NLM_F_ACK;
+	int err;
+
+	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 void cmd_info_help(void)
+{
+	pr_err("Usage: devlink info show [ DEV [ { versions [ VTYPE ] } ] ]\n"
+	       "       VTYPE := { fixed | running | stored }\n");
+}
+
+static int cmd_info(struct dl *dl)
+{
+	if (dl_no_arg(dl)) {
+		return cmd_info_show(dl);
+	} else if (dl_argv_match(dl, "help")) {
+		cmd_info_help();
+		return 0;
+	} else if (dl_argv_match(dl, "show")) {
+		dl_arg_inc(dl);
+		return cmd_info_show(dl);
+	}
+	pr_err("Command \"%s\" not found\n", dl_argv(dl));
+	return -ENOENT;
+}
+
 static void help(void)
 {
 	pr_err("Usage: devlink [ OPTIONS ] OBJECT { COMMAND | help }\n"
 	       "       devlink [ -f[orce] ] -b[atch] filename\n"
-	       "where  OBJECT := { dev | port | sb | monitor | dpipe | resource | region }\n"
+	       "where  OBJECT := { dev | port | sb | monitor | dpipe | resource | region | info }\n"
 	       "       OPTIONS := { -V[ersion] | -n[o-nice-names] | -j[son] | -p[retty] | -v[erbose] }\n");
 }
 
@@ -5594,6 +5812,9 @@  static int dl_cmd(struct dl *dl, int argc, char **argv)
 	} else if (dl_argv_match(dl, "region")) {
 		dl_arg_inc(dl);
 		return cmd_region(dl);
+	} else if (dl_argv_match(dl, "info")) {
+		dl_arg_inc(dl);
+		return cmd_info(dl);
 	}
 	pr_err("Object \"%s\" not found\n", dl_argv(dl));
 	return -ENOENT;
diff --git a/man/man8/devlink-info.8 b/man/man8/devlink-info.8
new file mode 100644
index 000000000000..f32b05091f51
--- /dev/null
+++ b/man/man8/devlink-info.8
@@ -0,0 +1,75 @@ 
+.TH DEVLINK\-INFO 8 "11 Jan 2019" "iproute2" "Linux"
+.SH NAME
+devlink-info \- devlink device info reporting
+.SH SYNOPSIS
+.sp
+.ad l
+.in +8
+.ti -8
+.B devlink
+.RI "[ " OPTIONS " ]"
+.B info
+.RI  " { " COMMAND " | "
+.BR help " }"
+.sp
+
+.ti -8
+.IR OPTIONS " := { "
+\fB\-V\fR[\fIersion\fR] |
+\fB\-n\fR[\fIno-nice-names\fR] }
+
+.ti -8
+.B devlink info show
+.RI "[ " DEV " ]"
+.RI "["
+.BR versions
+.RI "{ "
+.BR fixed " | " running " | " stored
+.RI "} "
+.RI "]"
+
+.ti -8
+.B devlink info help
+
+.SH "DESCRIPTION"
+.SS devlink info show - 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 devce, while
+.I stored
+reports the versions in device flash.
+.I Running
+and
+.I stored
+versions may be different after flash update, but before reboot.
+
+.PP
+.I "DEV"
+- specifies the devlink device to show.
+If this argument is omitted all devices are listed.
+
+.in +4
+Format is:
+.in +2
+BUS_NAME/BUS_ADDRESS
+
+.PP
+.BR versions
+.BR fixed
+.RI " | "
+.BR running
+.RI " | "
+.BR stored
+- specifies the versions category to show.
+If this argument is omitted all categories are listed.
+
+.SH SEE ALSO
+.BR devlink (8),
+.br
+
+.SH AUTHOR
+Jakub Kicinski <jakub.kicinski@netronome.com>
diff --git a/man/man8/devlink.8 b/man/man8/devlink.8
index 8d527e7e1d60..4cb3a1113117 100644
--- a/man/man8/devlink.8
+++ b/man/man8/devlink.8
@@ -78,6 +78,10 @@  Turn on verbose output.
 .B region
 - devlink address region access
 
+.TP
+.B info
+- devlink device info reporting.
+
 .SS
 .I COMMAND
 
@@ -109,6 +113,7 @@  Exit status is 0 if command was successful or a positive integer upon failure.
 .BR devlink-sb (8),
 .BR devlink-resource (8),
 .BR devlink-region (8),
+.BR devlink-info (8),
 .br
 
 .SH REPORTING BUGS