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

From: Andrew Lunn <andrew@lunn.ch>
To: Jakub Kicinski <kuba@kernel.org>
Cc: Ido Schimmel <idosch@idosch.org>,
	netdev@vger.kernel.org, davem@davemloft.net, mkubecek@suse.cz,
	pali@kernel.org, jacob.e.keller@intel.com, jiri@nvidia.com,
	vadimp@nvidia.com, mlxsw@nvidia.com,
	Ido Schimmel <idosch@nvidia.com>
Subject: Re: [RFC PATCH net-next v2 1/6] ethtool: Add ability to control transceiver modules' power mode
Date: Thu, 19 Aug 2021 00:55:43 +0200	[thread overview]
Message-ID: <YR2P7+1ZGiEBDtAq@lunn.ch> (raw)
In-Reply-To: <20210818153241.7438e611@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>

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