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

-----Original Message-----
From: Jakub Kicinski <kuba@kernel.org>
Sent: Tuesday, August 24, 2021 4:13 PM
To: Ido Schimmel <redacted>
Cc: netdev@vger.kernel.org; davem@davemloft.net; andrew@lunn.ch;
mkubecek@suse.cz; pali@kernel.org; Keller, Jacob E [off-list ref];
jiri@nvidia.com; vadimp@nvidia.com; mlxsw@nvidia.com; Ido Schimmel
[off-list ref]
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:

quoted

From: Ido Schimmel <idosch@nvidia.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.

quoted

Signed-off-by: Ido Schimmel <idosch@nvidia.com>

quoted

+The optional ``ETHTOOL_A_MODULE_POWER_MODE_POLICY`` attribute

encodes the

quoted

+transceiver module power mode policy enforced by the host. The default

policy

quoted

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

quoted

+/**
+ * struct ethtool_module_power_mode_params - module power mode

parameters

quoted

+ * @policy: The power mode policy enforced by the host for the plug-in

module.

quoted

+ * @mode: The operational power mode of the plug-in module. Should be

filled by

quoted

+ * device drivers on get operations.

Indent continuation lines by one tab.

quoted

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

coalesce settings have a valid mode don't they? Or at least an "accepted modes"?

Thanks,
Jake

quoted

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