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

[Ido Schimmel <idosch@idosch.org>]

On Wed, Aug 11, 2021 at 04:36:13PM +0200, Andrew Lunn wrote:
> On Wed, Aug 11, 2021 at 06:03:43AM -0700, Jakub Kicinski wrote:
> > On Wed, 11 Aug 2021 14:33:06 +0300 Ido Schimmel wrote:
> > > # ethtool --set-module swp13 low-power on
> > > 
> > > $ ethtool --show-module swp13
> > > Module parameters for swp13:
> > > low-power true
> > > 
> > > # ip link set dev swp13 up
> > > 
> > > $ ip link show dev swp13
> > > 127: swp13: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 1500 qdisc fq_codel state DOWN mode DEFAULT group default qlen 1000
> > >     link/ether 1c:34:da:18:55:49 brd ff:ff:ff:ff:ff:ff
> > > 
> > > $ ip link show dev swp14
> > > 128: swp14: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 1500 qdisc fq_codel state DOWN mode DEFAULT group default qlen 1000
> > >     link/ether 1c:34:da:18:55:4d brd ff:ff:ff:ff:ff:ff
> > 
> > Oh, so if we set low-power true the carrier will never show up?
> > I thought Andrew suggested the setting is only taken into account 
> > when netdev is down.
> 
> Yes, that was my intention. If this low power mode also applies when
> the interface is admin up, it sounds like a foot gun. ip link show
> gives you no idea why the carrier is down, and people will assume the
> cable or peer is broken. We at least need a new flag, LOWER_DISABLED
> or similar to give the poor user some chance to figure out what is
> going on.

The canonical way to report such errors is via LINKSTATE_GET and I will
add an extended sub-state describing the problem.

> 
> To me, this setting should only apply when the link is admin down.

I don't want to bake such an assumption into the kernel, but I have a
suggestion that resolves the issue.

We currently have a single attribute that encodes the desired state on
SET messages and the operational state on GET_REPLY messages
(ETHTOOL_A_MODULE_LOW_POWER_ENABLED):

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

It is not defined very well when a module is not connected despite being
a very interesting use case. We really need to have two attributes. The
first one describing the power mode policy and the second one describing
the operational power mode which is only reported when a module is
plugged in.

For the policy we can have these values:

1. low: Always transition the module to low power mode
2. high: Always transition the module to high power mode
3. high-on-up: Transition the module to high power mode when a port
using it is administratively up. Otherwise, low

A different policy for up/down seems like an overkill for me.

See example usage below.

No module connected:

$ ethtool --show-module swp11
Module parameters for swp11:
power-mode-policy high

Like I mentioned before, this is the default on Mellanox systems so this
new attribute allows user space to query the default policy.

Change to a different policy:

# ethtool --set-module swp11 power-mode-policy high-on-up

$ ethtool --show-module swp11
Module parameters for swp11:
power-mode-policy high-on-up

After a module was connected:

$ ethtool --show-module swp11
Module parameters for swp11:
power-mode-policy high-on-up
power-mode low

# ip link set dev swp11 up

$ ethtool --show-module swp11
Module parameters for swp11:
power-mode-policy high-on-up
low-power high

# ip link set dev swp11 down

# ethtool --set-module swp11 power-mode-policy low

# ip link set dev swp11 up

$ ethtool swp11
...
Link detected: no (Cable issue, Module is in low power mode)

I'm quite happy with the above. Might change a few things as I implement
it, but you get the gist. WDYT?