Re: lets settle the LED `function` property regarding the netdev trigger

On Wed, 6 Oct 2021 14:57:13 +0200
Andrew Lunn [off-list ref] wrote:

quoted

quoted

I agree with having a list, and we use the combination. If the
combination is not possible by the hardware, then -EINVAL, or
-EOPNOTSUPP.
  

quoted

- having separate functions for different link modes
    function = "link1000", "link100";    

I would suggest this, so you can use 

function = "link1000", "link100", "activity"  

The problem here is that LED core uses function to compose LED name:
  devicename:color:function
Should we use the first function? Then this LED will be named:
  ethphy42:green:link1000
but it also indicates link100...  

This makes no sense. Using function makes no sense, when the whole
point of using the LED framework is we have a uniform way of
setting/changing the function at run time.

An LED called ethphy42:green:link1000 which is actually showing
activity makes no sense.

ethphy42:green:state would be a better name.  The function of the LED
is to give you some idea of what the state of the PHY is. What state
it actually indicates is up to the user.

The LED device name is supposed to reflect what the LED was dedicated
for by vendor. So if on the device chassis next to the LED there is an
icon or text indicating that the LED is supposed to show link, then the
function part of the LED name should be "link", no matter what the user
does with the LED during runtime.

In many cases the icon/text by the LED on the chassis says only something
like "wan" or "lanN". On Turris Omnia for example this is so, and the LED
is supposed to show link and blink on activity.

For LEDs on ethernet ports usually the port is dedicated (i.e. it is a
wan port or a lanN port), and the LEDs are also dedicated in some way
(for example green for link1000, blink on activity, yellow for link100).

Currently device tree binding for LEDs allows setting function and
function-enumerator, and there are only macros
  LED_FUNCTION_WAN	(expands to "wan")
  LED_FUNCTION_LAN	(expands to "lan")
So the device tree can specify:
  color = <LED_COLOR_ID_GREEN>;
  function = LED_FUNCTION_WAN;
and the LED will be called
  <devicename>:green:wan (with no devicename just green:wan)
or
  color = <LED_COLOR_ID_GREEN>;
  function = LED_FUNCTION_LAN;
  function-enumerator = <2>;
and the LED will be called
  <devicename>:green:lan-2

We need to somehow keep the function part of the LED name consistent
with what the LED was dedicated for by vendor, but also to be able to
set more specific trigger settings (link1000, link100, link10,
activity).

Maybe it would make sense to keep the first string in the function
property "lan" / "wan", and the other strings to specify more specific
functions?

quoted

No, my current ideas about the netdev trigger extension are as follows
(not yet complete):

$ cd /sys/.../<LED>
$ echo netdev >trigger	# To enable netdev trigger
$ echo eth0 >device_name
$ echo 1 >ext		# To enable extended netdev trigger.
			# This will create directory modes if there is
			# a PHY attached to the interface  
$ ls modes/		
1000baseT_Full 100BaseT_Full 100BaseT_Half 10BaseT_Full 10BaseT_Half

$ cd modes/1000baseT_Full
$ ls
brightness link rx tx interval

So basically if you enable the extended netdev trigger, you will get
all the standard netdev settings for each PHY mode. (With a little
change to support blinking on link.)

With this you can set the LED:
  ON when linked and speed=1000m or 100m, blink on activity
or
  blink with 50ms interval when speed=1000m
  blink with 100ms interval when speed=100m
  blink with 200ms interval when speed=10m

(Note that these don't need to be supported by PHY. We are talking
 about SW control. If the PHY supports some of these in HW, then the
 trigger can be offloaded.)  

I see a number of problems with this

1) Not all PHYs support software control of the LEDs. i.e. software
on/off. But they do support different blink modes. Just looking at the
data sheets i have lying around like this:

LAN8740 
KSZ8041

and i'm sure there are more. So these PHYs LEDs will always be in
offloaded mode, you cannot do software blinking. But they do support
multiple blinking modes, so we want to be able to control that.

I am aware of such LEDs. Currently the LED subsystem does not support
such LEDs. I wanted first to extend the LED API to support offloading,
and wanted to look at LEDs that only can be HW controlled later.

2) Marvell PHY i have the datasheet open for at the moment has no way
to indicate duplex. So you cannot actually offload 100baseT_Full. You
need to user to configure the same blink mode for both 100baseT_Full
and then 100baseT_Half, so duplex is irrelevant, then you can offload
it, which is not very obvious.

This was just a proposal, I have not written code for this yet. We can
do this by speed only, if it makes more sense.

3) phylib does not actually tell you what link mode the PHY is
operating in. All you get is the resolved speed and duplex. And
mapping speed an duplex back to a link mode is not obvious. Take for
example 100baseT_Full & 100baseT1_Full. Both are 100Mbps, both full
duplex. Currently there is no PHY which actually implements both, so
currently you can work it out, but in general, you cannot. But this is
very true for higher speeds, where the MAC is providing the PHY LED
control, but ideally we want the same /sysfs interface:

        ETHTOOL_LINK_MODE_40000baseKR4_Full_BIT = 23,
        ETHTOOL_LINK_MODE_40000baseCR4_Full_BIT = 24,
        ETHTOOL_LINK_MODE_40000baseSR4_Full_BIT = 25,
        ETHTOOL_LINK_MODE_40000baseLR4_Full_BIT = 26,

Are you suggesting 4 different directories for the same speed?

I think you need duplex, KR4/CR4/SR4/LR4, T1/T2 as separate
attributes, which the LED might support, but are not required.

OK, it would make more sense to do this by speed only at first. And
later if someone needs to do it more specific, it can be extended.

$ cd /sys/class/leds/<LED>
$ echo netdev >trigger
$ echo speed >ext
$ ls modes
1000 100 10

Or maybe the subdirectories can be added by request:

$ cd /sys/class/leds/<LED>
$ echo netdev >trigger
$ cat ext
[none] speed link_mode
$ echo link_mode >ext
$ ls modes
add  delete
$ echo 40000baseKR4_Full >modes/add
$ ls modes
40000baseKR4_Full  add  delete

4) Software blinking can add quite a lot of overhead. Getting the
counters from the device can be expensive, particularly for Ethernet
switches on slow busses. If anybody sets the interval to 5ms, they
could saturate the MDIO bus. And blinking the LEDs is not for free.

This is how the netdev trigger works currently. I just wanted first to
extend it to support in SW things that are offloadable to some HW.

You are right that MDIO bus can be slow and saturated by SW blinking,
that is why we need offloading.

Also currently netdev trigger blink on activity does not work for DSA
switch ports (at least not for mv88e6xxx), unless the packet goes to
CPU also, since the trigger looks at CPU interface stats...

So either we need to indicate if software or hardware is used, or we
should limit the choices to those which the hardware can actually do,
so we guarantee offload.

I think we should indicate whether the trigger is offloaded and let the
user control the LED also in SW. An utility (ledtool) can be written
which could then list HW offloadable modes and let the user choose only
between those.

Marek