Re: [PATCH net-next v5 12/22] ethtool: provide string sets with GET_STRSET request

[Florian Fainelli]

On 3/25/2019 10:08 AM, Michal Kubecek wrote:
> Requests a contents of one or more string sets, i.e. indexed arrays of
> strings; this information is provided by ETHTOOL_GSSET_INFO and
> ETHTOOL_GSTRINGS commands of ioctl interface. There are three types of
> requests:
> 
>   - no NLM_F_DUMP, no device: get "global" stringsets
>   - no NLM_F_DUMP, with device: get string sets related to the device
>   - NLM_F_DUMP, no device: get device related string sets for all devices
> 
> It's possible to request all string sets of given type or only specific
> sets. With ETHA_STRSET_COUNTS flag, only set sizes (number of strings) are
> returned.
> 
> Signed-off-by: Michal Kubecek <mkubecek@xxxxxxx>
> ---

[snip]

> diff --git a/Documentation/networking/ethtool-netlink.txt b/Documentation/networking/ethtool-netlink.txt
> index 5e5d785fe215..1508c16a236e 100644
> --- a/Documentation/networking/ethtool-netlink.txt
> +++ b/Documentation/networking/ethtool-netlink.txt
> @@ -127,6 +127,8 @@ List of message types
>  ---------------------
>  
>      ETHNL_CMD_EVENT			notification only
> +    ETHNL_CMD_GET_STRSET
> +    ETHNL_CMD_SET_STRSET		response only
>  
>  All constants use ETHNL_CMD_ prefix, usually followed by "GET", "SET" or "ACT"
>  to indicate the type.
> @@ -167,6 +169,46 @@ and also multiple events of the same type (e.g. two or more newly registered
>  devices).
>  
>  
> +GET_STRSET
> +----------
> +
> +Requests contents of a string set as provided by ioctl commands
> +ETHTOOL_GSSET_INFO and ETHTOOL_GSTRINGS. String sets are not user writeable so
> +that the corresponding SET_STRSET message is only used in kernel replies.
> +There are two types of string sets: global (independent of a device, e.g.
> +device feature names) and device specific (e.g. device private flags).
> +
> +Request contents:
> +
> +    ETHA_STRSET_DEV		(nested)	device identification
> +    ETHA_STRSET_COUNTS		(flag)		request only string counts

Should not that be part of the nested attribute under
ETHA_STRSET_STRINGSET. We should probably think about adding another
flag which indicates that we want to get the stringset associated data,
see below why.

> +    ETHA_STRSET_STRINGSET	(nested)	string set to request
> +        ETHA_STRINGSET_ID		(u32)		set id
> +
> +Kernel response contents:
> +
> +    ETHA_STRSET_DEV		(nested)	device identification
> +    ETHA_STRSET_STRINGSET	(nested)	string set to request

string set requested?

> +        ETHA_STRINGSET_ID		(u32)		set id
> +        ETHA_STRINGSET_COUNT		(u32)		number of strings
> +        ETHA_STRINGSET_STRINGS		(nested)	array of strings
> +            ETHA_STRING_INDEX			(u32)		string index
> +            ETHA_STRING_VALUE			(string)	string value

This is one of the areas where the legacy ethtool ioctl() is painful
because we need to request a string set to know how many of those exist
to allocate space for those in both kernel and user space.

If we could find a way to have a single command that allows us to dump
stringset (count, values) and associated data, then we save ourselves a
context switch and having to pre-allocate memory accordingly.
-- 
Florian