Re: [PATCH] PCI: Introduce Cray ClusterStor E1000 NVMe slot LED driver

[Lukas Wunner <lukas@wunner.de>]

On Tue, Aug 27, 2024 at 02:03:48PM -0700, Tony Hutter wrote:
> Add driver to control the NVMe slot LEDs on the Cray ClusterStor E1000.
> The driver provides hotplug attention status callbacks for the 24 NVMe
> slots on the E1000.  This allows users to access the E1000's locate and
> fault LEDs via the normal /sys/bus/pci/slots/<slot>/attention sysfs
> entries.  This driver uses IPMI to communicate with the E1000 controller to
> toggle the LEDs.

The PCISIG has converged on the NPEM interface (PCIe r6.2 sec 6.28 and
7.9.19) to control LEDs on storage enclosures.  We're in the process of
upstreaming that:

https://lore.kernel.org/all/20240814122900.13525-1-mariusz.tkaczyk@linux.intel.com/

Of course proprietary interfaces such as the Cray one are still
upstreamable as long as you're willing to maintain them.

The NPEM implementation linked above models each LED as a led_classdev.
I'd suggest following that instead of using the legacy "attention"
interface in sysfs.  Overloading the {set,get}_attention_status()
callbacks like you're doing here is not acceptable upstream IMO.

You need to be careful about the lifetime of the pci_dev below which
you're adding led_classdevs (or of which you're modifying the
{set,get}_attention_status() callbacks) because pci_devs can be removed
via sysfs at any time.

Basically what you need to do is find the pci_dev in craye1k_new_smi()
and acquire a reference on it with pci_dev_get().  Install a bus notifier
so that you get notified when the pci_dev is removed.  In the notifier,
you need to remove the ledclass_devs and release the reference on the
pci_dev with pci_dev_put().  See here for an example how to add a
notifier for pci_bus_type:

https://github.com/l1k/linux/commit/d2d5296785c7

The statistics should live in debugfs instead of regular sysfs.

The command line options should likewise live in debugfs.
New command line options are generally ill received because the
expectation is that everything is configured correctly automatically
without the user having to fiddle with command line options.

Please add documentation for the user space ABI you're introducing to
Documentation/ABI/testing/sysfs-bus-pci.

The MODULE_SOFTDEP("pre: pciehp") doesn't really make any sense
because pciehp is always builtin or disabled, but never modular.

Thanks,

Lukas