Re: [bug report] iio: chemical: bme680: generalize read_*() functions

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Wed, 30 Oct 2024 19:40:02 +0300
Dan Carpenter <dan.carpenter@linaro.org> wrote:

> On Wed, Oct 30, 2024 at 05:36:31PM +0100, Vasileios Amoiridis wrote:
> > On Wed, Oct 30, 2024 at 01:47:20PM +0300, Dan Carpenter wrote:  
> > > On Wed, Oct 30, 2024 at 10:58:01AM +0100, Vasileios Amoiridis wrote:  
> > > > On Wed, Oct 30, 2024 at 12:26:13PM +0300, Dan Carpenter wrote:  
> > > > > Hello Vasileios Amoiridis,
> > > > > 
> > > > > Commit 9b4071ab8cbe ("iio: chemical: bme680: generalize read_*()
> > > > > functions") from Oct 21, 2024 (linux-next), leads to the following
> > > > > Smatch static checker warning:
> > > > > 
> > > > > 	drivers/iio/chemical/bme680_core.c:760 bme680_read_raw()
> > > > > 	warn: passing casted pointer '&chan_val' to 'bme680_read_temp()' 32 vs 16.
> > > > > 
> > > > > drivers/iio/chemical/bme680_core.c
> > > > >     738 static int bme680_read_raw(struct iio_dev *indio_dev,
> > > > >     739                            struct iio_chan_spec const *chan,
> > > > >     740                            int *val, int *val2, long mask)
> > > > >     741 {
> > > > >     742         struct bme680_data *data = iio_priv(indio_dev);
> > > > >     743         int chan_val, ret;
> > > > >     744 
> > > > >     745         guard(mutex)(&data->lock);
> > > > >     746 
> > > > >     747         /* set forced mode to trigger measurement */
> > > > >     748         ret = bme680_set_mode(data, true);
> > > > >     749         if (ret < 0)
> > > > >     750                 return ret;
> > > > >     751 
> > > > >     752         ret = bme680_wait_for_eoc(data);
> > > > >     753         if (ret)
> > > > >     754                 return ret;
> > > > >     755 
> > > > >     756         switch (mask) {
> > > > >     757         case IIO_CHAN_INFO_PROCESSED:
> > > > >     758                 switch (chan->type) {
> > > > >     759                 case IIO_TEMP:  
> > > > > --> 760                         ret = bme680_read_temp(data, (s16 *)&chan_val);  
> > > > > 
> > > > > The bme680_read_temp() function takes an s16 pointer but we're passing a s32.
> > > > > This will not work on big endian systems and even on little endian systems, we
> > > > > haven't initialized the last 16 bits of chan_val so it's an uninitialized
> > > > > variable bug.
> > > > >   
> > > > 
> > > > Hi Dan,
> > > > 
> > > > Thanks for letting me know! What I could do is instead of reusing the
> > > > int chan_val, I could use a local s16 temp_chan_val so there is no need
> > > > for typecasting here.  
> > > 
> > > That works.  Not a fan of the name though.  "temp" means "temperature" and "tmp"
> > > means "temporary".  chan_val16 perhaps?
> > > 
> > > regards,
> > > dan carpenter
> > >  
> > 
> > That's the reason I used temp in the name, because we are reading a
> > temperature channel in this line.  
> 
> Ha.  Okay.  Good then.
> 
> regards,
> dan carpenter
Yes, Vasilieos please spin a patch. I might fix it up directly first but a bit
short on time this week.

Jonathan

> 
>