kernel_npem: add support for kernel NPEM driver #270
Conversation
There was a problem hiding this comment.
Hi Stuart ,
Thank you for your contribution, Nice work!
After the review, I have a few points to clarify and some minor corrections.
It seems that the NPEM kernel still operates per slots, so my question is: did you use the implemented slot tests for the "NPEM kernel"? If not, please run them and verify that everything works correctly.
Also, please rebase the branch.
Thanks!
Add a new controller for the recently-added kernel NPEM/_DSM driver (in the kernel at drivers/pci/npem.c) that provides access to NPEM LEDs via sysfs. This allows access even if the kernel doesn't allow direct access to PCI config space (for NPEM), and also allows access to the PCI _DSM method of controlling PCIe SSD LEDs (PCI Firmware Specification, Revision 3.3, section 4.7 "_DSM Definitions for PCIe SSD Status LED"). The code was based on the npem controller code, but uses the sysfs interface rather than directly accessing the NPEM capability in PCI config space. Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
A few cosmetic changes for kernel_npem. Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
Include LED_CNTRL_TYPE_KERNEL_NPEM in led_controller_slot_support(). Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
Add the newly-added KERNEL_NPEM controller type to two places in block.c that were missed in the initial commit. Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
Update lib_unit_test.c to allow 7 controller types so it doesn't fail with KERNEL_NPEM. Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com>
stuarthayes
force-pushed
the
kernel_npem
branch
from
b645630 to
7088fe0
Compare
I hadn't run the tests. I found an issue when I ran the tests, which I fixed with another commit (7088fe0). |
Done, thanks for all the feedback! |
|
I manually tested this on a Dell PowerEdge R750. I'm seeing the following let states when listing the slots: The hardware only supports locate, and I can both set and read that state: Given that, I think it’s reasonable to report this as a known state (e.g., “OK”) rather than “UNKNOWN” when the hardware only implements a subset of the standard. We still need to be able to distinguish between:
On a related note, I’m a little unsure about exposing yet another controller type to the end user. From their perspective, this feels more like an implementation detail: the same hardware on an older kernel would use a different implementation but should ideally look the same from the tool’s interface. That said, I understand why you introduced a new controller type here; I just want to flag the end user/API difference. Notes
# ledctl failure=/dev/nvme16n1 ; echo $?;
/dev/shm/ledmon.conf: does not exist, using global config file
/etc/ledmon.conf: does not exist, using built-in defaults
0
# ledctl --set-slot --slot 0000:f4:00.0 --controller-type "Kernel NPEM" --state failure ; echo $?
/dev/shm/ledmon.conf: does not exist, using global config file
/etc/ledmon.conf: does not exist, using built-in defaults
10 |
5 participants
Add a new controller for the recently-added kernel NPEM/_DSM driver (in the kernel at drivers/pci/npem.c) that provides access to NPEM LEDs via sysfs. This allows access even if the kernel doesn't allow direct access to PCI config space (for NPEM), and also allows access to the PCI _DSM method of controlling PCIe SSD LEDs (PCI Firmware Specification, Revision 3.3, section 4.7 "_DSM Definitions for PCIe SSD Status LED").
The code was based on the npem controller code, but uses the sysfs interface rather than directly accessing the NPEM capability in PCI config space.