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] [day] [month] [year] [list]

Message-ID: <20210825072644.1b3aaf46@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Wed, 25 Aug 2021 07:26:44 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Ido Schimmel <idosch@...sch.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 Wed, 25 Aug 2021 14:14:28 +0300 Ido Schimmel wrote:
> 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."

SGTM

> > > + * @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.

Yup, I mean we mostly care about the policy but can adjust the state
enum as well ;) For stats 0 was a no-go, obviously but in general it
should work perfectly.

> 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.
> +

Modulo the enum in question, but 👍