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

[Mauro Carvalho Chehab <mchehab+huawei@kernel.org>]

Em Wed, 19 May 2021 14:11:02 +0200
Marek Behún <kabel@kernel.org> escreveu:

> On Wed, 19 May 2021 12:18:12 +0200
> Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:
> 
> > Em Wed, 19 May 2021 10:02:53 +0200
> > Marek Behún <kabel@kernel.org> escreveu:
> >   
> > > What possible configurations does this support?
> > > 
> > > Does this blink on rx/tx activity for a specific ethernet port?
> > >     
> > 
> > When the indicator is set to monitor Ethernet, it can work on either
> > LAN1, LAN2 or both LAN interfaces.
> >   
> > > There is a work in progress to add support for transparent offloading of
> > > LED triggers, with the netdev trigger being the first target.
> > > 
> > > This means that after that is done, you could implement this driver so
> > > that when netdev trigger is enabled on a supported interface, your
> > > driver will offload the blinking to the HW.    
> > 
> > On NUC leds, this is already offloaded to HW/firmware. 
> > 
> > All it takes is to tell the BIOS via ACPI/WMI what LED will be used
> > for monitoring the Ethernet activity, and on what port(s).  
> 
> Can the LED be put into software controlled mode and also into HW
> controlled mode so that HW blinks the LED on ethernet activity?

On a given time, a LED will either be in hardware-controlled mode or in
Software-controlled one. It should be noticed that software-controlled
mode should first be enabled by a BIOS option.

The default is to be hardware-controlled.

I don't intend to implement myself support for software-controlled
mode in Kernel, as this will probably be a lot worse than letting
the hardware control the led directly. 

See, changing the LED in software on NUC happens via ACPI calls, meaning
that the Kernel should be interrupted in order to run some code
inside the BIOS that will actually program the hardware. Switching to
BIOS produces context switching and may also interrupt the other
CPUs, as the BIOS may need to do CPU locking, in order to prevent 
L1/L2/L3 cache issues.

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.

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

> If so, then this is what I am talking about: transparent HW offloading
> of LED triggers:
> - I have a LED in /sys/class/leds
> - I set "netdev" trigger on this LED
> - I set ethernet interface for which the LED should blink
> - if the HW can blink the LED for that particular ethernet interface,
>   the driver should use HW blinking...

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).

Thanks,
Mauro