netdev - Re: [RFC PATCH net-next v2 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: <YR2P7+1ZGiEBDtAq@lunn.ch>
Date:   Thu, 19 Aug 2021 00:55:43 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Ido Schimmel <idosch@...sch.org>, netdev@...r.kernel.org,
        davem@...emloft.net, 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 v2 1/6] ethtool: Add ability to control
 transceiver modules' power mode

On Wed, Aug 18, 2021 at 03:32:41PM -0700, Jakub Kicinski wrote:
> On Wed, 18 Aug 2021 18:51:57 +0300 Ido Schimmel wrote:
> > +MODULE_SET
> > +==========
> > +
> > +Sets transceiver module parameters.
> > +
> > +Request contents:
> > +
> > +  ======================================  ======  ==========================
> > +  ``ETHTOOL_A_MODULE_HEADER``             nested  request header
> > +  ``ETHTOOL_A_MODULE_POWER_MODE_POLICY``  u8      power mode policy
> > +  ======================================  ======  ==========================
> > +
> > +When set, the optional ``ETHTOOL_A_MODULE_POWER_MODE_POLICY`` attribute is used
> > +to set the transceiver module power policy enforced by the host. Possible
> > +values are:
> > +
> > +.. kernel-doc:: include/uapi/linux/ethtool.h
> > +    :identifiers: ethtool_module_power_mode_policy
> > +
> > +For SFF-8636 modules, low power mode is forced by the host according to table
> > +6-10 in revision 2.10a of the specification.
> > +
> > +For CMIS modules, low power mode is forced by the host according to table 6-12
> > +in revision 5.0 of the specification.
> > +
> > +To avoid changes to the operational state of the device, power mode policy can
> > +only be set when the device is administratively down.
> 
> Would you mind explaining why?

Part of the issue is we have two different sorts of policy mixed
together.

ETHTOOL_MODULE_POWER_MODE_POLICY_LOW and
ETHTOOL_MODULE_POWER_MODE_POLICY_HIGH change the state now. This could
be a surprise to a user when there link disappears for the
ETHTOOL_MODULE_POWER_MODE_POLICY_LOW case, when the interface is admin up.

ETHTOOL_MODULE_POWER_MODE_POLICY_HIGH_ON_UP however follows the state
of the interface. So there should not be any surprises.

I actually think ETHTOOL_MODULE_POWER_MODE_POLICY_HIGH_ON_UP should be
allowed at any time, just to make it easier to use.

> > +/**
> > + * enum ethtool_module_power_mode_policy - plug-in module power mode policy
> > + * @ETHTOOL_MODULE_POWER_MODE_POLICY_LOW: Module is always in low power mode.
> 
> Did you have a use case for this one or is it for completeness? Seems
> like user can just bring the port down if they want no carrier? My
> understanding was you primarily wanted the latter two, and those can
> be freely changed when netdev is running, right?
> 
> > + * @ETHTOOL_MODULE_POWER_MODE_POLICY_HIGH: Module is always in high power mode.
> > + * @ETHTOOL_MODULE_POWER_MODE_POLICY_HIGH_ON_UP: Module is transitioned by the
> > + *	host to high power mode when the first port using it is put
> > + *	administratively up and to low power mode when the last port using it
> > + *	is put administratively down.
> 
> s/HIGH_ON_UP/AUTO/ ?
> high on up == low on down, right, seems arbitrary to pick one over the
> other

Should we also document what the default is? Seems like
ETHTOOL_MODULE_POWER_MODE_POLICY_HIGH_ON_UP is the generic network
interface default, so maybe it should also be the default for SFPs?

	  Andrew