diff mbox series

[net-next,v2,1/7] devlink: add device information API

Message ID 20190130190513.25718-2-jakub.kicinski@netronome.com
State Superseded
Delegated to: David Miller
Headers show
Series devlink: add device (driver) information API | expand

Commit Message

Jakub Kicinski Jan. 30, 2019, 7:05 p.m. UTC
ethtool -i has served us well for a long time, but its showing
its limitations more and more.  The device information should
also be reported per device not per-netdev.

Lay foundation for a simple devlink-based way of reading device
info.  Add driver name and device serial number as initial pieces
of information exposed via this new API.

RFC v2:
 - wrap the skb into an opaque structure (Jiri);
 - allow the serial number of be any length (Jiri & Andrew);
 - add driver name (Jonathan).

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 include/net/devlink.h        |  18 ++++++
 include/uapi/linux/devlink.h |   5 ++
 net/core/devlink.c           | 114 +++++++++++++++++++++++++++++++++++
 3 files changed, 137 insertions(+)

Comments

Jiri Pirko Jan. 30, 2019, 9:16 p.m. UTC | #1
Wed, Jan 30, 2019 at 08:05:07PM CET, jakub.kicinski@netronome.com wrote:
>ethtool -i has served us well for a long time, but its showing
>its limitations more and more.  The device information should

Double space here -------------^^


>also be reported per device not per-netdev.
>
>Lay foundation for a simple devlink-based way of reading device
>info.  Add driver name and device serial number as initial pieces
      ^^---------------+
Double space here -----+


>of information exposed via this new API.
>
>RFC v2:
> - wrap the skb into an opaque structure (Jiri);
> - allow the serial number of be any length (Jiri & Andrew);
> - add driver name (Jonathan).
>
>Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>---
> include/net/devlink.h        |  18 ++++++
> include/uapi/linux/devlink.h |   5 ++
> net/core/devlink.c           | 114 +++++++++++++++++++++++++++++++++++
> 3 files changed, 137 insertions(+)
>
>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index 85c9eabaf056..5ef3570a3859 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -429,6 +429,7 @@ enum devlink_param_wol_types {
> }
> 
> struct devlink_region;
>+struct devlink_info_req;
> 
> typedef void devlink_snapshot_data_dest_t(const void *data);
> 
>@@ -484,6 +485,8 @@ struct devlink_ops {
> 	int (*eswitch_encap_mode_get)(struct devlink *devlink, u8 *p_encap_mode);
> 	int (*eswitch_encap_mode_set)(struct devlink *devlink, u8 encap_mode,
> 				      struct netlink_ext_ack *extack);
>+	int (*info_get)(struct devlink *devlink, struct devlink_info_req *req,
>+			struct netlink_ext_ack *extack);
> };
> 
> static inline void *devlink_priv(struct devlink *devlink)
>@@ -607,6 +610,10 @@ u32 devlink_region_shapshot_id_get(struct devlink *devlink);
> int devlink_region_snapshot_create(struct devlink_region *region, u64 data_len,
> 				   u8 *data, u32 snapshot_id,
> 				   devlink_snapshot_data_dest_t *data_destructor);
>+int devlink_info_report_serial_number(struct devlink_info_req *req,
>+				      const char *sn);

I don't like the "report" part. The rest of the code uses "put".

Also. I think that verb should be at the
end of the function name, as it is common in the rest of the code.

So please rename to:
devlink_info_serial_number_put()
Same for the rest.


>+int devlink_info_report_driver_name(struct devlink_info_req *req,
>+				    const char *name);
> 
> #else
> 
>@@ -905,6 +912,17 @@ devlink_region_snapshot_create(struct devlink_region *region, u64 data_len,
> 	return 0;
> }
> 
>+static inline int
>+devlink_info_report_driver_name(struct devlink_info_req *req, const char *name)
>+{
>+	return 0;
>+}
>+
>+static inline int
>+devlink_info_report_serial_number(struct devlink_info_req *req, const char *sn)
>+{
>+	return 0;
>+}
> #endif
> 
> #endif /* _NET_DEVLINK_H_ */
>diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>index 61b4447a6c5b..fd089baa7c50 100644
>--- a/include/uapi/linux/devlink.h
>+++ b/include/uapi/linux/devlink.h
>@@ -94,6 +94,8 @@ enum devlink_command {
> 	DEVLINK_CMD_PORT_PARAM_NEW,
> 	DEVLINK_CMD_PORT_PARAM_DEL,
> 
>+	DEVLINK_CMD_INFO_GET,		/* can dump */
>+
> 	/* add new commands above here */
> 	__DEVLINK_CMD_MAX,
> 	DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1
>@@ -290,6 +292,9 @@ enum devlink_attr {
> 	DEVLINK_ATTR_REGION_CHUNK_ADDR,         /* u64 */
> 	DEVLINK_ATTR_REGION_CHUNK_LEN,          /* u64 */
> 
>+	DEVLINK_ATTR_INFO_DRV_NAME,		/* string */

Please be consistent across the names of function, attr etc. So:
	DEVLINK_ATTR_INFO_DRIVER_NAME,


Otherwise, this looks good.

Thanks!

[...]
diff mbox series

Patch

diff --git a/include/net/devlink.h b/include/net/devlink.h
index 85c9eabaf056..5ef3570a3859 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -429,6 +429,7 @@  enum devlink_param_wol_types {
 }
 
 struct devlink_region;
+struct devlink_info_req;
 
 typedef void devlink_snapshot_data_dest_t(const void *data);
 
@@ -484,6 +485,8 @@  struct devlink_ops {
 	int (*eswitch_encap_mode_get)(struct devlink *devlink, u8 *p_encap_mode);
 	int (*eswitch_encap_mode_set)(struct devlink *devlink, u8 encap_mode,
 				      struct netlink_ext_ack *extack);
+	int (*info_get)(struct devlink *devlink, struct devlink_info_req *req,
+			struct netlink_ext_ack *extack);
 };
 
 static inline void *devlink_priv(struct devlink *devlink)
@@ -607,6 +610,10 @@  u32 devlink_region_shapshot_id_get(struct devlink *devlink);
 int devlink_region_snapshot_create(struct devlink_region *region, u64 data_len,
 				   u8 *data, u32 snapshot_id,
 				   devlink_snapshot_data_dest_t *data_destructor);
+int devlink_info_report_serial_number(struct devlink_info_req *req,
+				      const char *sn);
+int devlink_info_report_driver_name(struct devlink_info_req *req,
+				    const char *name);
 
 #else
 
@@ -905,6 +912,17 @@  devlink_region_snapshot_create(struct devlink_region *region, u64 data_len,
 	return 0;
 }
 
+static inline int
+devlink_info_report_driver_name(struct devlink_info_req *req, const char *name)
+{
+	return 0;
+}
+
+static inline int
+devlink_info_report_serial_number(struct devlink_info_req *req, const char *sn)
+{
+	return 0;
+}
 #endif
 
 #endif /* _NET_DEVLINK_H_ */
diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 61b4447a6c5b..fd089baa7c50 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -94,6 +94,8 @@  enum devlink_command {
 	DEVLINK_CMD_PORT_PARAM_NEW,
 	DEVLINK_CMD_PORT_PARAM_DEL,
 
+	DEVLINK_CMD_INFO_GET,		/* can dump */
+
 	/* add new commands above here */
 	__DEVLINK_CMD_MAX,
 	DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1
@@ -290,6 +292,9 @@  enum devlink_attr {
 	DEVLINK_ATTR_REGION_CHUNK_ADDR,         /* u64 */
 	DEVLINK_ATTR_REGION_CHUNK_LEN,          /* u64 */
 
+	DEVLINK_ATTR_INFO_DRV_NAME,		/* string */
+	DEVLINK_ATTR_INFO_SERIAL_NUMBER,	/* string */
+
 	/* add new attributes above here, update the policy in devlink.c */
 
 	__DEVLINK_ATTR_MAX,
diff --git a/net/core/devlink.c b/net/core/devlink.c
index e6f170caf449..1b941493fdff 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -3714,6 +3714,112 @@  static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
 	return 0;
 }
 
+struct devlink_info_req {
+	struct sk_buff *msg;
+};
+
+int devlink_info_report_driver_name(struct devlink_info_req *req,
+				    const char *name)
+{
+	return nla_put_string(req->msg, DEVLINK_ATTR_INFO_DRV_NAME, name);
+}
+EXPORT_SYMBOL_GPL(devlink_info_report_driver_name);
+
+int devlink_info_report_serial_number(struct devlink_info_req *req,
+				      const char *sn)
+{
+	return nla_put_string(req->msg, DEVLINK_ATTR_INFO_SERIAL_NUMBER, sn);
+}
+EXPORT_SYMBOL_GPL(devlink_info_report_serial_number);
+
+static int
+devlink_nl_info_fill(struct sk_buff *msg, struct devlink *devlink,
+		     enum devlink_command cmd, u32 portid,
+		     u32 seq, int flags, struct netlink_ext_ack *extack)
+{
+	struct devlink_info_req req;
+	void *hdr;
+	int err;
+
+	hdr = genlmsg_put(msg, portid, seq, &devlink_nl_family, flags, cmd);
+	if (!hdr)
+		return -EMSGSIZE;
+
+	err = -EMSGSIZE;
+	if (devlink_nl_put_handle(msg, devlink))
+		goto err_cancel_msg;
+
+	req.msg = msg;
+	err = devlink->ops->info_get(devlink, &req, extack);
+	if (err)
+		goto err_cancel_msg;
+
+	genlmsg_end(msg, hdr);
+	return 0;
+
+err_cancel_msg:
+	genlmsg_cancel(msg, hdr);
+	return err;
+}
+
+static int devlink_nl_cmd_info_get_doit(struct sk_buff *skb,
+					struct genl_info *info)
+{
+	struct devlink *devlink = info->user_ptr[0];
+	struct sk_buff *msg;
+	int err;
+
+	if (!devlink->ops || !devlink->ops->info_get)
+		return -EOPNOTSUPP;
+
+	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+
+	err = devlink_nl_info_fill(msg, devlink, DEVLINK_CMD_INFO_GET,
+				   info->snd_portid, info->snd_seq, 0,
+				   info->extack);
+	if (err) {
+		nlmsg_free(msg);
+		return err;
+	}
+
+	return genlmsg_reply(msg, info);
+}
+
+static int devlink_nl_cmd_info_get_dumpit(struct sk_buff *msg,
+					  struct netlink_callback *cb)
+{
+	struct devlink *devlink;
+	int start = cb->args[0];
+	int idx = 0;
+	int err;
+
+	mutex_lock(&devlink_mutex);
+	list_for_each_entry(devlink, &devlink_list, list) {
+		if (!net_eq(devlink_net(devlink), sock_net(msg->sk)))
+			continue;
+		if (idx < start) {
+			idx++;
+			continue;
+		}
+
+		mutex_lock(&devlink->lock);
+		err = devlink_nl_info_fill(msg, devlink, DEVLINK_CMD_INFO_GET,
+					   NETLINK_CB(cb->skb).portid,
+					   cb->nlh->nlmsg_seq, NLM_F_MULTI,
+					   cb->extack);
+		mutex_unlock(&devlink->lock);
+		if (err)
+			break;
+		idx++;
+	}
+	mutex_unlock(&devlink_mutex);
+
+	cb->args[0] = idx;
+	return msg->len;
+}
+
 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 },
@@ -3974,6 +4080,14 @@  static const struct genl_ops devlink_nl_ops[] = {
 		.flags = GENL_ADMIN_PERM,
 		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK,
 	},
+	{
+		.cmd = DEVLINK_CMD_INFO_GET,
+		.doit = devlink_nl_cmd_info_get_doit,
+		.dumpit = devlink_nl_cmd_info_get_dumpit,
+		.policy = devlink_nl_policy,
+		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK,
+		/* can be retrieved by unprivileged users */
+	},
 };
 
 static struct genl_family devlink_nl_family __ro_after_init = {