On Wed, 18 Jul 2018, Daniel Thompson wrote:
On Wed, Jul 18, 2018 at 04:55:44PM +0100, Lee Jones wrote:
On Wed, 18 Jul 2018, Daniel Thompson wrote:
On Wed, Jul 18, 2018 at 02:08:53PM +0100, Lee Jones wrote:
> > No, then we are back to the initial issue of num_steps > > potentially not > > being initialised. We really want both of_property_read_u32() to > > succeed AND num_steps to actually be set. > > I also think num_steps should be pre-initialised.
Yes, I guess it definitely does not hurt.
> Then it will only be set if of_property_read_u32() succeeds.
Yes, but we still need to check for both, the function not failing and num_steps to actually be non zero.
Why? You don't do anything differently if it fails.
Only if you initialize num_steps...
We should either initialize to zero and not worry about the return code[1] or we check the return code and not worry about initialization[2]. I don't think both are worthwhile.
Whilst initialization can fix this specific instance we generally avoid overusing it since it messes up static analysis and, in this instance, distance from declaration to use is >25 lines, hence current patchset.
Daniel.
[1] https://lkml.org/lkml/2018/7/16/399 [2] https://lkml.org/lkml/2018/7/16/1042
Or...
We check the return code and leave number
num_steps is uninitialized and stack allocated so it only has a valid value if of_property_read_u32() succeeds.
We can (and I originally did) fix the bug by initializing num_steps to 0 but its quite some distance between declaration and use so I accepted Marcel's counter proposal to check the return code instead.
Only checking the return value of of_property_read_u32() is also suitable.
I did think about that case... I concluded that it isn't wrong for a DT to set to this property to 0 (effectively meaning "no interpolated steps please").
If we take the branch when num_steps is zero we get a bunch of pointless housekeeping that amounts to no more than an extremely elaborate malloc/memcpy/free.
Yet in the latest patch, you do it anyway? Or have I misread it?