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

[Bjorn Helgaas <helgaas@kernel.org>]

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