Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH net-next v5 12/22] ethtool: provide string sets with
 GET_STRSET request
To:     Michal Kubecek <mkubecek@suse.cz>,
        David Miller <davem@davemloft.net>, netdev@vger.kernel.org
Cc:     Jakub Kicinski <jakub.kicinski@netronome.com>,
        Jiri Pirko <jiri@resnulli.us>, Andrew Lunn <andrew@lunn.ch>,
        John Linville <linville@tuxdriver.com>,
        Stephen Hemminger <stephen@networkplumber.org>,
        linux-kernel@vger.kernel.org
References: <cover.1553532199.git.mkubecek@suse.cz>
 <f5af581e33b4976b84eae7c7788e3fde9adfa83c.1553532199.git.mkubecek@suse.cz>
From:   Florian Fainelli <f.fainelli@gmail.com>
Openpgp: preference=signencrypt
Message-ID: <2c29310b-a2a0-3867-a09f-51f2dc47ecd3@gmail.com>
Date:   Wed, 27 Mar 2019 19:25:47 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.6.1
MIME-Version: 1.0
In-Reply-To: <f5af581e33b4976b84eae7c7788e3fde9adfa83c.1553532199.git.mkubecek@suse.cz>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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