Re: [RFC PATCH net-next v2 09/17] ethtool: implement GET_DRVINFO message

[Andrew Lunn]

On Mon, Jul 30, 2018 at 02:53:27PM +0200, Michal Kubecek wrote:
> Requests the same information as ETHTOOL_GDRVINFO command in ioct
> interface. This is read-only so that corresponding SET_DRVINFO exists but
> is only used in kernel replies.
> 
> Signed-off-by: Michal Kubecek <mkubecek@xxxxxxx>
> ---
>  Documentation/networking/ethtool-netlink.txt |  38 +++++-
>  include/uapi/linux/ethtool_netlink.h         |  22 ++++
>  net/ethtool/Makefile                         |   4 +-
>  net/ethtool/common.c                         |  43 ++++++
>  net/ethtool/common.h                         |   3 +
>  net/ethtool/drvinfo.c                        | 131 +++++++++++++++++++
>  net/ethtool/ioctl.c                          |  42 +-----
>  net/ethtool/netlink.c                        |   8 ++
>  8 files changed, 252 insertions(+), 39 deletions(-)
>  create mode 100644 net/ethtool/drvinfo.c
> 
> diff --git a/Documentation/networking/ethtool-netlink.txt b/Documentation/networking/ethtool-netlink.txt
> index 8b43f41a8140..1e3d5ffc97ab 100644
> --- a/Documentation/networking/ethtool-netlink.txt
> +++ b/Documentation/networking/ethtool-netlink.txt
> @@ -121,6 +121,8 @@ List of message types
>      ETHNL_CMD_EVENT			notification only
>      ETHNL_CMD_GET_STRSET
>      ETHNL_CMD_SET_STRSET		response only
> +    ETHNL_CMD_GET_DRVINFO
> +    ETHNL_CMD_SET_DRVINFO		response only
>  
>  All constants use ETHNL_CMD_ prefix, usually followed by "GET", "SET" or "ACT"
>  to indicate the type.
> @@ -156,6 +158,40 @@ and also multiple events of the same type (e.g. two or more newly registered
>  devices).
>  
>  
> +GET_DRVINFO
> +-----------
> +
> +GET_DRVINFO request corresponds to ETHTOOL_GDRVINFO ioctl command and provides
> +basic driver information.
> +
> +Request contents:
> +
> +    ETHA_DRVINFO_DEV		(nested)	device identification
> +
> +Kernel response contents:
> +
> +    ETHA_DRVINFO_DEV		(nested)	device identification
> +    ETHA_DRVINFO_DRIVER		(string)	driver name
> +    ETHA_DRVINFO_VERSION	(string)	driver version

Driver version is generally useless. Is version X.Y.Z of a driver the
same when backported to some ancient enterprise kernel with 1000s of
patches? Now seems like a good opportunity to drop it.

> +    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

I know there is at least one driver that has somewhat dynamic number
of statistics. It would be better to have the strings and the values
in the same message, so there is no need to first get the number of
strings, allocate the memory, get the strings, then get the values.

> +    ETHA_DRVINFO_TESTINFO_LEN	(u32)		number of test results

In theory, this also does not need to be fixed. 

> +    ETHA_DRVINFO_EEDUMP_LEN	(u32)		EEPROM dump size
> +    ETHA_DRVINFO_REGDUMP_LEN	(u32)		register dump size

I would suggest removing all these _LEN properties, and let netlink
return what it needs to return.

       Andrew