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

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?

>>  > - 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.)

	# 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.
	-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.

Thanks!



-- 
Mauricio Faria de Oliveira
IBM Linux Technology Center


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