On Fri, 5 Jul 2024, 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. > > Each enabled indication (capability register bit on) is represented as a > ledclass_dev which can be controlled through sysfs. For every ledclass > device only 2 brightness states are allowed: LED_ON (1) or LED_OFF (0). > It is corresponding to NPEM control register (Indication bit on/off). > > Ledclass devices appear in sysfs as child devices (subdirectory) of PCI > device which has an NPEM Extended Capability and indication is enabled > in NPEM capability register. For example, these are leds created for > pcieport "10000:02:05.0" on my setup: > > leds/ > ├── 10000:02:05.0:enclosure:fail > ├── 10000:02:05.0:enclosure:locate > ├── 10000:02:05.0:enclosure:ok > └── 10000:02:05.0:enclosure:rebuild > > They can be also found in "/sys/class/leds" directory. Parent PCIe device > bdf is used to guarantee uniqueness across leds subsystem. > > To enable/disable fail indication "brightness" file can be edited: > echo 1 > ./leds/10000:02:05.0:enclosure:fail/brightness > echo 0 > ./leds/10000:02:05.0:enclosure:fail/brightness > > PCIe r6.1, sec 7.9.19.2 defines the possible indications. > > Multiple indications for same parent PCIe device can conflict and > hardware may update them when processing new request. To avoid issues, > driver refresh all indications by reading back control register. > > 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. > > 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] > Suggested-by: Lukas Wunner <lukas@xxxxxxxxx> > Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@xxxxxxxxxxxxxxx> Looks to be in quite good shape already, one comment below I think should be addressed before this is ready to go. > +static int npem_set_active_indications(struct npem *npem, u32 inds) > +{ > + int ctrl, ret, ret_val; > + u32 cc_status; > + > + lockdep_assert_held(&npem->lock); > + > + /* This bit is always required */ > + ctrl = inds | PCI_NPEM_CTRL_ENABLE; > + > + ret = npem_write_ctrl(npem, ctrl); > + if (ret) > + return ret; > + > + /* > + * For the case where a NPEM command has not completed immediately, > + * it is recommended that software not continuously “spin” on polling > + * the status register, but rather poll under interrupt at a reduced > + * rate; for example at 10 ms intervals. > + * > + * PCIe r6.1 sec 6.28 "Implementation Note: Software Polling of NPEM > + * Command Completed" > + */ > + ret = read_poll_timeout(npem_read_reg, ret_val, > + ret_val || (cc_status & PCI_NPEM_STATUS_CC), > + 10 * USEC_PER_MSEC, USEC_PER_SEC, false, npem, > + PCI_NPEM_STATUS, &cc_status); > + if (ret) > + return ret_val; Will this work as intended? If ret_val gets set, cond in read_poll_timeout() is true and it returns 0 so the return branch is not taken. Also, when read_poll_timeout() times out, ret_val might not be non-zero. > + > + /* > + * All writes to control register, including writes that do not change > + * the register value, are NPEM commands and should eventually result > + * in a command completion indication in the NPEM Status Register. > + * > + * PCIe Base Specification r6.1 sec 7.9.19.3 > + * > + * Register may not be updated, or other conflicting bits may be > + * cleared. Spec is not strict here. Read NPEM Control register after > + * write to keep cache in-sync. > + */ > + return npem_get_active_indications(npem, &npem->active_indications); > +} -- i.
