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

On 11/16/2016 09:47 PM, Mauricio Faria de Oliveira wrote:
> Hi Vasant,
>
>
> On 11/13/2016 01:24 PM, Vasant Hegde wrote:
>> Overall patchset looks good.. Had few minor issues which I've fixed
>> locally. [snip]
>
> Okay, I see the if-style changes in mv_unmap_bar5(), a couple places where I
> forgot to remove or add a return statement (sorry, I had the
> impression I fixed it..) and header guards in indicator_marvell.h...
> nice catch! Plus the vfprintf/printf change in debug messages patch.
>
> Thanks for handling those!

:-)

Generally if its trivial changes I do it myself rather than asking for another 
respin. That way we can save some time.


>
>  > [snip] We still have few issues to fix.. Since I forgot to mention
>  > this in previous iteration and also it can come as separate patch,
>  > I've merged this patchset.
>
> Cool, thanks.
>
>  > - opal_get_indicator_mode()
>  >   We need to add check in this function.
>
> Can you clarify this request?
>
> AFAICT opal_get_indicator_mode() is only called for LED_TYPE_FAULT,
> but the marvell LEDs are LED_TYPE_IDENT only.
>
>      main() @ usysident.c:

This is what happens if  I merge patches over weekend!

So we also have lp_diag (ncurses based UI)...which uses same functions.

Basically it calls get_indicator_mode() to LED operating mode for operating on 
fault indicator.

Also operating_mode variable is set by get_indicator_mode() and then used by 
opal_get_indicator_list().
Original code always assumed system level LEDs are supported always.

Now that we have special case, we should fix this function.

>
>          if (indicator == LED_TYPE_FAULT) {
>                  rc = get_indicator_mode();
>                  if (rc)
>                          return rc;
>          }
>
>
>  > - opal_get_indicator_list()
>  >   It worked today because we had sysfs leds on system. May be we should
>  > fix this function continue even though we fail  to get led list from
>  > sysfs led class.
>
> I believe it still works w/out any LED class modules; I recall testing
> in that scenario.


opal_get_indicator_list -> 	opal_get_indices -> sysfs led class dir not exists 
-> return -1 ..

>
> I'd guess it's because
> - 2 of the 3 early 'return rc' calls are for LED_TYPE_FAULT
>   (this uses LED_TYPE_IDENT),
> - 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?

>
> Anyway, do you mind explaining the expected behavior a bit, so that I
> can write the changes correctly? I have this question..

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 .



>
> Should that function really return early (w/out reaching the 'ses' and
> 'marvell' get_indices calls)...

No.. we should just continue and list from other source.


Vasant


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