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

[Lukas Wunner <lukas@wunner.de>]

On Wed, Jul 31, 2024 at 05:17:01PM +0200, Marek Behún wrote:
> On Wed, Jul 31, 2024 at 01:51:17PM +0200, Mariusz Tkaczyk wrote:
> > 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.
> 
> My point is about what a LED class device in kernel means, and what the brightness
> attribute means (in terms of intended behavior).
> One LED class device should represent one hardware LED (possibly multicolor), and
> nothing else.
> Setting the brightness attribute to the value of max_brightness should light the LED
> on, and setting it to 0 should light it off.
> So if on some device doing
>   echo 1 >brightness
> makes the LED for example blink, it is wrong.
> 
> That's why I am asking whether it is possible to determine what the hardware is
> doing from some description, like ACPI or device-tree.

The PCIe Base Specification and the PCI Firmware Specification do not
provide a way to query what the effect of a set bit will be.

We've had lengthy mailing list discussions how to represent NPEM bits
in the kernel.  Representing each bit as a distinct led_classdev seemed
like the most reasonable way and I thought we had reached consensus
on that.  Objecting against the chosen representation at this point,
not to mention without suggesting a better alternative, is unreasonable
from my point of view.

I think it is reasonable to assume that usually each bit is a distinct
LED.  The spec doesn't rule out other form factors, such as multiple
bits being represented by distinct colors of a multi-color LED.
However I think such form factors will remain esoteric and theoretical
for the most part.  We need to be pragmatic here.


> If setting brightness to 1 makes some LED blink (without a LED trigger), than
> the device does not behave according to the LED class expectations.
[...]
> Look for example at the netdev trigger. Originally it was software only, and you
> could set it up so that it would for example blink on rx/tx activity of a network
> interface.

I think you're confusing two different things:

"Blinking" in the rx/tx activity context means that the LED is turned on
when traffic is flowing and off when it is not flowing.  Because traffic
is usually not flowing constantly, the LED is "blinking".

In the NPEM context, my understanding is "blinking" means the LED turns
on or off *in a regular interval* to indicate that the corresponding
NPEM bit has been set.

Thanks,

Lukas