Re: [PATCH] iio: Fix uninitialized symbol 'ret'

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 31 Oct 2024 09:13:16 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> Hi Zicheng,
> 
> Thanks for the patch.
> 
> On 31/10/2024 03:45, Zicheng Qu wrote:
> > Initialize the variable ret at the time of declaration to prevent it from
> > being returned without a defined value. Fixes smatch warning:
> > drivers/iio/industrialio-gts-helper.c:256 gain_to_scaletables() error:
> > uninitialized symbol 'ret'.
> > 
> > Cc: stable@vger.kernel.org # v6.6+
> > Fixes: 38416c28e168 ("iio: light: Add gain-time-scale helpers")
> > Signed-off-by: Zicheng Qu <quzicheng@huawei.com>
> > ---
> >   drivers/iio/industrialio-gts-helper.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/industrialio-gts-helper.c b/drivers/iio/industrialio-gts-helper.c
> > index 59d7615c0f56..c5dc5b51693d 100644
> > --- a/drivers/iio/industrialio-gts-helper.c
> > +++ b/drivers/iio/industrialio-gts-helper.c
> > @@ -167,7 +167,7 @@ static int iio_gts_gain_cmp(const void *a, const void *b)
> >   
> >   static int gain_to_scaletables(struct iio_gts *gts, int **gains, int **scales)
> >   {
> > -	int ret, i, j, new_idx, time_idx;
> > +	int i, j, new_idx, time_idx, ret = 0;
> >   	int *all_gains;
> >   	size_t gain_bytes;
> >     
> 
> So, if I read it right, this handles a (corner) case where there is no 
> times given. I am not sure how well such use has been considered because 
> the point of GTS is helping out with cases where the gain and 
> integration time both impact to scale.
> 
> How do you see the benefits of the gts if there is no such shared impact 
> to scale? Sure the gts could still provide the 'standard table format' 
> to present the gains (or times), and conversions from the register 
> values to gains (or times), and perhaps the available scale table(s) - 
> but I suppose it also brings a lot of unused code and some 
> initialization overhead. (I have a vague feeling this was discussed with 
> Jonathan during the reviews).
> 
> Reason I am asking these questions is that I wonder if the usage should 
> be limited to cases where we have both gains and times? We could check 
> this in the iio_gts_sanity_check(). (And, I am actually a bit surprized 
> this check was not implemented).
> 
> Well, initialization fixes a potential bug here and does not really cost 
> much - so big thanks to you :)
> 
> Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>
Indeed I'm not convinced this is a a bug that can be hit, but it is
obviously good hardening so applied to the fixes togreg branch of iio.git.
Note I'd like a follow up to use __free() + early returns in this function.
Will reduce complexity and that last line will become a return 0;

> 
> Yours,
>   -- Matti Vaittinen
>