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: <YR4NTylFy2ejODV6@shredder>
Date:   Thu, 19 Aug 2021 10:50:39 +0300
From:   Ido Schimmel <idosch@...sch.org>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Jakub Kicinski <kuba@...nel.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 Thu, Aug 19, 2021 at 12:55:43AM +0200, Andrew Lunn wrote:
> 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.

Yes

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

I will add a note in Documentation/networking/ethtool-netlink.rst that
the default power mode policy is driver-dependent (can be queried) and
that it can either be 'high' or 'auto'.

> 
> 	  Andrew