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: <20210824161231.5e281f1e@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Tue, 24 Aug 2021 16:12:31 -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 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.

> 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? Like:

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

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

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

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

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