Re: [PATCH v4 2/3] PCI/NPEM: Add Native PCIe Enclosure Management support

[Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>]

On Fri, 26 Jul 2024 09:29:36 +0200
Marek Behún <kabel@kernel.org> wrote:

> On Thu, Jul 11, 2024 at 10:30:08AM +0200, Mariusz Tkaczyk wrote:
> > Native PCIe Enclosure Management (NPEM, PCIe r6.1 sec 6.28) allows
> > managing LED in storage enclosures. NPEM is indication oriented
> > and it does not give direct access to LED. Although each of
> > the indications *could* represent an individual LED, multiple
> > indications could also be represented as a single,
> > multi-color LED or a single LED blinking in a specific interval.
> > The specification leaves that open.  
> 
> The specification leaves it open, but isn't there a way to determine
> how it is implemented? In ACPI, maybe?


What would be a point of that? There are blinking patterns standards for 2-LED
systems and 3-LED systems but NPEM is projected to not be limited to
the led system you have. I mean that we shouldn't try to determine what hardware
does - it belongs to hardware. Kernel task is just to read what NPEM registers
are presenting and trust it.

I can realize NPEM with separate LED for each indication. Who knows, maybe in
the future it would become real.

> 
> > Each enabled indication (capability register bit on) is represented as a
> > ledclass_dev which can be controlled through sysfs. For every ledclass
> > device only 2 brightness states are allowed: LED_ON (1) or LED_OFF (0).
> > It is corresponding to NPEM control register (Indication bit on/off).
> > 
> > Ledclass devices appear in sysfs as child devices (subdirectory) of PCI
> > device which has an NPEM Extended Capability and indication is enabled
> > in NPEM capability register. For example, these are leds created for
> > pcieport "10000:02:05.0" on my setup:
> > 
> > leds/
> > ├── 10000:02:05.0:enclosure:fail
> > ├── 10000:02:05.0:enclosure:locate
> > ├── 10000:02:05.0:enclosure:ok
> > └── 10000:02:05.0:enclosure:rebuild
> > 
> > They can be also found in "/sys/class/leds" directory. Parent PCIe device
> > bdf is used to guarantee uniqueness across leds subsystem.
> > 
> > To enable/disable fail indication "brightness" file can be edited:
> > echo 1 > ./leds/10000:02:05.0:enclosure:fail/brightness
> > echo 0 > ./leds/10000:02:05.0:enclosure:fail/brightness  
> 
> Have you considered implemtening this via a led trigger?
> 
> Something like:
>   echo pcie-enclosure > /sys/class/leds/<LED>/trigger
>   echo 1 >/sys/class/leds/<LED>/fail
> but properly thought up.
> 

No I didn't. I understand the triggers as an actions that may involve led
change we can configure. I thought, it should be cross driver reference (for
example, change LED if keyboard capslock is pressed) and triggers are optional.

For that reasons I did not consider it. Please explain this concept in details.

I think that forcing one and only trigger you can use may we even worse because
it seems to be definitely design incompatible (triggers are optional) but I'm
not an expert.

Thanks,
Mariusz