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

Florian Fainelli
2019-03-28T02:25:56Z

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@suse.cz>
> ---

[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