|
From: Vasant H. <heg...@li...> - 2016-11-17 14:43:11
|
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
|
