Re: [patch 2/2 v2] leds: netxbig: silence a static checker warning

[Jacek Anaszewski <j.anaszewski@samsung.com>]

On 04/13/2015 12:16 PM, Simon Guinot wrote:
> On Mon, Apr 13, 2015 at 09:35:19AM +0200, Jacek Anaszewski wrote:
>> On 04/10/2015 09:52 PM, Simon Guinot wrote:
>>> On Fri, Apr 10, 2015 at 06:50:34PM +0300, Dan Carpenter wrote:
>>>> I've looked at this some more.  Most of the places which call
>>>> of_property_read_u32_index() check the return code.  The ones that don't
>>>> mostly initialize their values going in.  The remainder introduce static
>>>> checker warnings like:
>>>>
>>>> 	drivers/clk/ti/divider.c:472 ti_clk_get_div_table()
>>>> 	error: potentially using uninitialized 'val'.
>>>>
>>>> These warnings cause me pain.  It calls of_get_property() earlier so
>>>> it won't return -EINVAL.  I don't know if it can return -ENODATA or
>>>> -EOVERFLOW?
>>>>
>>>> I guess not.
>>>
>>> I think it can't. Above, we are calling of_property_count_u32_elems() to
>>> count the number of u32 elements in the "timers" property. After we are
>>> ensuring that there is three u32 elements available per timer. That's
>>> why the return codes for the three of_property_read_u32_index() calls
>>> are not checked.
>>
>> After looking at Documentation/devicetree/bindings/leds/leds-netxbig.txt
>> I noticed inconsistency: timers property is defined as required, but
>> the comment over the call to of_property_count_u32_elems says that it
>> is optional.
>>
>> I think that DT documentation should be changed to make the property
>> optional. How do you think?
>
> Thanks for spotting this. At first, I wrote the binding document. And
> since there is always "blink timers" defined with this LED mechanism,
> I made the property mandatory. But after, in the code, I made it
> optional because there is no point in discarding a LED if timers are
> missing.
>
> I'll update the documentation accordingly.
>
>>
>> Besides, I am wondering if we shouldn't check if the values read are
>> sane? In such a case initializing delay_on and delay_off to 0 would be
>> useful. We could check if both delays don't equal 0, which could happen
>> if the of_property_read_u32_index returned negative value because of
>> providing values out of bounds or not numerical values.
>
> Well, here I didn't checked too much intentionally. IMO, calling
> of_property_count_u32_elems() is enough to make sure that the following
> u32 read will succeed. After that, there is not real way to check if a
> delay value is sane, or not. To make work an hardware timer, correct
> values have to be defined in the DT. If not, then this hardware timer
> will be either broken or not reachable. It is not really harmful.

OK, that makes sense.

> Now, I can see, that I have also missed a check on the timer "mode"
> value. And since mode is used as an array index, I think it is a little
> bit more serious...

Yeah, the mode needs to be validated.

> Dan, given that the patches adding DT support for the leds-netxbig
> driver have not been merged with Linux v4.1, I propose you drop your
> patch. Instead, I'll send a v2, trying to take into account all the
> comments here.
>
> Thanks for the review and the comments.
>
> Simon
>


-- 
Best Regards,
Jacek Anaszewski