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:   Tue, 19 Feb 2019 11:35:08 +0100
From:   Jiri Pirko <jiri@resnulli.us>
To:     Michal Kubecek <mkubecek@suse.cz>
Cc:     netdev@vger.kernel.org, David Miller <davem@davemloft.net>,
        Andrew Lunn <andrew@lunn.ch>,
        Jakub Kicinski <jakub.kicinski@netronome.com>,
        linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH net-next v3 00/21] ethtool netlink interface, part 1
Message-ID: <20190219103508.GD3080@nanopsycho>
References: <cover.1550513384.git.mkubecek@suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <cover.1550513384.git.mkubecek@suse.cz>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Mon, Feb 18, 2019 at 07:21:24PM CET, mkubecek@suse.cz wrote:
>Note: this is marked as RFC because it's rather late in the cycle; the plan
>is to make a regular submission (with changes based on review) once
>net-next reopens after the 5.1 merge window. The full (work in progress)
>series, together with the (userspace) ethtool counterpart can be found at
>https://github.com/mkubecek/ethnl
>
>This is first part of alternative userspace interface for ethtool. The aim
>is to address some long known issues with the ioctl interface, mainly lack
>of extensibility, raciness, limited error reporting and absence of
>notifications.
>
>The interface uses generic netlink family "ethtool"; it provides multicast
>group "monitor" which is used for notifications. Documentation for the
>interface is in Documentation/networking/ethtool-netlink.txt
>
>Basic concepts:
>
>- the goal is to provide all features of ioctl interface but allow
>  easier future extensions; at some point, it should be possible to have
>  full ethtool functionality without using the ioctl interface

I'm not sure it is a good idea to map the existing iface to netlink
fully. There are things in ethtool which are not really unique for
Ethernet. Those things should be put somewhere else.


>- inextensibility of ioctl interface resulted in way too many commands,
>  many of them obsoleted by newer ones; reduce the number by  ignoring the
>  obsolete commands and grouping some together
>- for "set" type commands, allows passing only the attributes to be
>  changed; therefore we don't need a get-modify-set cycle (which is
>  inherently racy), userspace can simply say what it wants to change
>- provide notifications to multicast group "monitor" like rtnetlink does,
>  i.e. in the form of messages close to replies to "get" requests
>- allow dump requests to get some information about all network devices
>  providing it
>- be less dependent on ethtool and kernel being in sync; allow e.g. saying
>  "ethtool -s eth0 advertise foo off" without ethtool knowing what "foo"
>  means; it's kernel's job to know what mode "xyz" is and if it exists and
>  is supported
>
>Main changes between RFC v2 and RFC v3:
>
>- do not allow building as a module (no netdev notifiers needed)
>- drop some obsolete fields
>- add permanent hw address, timestamping and private flags support
>- rework bitset handling to get rid of variable length arrays
>- notify monitor on device renames
>- restructure GET_SETTINGS/SET_SETTINGS messages
>- split too long patches and submit only first part of the series
>
>Main changes between RFC v1 and RFC v2:
>
>- support dumps for all "get" requests
>- provide notifications for changes related to supported request types
>- support getting string sets (both global and per device)
>- support getting/setting device features
>- get rid of family specific header, everything passed as attributes
>- split netlink code into multiple files in net/ethtool/ directory
>
>ToDo / open questions:
>
>- some features provided by ethtool would rather belong to devlink (and
>  some are already superseded by devlink); however, only few drivers
>  provide devlink interface at the moment and as recent discussion on
>  flashing revealed, we cannot rely on devlink's presence

Could you explain why please?


>
>- while the netlink interface allows easy future extensions, ethtool_ops
>  interface does not; some settings could be implemented using tunables and
>  accessed via relevant netlink messages (as well as tunables) from
>  userspace but in the long term, something better will be needed
>
>- currently, all communication with drivers via ethtool_ops is done
>  under RTNL as this is what ioctl interface does and likely many
>  ethtool_ops handlers rely on that; if we are going to rework ethtool_ops
>  in the future ("phase two"), it would be nice to get rid of it
>
>- ethtool_ops should pass extack pointer to allow drivers more meaningful
>  error reporting; it's not clear, however, how to pass information about
>  offending attribute
>
>- notifications are sent whenever a change is done via netlink API or
>  ioctl API and for netdev features also whenever they are updated using
>  netdev_change_features(); it would be desirable to notify also about
>  link state and negotiation result (speed/duplex and partner link
>  modes) but it would be more tricky
>
>Michal Kubecek (21):
>  netlink: introduce nla_put_bitfield32()
>  ethtool: move to its own directory
>  ethtool: introduce ethtool netlink interface
>  ethtool: helper functions for netlink interface
>  ethtool: netlink bitset handling
>  ethtool: support for netlink notifications
>  ethtool: implement EVENT notifications
>  ethtool: generic handlers for GET requests
>  ethtool: move string arrays into common file
>  ethtool: provide string sets with GET_STRSET request
>  ethtool: provide driver/device information in GET_INFO request
>  ethtool: provide permanent hardware address in GET_INFO request
>  ethtool: provide timestamping information in GET_INFO request
>  ethtool: provide link mode names as a string set
>  ethtool: provide link settings and link modes in GET_SETTINGS request
>  ethtool: provide WoL information in GET_SETTINGS request
>  ethtool: provide message level in GET_SETTINGS request
>  ethtool: provide link state in GET_SETTINGS request
>  ethtool: provide device features in GET_SETTINGS request
>  ethtool: provide private flags in GET_SETTINGS request
>  ethtool: send netlink notifications about setting changes
>
> Documentation/networking/ethtool-netlink.txt | 441 +++++++++++++
> include/linux/ethtool_netlink.h              |  17 +
> include/linux/netdevice.h                    |  14 +
> include/net/netlink.h                        |  15 +
> include/uapi/linux/ethtool.h                 |  10 +
> include/uapi/linux/ethtool_netlink.h         | 265 ++++++++
> include/uapi/linux/net_tstamp.h              |  13 +
> net/Kconfig                                  |   8 +
> net/Makefile                                 |   2 +-
> net/core/Makefile                            |   2 +-
> net/ethtool/Makefile                         |   7 +
> net/ethtool/bitset.c                         | 572 +++++++++++++++++
> net/ethtool/bitset.h                         |  40 ++
> net/ethtool/common.c                         | 227 +++++++
> net/ethtool/common.h                         |  29 +
> net/ethtool/info.c                           | 332 ++++++++++
> net/{core/ethtool.c => ethtool/ioctl.c}      | 244 ++-----
> net/ethtool/netlink.c                        | 634 +++++++++++++++++++
> net/ethtool/netlink.h                        | 252 ++++++++
> net/ethtool/settings.c                       | 559 ++++++++++++++++
> net/ethtool/strset.c                         | 461 ++++++++++++++
> 21 files changed, 3937 insertions(+), 207 deletions(-)
> create mode 100644 Documentation/networking/ethtool-netlink.txt
> create mode 100644 include/linux/ethtool_netlink.h
> create mode 100644 include/uapi/linux/ethtool_netlink.h
> create mode 100644 net/ethtool/Makefile
> create mode 100644 net/ethtool/bitset.c
> create mode 100644 net/ethtool/bitset.h
> create mode 100644 net/ethtool/common.c
> create mode 100644 net/ethtool/common.h
> create mode 100644 net/ethtool/info.c
> rename net/{core/ethtool.c => ethtool/ioctl.c} (91%)
> create mode 100644 net/ethtool/netlink.c
> create mode 100644 net/ethtool/netlink.h
> create mode 100644 net/ethtool/settings.c
> create mode 100644 net/ethtool/strset.c
>
>-- 
>2.20.1
>