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:

quoted

quoted

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

quoted

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

quoted

+        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