* [bug report] iio: chemical: bme680: generalize read_*() functions
@ 2024-10-30 9:26 Dan Carpenter
2024-10-30 9:58 ` Vasileios Amoiridis
0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2024-10-30 9:26 UTC (permalink / raw)
To: Vasileios Amoiridis; +Cc: linux-iio
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.
761 if (ret)
762 return ret;
763
764 *val = chan_val * 10;
765 return IIO_VAL_INT;
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [bug report] iio: chemical: bme680: generalize read_*() functions 2024-10-30 9:26 [bug report] iio: chemical: bme680: generalize read_*() functions Dan Carpenter @ 2024-10-30 9:58 ` Vasileios Amoiridis 2024-10-30 10:25 ` Vasileios Amoiridis 2024-10-30 10:47 ` Dan Carpenter 0 siblings, 2 replies; 7+ messages in thread From: Vasileios Amoiridis @ 2024-10-30 9:58 UTC (permalink / raw) To: Dan Carpenter; +Cc: linux-iio 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. How does that sound to you Jonathan? Cheers, Vasilis > 761 if (ret) > 762 return ret; > 763 > 764 *val = chan_val * 10; > 765 return IIO_VAL_INT; > > regards, > dan carpenter ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] iio: chemical: bme680: generalize read_*() functions 2024-10-30 9:58 ` Vasileios Amoiridis @ 2024-10-30 10:25 ` Vasileios Amoiridis 2024-10-30 10:47 ` Dan Carpenter 1 sibling, 0 replies; 7+ messages in thread From: Vasileios Amoiridis @ 2024-10-30 10:25 UTC (permalink / raw) To: Dan Carpenter, jic23, vassilisamir; +Cc: linux-iio 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, Adding also Jonathan in the thread. > > > > 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. How does that sound to you Jonathan? > > Cheers, > Vasilis > > > 761 if (ret) > > 762 return ret; > > 763 > > 764 *val = chan_val * 10; > > 765 return IIO_VAL_INT; > > > > regards, > > dan carpenter ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] iio: chemical: bme680: generalize read_*() functions 2024-10-30 9:58 ` Vasileios Amoiridis 2024-10-30 10:25 ` Vasileios Amoiridis @ 2024-10-30 10:47 ` Dan Carpenter 2024-10-30 16:36 ` Vasileios Amoiridis 1 sibling, 1 reply; 7+ messages in thread From: Dan Carpenter @ 2024-10-30 10:47 UTC (permalink / raw) To: Vasileios Amoiridis; +Cc: linux-iio 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] iio: chemical: bme680: generalize read_*() functions 2024-10-30 10:47 ` Dan Carpenter @ 2024-10-30 16:36 ` Vasileios Amoiridis 2024-10-30 16:40 ` Dan Carpenter 0 siblings, 1 reply; 7+ messages in thread From: Vasileios Amoiridis @ 2024-10-30 16:36 UTC (permalink / raw) To: Dan Carpenter; +Cc: linux-iio 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. Cheers, Vasilis ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] iio: chemical: bme680: generalize read_*() functions 2024-10-30 16:36 ` Vasileios Amoiridis @ 2024-10-30 16:40 ` Dan Carpenter 2024-10-30 18:04 ` Jonathan Cameron 0 siblings, 1 reply; 7+ messages in thread From: Dan Carpenter @ 2024-10-30 16:40 UTC (permalink / raw) To: Vasileios Amoiridis; +Cc: linux-iio 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] iio: chemical: bme680: generalize read_*() functions 2024-10-30 16:40 ` Dan Carpenter @ 2024-10-30 18:04 ` Jonathan Cameron 0 siblings, 0 replies; 7+ messages in thread From: Jonathan Cameron @ 2024-10-30 18:04 UTC (permalink / raw) To: Dan Carpenter; +Cc: Vasileios Amoiridis, linux-iio 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 > > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-10-30 18:04 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-10-30 9:26 [bug report] iio: chemical: bme680: generalize read_*() functions Dan Carpenter 2024-10-30 9:58 ` Vasileios Amoiridis 2024-10-30 10:25 ` Vasileios Amoiridis 2024-10-30 10:47 ` Dan Carpenter 2024-10-30 16:36 ` Vasileios Amoiridis 2024-10-30 16:40 ` Dan Carpenter 2024-10-30 18:04 ` Jonathan Cameron
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.