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
Date: Thu, 28 Mar 2019 18:35:24 +0100 [thread overview]
Message-ID: <20190328173524.GR14297@nanopsycho> (raw)
In-Reply-To: <20190328140428.GG26076@unicorn.suse.cz>
Thu, Mar 28, 2019 at 03:04:28PM CET, mkubecek@suse.cz wrote:
>On Thu, Mar 28, 2019 at 02:43:13PM +0100, Jiri Pirko wrote:
>>
>> 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,
>> };
>
>We already have such enum. The problem with your "no string" approach is
>that it requires all userspace applications to (1) keep this enum in
That is how it is usually done. UAPI defines ATTRS and values, userspace
assigns appropriate strings.
>sync with kernel and (2) maintain their our tables of names. Experience
>shows we are not very good and satisfying these conditions even for the
>one which should be best at keeping up.
Moreover, I think that speed, duplex and type should be sent
separatelly:
ETHTOOL_A_LINK_MODE_LIST_OUR start nest
ETHTOOL_A_LINK_MODE start nest
ETHTOOL_A_LINK_MODE_SPEED = 100 /* this should be u64 */
ETHTOOL_A_LINK_MODE_DUPLEX = ETHTOOL_LINK_MODE_DUPLEX_FULL
ETHTOOL_A_LINK_MODE_TYPE = ETHTOOL_LINK_MODE_TYPE_BASET
ETHTOOL_A_LINK_MODE start end
ETHTOOL_A_LINK_MODE start nest
ETHTOOL_A_LINK_MODE_SPEED = 10
ETHTOOL_A_LINK_MODE_DUPLEX = ETHTOOL_LINK_MODE_DUPLEX_HALF
ETHTOOL_A_LINK_MODE_TYPE = ETHTOOL_LINK_MODE_TYPE_BASET
ETHTOOL_A_LINK_MODE start end
ETHTOOL_A_LINK_MODE_LIST_PEER end nest
Does not really make sense to combine those 3 attributes together.
>
>> 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.
>
>A bit too simple, actually. You would need third nest to distinguish
It is just an example.
>supported and advertised modes. And for setting, you would also need two
>arrays if you want to set only some of the modes (unless you introduce
>something that would be similar to mine except for omitting the names).
>
>More important: you still didn't explain how is your "no strings"
>approach supposed to work for bit sets where userspace cannot possibly
>know the set of available flags (e.g. the private flags).
I know, that is a different part of this thread.
>
>Michal
next prev parent reply other threads:[~2019-03-28 17:35 UTC|newest]
Thread overview: 109+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-25 17:07 [PATCH net-next v5 00/22] ethtool netlink interface, part 1 Michal Kubecek
2019-03-25 17:07 ` [PATCH net-next v5 01/22] rtnetlink: provide permanent hardware address in RTM_NEWLINK Michal Kubecek
2019-03-26 10:08 ` Jiri Pirko
2019-03-26 10:31 ` Michal Kubecek
2019-03-26 11:38 ` Jiri Pirko
2019-03-26 19:46 ` David Miller
2019-03-25 17:08 ` [PATCH net-next v5 02/22] netlink: introduce nla_put_bitfield32() Michal Kubecek
2019-03-26 10:09 ` Jiri Pirko
2019-03-28 1:56 ` Florian Fainelli
2019-03-25 17:08 ` [PATCH net-next v5 03/22] netlink: add strict version of nla_parse_nested() Michal Kubecek
2019-03-26 10:11 ` Jiri Pirko
2019-03-28 1:57 ` Florian Fainelli
2019-03-25 17:08 ` [PATCH net-next v5 04/22] ethtool: move to its own directory Michal Kubecek
2019-03-26 10:14 ` Jiri Pirko
2019-03-28 1:57 ` Florian Fainelli
2019-03-25 17:08 ` [PATCH net-next v5 05/22] ethtool: introduce ethtool netlink interface Michal Kubecek
2019-03-26 12:09 ` Jiri Pirko
2019-03-26 13:24 ` Michal Kubecek
2019-03-26 13:42 ` Jiri Pirko
2019-03-27 9:26 ` Michal Kubecek
2019-03-27 9:50 ` Jiri Pirko
2019-03-28 2:05 ` Florian Fainelli
2019-03-28 8:10 ` Jiri Pirko
2019-03-28 9:37 ` Michal Kubecek
2019-03-28 13:23 ` Jiri Pirko
2019-03-28 17:00 ` Florian Fainelli
2019-03-28 17:28 ` Jiri Pirko
2019-03-26 16:36 ` Jiri Pirko
2019-03-26 17:32 ` Michal Kubecek
2019-03-27 9:26 ` Jiri Pirko
2019-03-25 17:08 ` [PATCH net-next v5 06/22] ethtool: helper functions for " Michal Kubecek
2019-03-26 12:38 ` Jiri Pirko
2019-03-26 14:19 ` Michal Kubecek
2019-03-26 16:22 ` Jiri Pirko
2019-03-25 17:08 ` [PATCH net-next v5 07/22] ethtool: netlink bitset handling Michal Kubecek
2019-03-26 15:59 ` Jiri Pirko
2019-03-26 17:59 ` Michal Kubecek
2019-03-27 9:57 ` Jiri Pirko
2019-03-27 10:19 ` Michal Kubecek
2019-03-25 17:08 ` [PATCH net-next v5 08/22] ethtool: support for netlink notifications Michal Kubecek
2019-03-26 16:34 ` Jiri Pirko
2019-03-26 18:17 ` Michal Kubecek
2019-03-27 9:38 ` Jiri Pirko
2019-03-27 9:51 ` Andrew Lunn
2019-03-27 10:04 ` Jiri Pirko
2019-03-27 10:16 ` Andrew Lunn
2019-03-27 10:41 ` Jiri Pirko
2019-03-27 9:59 ` Michal Kubecek
2019-03-27 10:43 ` Jiri Pirko
2019-03-25 17:08 ` [PATCH net-next v5 09/22] ethtool: implement EVENT notifications Michal Kubecek
2019-03-27 13:04 ` Jiri Pirko
2019-03-27 14:14 ` Michal Kubecek
2019-03-28 2:14 ` Florian Fainelli
2019-03-28 6:41 ` Michal Kubecek
2019-03-28 9:16 ` Jiri Pirko
2019-03-25 17:08 ` [PATCH net-next v5 10/22] ethtool: generic handlers for GET requests Michal Kubecek
2019-03-27 16:35 ` Jiri Pirko
2019-03-27 21:53 ` Michal Kubecek
2019-03-28 13:32 ` Jiri Pirko
2019-03-25 17:08 ` [PATCH net-next v5 11/22] ethtool: move string arrays into common file Michal Kubecek
2019-03-28 2:17 ` Florian Fainelli
2019-03-25 17:08 ` [PATCH net-next v5 12/22] ethtool: provide string sets with GET_STRSET request Michal Kubecek
2019-03-27 20:12 ` Jiri Pirko
2019-03-27 22:56 ` Michal Kubecek
2019-03-29 8:43 ` Jiri Pirko
2019-03-28 2:25 ` Florian Fainelli
2019-03-28 7:18 ` Michal Kubecek
2019-03-28 13:43 ` Jiri Pirko
2019-03-28 14:04 ` Michal Kubecek
2019-03-28 17:35 ` Jiri Pirko [this message]
2019-03-28 18:52 ` Jakub Kicinski
2019-03-28 20:43 ` Michal Kubecek
2019-03-28 21:06 ` Jakub Kicinski
2019-03-29 6:52 ` Jiri Pirko
2019-03-28 22:28 ` Michal Kubecek
2019-03-25 17:08 ` [PATCH net-next v5 13/22] ethtool: provide driver/device information in GET_INFO request Michal Kubecek
2019-03-27 20:14 ` Jiri Pirko
2019-03-27 22:25 ` Michal Kubecek
2019-03-28 2:30 ` Florian Fainelli
2019-03-28 9:21 ` Jiri Pirko
2019-03-28 9:53 ` Michal Kubecek
2019-03-28 16:34 ` Jiri Pirko
2019-03-28 20:09 ` Jakub Kicinski
2019-03-28 22:46 ` Michal Kubecek
2019-03-29 18:48 ` Jakub Kicinski
2019-03-29 18:53 ` Florian Fainelli
2019-03-25 17:08 ` [PATCH net-next v5 14/22] ethtool: provide timestamping " Michal Kubecek
2019-03-28 3:36 ` Florian Fainelli
2019-03-28 10:03 ` Michal Kubecek
2019-03-25 17:08 ` [PATCH net-next v5 15/22] ethtool: provide link mode names as a string set Michal Kubecek
2019-03-28 3:52 ` Florian Fainelli
2019-03-25 17:08 ` [PATCH net-next v5 16/22] ethtool: provide link settings and link modes in GET_SETTINGS request Michal Kubecek
2019-03-28 3:44 ` Florian Fainelli
2019-03-28 10:04 ` Michal Kubecek
2019-03-25 17:08 ` [PATCH net-next v5 17/22] ethtool: set link settings and link modes with SET_SETTINGS request Michal Kubecek
2019-03-25 17:08 ` [PATCH net-next v5 18/22] ethtool: provide link state in GET_SETTINGS request Michal Kubecek
2019-03-25 17:08 ` [PATCH net-next v5 19/22] ethtool: provide WoL information " Michal Kubecek
2019-03-28 3:42 ` Florian Fainelli
2019-03-28 10:10 ` Michal Kubecek
2019-03-25 17:08 ` [PATCH net-next v5 20/22] ethtool: set WoL settings with SET_SETTINGS request Michal Kubecek
2019-03-28 3:49 ` Florian Fainelli
2019-03-25 17:08 ` [PATCH net-next v5 21/22] ethtool: provide message level in GET_SETTINGS request Michal Kubecek
2019-03-28 3:46 ` Florian Fainelli
2019-03-25 17:09 ` [PATCH net-next v5 22/22] ethtool: set message level with SET_SETTINGS request Michal Kubecek
2019-03-28 3:48 ` Florian Fainelli
2019-03-27 13:09 ` [PATCH net-next v5 00/22] ethtool netlink interface, part 1 Jiri Pirko
2019-03-27 14:28 ` Michal Kubecek
2019-03-27 15:19 ` Jiri Pirko
2019-03-27 19:12 ` David Miller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190328173524.GR14297@nanopsycho \
--to=jiri@resnulli.us \
--cc=andrew@lunn.ch \
--cc=davem@davemloft.net \
--cc=f.fainelli@gmail.com \
--cc=jakub.kicinski@netronome.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linville@tuxdriver.com \
--cc=mkubecek@suse.cz \
--cc=netdev@vger.kernel.org \
--cc=stephen@networkplumber.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).