MIME-Version: 1.0
In-Reply-To: <cover.1513000306.git.mkubecek@suse.cz>
References: <cover.1513000306.git.mkubecek@suse.cz>
From: Roopa Prabhu <roopa@cumulusnetworks.com>
Date: Tue, 12 Dec 2017 07:32:44 -0800
Message-ID: <CAJieiUgP6muLZbVczd4wVWfkCz4c4K6AgZtV9TPVsxUmEnYjBw@mail.gmail.com>
Subject: Re: [RFC PATCH 0/9] ethtool netlink interface (WiP)
To: Michal Kubecek <mkubecek@suse.cz>
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4133
Lines: 87

On Mon, Dec 11, 2017 at 5:53 AM, Michal Kubecek <mkubecek@suse.cz> 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