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: <20210819073822.0f205af3@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Thu, 19 Aug 2021 07:38:22 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Ido Schimmel <idosch@...sch.org>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, andrew@...n.ch,
        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, 19 Aug 2021 10:47:03 +0300 Ido Schimmel wrote:
> > > + * 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?  
> 
> In all the implementations I could find (3), the user interface is
> high/low (on/off). The proposed interface is more flexible as it
> contains both the 'high' / 'low' policies in addition to the more user
> friendly 'high-on-up' ('auto') policy.

on/off is probably due to blindly setting the SFP bit. Our intention
here is to set a policy with respect to the netdev state (or integrate
the setting with the rest of the stack, if you will) not just control 
the bit.

IMO we should leave the value out of the enum until the use case for 
it becomes clear. Adding it later is simple enough.

> Given that keeping the 'low' policy does not complicate the
> implementation / maintenance

I'd argue it does. The netif_running() check is exactly to prevent
carrier from going away, so it only makes sense if the low setting 
exists. We can switch between 'auto' and 'high' any time.

> and that it provides users with a similar interface to what they are
> used to from other implementations, I would like to keep it in
> addition to the other two policies.

IIUC the existing interfaces are build around the architecture where
driver/fw do not control the SFP automatically IOW 'auto' does not
exist. 'low' is there for disabling the SFP when interface is down.

The interface where 'low' can't be set while the netdev is up is
*already* not 1:1 with the out of tree APIs, right?

I'd bet that if you convert users of existing APIs to map 'on' to
(always-)high and 'off' to auto everyone will be happy.