Messages in this thread | | | Date | Thu, 28 Mar 2019 14:43:13 +0100 | From | Jiri Pirko <> | Subject | Re: [PATCH net-next v5 12/22] ethtool: provide string sets with GET_STRSET request |
| |
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
| |