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

From: Ido Schimmel <idosch@idosch.org>
To: Andrew Lunn <andrew@lunn.ch>
Cc: Jakub Kicinski <kuba@kernel.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 10:50:39 +0300	[thread overview]
Message-ID: <YR4NTylFy2ejODV6@shredder> (raw)
In-Reply-To: <YR2P7+1ZGiEBDtAq@lunn.ch>

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