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
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
