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;
Date:   Thu, 28 Mar 2019 14:43:13 +0100
From:   Jiri Pirko <jiri@resnulli.us>
To:     Michal Kubecek <mkubecek@suse.cz>
Cc:     Florian Fainelli <f.fainelli@gmail.com>,
        David Miller <davem@davemloft.net>, netdev@vger.kernel.org,
        Jakub Kicinski <jakub.kicinski@netronome.com>,
        Andrew Lunn <andrew@lunn.ch>,
        John Linville <linville@tuxdriver.com>,
        Stephen Hemminger <stephen@networkplumber.org>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next v5 12/22] ethtool: provide string sets with
 GET_STRSET request
Message-ID: <20190328134313.GO14297@nanopsycho>
References: <cover.1553532199.git.mkubecek@suse.cz>
 <f5af581e33b4976b84eae7c7788e3fde9adfa83c.1553532199.git.mkubecek@suse.cz>
 <2c29310b-a2a0-3867-a09f-51f2dc47ecd3@gmail.com>
 <20190328071853.GY26076@unicorn.suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190328071853.GY26076@unicorn.suse.cz>
User-Agent: Mutt/1.11.3 (2019-02-01)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Thu, Mar 28, 2019 at 08:18:53AM CET, mkubecek@suse.cz wrote:
>On Wed, Mar 27, 2019 at 07:25:47PM -0700, Florian Fainelli wrote:
>> > +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.
>
>The way the proposed interface is designed, we can get both without an
>extra roundtrip but it's done the other way around, i.e. passing the
>strings with the request for data. The API allows two modes of
>operation:
>
>1. One shot requests, e.g. typical ethtool use. We use "verbose" mode
>(no ETHA_*_COMPACT flag in the request) and data is provided together
>with the names. E.g. "ethtool eth2" request would look like
>
>  ETHA_SETTINGS_DEV
>      ETHA_DEV_NAME = "eth2"
>  ETHA_SETTINGS_INFO_MASK = ... | ETH_SETTINGS_IM_LINKMODES | ...
>
>and the reply would be
>
>  ETHA_SETTINGS_DEV = {
>      ETHA_DEV_INDEX = 4
>      ETHA_DEV_NAME = "eth2"
>  }
>  ETHA_SETTINGS_LINK_INFO = {
>      ...
>  }
>  ETHA_SETTRINGS_LINK_MODES = {
>      ETHA_LINKMODES_AUTONEG = 1 (AUTONEG_ENABLE)
>      ETHA_LINKMODES_OURS = {
>          ETHA_BITSET_SIZE = 67
>          ETHA_BITSET_BITS = {
>              ETHA_BITS_BIT = {
>                  ETHA_BIT_INDEX = 0 (ETHTOOL_LINK_MODE_10baseT_Half_BIT)
>                  ETHA_BIT_NAME = "10baseT/Half"
>              }
>              ETHA_BITS_BIT = {
>                  ETHA_BIT_INDEX = 1 (ETHTOOL_LINK_MODE_10baseT_Full_BIT)
>                  ETHA_BIT_NAME = "10baseT/Full"
>              }
>              ETHA_BITS_BIT = {
>                  ETHA_BIT_INDEX = 2 (ETHTOOL_LINK_MODE_100baseT_Half_BIT)
>                  ETHA_BIT_NAME = "100baseT/Half"
>                  ETHA_BIT_VALUE
>              }
>              ETHA_BITS_BIT = {
>                  ETHA_BIT_INDEX = 3 (ETHTOOL_LINK_MODE_100baseT_Full_BIT)
>                  ETHA_BIT_NAME = "100baseT/Full"
>                  ETHA_BIT_VALUE
>              }
>              ETHA_BITS_BIT = {
>                  ETHA_BIT_INDEX = 5 (ETHTOOL_LINK_MODE_1000baseT_Full_BIT)
>                  ETHA_BIT_NAME = "1000baseT/Full"
>                  ETHA_BIT_VALUE

I don't like this. This should not be bitfield/set. This should be
simply nested array of enum values:

enum ethtool_link_mode {
	ETHTOOL_LINK_MODE_10baseT_Half,
	ETHTOOL_LINK_MODE_10baseT_Full,
	ETHTOOL_LINK_MODE_100baseT_Half,
	ETHTOOL_LINK_MODE_100baseT_Full,
	ETHTOOL_LINK_MODE_1000baseT_Full,
};

and then there should be 2 attrs:
ETHTOOL_A_LINK_MODE_LIST_OUR	/* nest */
ETHTOOL_A_LINK_MODE_LIST_PEER	/* nest */
ETHTOOL_A_LINK_MODE		/* u32 */

and then the message should look like:

   ETHTOOL_A_LINK_MODE_LIST_OUR start nest
     ETHTOOL_A_LINK_MODE = ETHTOOL_LINK_MODE_10baseT_Half
     ETHTOOL_A_LINK_MODE = ETHTOOL_LINK_MODE_10baseT_Full
     ETHTOOL_A_LINK_MODE = ETHTOOL_LINK_MODE_100baseT_Half
     ETHTOOL_A_LINK_MODE = ETHTOOL_LINK_MODE_100baseT_Full
     ETHTOOL_A_LINK_MODE = ETHTOOL_LINK_MODE_1000baseT_Full
   ETHTOOL_A_LINK_MODE_LIST_OUR end nest
   ETHTOOL_A_LINK_MODE_LIST_PEER start nest
     ETHTOOL_A_LINK_MODE = ETHTOOL_LINK_MODE_10baseT_Half
     ETHTOOL_A_LINK_MODE = ETHTOOL_LINK_MODE_10baseT_Full
     ETHTOOL_A_LINK_MODE = ETHTOOL_LINK_MODE_100baseT_Half
     ETHTOOL_A_LINK_MODE = ETHTOOL_LINK_MODE_100baseT_Full
     ETHTOOL_A_LINK_MODE = ETHTOOL_LINK_MODE_1000baseT_Full
   ETHTOOL_A_LINK_MODE_LIST_PEER end nest

Nice and simple. No bits, no strings.


>              }
>          }
>      }
>      ETHA_LINKMODES_PEER = {
>          ...
>      }
>      ETHA_LINKMODES_SPEED = 1000 (SPEED_1000)
>      ETHA_LINKMODES_DUPLEX = 1 (DUPLEX_FULL)
>  }
>  ...
>
>2. Long time running applications, e.g. "ethtool --monitor", wicked,
>systemd-networkd, ... For these, it would make sense to cache the
>strings and either retrieve them in advance (on start and when new
>device appears) or when they are needed for the first time. The request
>would then include the ETHA_SETTINGS_COMPACT flag and reply would be
>
>  ...
>  ETHA_LINKMODES_OURS = {
>      ETHA_BITSET_SIZE = 67
>      ETHA_BITSET_VALUE = 0x0000002c 0x00000000 0x00000000
>      ETHA_BITSET_MASK = 0x0000002f 0x00000000 0x00000000
>  }
>
>For "set" type requests, it's up to the application if it uses verbose
>or compact form. The verbose form is most useful when we want e.g. to
>set the values of one or two bits which may be identified by their names
>(on command line or in config file).
>
>Michal