Date:   Thu, 29 Jul 2021 10:59:01 +0200
From:   Marek Behún <kabel@...nel.org>
To:     Heiner Kallweit <hkallweit1@...il.com>
Cc:     Michael Walle <michael@...le.cc>, andrew@...n.ch,
        anthony.l.nguyen@...el.com, bigeasy@...utronix.de,
        davem@...emloft.net, dvorax.fuxbrumer@...ux.intel.com,
        f.fainelli@...il.com, jacek.anaszewski@...il.com, kuba@...nel.org,
        kurt@...utronix.de, linux-leds@...r.kernel.org,
        netdev@...r.kernel.org, pavel@....cz, sasha.neftin@...el.com,
        vinicius.gomes@...el.com, vitaly.lifshits@...el.com
Subject: Re: [PATCH net-next 5/5] igc: Export LEDs

Hello Heiner,

On Wed, 28 Jul 2021 22:43:30 +0200
Heiner Kallweit <hkallweit1@...il.com> wrote:

> Did we come to any conclusion?
> 
> My preliminary r8169 implementation now creates the following LED names:
> 
> lrwxrwxrwx 1 root root 0 Jul 26 22:50 r8169-led0-0300 -> ../../devices/pci0000:00/0000:00:1d.0/0000:03:00.0/net/enp3s0/r8169-led0-0300
> lrwxrwxrwx 1 root root 0 Jul 26 22:50 r8169-led1-0300 -> ../../devices/pci0000:00/0000:00:1d.0/0000:03:00.0/net/enp3s0/r8169-led1-0300
> lrwxrwxrwx 1 root root 0 Jul 26 22:50 r8169-led2-0300 -> ../../devices/pci0000:00/0000:00:1d.0/0000:03:00.0/net/enp3s0/r8169-led2-0300
> 
> I understood that LEDs should at least be renamed to r8169-0300::link-0
> to link-2 Is this correct? Or do we have to wait with any network LED support
> for a name discussion outcome?

I would expect some of the LEDs to, by default, indicate activity.
So maybe look at the settings BIOS left, and if the setting is to
indicate link, use the "link" function, and if activity, use the
"activity" function? 

> For the different LED modes I defined private hw triggers (using trigger_type
> to make the triggers usable with r8169 LEDs only). The trigger attribute now
> looks like this:
> 
> [none] link_10_100 link_1000 link_10_100_1000 link_ACT link_10_100_ACT link_1000_ACT link_10_100_1000_ACT
>
> Nice, or? Issue is just that these trigger names really should be made a
> standard for all network LEDs. I don't care about the exact naming, important
> is just that trigger names are the same, no matter whether it's about a r8169-
> or igc- or whatever network chip controlled LEDs.

This is how I at first proposed doing this, last year. But this is
WRONG!

First, we do not want a different trigger for each possible
configuration. We want one trigger, and then choose configuration via
other sysfs file. I.e. a "hw" trigger, which, when activated, would
create sysfs files "link" and "act", via which you can configure those
options.

Second, we already have a standard LED trigger for network devices,
netdev! So what we should do is use the netdev trigger, and offload
blinking to the LED controller if it supports it. The problems with
this are:
1. not yet implemented in upstream, see my latest version
   https://lore.kernel.org/linux-leds/20210601005155.27997-1-kabel@kernel.org/
2. netdev trigger currently does not support all these different link
   functions. We have these settings:
     device_name: network interface name, i.e. eth0
     link: 0 - do not indicate link
           1 - indicate link (ON when linked)
     tx: 0 - do not blink on transmit
         1 - blink on transmit
     rx: 0 - do not blink on receive
         1 - blink on receive
     interval: duration of LED blink in ms

I would like to extend netdev trigger to support different
configurations. Currently my ideas are as follows:
- a new sysfs file, "advanced", will show up when netdev trigger is
  enabled (and if support is compiled in)
- when advanced is set to 1, for each possible link mode (10base-t,
  100base-t, 1000base-t, ...) a new sysfs directory will show up, and
  in each of these directories the following files:
    rx, tx, link, interval, brightness
    multi_intensity (if the LED is a multi-color LED)
  and possibly even
    pattern
With this, the user can configure more complicated configurations:
- different LED color for different link speeds
- different blink speed for different link speeds
And if some of the configurations are offloadable to the HW, the drivers
can be written to support such offloading. (Maybe even add a read-only
file "offloaded" to indicate if the trigger was offloaded.)

I will work on these ideas in the following weeks and will sent
proposals to linux-leds.

> And I don't have a good solution for initialization yet. LED mode is whatever
> BIOS sets, but initial trigger value is "none". I would have to read the
> initial LED control register values, iterate over the triggers to find the
> matching one, and call led_trigger_set() to properly set this trigger as
> current trigger.

You can set led_cdev->default_trigger prior registering the LED. But
this is moot: we do not want a different trigger for each PHY interface
mode.

Marek

Powered by blists - more mailing lists