Re: [PATCH v3 0/3] Support for Marvell SATA HDD LEDs on IBM Power System S822LC for HPC

On 11/18/2016 04:30 AM, Mauricio Faria de Oliveira wrote:
> Hi Vasant,
>
> On 11/17/2016 12:42 PM, Vasant Hegde wrote:
>>>  > - opal_get_indicator_mode()
>>>  >   We need to add check in this function.
>>>
>>> Can you clarify this request?
>
>> Now that we have special case, we should fix this function.
>
> Sorry; even w/ your explanation I didn't get why/what exactly
> needs to be fixed in this function. Can you say it for dummies?

Don't worry.. I will fix it.

>
>>>  > - opal_get_indicator_list()
> [snip]
>> opal_get_indicator_list ->     opal_get_indices -> sysfs led class dir
>> not exists -> return -1 ..
> [snip]
>>> - the other call is for opal_get_indices(),
>>>    which does not fail because scandir() should have returned 0
>>>    (no entries), not -1 in case of error.
>>
>> On error scandir returns -1 right?
>
> Yes, but is doesn't error in this case. The /sys/class/leds dir is
> present even without LED modules loaded (led-class.o is built-in.)


I was talking about the scenario where kernel won't create sysfs node. Probably 
we don't need to worry about this case for now.



>
>      # ls /sys/class/leds
>      #
>      # lsmod | grep -i led
>      #
>
>      # ./lpd/usysident
>      Some service indicators are not supported on this system.
>      Make sure 'leds_powernv' kernel module is loaded.

This may confuse the user. I will discard above message. (will send patch for that).

>      -B0-T0-L0     [on]
>      -B0-T0-L0     [off]
>
> And on empty dirs, its rc is 2 (for . and ..)
>
>      $ cat scandir.c
>      #include <dirent.h>
>      #include <stdlib.h>
>
>      int main() {
>          struct dirent **dirlist;
>          return scandir("emptydir", &dirlist, NULL, alphasort);
>      }
>
>      $ gcc -o scandir scandir.c
>
>      $ mkdir emptydir
>      $ ./scandir; echo $?
>      2
>      $ rmdir emptydir
>      $ ./scandir; echo $?
>      255
>
> Well, but with the patch for the section below, I guess this topic
> is no longer necessary :- ) as a patch makes opal_get_indicator_list()
> not to return early if opal_get_indices() fails.
>
>> So now we have number of sources to get LEDs (sysfs, ses_leds, marvel
>> leds).
>> If we fail to get LEDs from one source, lets continue with other sources
>> and build list instead of returning .
>
> Okay, submitting a patch for this. It checks whether any of the function
> calls initialized the list pointer to non-NULL (ie, allocated an entry
> for an indicator).
>
> Would you please check if the assumption to remove the fault mode check
> is correct? I'm afraid I don't understand enough about this to be sure.

I've tweaked your patch slightly and merged it.

-Vasant


------------------------------------------------------------------------------