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

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!

 > [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:

         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.

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.

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

Should that function really return early (w/out reaching the 'ses' and
'marvell' get_indices calls)...
- only in the 2 checks for LED_TYPE_FAULT, but not in the check for
   opal_get_indices()...
- or not return early in case of failure in all of them?

Thank you!

-- 
Mauricio Faria de Oliveira
IBM Linux Technology Center


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