Re: [RFC PATCH 0/9] ethtool netlink interface (WiP)

On Mon, Dec 11, 2017 at 5:53 AM, Michal Kubecek [off-list ref] wrote:

This is still work in progress and only a very small part of the ioctl
interface is reimplemented but I would like to get some comments before
the patchset becomes too big and changing things becomes too tedious.

The interface used for communication between ethtool and kernel is based on
ioctl() and suffers from many problems. The most pressing seems the be the
lack of extensibility. While some of the newer commands use structures
designed to allow future extensions (e.g. GFEATURES or TEST), most either
allow no extension at all (GPAUSEPARAM, GCOALESCE) or only limited set of
reserved fields (GDRVINFO, GEEE). Even most of those which support future
extensions limit the data types that can be used.

This series aims to provide an alternative interface based on netlink which
is what other network configuration utilities use. In particular, it uses
generic netlink (family "ethtool"). The goal is to provide an interface
which would be extensible, flexible and practical both for ethtool and for
other network configuration tools (e.g. wicked or systemd-networkd).

The interface is documented in Documentation/networking/ethtool-netlink.txt

I'll post RFC patch series for ethtool in a few days, it still needs some
more polishing.

Basic concepts:

- the interface is based on generic netlink (family name "ethtool")

- provide everything ioctl can do but allow easier future extensions

- 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, netlink allows providing only the attributes to
  be changed; therefore we don't need a get-modify-set cycle, userspace can
  simply say what it wants to change and how

- be less dependent on ethtool and kernel being in sync; allow e.g. saying
  "ethtool -s eth0 advertise xyz off" without knowing what "xyz" means;
  it's kernel's job to know what mode "xyz" is and if it exists and is
  supported

Unresolved questions/tasks:

- allow dumps for "get" type requests, e.g. listing EEE settings for all
  interfaces in current netns

- find reasonable format for data transfers (e.g. eeprom dump or flash)

- 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

- it would be nice if driver could provide useful error/warning messages to
  be passed to userspace via extended ACK; example: while testing, I found
  a driver which only allows values 0, 1, 3 and 10000 for certain parameter
  but the only way poor user can find out is either by trying all values or
  by checking driver source

Michal Kubecek (9):
  netlink: introduce nla_put_bitfield32()
  ethtool: introduce ethtool netlink interface
  ethtool: helper functions for netlink interface
  ethtool: netlink bitset handling
  ethtool: implement GET_DRVINFO message
  ethtool: implement GET_SETTINGS message
  ethtool: implement SET_SETTINGS message
  ethtool: implement GET_PARAMS message
  ethtool: implement SET_PARAMS message

Thanks for working on this!. Agree with most comments already
discussed on this thread.
I would prefer if we fold ethtool netlink into devlink since there is
already an overlap.
many reasons:
- have just one driver api for device global and per port config
(devlink already provides that)
- some of the devlink commands like port split/unsplit can already be
applied per netdev (and since you bring up network interface managers,
we are looking at getting these in network managers for switch ports)
- if we keep them separate, we will soon see that drivers will need
handlers for both devlink and ethtool
       - and the overlap is going to be confusing for both drivers and
user-space