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

[Lukas Wunner <lukas@wunner.de>]

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.


> > +#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.


> > +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.


> > --- 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.

Thanks,

Lukas