netdev - Re: [RFC PATCH net-next v3 1/6] ethtool: Add ability to control transceiver modules' power mode

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: <YSYmFEDWJIu6eDvR@shredder>
Date:   Wed, 25 Aug 2021 14:14:28 +0300
From:   Ido Schimmel <idosch@...sch.org>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, andrew@...n.ch,
        mkubecek@...e.cz, pali@...nel.org, jacob.e.keller@...el.com,
        jiri@...dia.com, vadimp@...dia.com, mlxsw@...dia.com,
        Ido Schimmel <idosch@...dia.com>
Subject: Re: [RFC PATCH net-next v3 1/6] ethtool: Add ability to control
 transceiver modules' power mode

On Tue, Aug 24, 2021 at 04:12:31PM -0700, Jakub Kicinski wrote:
> On Tue, 24 Aug 2021 16:03:39 +0300 Ido Schimmel wrote:
> > From: Ido Schimmel <idosch@...dia.com>
> > 
> > Add a pair of new ethtool messages, 'ETHTOOL_MSG_MODULE_SET' and
> > 'ETHTOOL_MSG_MODULE_GET', that can be used to control transceiver
> > modules parameters and retrieve their status.
> 
> Lgtm! A few "take it or leave it" nit picks below.

Thanks and thanks a lot for the reviews!

> 
> > Signed-off-by: Ido Schimmel <idosch@...dia.com>
> 
> > +The optional ``ETHTOOL_A_MODULE_POWER_MODE_POLICY`` attribute encodes the
> > +transceiver module power mode policy enforced by the host. The default policy
> > +is driver-dependent and can be queried using this attribute.
> 
> Should we make a recommendation for those who don't have to worry about
> legacy behavior? 

Yes

> Like:
> 
>   The default policy is driver-dependent (but "auto" is the recommended
>   and generally assumed to be used for drivers no implementing this API).

I think "generally assumed to be used for drivers no implementing this
API" is problematic given that it is most likely the exact opposite of
what actually happens. I imagine most vendors supporting these modules
just went with "high" policy instead of implementing "auto" policy in
firmware.

So I suggest:

"The default policy is driver-dependent, but "auto" is the recommended
default and it should be implemented by new drivers and drivers where
conformance to a legacy behavior is not critical."

> 
> IMHO the "and can be queried using this attribute" part can be skipped.

OK

> 
> > +/**
> > + * struct ethtool_module_power_mode_params - module power mode parameters
> > + * @policy: The power mode policy enforced by the host for the plug-in module.
> > + * @mode: The operational power mode of the plug-in module. Should be filled by
> > + * device drivers on get operations.
> 
> Indent continuation lines by one tab.

Oops, I see that I did do that for other kdoc comments. Will fix.

> 
> > + * @mode_valid: Indicates the validity of the @mode field. Should be set by
> > + * device drivers on get operations when a module is plugged-in.
> 
> Should we make a firm decision on whether we want to use these kind of
> valid bits or choose invalid defaults? As you may guess my preference
> is the latter since that's what I usually do, that way drivers don't
> have to write two fields.
> 
> Actually I think this may be the first "valid" in ethtool, I thought we
> already had one but I don't see it now..

I was thinking about this as well, but I wasn't sure if it's valid to
adjust uAPI values in order to make in-kernel APIs simpler. I did see it
in some other places, but wasn't sure if it's a pattern that should be
copied.

Do you mean something like this?

diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 7d453f0e993b..d61049091538 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -732,7 +732,7 @@ enum ethtool_module_power_mode_policy {
  * @ETHTOOL_MODULE_POWER_MODE_HIGH: Module is in high power mode.
  */
 enum ethtool_module_power_mode {
-       ETHTOOL_MODULE_POWER_MODE_LOW,
+       ETHTOOL_MODULE_POWER_MODE_LOW = 1,
        ETHTOOL_MODULE_POWER_MODE_HIGH,
 };

I prefer this over memsetting a struct to 0xff.

If the above is fine, I can make the following patch:

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index c258b3f30a2e..d304df39ee5c 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -41,6 +41,11 @@ In the message structure descriptions below, if an attribute name is suffixed
 with "+", parent nest can contain multiple attributes of the same type. This
 implements an array of entries.
 
+Attributes that need to be filled-in by device drivers and that are dumped to
+user space based on whether they are valid or not should not use zero as a
+valid value. For example, ``ETHTOOL_A_MODULE_POWER_MODE``. This avoids the need
+to explicitly signal the validity of the attribute in the device driver API.
+
 
 Request header
 ==============

> 
> > +struct ethtool_module_power_mode_params {
> > +	enum ethtool_module_power_mode_policy policy;
> > +	enum ethtool_module_power_mode mode;
> > +	u8 mode_valid:1;
> > +};