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

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:

quoted

On Wed, 11 Aug 2021 14:33:06 +0300 Ido Schimmel wrote:

quoted

# 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?