[v4,0/3] PCIe Enclosure LED Management

Message

Mariusz Tkaczyk July 11, 2024, 8:30 a.m. UTC

Patchset is named as PCIe Enclosure LED Management because it adds two features:
- Native PCIe Enclosure Management (NPEM)
- PCIe SSD Status LED Management (DSM)

Both are pattern oriented standards, they tell which "indication" should blink.
It doesn't control physical LED or pattern visualization.

Overall, driver is simple but it was not simple to fit it into interfaces
we have in kernel (We considered leds and enclosure interfaces). It reuses
leds interface, this approach seems to be the best because:
- leds are actively maintained, no new interface added.
- leds do not require any extensions, enclosure needs to be adjusted first.

There are trade-offs:
- "brightness" is the name of sysfs file to control led. It is not
  natural to use brightness to set patterns, that is why multiple led
  devices are created (one per indication);
- Update of one led may affect other leds, led triggers may not work
  as expected.

Changes from v1:
- Renamed "pattern" to indication.
- DSM support added.
- fixed nits reported by Bjorn.

Changes from v2:
- Introduce lazy loading to allow DELL _DSM quirks to work, reported by Stuart.
- leds class initcall moved up in Makefile, proposed by Dan.
- fix other nits reported by Dan and Iipo.

Changes from v3:
- Remove unnecessary packed attr.
- Fix doc issue reported by lkp.
- Fix read_poll_timeout() error handling reported by Iipo.
- Minor fixes reported by Christoph.

No feedback received from leds maintainers. No updates in Kconifg. No functional changes from v3.

I hope, it is good to go! Thanks everyone for support.

Suggested-by: Lukas Wunner <[e-mail]>
Reviewed-by: Christoph Hellwig <[e-mail]>
Reviewed-by: Ilpo Järvinen <[e-mail]>
Tested-by: Stuart Hayes <[e-mail]>

Cc: Arnd Bergmann <[e-mail]>
Cc: Bjorn Helgaas <[e-mail]>
Cc: Dan Williams <[e-mail]>
Cc: Greg Kroah-Hartman <[e-mail]>
Cc: Ilpo Jarvinen <[e-mail]>
Cc: Lukas Wunner <[e-mail]>
Cc: Keith Busch <[e-mail]>
Cc: Marek Behun <[e-mail]>
Cc: Pavel Machek <[e-mail]>
Cc: Randy Dunlap <[e-mail]>
Cc: Andy Shevchenko <[e-mail]>
Cc: Stuart Hayes <[e-mail]>
Link: https://lore.kernel.org/linux-pci/[e-mail]/

Mariusz Tkaczyk (3):
  leds: Init leds class earlier
  PCI/NPEM: Add Native PCIe Enclosure Management support
  PCI/NPEM: Add _DSM PCIe SSD status LED management

 drivers/Makefile              |   4 +-
 drivers/pci/Kconfig           |   9 +
 drivers/pci/Makefile          |   1 +
 drivers/pci/npem.c            | 590 ++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h             |   8 +
 drivers/pci/probe.c           |   2 +
 drivers/pci/remove.c          |   2 +
 include/linux/pci.h           |   3 +
 include/uapi/linux/pci_regs.h |  35 ++
 9 files changed, 653 insertions(+), 1 deletion(-)
 create mode 100644 drivers/pci/npem.c

Comments

Bjorn Helgaas July 25, 2024, 8:08 p.m. UTC | #1

[+cc Blazej]

On Thu, Jul 11, 2024 at 10:30:06AM +0200, Mariusz Tkaczyk wrote:
> Patchset is named as PCIe Enclosure LED Management because it adds two features:
> - Native PCIe Enclosure Management (NPEM)
> - PCIe SSD Status LED Management (DSM)
> 
> Both are pattern oriented standards, they tell which "indication" should blink.
> It doesn't control physical LED or pattern visualization.
> 
> Overall, driver is simple but it was not simple to fit it into interfaces
> we have in kernel (We considered leds and enclosure interfaces). It reuses
> leds interface, this approach seems to be the best because:
> - leds are actively maintained, no new interface added.
> - leds do not require any extensions, enclosure needs to be adjusted first.
> 
> There are trade-offs:
> - "brightness" is the name of sysfs file to control led. It is not
>   natural to use brightness to set patterns, that is why multiple led
>   devices are created (one per indication);
> - Update of one led may affect other leds, led triggers may not work
>   as expected.

I see the sysfs interface (/sys/.../leds/10000:02:05.0:enclosure:fail,
etc).  I assume this is intended for things like ledmon?  I think this
should be documented somewhere in Documentation/ABI/ if it's not
already there.

I think that sysfs interface is the same for NPEM and _DSM?

I guess this is basically a newer, better, more generic approach to
the pciehp functionality added by 576243b3f9ea ("PCI: pciehp: Allow
exclusive userspace control of indicators") for NVMe behind VMD?

I suppose it's too late for any hope of unifying all these things in
terms of the user interface?  I guess we're stuck with maintaining
576243b3f9ea regardless since users are using it, but the VMD stuff in
ledmon seems like kind of an ugly special case.

Bjorn

Mariusz Tkaczyk July 26, 2024, 7:29 a.m. UTC | #2

On Thu, 25 Jul 2024 15:08:19 -0500
Bjorn Helgaas <[e-mail]> wrote:

> [+cc Blazej]
> 
> On Thu, Jul 11, 2024 at 10:30:06AM +0200, Mariusz Tkaczyk wrote:
> > Patchset is named as PCIe Enclosure LED Management because it adds two
> > features:
> > - Native PCIe Enclosure Management (NPEM)
> > - PCIe SSD Status LED Management (DSM)
> > 
> > Both are pattern oriented standards, they tell which "indication" should
> > blink. It doesn't control physical LED or pattern visualization.
> > 
> > Overall, driver is simple but it was not simple to fit it into interfaces
> > we have in kernel (We considered leds and enclosure interfaces). It reuses
> > leds interface, this approach seems to be the best because:
> > - leds are actively maintained, no new interface added.
> > - leds do not require any extensions, enclosure needs to be adjusted first.
> > 
> > There are trade-offs:
> > - "brightness" is the name of sysfs file to control led. It is not
> >   natural to use brightness to set patterns, that is why multiple led
> >   devices are created (one per indication);
> > - Update of one led may affect other leds, led triggers may not work
> >   as expected.  
> 
> I see the sysfs interface (/sys/.../leds/10000:02:05.0:enclosure:fail,
> etc).  I assume this is intended for things like ledmon?  I think this
> should be documented somewhere in Documentation/ABI/ if it's not
> already there.

Currently, ledmon is not familiar with the kernel LEDs class so I do not expect
have it documented in kernel.

I will create Documentation/ABI/testing/sysfs-class-led-npem-dsm and resend it.

> 
> I think that sysfs interface is the same for NPEM and _DSM?

Yes, in this implementation the backend (NPEM/DSM) is not presented to
userspace.

> 
> I guess this is basically a newer, better, more generic approach to
> the pciehp functionality added by 576243b3f9ea ("PCI: pciehp: Allow
> exclusive userspace control of indicators") for NVMe behind VMD?

So far I know, the motivation of NPEM was to provide enterprise enclosure led
management for NVMes. NPEM capable hardware is replacing VMD blinking on Intel
platforms as you expect.

In ledmon, if both VMD and NPEM detected, NPEM has higher priority.
Ledmon is able to manipulate NPEM directly now, we will switch it to kernel
driver.

> 
> I suppose it's too late for any hope of unifying all these things in
> terms of the user interface?  I guess we're stuck with maintaining
> 576243b3f9ea regardless since users are using it, but the VMD stuff in
> ledmon seems like kind of an ugly special case.

Yes, we cannot abandon VMD blinking :(
NPEM is a hardware feature, we cannot just switch to NPEM with new kernels,
hardware must support it.

Mariusz