SES: Report additional LED states

[cjs-nexsan]

Issue:
The LED states reported from SES enclosures only support the LOCATE, LOCATE_AND_FAIL, and FAILURE patterns; all other states are reported as NORMAL. As "ledctl" compares the requested state against the current reported state, this also makes it impossible to reset an SES slot to the NORMAL state if any other state has been configured - e.g. the following sequence will leave the slot set to the REBUILD pattern:

ledctl rebuild=/dev/sdc
ledctl normal=/dev/sdc

Cause:
The SES get_led_status() function only checks three bits in the element status (IDENT, FAULT SENSED, and FAULT REQSTD).

Solution:
Check additional element status bits that can be configured via ses_set_message() and return the appropriate state. As SES supports multiple simultaneous status bits, check the bits in priority order, starting with standard IBPI patterns in the same order as "ibpi_to_npem_capability", followed by SES-specific patterns.

[mtkaczyk]

@tasleson could you please take a look? I have no experience with SES

[mtkaczyk]

And probably you don't have experience either, but I have to ask :)

[tasleson]

@mtkaczyk Investigating

[tasleson]

@cjs-nexsan Thanks for the PR, some comments.

Priority order

This PR checks LOCATE before the array slot and remaining byte 2/3 bits:

  LOCATE_AND_FAIL > LOCATE > FAILED_DRIVE > PFA > (array slot block) > (byte 2/3 block)

Because the array slot and byte 2/3 blocks are gated by *led_status == LED_IBPI_PATTERN_NORMAL, if both LOCATE and (say) REBUILD are set simultaneously, this PR reports LOCATE. I think it would be better to put fault/rebuild conditions above LOCATE:

 LOCATE_AND_FAIL > FAILED_DRIVE > PFA > FAILED_ARRAY > REBUILD > DEGRADED > HOTSPARE > LOCATE > (SES specific)

With a single flat if/else if chain, if both LOCATE and REBUILD are set, the result is REBUILD, which seems more
operationally useful since an operator would want to know about the rebuild state rather than just "someone pressed the locate button."

Element type guard

The PR adds:

element_type ele_type = sp->page1_types[0].element_type;

and gates the array_slot_control checks with ele_type == SES_ARRAY_DEVICE_SLOT. The intent is correct, SES-4 defines byte 1 differently for Device Slot vs Array Device Slot elements. However, page1_types[0] always reads the first type descriptor, not the type for the given idx. If the enclosure has multiple element types before the slot entries, this would check the wrong type.

An alternative approach is to skip the guard entirely. The write path (ses_write_msg) already zeroes array_slot_control
for non-Array Device Slot elements, so in practice those bits should never be set on the status page for the wrong element type.

Structure

The three separate if blocks (with the second and third re-checking *led_status == LED_IBPI_PATTERN_NORMAL) are
functionally equivalent to a single flat if/else if chain, but the flat chain makes the overall priority ordering
immediately obvious and avoids any ambiguity about which conditions can override which.

Style

C++ style comments (//) — the rest of ses.c uses C style (/* */). If this passes check patch, I don't have issues with it.

Alternative implementation for comparison

  static void get_led_status(struct ses_pages *sp, int idx, enum led_ibpi_pattern *led_status)
  {
        struct ses_slot_ctrl_elem *descriptors = (void *)(sp->page2.buf + 8);
        struct ses_slot_ctrl_elem *desc_element = NULL;
        descriptors++;
        desc_element = &descriptors[idx];

        *led_status = LED_IBPI_PATTERN_NORMAL;

        /*
         * SES supports multiple simultaneous status bits. Check in priority
         * order with standard IBPI patterns first, followed by SES-specific
         * patterns. Byte 1 bit positions are the same in both pages for
         * Array Device Slot elements. Bytes 2-3 differ: RQST ACTIVE and
         * RQST MISSING have no status-page equivalents (SES-4 Tables 83/84).
         */

        /* Compound state: LOCATE + FAULT */
        if ((desc_element->b2 & 0x02) && (desc_element->b3 & 0x60))
                *led_status = LED_IBPI_PATTERN_LOCATE_AND_FAIL;
        /* Standard IBPI patterns - highest priority first */
        else if (desc_element->b3 & 0x60)
                *led_status = LED_IBPI_PATTERN_FAILED_DRIVE;
        else if (desc_element->common_control & 0x40)
                *led_status = LED_IBPI_PATTERN_PFA;
        else if (desc_element->array_slot_control & 0x04)
                *led_status = LED_IBPI_PATTERN_FAILED_ARRAY;
        else if (desc_element->array_slot_control & 0x02)
                *led_status = LED_IBPI_PATTERN_REBUILD;
        else if (desc_element->array_slot_control & 0x08)
                *led_status = LED_IBPI_PATTERN_DEGRADED;
        else if (desc_element->array_slot_control & 0x20)
                *led_status = LED_IBPI_PATTERN_HOTSPARE;
        else if (desc_element->b2 & 0x02)
                *led_status = LED_IBPI_PATTERN_LOCATE;
        /* SES-specific patterns */
        else if (desc_element->array_slot_control & 0x01)
                *led_status = LED_SES_REQ_ABORT;
        else if (desc_element->array_slot_control & 0x10)
                *led_status = LED_SES_REQ_CONS_CHECK;
        else if (desc_element->array_slot_control & 0x40)
                *led_status = LED_SES_REQ_RSVD_DEV;
        else if (desc_element->b2 & 0x04)
                *led_status = LED_SES_REQ_RM;
        else if (desc_element->b2 & 0x08)
                *led_status = LED_SES_REQ_INS;
        else if (desc_element->b2 & 0x40)
                *led_status = LED_SES_REQ_DNR;
        else if (desc_element->b3 & 0x04)
                *led_status = LED_SES_REQ_EN_BB;
        else if (desc_element->b3 & 0x08)
                *led_status = LED_SES_REQ_EN_BA;
        else if (desc_element->b3 & 0x10)
                *led_status = LED_SES_REQ_DEV_OFF;
  }

[cjs-nexsan]

@tasleson Thanks for considering this PR and the detailed review. I've replied to your points with some explanation/comments below, but I might well have misinterpeted/misunderstood something - I appreciate any further feedback you might have.

Priority order

...
With a single flat if/else if chain, if both LOCATE and REBUILD are set, the result is REBUILD, which seems more operationally useful since an operator would want to know about the rebuild state rather than just "someone pressed the locate button."

There were two reasons for the suggested ordering:

1. Maintain the existing behaviour to avoid potential regressions with scripts etc. that might use the existing values (e.g. setting LOCATE and expecting to see LOCATE, regardless of any other bits).
2. Match the behaviour of the "ibpi_to_npem_capability" table, where get_by_bits() will select the last entry with any bits set (i.e. LOCATE being the highest-priority state).

Locate is a specific user-initiated action versus the autonomous OS usage of the drive/slot, and there may be reasons to want to identify one specific drive/slot regardless of its usage, so locate being the highest-priority state seems appropriate, as well as maintaining the current behaviour and being consistent with the npem ordering (unless I've misinterpreted that). This also ensures that both Array Device Slot and Device Slot enclosures behave similarly for all non-SES-specific states they support (Device Slot enclosures will just not indicate or report array-specific states if the application tries to set them).

Element type guard

...
gates the array_slot_control checks with ele_type == SES_ARRAY_DEVICE_SLOT. The intent is correct, SES-4 defines byte 1 differently for Device Slot vs Array Device Slot elements. However, page1_types[0] always reads the first type descriptor, not the type for the given idx. If the enclosure has multiple element types before the slot entries, this would check the wrong type.

The SES specification requires Array Device Slot and/or Device Slot elements to be listed before any other element types. As I understand the code, ses_get_slots() returns after finding the first element type that matches either Array Device Slot or Device Slot; even if both types were present, only elements of the first listed type would be discovered/reported by ledmon - so types[0] will always be correct.

An alternative approach is to skip the guard entirely. The write path (ses_write_msg) already zeroes array_slot_control for non-Array Device Slot elements, so in practice those bits should never be set on the status page for the wrong element type.

I believe the type guard is required, as "ledmon" supports both Device Slot and Array Device Slot elements. For Device Slot elements, byte 1 is reserved on control, but defined as "Slot Address" with vendor-specific content in status - i.e. any bits could be set on any/all slots in a vendor-unique way, so interpreting them in get_led_status() could report the incorrect state.

Structure

The three separate if blocks (with the second and third re-checking *led_status == LED_IBPI_PATTERN_NORMAL) are
functionally equivalent to a single flat if/else if chain, but the flat chain makes the overall priority ordering
immediately obvious and avoids any ambiguity about which conditions can override which.

The separate blocks were mostly to avoid excessive repetitive "((ele_type == SES_ARRAY_DEVICE_SLOT) && ...)" conditions with splitting up the three classes of states (device/slot, array, SES) as a side benefit, but if you think a flat if/else chain would be clearer, that's fine.

Style

C++ style comments (//) — the rest of ses.c uses C style (/* */). If this passes check patch, I don't have issues with it.

This did pass check patch, but should have been kept consistent with the rest of the file - I can change that along with the structure.

[cjs-nexsan]

@tasleson I've pushed a commit to address most of your comments. The only functional change is to explicitly pass the element type into get_led_status(), but I've also flattened the if/else chain and added your comment block (slightly modified).

I've not changed the priority order pending further feedback. As a specific observation about your proposed order:
LOCATE_AND_FAIL > FAILED_DRIVE > PFA > FAILED_ARRAY > REBUILD > DEGRADED > HOTSPARE > LOCATE
If multiple drives are in the FAILED_ARRAY state, prioritising this state above LOCATE makes it impossible to identify a specific drive for replacement/removal, or to be aware that LOCATE has been enabled for a drive potentially leading to the incorrect drive being replaced/removed.

[tasleson]

I've not changed the priority order pending further feedback. As a specific observation about your proposed order:
LOCATE_AND_FAIL > FAILED_DRIVE > PFA > FAILED_ARRAY > REBUILD > DEGRADED > HOTSPARE > LOCATE
If multiple drives are in the FAILED_ARRAY state, prioritising this state above LOCATE makes it impossible to identify a specific drive for replacement/removal, or to be aware that LOCATE has been enabled for a drive potentially leading to the incorrect drive being replaced/removed.

@cjs-nexsan Thanks for the updates, initial review looks good, I'll want to scrutinize it a bit more before merge. I think the only open question is the ordering priority. I can see advantages and disadvantages of either approach. I would like @mtkaczyk to respond with their thoughts. If we look at the project history, keeping behavior that same has been maintained, thus I'm guessing that we will go with what you have in this PR.

[mtkaczyk]

@cjs-nexsan Thanks for the updates, initial review looks good, I'll want to scrutinize it a bit more before merge. I think the only open question is the ordering priority. I can see advantages and disadvantages of either approach. I would like @mtkaczyk to respond with their thoughts. If we look at the project history, keeping behavior that same has been maintained, thus I'm guessing that we will go with what you have in this PR.

Yes, please let stay with the current behavior, it is lower risk.
This is another case to start supporting multi IPBI states at the same time. Maybe we should start printing them all?
But this is not related to the change we have here.

[cjs-nexsan]

I'm not familiar with the automated checks on GitHub (or this project in particular). I believe all of the reported issues have been addressed, but it is still reporting "PR run failed", and the "Files changed" tab still shows the outdated check failures that have been addressed in the later commits. Is there some action I need to take to clear these and/or otherwise make this PR acceptable?

[mtkaczyk]

-------------------------------------------------------------
[1/5] Check commit - daca1163f584ba7a430173d12f2040b05c8b0cc2
-------------------------------------------------------------
WARNING:COMMIT_MESSAGE: Missing commit description - Add an appropriate one
Error: WARNING:COMMIT_MESSAGE: Missing commit description - Add an appropriate one


ERROR:OPEN_BRACE: that open brace { should be on the previous line
#30: FILE: src/lib/ses.c:480:
+ if ((*led_status == LED_IBPI_PATTERN_NORMAL) && (ele_type == SES_ARRAY_DEVICE_SLOT))
+ {
Error: ERROR:OPEN_BRACE: that open brace { should be on the previous line


ERROR:OPEN_BRACE: that open brace { should be on the previous line
#48: FILE: src/lib/ses.c:498:
+ if (*led_status == LED_IBPI_PATTERN_NORMAL)
+ {
Error: ERROR:OPEN_BRACE: that open brace { should be on the previous line


ERROR:MISSING_SIGN_OFF: Missing Signed-off-by: line(s)
Error: ERROR:MISSING_SIGN_OFF: Missing Signed-off-by: line(s)


total: 3 errors, 1 warnings, 50 lines checked
>> Failure

-------------------------------------------------------------
[2/5] Check commit - a9b73c18986a65fbace68e849774ee3aff8fbdb0
-------------------------------------------------------------
total: 0 errors, 0 warnings, 18 lines checked
>> Success

-------------------------------------------------------------
[3/5] Check commit - 196f896cb84dbe2476df747cb719afe324864542
-------------------------------------------------------------
WARNING:LONG_LINE: line length of 115 exceeds 100 columns
#24: FILE: src/lib/ses.c:460:
+static void get_led_status(struct ses_pages *sp, element_type ele_type, int idx, enum led_ibpi_pattern *led_status)
Error: WARNING:LONG_LINE: line length of 115 exceeds 100 columns


WARNING:LONG_LINE: line length of 107 exceeds 100 columns
#126: FILE: src/lib/ses.c:567:
+ get_led_status(sp, t->element_type, slots[j].index, &slots[j].ibpi_status);
Error: WARNING:LONG_LINE: line length of 107 exceeds 100 columns


total: 0 errors, 2 warnings, 109 lines checked
>> Failure

You are using model of pushing fixes to new patches. That is usually correct unless specific project has per commit validation (like we have here) - commit title, commit message and commit body- all validated.

In our case, this is a result of years of integration with linux kernel, that is why we are requesting the same style and following similar rules.

Please squash this to one meaningful commit and push with force :) That is generally the way!

[cjs-nexsan]

Thanks for clarifying - I have squashed and force-pushed the commit, so hopefully it is now correct. Apologies for the multiple changes getting this PR into shape.

[tasleson]

This PR looks good. However, after reviewing and looking at npem, I have a question about the differences between the two which I've opened up an issue for discussion, here #282

I think we can commit this and then address that issue and make any changes in a new PR.

[tasleson]

@cjs-nexsan Please re-base changes