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

- From: Keller, Jacob E \<jacob.e.keller@intel.com\>
- Date: 2021-08-24 23:19:01 
- List: netdev
- Page: https://ratatoskr.run/netdev/2021/08/11210073
- Thread: https://ratatoskr.run/netdev/2021/08/11210023/t.md

```text


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


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

Thanks,
Jake

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