Re: [PATCH v2] iio: Fix uninitialized variable

[Yo-Jung Lin]

Hi Javier,

On Fri, Oct 11, 2024 at 8:31 PM Javier Carrasco
<javier.carrasco.cruz@xxxxxxxxx> wrote:
>
> On 11/10/2024 13:52, Yo-Jung (Leo) Lin wrote:
> > clang found that the "offset" in bmp580_trigger_handler doesn't get
> > initialized before access. Add proper initialization to this variable.
> >
> > Signed-off-by: Yo-Jung (Leo) Lin <0xff07@xxxxxxxxx>
> > ---
> > Change in v2:
> > - Make value initialization immediate before its first use.
> > - Link to v1: https://lore.kernel.org/all/20241011093752.30685-1-0xff07@xxxxxxxxx/
> >
> > ---
> >  drivers/iio/pressure/bmp280-core.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> > index f4df222ed0c3..682329f81886 100644
> > --- a/drivers/iio/pressure/bmp280-core.c
> > +++ b/drivers/iio/pressure/bmp280-core.c
> > @@ -2222,6 +2222,8 @@ static irqreturn_t bmp580_trigger_handler(int irq, void *p)
> >               goto out;
> >       }
> >
> > +     offset = 0;
> > +
> >       /* Pressure calculations */
> >       memcpy(&data->sensor_data[offset], &data->buf[3], 3);
> >
>
> That was a quick reply. I would recommend you to wait a little bit while
> the first version is under discussion.

It was my bad not waiting for enough feedback to finalize another
patch. I’ll definitely do better next time.

I feel that initializing it as sizeof(s32) in the beginning and not
using it until the second memcpy() only widens the gap between value
initialization and its first access, which doesn’t address
Shevchenko’s concern.

>
> I still see the offset thing a bit weird. data->sensor_data uses an
> offset to avoid hard-coded numbers, but for data->buf we do exactly
> that, in the very same lines.
>
> Setting offset to 0 to access the first element i.e. no offset required,
> and then adding the actual offset sizeof(s32), which could even be a
> const if the first access was to sensor_data[0], looks to verbose.

While that is also true, it’ll take a more general refactoring to make
it driver-wise consistent. Maybe that refactoring should be on top of
a fix, instead of being part of a fix.

>
> These things are of course not critical, and the proposed fix is
> definitely ok, but I am missing some consistency here.

Best,
Leo