Re: [RFC PATCH 5/9] ethtool: implement GET_DRVINFO message

[Jiri Pirko]

Mon, Dec 11, 2017 at 02:54:01PM CET, mkubecek@xxxxxxx wrote:
>Request the same information as ETHTOOL_GDRVINFO. This is read-only so that
>corresponding SET_DRVINFO exists but is only used in kernel replies. Rip
>the code to query the driver out of the legacy interface and move it to
>a new file ethtool_common.c so that both interfaces can use it.
>
>Signed-off-by: Michal Kubecek <mkubecek@xxxxxxx>
>---
> Documentation/networking/ethtool-netlink.txt | 30 ++++++++++-
> include/uapi/linux/ethtool_netlink.h         | 21 ++++++++
> net/core/Makefile                            |  2 +-
> net/core/ethtool.c                           | 42 +++-------------
> net/core/ethtool_common.c                    | 46 +++++++++++++++++
> net/core/ethtool_common.h                    | 11 ++++
> net/core/ethtool_netlink.c                   | 75 ++++++++++++++++++++++++++++
> 7 files changed, 189 insertions(+), 38 deletions(-)
> create mode 100644 net/core/ethtool_common.c
> create mode 100644 net/core/ethtool_common.h
>
>diff --git a/Documentation/networking/ethtool-netlink.txt b/Documentation/networking/ethtool-netlink.txt
>index 893e5156f6a7..cb992180b211 100644
>--- a/Documentation/networking/ethtool-netlink.txt
>+++ b/Documentation/networking/ethtool-netlink.txt
>@@ -107,6 +107,9 @@ which the request applies.
> List of message types
> ---------------------
> 
>+    ETHTOOL_CMD_GET_DRVINFO
>+    ETHTOOL_CMD_SET_DRVINFO		response only
>+
> All constants use ETHTOOL_CMD_ prefix followed by "GET", "SET" or "ACT" to
> indicate the type.
> 
>@@ -119,6 +122,31 @@ messages marked as "response only" in the table above.
> Later sections describe the format and semantics of these request messages.
> 
> 
>+GET_DRVINFO
>+-----------
>+
>+GET_DRVINFO request corresponds to ETHTOOL_GDRVINFO ioctl command and provides
>+basic driver information. The request doesn't use any attributes and flags,
>+info_mask and index field in request header are ignored. Kernel response
>+contents:
>+
>+    ETHA_DRVINFO_DRIVER		(string)	driver name
>+    ETHA_DRVINFO_VERSION	(string)	driver version

You use 3 prefixes:
ETHTOOL_ for cmd
ETHA_ for attrs
ethnl_ for function

I suggest to sync this, perhaps to:
ETHNL_CMD_*
ETHNL_ATTR_*
ethnl_*
?


>+    ETHA_DRVINFO_FWVERSION	(string)	firmware version
>+    ETHA_DRVINFO_BUSINFO	(string)	device bus address
>+    ETHA_DRVINFO_EROM_VER	(string)	expansion ROM version
>+    ETHA_DRVINFO_N_PRIV_FLAGS	(u32)		number of private flags
>+    ETHA_DRVINFO_N_STATS	(u32)		number of device stats
>+    ETHA_DRVINFO_TESTINFO_LEN	(u32)		number of test results
>+    ETHA_DRVINFO_EEDUMP_LEN	(u32)		EEPROM dump size
>+    ETHA_DRVINFO_REGDUMP_LEN	(u32)		register dump size

We are now working on providing various fw memory regions dump in
devlink. It makes sense to have it in devlink for couple of reasons:
1) per-asic, not netdev specific, therefore does not really make sense
   to have netdev as handle, but rather devlink handle.
2) snapshot support - we need to provide support for getting snapshot
   (for example on failure), transferring to user and deleting it
   (remove from driver memory).

Also, driver name, version, fwversion, etc is per-asic. Would make sense
to have it in devlink as well.

I think this is great opprotunity to move things where they should be to
be alligned with the current world and kernel infrastructure.