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

Bjorn Helgaas
2024-08-14T21:49:32Z

On Wed, Aug 14, 2024 at 02:28:59PM +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.
> ...

> Driver is projected to be exclusive NPEM extended capability manager.
> It waits up to 1 second after imposing new request, it doesn't verify if
> controller is busy before write, assuming that mutex lock gives protection
> from concurrent updates.

> Driver is not registered if _DSM LED management
> is available.

IMO we should drop this sentence (more details below).

> NPEM is a PCIe extended capability so it should be registered in
> pcie_init_capabilities() but it is not possible due to LED dependency.
> Parent pci_device must be added earlier for led_classdev_register()
> to be successful. NPEM does not require configuration on kernel side, it
> is safe to register LED devices later.
>
> Link: https://members.pcisig.com/wg/PCI-SIG/document/19849 [1]

I can update this myself, no need to repost just for this, but I think
these links are pointless because they're useless except for PCI-SIG
members, and I don't want to rely them being permalinks anyway.

A reference like "PCIe r6.1" is universally and permanently
meaningful.

> +struct npem {
> + struct pci_dev *dev;
> + const struct npem_ops *ops;
> + struct mutex lock;
> + u16 pos;
> + u32 supported_indications;
> + u32 active_indications;
> +
> + /*
> + * Use lazy loading for active_indications to not play with initcalls.
> + * It is needed to allow _DSM initialization on DELL platforms, where
> + * ACPI_IPMI must be loaded first.
> + */
> + unsigned int active_inds_initialized:1;

What's going on here? I hope we can at least move this to the _DSM
patch since it seems related to that, not to the NPEM capability. I
don't understand the initcall reference or what "lazy loading" means.

Is there some existing ACPI ordering that guarantees ACPI_IPMI happens
first? Why do we need some Dell-specific thing here?

What is ACPI_IPMI? I guess it refers to the "acpi_ipmi" module,
acpi_ipmi.c?

> +#define DSM_GUID GUID_INIT(0x5d524d9d, 0xfff9, 0x4d4b, 0x8c, 0xb7, 0x74, 0x7e,\
> + 0xd5, 0x1e, 0x19, 0x4d)
> +#define GET_SUPPORTED_STATES_DSM 1
> +#define GET_STATE_DSM 2
> +#define SET_STATE_DSM 3
> +
> +static const guid_t dsm_guid = DSM_GUID;
> +
> +static bool npem_has_dsm(struct pci_dev *pdev)
> +{
> + acpi_handle handle;
> +
> + handle = ACPI_HANDLE(&pdev->dev);
> + if (!handle)
> + return false;
> +
> + return acpi_check_dsm(handle, &dsm_guid, 0x1,
> + BIT(GET_SUPPORTED_STATES_DSM) |
> + BIT(GET_STATE_DSM) | BIT(SET_STATE_DSM));
> +}

> +void pci_npem_create(struct pci_dev *dev)
> +{
> + const struct npem_ops *ops = &npem_ops;
> + int pos = 0, ret;
> + u32 cap;
> +
> + if (!npem_has_dsm(dev)) {
> + pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_NPEM);
> + if (pos == 0)
> + return;
> +
> + if (pci_read_config_dword(dev, pos + PCI_NPEM_CAP, &cap) != 0 ||
> + (cap & PCI_NPEM_CAP_CAPABLE) == 0)
> + return;
> + } else {
> + /*
> + * OS should use the DSM for LED control if it is available
> + * PCI Firmware Spec r3.3 sec 4.7.
> + */
> + return;
> + }

I know this is sort of a transient state since the next patch adds
full _DSM support, but I do think (a) the fact that NPEM will stop
working simply because firmware adds _DSM support is unexpected
behavior, and (b) npem_has_dsm() and the other ACPI-related stuff
would fit better in the next patch. It's a little strange to have
them mixed here.

> +++ b/include/uapi/linux/pci_regs.h
> ...

> +#define PCI_NPEM_CAP 0x04 /* NPEM capability register */
> +#define PCI_NPEM_CAP_CAPABLE 0x00000001 /* NPEM Capable */
> +
> +#define PCI_NPEM_CTRL 0x08 /* NPEM control register */
> +#define PCI_NPEM_CTRL_ENABLE 0x00000001 /* NPEM Enable */

Spaces instead of tabs after #define, as you did below (mostly), would
make the diff prettier.

> +#define PCI_NPEM_CMD_RESET 0x00000002 /* NPEM Reset Command */
> +#define PCI_NPEM_IND_OK 0x00000004 /* NPEM indication OK */
> +#define PCI_NPEM_IND_LOCATE 0x00000008 /* NPEM indication Locate */
> ...

> +#define PCI_NPEM_STATUS 0x0c /* NPEM status register */
> +#define PCI_NPEM_STATUS_CC 0x00000001 /* NPEM Command completed */

Ditto.

Bjorn