Date:   Tue, 10 Aug 2021 15:05:44 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Ido Schimmel <idosch@...sch.org>
Cc:     Andrew Lunn <andrew@...n.ch>,
        "Keller, Jacob E" <jacob.e.keller@...el.com>,
        netdev@...r.kernel.org, davem@...emloft.net, mkubecek@...e.cz,
        pali@...nel.org, vadimp@...dia.com, mlxsw@...dia.com,
        Ido Schimmel <idosch@...dia.com>
Subject: Re: [RFC PATCH net-next 1/8] ethtool: Add ability to control
 transceiver modules' low power mode

On Tue, 10 Aug 2021 23:46:28 +0300 Ido Schimmel wrote:
> On Tue, Aug 10, 2021 at 06:59:54AM -0700, Jakub Kicinski wrote:
> > On Tue, 10 Aug 2021 15:52:20 +0200 Andrew Lunn wrote:  
> > > O.K. Thanks for the better explanation. Some of this should go into
> > > the commit message.
> > > 
> > > I suggest it gets a different name and semantics, to avoid
> > > confusion. I think we should consider this the default power mode for
> > > when the link is administratively down, rather than direct control
> > > over the modules power mode. The driver should transition the module
> > > to this setting on link down, be it high power or low power. That
> > > saves a lot of complexity, since i assume you currently need a udev
> > > script or something which sets it to low power mode on link down,
> > > where as you can avoid this be configuring the default and let the
> > > driver do it.  
> > 
> > Good point. And actually NICs have similar knobs, exposed via ethtool
> > priv flags today. Intel NICs for example. Maybe we should create a
> > "really power the port down policy" API?  
> 
> See below about Intel. I'm not sure it's the same thing...
> 
> I'm against adding a vague "really power the port down policy" API. The
> API proposed in the patch is well-defined, its implementation is
> documented in standards, its implications are clear and we offer APIs
> that give user space full observability into its operation.
> 
> A vague API means that it is going to be abused and user space will get
> different results over different implementations. After reading the
> *commit messages* about the private flags, I'm not sure what the flags
> really do, what is their true motivation, implications or how do I get
> observability into their operation. I'm not too hopeful about the user
> documentation.
> 
> Also, like I mentioned in the cover letter, given the complexity of
> these modules and as they become more common, it is likely that we will
> need to extend the API to control more parameters and expose more
> diagnostic information. I would really like to keep it clean and
> contained in 'ETHTOOL_MSG_MODULE_*' messages and not spread it over
> different APIs.

The patch is well defined but it doesn't provide user with the answer
to the question "why is the SFP still up if I asked it to be down?"
It's good to match specs closely but Linux may need to reconcile
multiple policies.

IIUC if Intel decides to keep the SFP up for "other" reasons the
situation will look like this:

 $ ethtool --show-module eth0
 Module parameters for eth0:
 low-power true

 # ethtool -m eth0
 Module State                              : 0x03 (ModuleReady)
 LowPwrAllowRequestHW                      : Off
 LowPwrRequestSW                           : Off


IOW the low-power mode is a way for user to express preference to
shut down/keep up the SFP, but it's not necessarily going to be
the only "policy" that matters. If other policies (e.g. NC-SI) express
preference to keep the interface up it will stay up, right?

The LowPwrRequestSW is not directly controlled by low-power && IF_UP.

What I had in mind was something along the lines of a bitmap of reasons
which are allowed to keep the interface up, and for your use case
the reason would be something like SFP_ALWAYS_ON, but other reasons
(well defined) may also keep the ifc up.

That's just to explain what I meant, if it's going to be clear to
everyone that low-power != LowPwrRequestSW I'm fine either way.

Powered by blists - more mailing lists