netdev - Re: [RFC PATCH net-next 0/2] Configuring PFC stall prevention via ethtool

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20171116123929.cuaz75qxghrr2ic5@unicorn.suse.cz>
Date:   Thu, 16 Nov 2017 13:39:29 +0100
From:   Michal Kubecek <mkubecek@...e.cz>
To:     Eran Ben Elisha <eranlinuxmellanox@...il.com>
Cc:     Eran Ben Elisha <eranbe@...lanox.com>,
        Linux Netdev List <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        "John W. Linville" <linville@...driver.com>,
        Saeed Mahameed <saeedm@...lanox.com>,
        Gal Pressman <galp@...lanox.com>,
        Ariel Almog <ariela@...lanox.com>,
        Inbar Karmy <inbark@...lanox.com>
Subject: Re: [RFC PATCH net-next 0/2] Configuring PFC stall prevention via
 ethtool

On Thu, Nov 16, 2017 at 02:03:21PM +0200, Eran Ben Elisha wrote:
> On Thu, Nov 16, 2017 at 10:44 AM, Michal Kubecek <mkubecek@...e.cz> wrote:
> >
> > I don't like adding another ethtool_ops callback tightly tied to the
> > structures passed via ioctl() but when I started to think what to
> > suggest as an alternative, I started to wonder if it is really necessary
> > to add a new ethtool command at all. Couldn't this be handled as
> > a tunable?
> 
> tunable seems as a good infrastructure to PHY tuning, however this
> feature is not a PHY feature.

There are two kinds: tunables (ETHTOOL_{G,S}TUNABLE) and phy tunables
(ETHTOOL_PHY_{G,S}TUNABLE). My understanding is that former is meant as
a generic interface for parameters related to net_device, latter for
parameters related to phydev.

It's only my guess but IMHO the idea behind (both kinds of) tunables was
to add at least some extensibility to the ioctl() interface so that we
don't have to add more and more new one-purpose commands (until we can
switch to netlink).

> To me, it's looks fit to ethtool -a where pause operations are being
> controlled.  Unfortunately set/get_pauseparam is not extensible and
> need a new operation.

Yes, logically it belongs there, no question about that. But the
relation between ethtool subcommands (on command line) and ioctl
commands (cmd member of the structures) is not 1:1 in general (this is
one of the reasons. After all, your patchset uses another command
anyway; my suggestion is to use an existing one (ETHTOOL_{G,S}TUNABLE)
rather than adding a new one (and new callback to ethtool_ops).

Michal Kubecek