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
