linux-kernel - Re: [PATCH v2 00/17] Adding support for controlling the leds found on Intel NUC

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210520214356.0392f374@thinkpad>
Date:   Thu, 20 May 2021 21:43:56 +0200
From:   Marek Behún <kabel@...nel.org>
To:     Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
Cc:     Pavel Machek <pavel@....cz>, linuxarm@...wei.com,
        mauro.chehab@...wei.com, gregkh@...uxfoundation.org,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-leds@...r.kernel.org
Subject: Re: [PATCH v2 00/17] Adding support for controlling the leds found
 on Intel NUC

On Thu, 20 May 2021 21:16:15 +0200
Mauro Carvalho Chehab <mchehab+huawei@...nel.org> wrote:

> So, assuming that we will have one trigger per each hardware
> state, it could have something like (names subject to change):
> 
> 	- hw:powerstate
> 	- hw:disk_activity
> 	- hw:ethernet_activity
> 	- hw:wifi_active
> 	- hw:power_limit
> 
> Right?

Yes, but we should really try to map ethernet_activity to netdev and
disk_activity to a potential blkdev trigger :-) That's my opinion.

> It still needs to indicate two other possible states:
> 
> 	- software controlled led;
> 	- led is disabled.
> 
> Setting led's brightness to zero is different than disabling
> it. 
>
> Disabling can be done via BIOS, but BIOS config doesn't allow
> setting the brightness. There are other difference on BIOS settings:
> it allow disabling each/all LED controls and/or to disable software 
> control of each LED.
> 
> So, we need a way at the API to uniquely identify when the LED
> is software-controlled and when it is disabled.
> Would it be something like:
> 
> 	- hw:disable
> 
> trigger? or better to implement it on a different way?

What is the functional difference (visible to the user) between zero
brightness and disabled LED? IMO if user says
  echo 0 >brightness
you can just disable the LED. Or is this impossible?
 
> > Is the speed of breathing/strobing also adjustable? Or only when
> > pulsing?  
> 
> Yes, speed is also adjustable, from 0.1 to 1.0 HZ, in 0.1 Hz
> (NUC 8 and above).
> 
> The NUC6 API is more limited than NUC8+: it has just two
> blink patterns (blink, fade), and only 3 frequencies are allowed
> (0.25 Hz, 0.50 Hz and 1.0 Hz).
> 
> > When this "hw:powerstate" trigger is enabled for this LED,
> > only then another sysfs files should appear in this LED's sysfs
> > directory.  
> 
> OK, makes sense. 
> 
> Out of curiosity: is it reliable to make sysfs nodes appear and
> disappear dynamically? Does inotify (or something similar) can
> be used to identify when such nodes appear/disappear?
> 
> I remember a long time ago I wanted to use something like that 
> at the media (or edac?) subsystem, but someone (Greg, I think)
> recommended otherwise due to some potential racing issues.

No idea, but I would guess yes.

> > I'd rather use one file for frequencies and one for intervals, and map
> > in to an array, but that is just my preference...  
> 
> By intervals are you meaning 1/frequency? So, basically exposing
> the frequency as two fields? If so, it sounds overkill to me to have both. 

Sorry, I meant one file for frequencies and one for patterns.
> 
> Btw, maybe instead of "blink_behavior" it could use "blink_pattern".
> 
> This would diverge from the datahseet name, but it probably describes
> better what will be controlled when blink is enabled:
> 
> 	- frequency (or inverval)
> 	- pattern
> 
> > Regarding the enum with 8 colors: are these
> > colors red, yellow, green, cyan, blue, magenta? Because if so, then
> > this is RGB with each channel being binary :) So you can again use
> > multicolor framework.  
> 
> The dual-colored ones aren't RGB. Two types are supported:
> 	- Blue/Amber
> 	- Blue/White

These would need a new API, ignore these for now.

Marek