linux-kernel - Re: [PATCH 17/17] staging: nuc-led: update the TODOs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20210517141935.4a7cb905@coco.lan>
Date:   Mon, 17 May 2021 14:19:35 +0200
From:   Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
To:     Pavel Machek <pavel@....cz>
Cc:     gregkh@...uxfoundation.org, linuxarm@...wei.com,
        mauro.chehab@...wei.com,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org,
        linux-leds@...r.kernel.org, linux-staging@...ts.linux.dev
Subject: Re: [PATCH 17/17] staging: nuc-led: update the TODOs

Em Mon, 17 May 2021 10:05:27 +0200
Pavel Machek <pavel@....cz> escreveu:

> No. Take a look at triggers; for example hdd monitor should look very
> much like existing disk trigger.

Hmm... after looking at triggers, I'm not sure if this is the right
interface, nor if we're talking about the same thing.

See, at least the way ledtrig-disk.c uses it, two drivers are triggering 
the LED based on software events:

	drivers/ata/libata-core.c:      ledtrig_disk_activity(!!(qc->tf.flags & ATA_TFLAG_WRITE));
	drivers/ide/ide-disk.c: ledtrig_disk_activity(rq_data_dir(rq) == WRITE);

This is not how the NUC LEDs are work. On NUC, the hardware itself 
triggers the event and/or blinks the LED(*).

(*) By default, blink is enabled only when the device is suspended
    or it is hibernating.

There's no need to do any software emulation.

The API is meant to just program the hardware (and/or the firmware) 
for it to do the behavior expected by the user.

So, for instance, setting the indicator event that will trigger the
LED is done by sending a WMI message for this GUID object:
8C5DA44C-CDC3-46b3-8619-4E26D34390B7 (somewhat similar to using
the way ACPI works, but WMI is a different firmware interface).

The method at the WMI API which sets the LED indicator is this one:

	0x05 message (Set an Indicator option for the LED type)

Such method receives two parameters. The first one is the LED 
number, which can be:

	0 - Power button LED
	1 - HDD LED
	2 - Skull LED
	3 - Eyes LED
	4 - Front LED 1
	5 - Front LED 1
	6 - Front LED 1

The second one tells which hardware event will trigger the LED:

=====	==============	=======================================================
Value	Indicator type	Meaning
=====	==============	=======================================================
0	Power State	Shows if the device is powered and what power level
			it is (e. g. if the device is suspended or not, and
			on which kind of suspended level).
1	HDD Activity	Indicates if the LED is measuring the hard disk (or
			SDD) activity.
2	Ethernet	Indicates the activity Ethernet adapter(s)
3	WiFi		Indicates if WiFi is enabled
4	Software	Doesn't indicate any hardware level. Instead, the LED
			status is controlled via software.
5	Power Limit	Changes the LED color when the computer is throttling
			its power limits.
6	Disable		The LED was disabled (either by BIOS or via WMI).
=====	==============	=======================================================

There is an example at page 7 of the datasheet:

	https://raw.githubusercontent.com/nomego/intel_nuc_led/master/specs/INTEL_WMI_LED_0.64.pdf

Where it shows what happens if the WMI message is filled with:

	<0x05>  <0x00>  <0x01>

On such example, it changes the behavior of the button named "Power button" 
to change it to monitor the disk activity, instead of monitoring if the
device is powered on or not.

Such setting is even persistent after rebooting, and the event is
hardware/firmware triggered.

So, IMO, it would only makes sense to use something else from the LED
class if are there a way for the sysfs nodes to dynamically be shown/hidden
when the indicator changes.

At least on my eyes, that doesn't seem to be what triggers do.

Thanks,
Mauro