Re: [PATCH v3] Add support for PCIe SSD status LED management

["Tkaczyk, Mariusz" <mariusz.tkaczyk@linux.intel.com>]

On 06.10.2021 22:15, Dan Williams wrote:
>>>> +static struct led_state led_states[] = {
>>>> +       { .name = "ok",         .bit = 2 },
>>>> +       { .name = "locate",     .bit = 3 },
>>>> +       { .name = "failed",     .bit = 4 },
>>>> +       { .name = "rebuild",    .bit = 5 },
>>>> +       { .name = "pfa",        .bit = 6 },
>>>> +       { .name = "hotspare",   .bit = 7 },
>>>> +       { .name = "ica",        .bit = 8 },
>>>> +       { .name = "ifa",        .bit = 9 },
>>>> +       { .name = "invalid",    .bit = 10 },
>>>> +       { .name = "disabled",   .bit = 11 },
>>>> +};
>>> include/linux/enclosure.h has common ABI definitions of industry
>>> standard enclosure LED settings. The above looks to be open coding the
>>> same?
>>>
>> The LED states in inluce/linux/enclosure.h aren't exactly the same...
>> there are states defined in NPEM/_DSM that aren't defined in
>> enclosure.h.  In addition, while the enclosure driver allows "locate" to
>> be controlled independently, it looks like it will only allow a single
>> state (unsupported/ok/critical/etc) to be active at a time, while the
>> NPEM/_DSM allow all of the state bits to be independently set or
>> cleared.  Maybe only one of those states would need to be set at a time,
>> I don't know, but that would impose a limitation on what NPEM/_DSM can
>> do.  I'll take a closer look at this as an alternative to using
>> drivers/leds/led-class.c.
> Have a look. Maybe Mariusz can weigh in here with his experience with
> ledmon with what capabilities ledmon would need from this driver so we
> can decide what if any extensions need to be made to the enclosure
> infrastructure?

Hmm... In ledmon we are expecting one state to be set at the time. So,
I would expected from kernel to work the same.

Looking into ledmon code, all capabilities from this list could be
used[1].

 >>>> +       { .name = "ok",         .bit = 2 },
 >>>> +       { .name = "locate",     .bit = 3 },
 >>>> +       { .name = "failed",     .bit = 4 },
 >>>> +       { .name = "rebuild",    .bit = 5 },
 >>>> +       { .name = "pfa",        .bit = 6 },
 >>>> +       { .name = "hotspare",   .bit = 7 },
 >>>> +       { .name = "ica",        .bit = 8 },
 >>>> +       { .name = "ifa",        .bit = 9 },

[1]https://github.com/intel/ledmon/blob/master/src/ibpi.h#L60

Thanks,
Mariusz