Re: [RFC PATCH v2 1/5] leds: trigger: add API for HW offloading of triggers

[Marek Behún]

On Mon, 8 Nov 2021 18:58:38 +0100
Ansuel Smith <ansuelsmth@xxxxxxxxx> wrote:

> Are you aware of any device that can have some trigger offloaded and
> still have the led triggered manually?

I don't understand why we would need such a thing.

Look, just to make it clear via an example: I have a device with a
Marvell PHY chip inside. There is a LED connected to one of the PHY LED
pins.

Marvell PHY has LED[0] control register, which supports the following
modes:
  LED is OFF
  LED is ON
  LED is ON when Link is up
  LED blinks on RX activity
  LED blinks on TX activity
  LED blinks on RX/TX activity
  LED is ON and blinks on RX/TX activity
  ...

I have code that exports this LED as a LED classdev

When I activate netdev trigger on this LED, the netdev trigger currently
just blinks the LED in software, by calling the .brightness_set()
method, which configures LED[0] control register to one of the first
two modes above (LED is OFF, LED is ON).

But I have also another patch that adds support to offloading netdev
trigger upon offloadable settings. The netdev trigger code calls the
.trigger_offload() method, which is implemented in PHY driver. This
method checks whether it is a netdev trigger that is to be offloaded,
and whether device_name is the name of the device attached to the PHY,
and then chooses one of the modes above, according to netdev trigger
settings.

So when I request netdev trigger for eth0, to indicate link and blink
on activity, the netdev trigger doesn't do anything in software. It
just calls the offload method ONCE (at the moment I am changing netdev
trigger settings). The blinking is then done by the PHY chip. Netdev
trigger doesn't do anything, at least not until I change the settings
again.

> Talking about mixed mode, so HW and SW.

What exactly do you mean by mixed mode? There is no mixed mode.

> Asking to understand as currently the only way to impement all
> of this in netdev trigger is that:
> IF any hw offload trigger is supported (and enabled) then the entire
> netdev trigger can't work as it won't be able to simulate missing
> trigger in SW. And that would leave some flexibility.

What do you mean by missing trigger here? I think we need to clarify
what we mean by the word "trigger". Are you talking about the various
blinking modes that the PHY supports? If so, please let's call them HW
control modes, and not triggers. By "triggers" I understand triggers
that can be enabled on a LED via /sys/class/leds/<LED>/trigger.

> We need to understand how to operate in this condition. Should netdev
> detect that and ""hide"" the sysfs triggers? Should we report error?

So if I understand you correctly, you are asking about what should we
do if user asked for netdev trigger settings (currently only link, rx,
tx, interval) that can't be offloaded to the PHY chip.

Well, if the PHY allows to manipulate the LEDs ON/OFF state (in other
words "full control by SW", or ability to implement brightness_set()
method), then netdev trigger should blink the LED in SW via this
mechanism (which is something it would do now). A new sysfs file,
"offloaded", can indicate whether the trigger is offloaded to HW or not.

If, on the other hand, the LED cannot be controlled by SW, and it only
support some HW control modes, then there are multiple ways how to
implement what should be done, and we need to discuss this.

For example suppose that the PHY LED pin supports indicating LINK,
blinking on activity, or both, but it doesn't support blinking on rx
only, or tx only.

Since the LED is always indicating something about one network device,
the netdev trigger should be always activated for this LED and it
should be impossible to deactivate it. Also, it should be impossible to
change device_name.

  $ cd /sys/class/leds/<LED>
  $ cat device_name
  eth0
  $ echo eth1 >device_name
  Operation not supported.
  $ echo none >trigger
  Operation not supported.

Now suppose that the driver by default enabled link indication, so we
have:
  $ cat link
  1
  $ cat rx
  0
  $ cat tx
  0

We want to enable blink on activity, but the LED supports only blinking
on both rx/tx activity, rx only or tx only is not supported.

Currently the only way to enable this is to do
  $ echo 1 >rx
  $ echo 1 >tx
but the first call asks for (link=1, rx=1, tx=0), which is impossible.

There are multiple things which can be done:
- "echo 1 >rx" indicates error, but remembers the setting
- "echo 1 >rx" quietly fails, without error indication. Something can
  be written to dmesg about nonsupported mode
- "echo 1 >rx" succeeds, but also sets tx=1
- rx and tx are non-writable, writing always fails. Another sysfs file
  is created, which lists modes that are actually supported, and allows
  to select between them. When a mode is selected, link,rx,tx are
  filled automatically, so that user may read them to know what the LED
  is actually doing
- something different?

Marek