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? -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
