On 6/12/2024 6:40 AM, Mariusz Tkaczyk wrote:
Hi,
Thanks for feedback Dan!
On Wed, 29 May 2024 11:38:12 +0200
Lukas Wunner <lukas@xxxxxxxxx> wrote:
On Tue, May 28, 2024 at 10:21:10PM -0700, Dan Williams wrote:
Mariusz Tkaczyk wrote:
+config PCI_NPEM
+ bool "Native PCIe Enclosure Management"
+ depends on LEDS_CLASS=y
I would have expected
depends on NEW_LEDS
select LEDS_CLASS
Hm, a quick "git grep -C 2 'depends on NEW_LEDS'" shows that noone else
does that. Everyone else either selects both NEW_LEDS and LEDS_CLASS
or depends on both or depends on just LEDS_CLASS.
(Since LEDS_CLASS is constrained to "if NEW_LEDS", depending on both
seems pointless, so I'm not sure why some people do that.)
I guess it would be good to get guidance from leds maintainers what
the preferred modus operandi is.
Pavel, could you please advice?
I have no clue which way I should take so I prefer to keep current approach.
+#define for_each_indication(ind, inds) \
+ for (ind = inds; ind->bit; ind++)
+
+/* To avoid confusion, do not keep any special bits in indications */
I am confused by this comment. What "special bits" is this referring to?
I think it's referring to bit 0 in the Status and Control register,
which is a master "NPEM Capable" and "NPEM Enable" bit.
Yes, there are 2 special bits for capability/control
NPEM_CAP_CAPABLE/NPEM_ENABLE and NPEM_CAP_RESET/NPEM_RESET.
I wanted to highlight that these bits are not included in the cache. I will try
to make it more precise in v3.
+struct npem_ops {
+ const struct indication *inds;
@inds is not an operation, it feels like something that belongs as
another member in 'struct npem'. What drove this data to join 'struct
npem_ops'?
The native NPEM register interface supports enclosure-specific indications
which the DSM interface does not support. So those indications are
present in the native npem_ops->inds and not present in the DSM
npem_ops->inds.
Yes, I need to differentiate DSM and NPEM indications. DSM has own indications
list.
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
[...]
+#define PCI_NPEM_IND_SPEC_0 0x00800000
+#define PCI_NPEM_IND_SPEC_1 0x01000000
+#define PCI_NPEM_IND_SPEC_2 0x02000000
+#define PCI_NPEM_IND_SPEC_3 0x04000000
+#define PCI_NPEM_IND_SPEC_4 0x08000000
+#define PCI_NPEM_IND_SPEC_5 0x10000000
+#define PCI_NPEM_IND_SPEC_6 0x20000000
+#define PCI_NPEM_IND_SPEC_7 0x40000000
Given no other driver needs this, I would define them locally in
drivers/pci/npem.c.
This is a uapi header, so could be used not just by other drivers
but by user space.
It's common to add spec-defined register bits to this header file
even if they're only used by a single source file in the kernel.
I will stay with current state while waiting for Bjorn's voice here.
I will send v3 with fixes requested by Dan and Ilpo but I still need Stuart
feedback on DSM patch.
Thanks,
Mariusz
I'm working on testing this now, sorry for the delay.
Thanks! Stuart
