|
From: Mauricio F. de O. <mau...@li...> - 2016-11-16 16:17:46
|
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
|
