Re: [PATCH v2 16/17] leds: leds-nuc: add support for changing the ethernet type indicator

[Marek Behún]

On Wed, 19 May 2021 16:24:13 +0200
Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx> wrote:

> On other words, if no extra care is taken, it could have bad side 
> effects at the machine's performance and affect system's latency,
> eventually resulting on things like audio clicks and pops, if some
> audio is playing while such calls keep happening.

In general we want for every LED that is registered into kernel as a LED
classdev to be possible to control the brightness by software. If the
hardware supports it, it should be available. There is a _blocking
.brightness_set_blocking callback for LEDs which may block when setting
brightness.
But even if we did not want to support software control, the transparent
trigger offloading is still relevant. See below.

> So, IMO, there's very little sense on trying to re-implement the
> already existing hardware-controlled events via software emulation.

We have a misunderstanding here, probably because of my bad
explanation, I will try to clarify.

> Sorry, but I guess I missed something here. Are you meaning to use
> the code under "ledtrig-netdev.c" or something else? 
> 
> The code at ledtrig-netdev.c allocates a trigger data, initializes a
> spin lock, initializes a delayed work, registers a notifier, sets a 
> trigger interval, etc. It is perfectly fine for software-controlled
> LEDs, but none of those will ever be used by the NUC driver, 
> if it only implements HW blinking for the Ethernet interfaces
> (and, as said before, there's little sense emulating it via software
> on such devices).

The idea of transparent offloading of LED triggers to HW (if HW
supports it) is to have a consistent and unified interface.

Currently we have a driver (leds-ns2 I think) which allows putting the
LED into HW controlled mode (to blink on SATA disk activity). This is
done by writing 1 into /sys/class/leds/<LED>/sata.

In your proposal you are creating several sysfs files:
  indicator
  hdd_default (notice difference from "sata" sysfs file in leds-ns2
               driver)
  ethernet_type

So the problem here is that this API is not unified. This is different
from how leds-ns2 driver does this, and both of these solutions are
wrong, because they are not extendable.

The correct way to do this is via LED triggers, i.e. if I want a LED to
blink on network activity, then I should use netdev trigger and nothing
else. The netdev trigger should determine whether the underlying LED
driver can set the LED to blink on network activity in HW. If HW
supports it, netdev trigger should use this, otherwise netdev trigger
should blink the LED in software.

Currently the netdev trigger does the blinking in software only
(code in "ledtrig-netdev.c" file). There is a WIP to add the necessary
support for the netdev trigger to have the ability to offload blinking
to HW. I will try to respin this WIP and send patches for review.

Once netdev trigger supports this feature, you can implement your
driver in this way. You can even make your driver depend on netdev
trigger and set the specific LED into netdev triggering by default, and
even forbidding anything else. But this is the corrent way to do this,
instead of creating new sysfs API that is non-extendable.

I am sorry that I did not explain this thoroughly in previous mails.
Hopefully this explanation is better.

Marek

PS: This is relevant for disk activity as well.