netdev - Re: [PATCH net-next v2 1/7] devlink: add device information API

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190130211616.GA349@nanopsycho.orion>
Date:   Wed, 30 Jan 2019 22:16:16 +0100
From:   Jiri Pirko <jiri@...nulli.us>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     davem@...emloft.net, netdev@...r.kernel.org,
        oss-drivers@...ronome.com, andrew@...n.ch, f.fainelli@...il.com,
        mkubecek@...e.cz, eugenem@...com, jonathan.lemon@...il.com
Subject: Re: [PATCH net-next v2 1/7] devlink: add device information API

Wed, Jan 30, 2019 at 08:05:07PM CET, jakub.kicinski@...ronome.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@...ronome.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!

[...]