* [bug report] staging:iio:sca3000 extract old event handling and move to poll for events from buffer
@ 2021-05-28 12:00 Dan Carpenter
2021-06-03 16:38 ` Jonathan Cameron
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2021-05-28 12:00 UTC (permalink / raw)
To: jic23; +Cc: linux-iio
Hello Jonathan Cameron,
The patch 25888dc51163: "staging:iio:sca3000 extract old event
handling and move to poll for events from buffer" from May 18, 2011,
leads to the following static checker warning:
drivers/iio/accel/sca3000.c:734 sca3000_read_raw()
warn: no-op. '((*val) << 19) >> 19'
drivers/iio/accel/sca3000.c
709 static int sca3000_read_raw(struct iio_dev *indio_dev,
710 struct iio_chan_spec const *chan,
711 int *val,
712 int *val2,
713 long mask)
714 {
715 struct sca3000_state *st = iio_priv(indio_dev);
716 int ret;
717 u8 address;
718
719 switch (mask) {
720 case IIO_CHAN_INFO_RAW:
721 mutex_lock(&st->lock);
722 if (chan->type == IIO_ACCEL) {
723 if (st->mo_det_use_count) {
724 mutex_unlock(&st->lock);
725 return -EBUSY;
726 }
727 address = sca3000_addresses[chan->address][0];
728 ret = sca3000_read_data_short(st, address, 2);
729 if (ret < 0) {
730 mutex_unlock(&st->lock);
731 return ret;
732 }
733 *val = (be16_to_cpup((__be16 *)st->rx) >> 3) & 0x1FFF;
734 *val = ((*val) << (sizeof(*val) * 8 - 13)) >>
^^^^^^^^^^^^^^^^^^^^^^^
735 (sizeof(*val) * 8 - 13);
^^^^^^^^^^^^^^^^^^^^^^^
This code works, but it relies on undefined behavior of left shift
overflow and it's very unsatisfying. Pretty sure there is a UBSan
warning for this at runtime.
736 } else {
737 /* get the temperature when available */
738 ret = sca3000_read_data_short(st,
739 SCA3000_REG_TEMP_MSB_ADDR,
740 2);
741 if (ret < 0) {
742 mutex_unlock(&st->lock);
743 return ret;
744 }
745 *val = ((st->rx[0] & 0x3F) << 3) |
746 ((st->rx[1] & 0xE0) >> 5);
747 }
748 mutex_unlock(&st->lock);
749 return IIO_VAL_INT;
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [bug report] staging:iio:sca3000 extract old event handling and move to poll for events from buffer
2021-05-28 12:00 [bug report] staging:iio:sca3000 extract old event handling and move to poll for events from buffer Dan Carpenter
@ 2021-06-03 16:38 ` Jonathan Cameron
2021-06-04 6:45 ` Dan Carpenter
2021-06-04 17:11 ` Andy Shevchenko
0 siblings, 2 replies; 4+ messages in thread
From: Jonathan Cameron @ 2021-06-03 16:38 UTC (permalink / raw)
To: Dan Carpenter; +Cc: jic23, linux-iio
On Fri, 28 May 2021 15:00:52 +0300
Dan Carpenter <dan.carpenter@oracle.com> wrote:
> Hello Jonathan Cameron,
>
> The patch 25888dc51163: "staging:iio:sca3000 extract old event
> handling and move to poll for events from buffer" from May 18, 2011,
> leads to the following static checker warning:
>
> drivers/iio/accel/sca3000.c:734 sca3000_read_raw()
> warn: no-op. '((*val) << 19) >> 19'
>
> drivers/iio/accel/sca3000.c
> 709 static int sca3000_read_raw(struct iio_dev *indio_dev,
> 710 struct iio_chan_spec const *chan,
> 711 int *val,
> 712 int *val2,
> 713 long mask)
> 714 {
> 715 struct sca3000_state *st = iio_priv(indio_dev);
> 716 int ret;
> 717 u8 address;
> 718
> 719 switch (mask) {
> 720 case IIO_CHAN_INFO_RAW:
> 721 mutex_lock(&st->lock);
> 722 if (chan->type == IIO_ACCEL) {
> 723 if (st->mo_det_use_count) {
> 724 mutex_unlock(&st->lock);
> 725 return -EBUSY;
> 726 }
> 727 address = sca3000_addresses[chan->address][0];
> 728 ret = sca3000_read_data_short(st, address, 2);
> 729 if (ret < 0) {
> 730 mutex_unlock(&st->lock);
> 731 return ret;
> 732 }
> 733 *val = (be16_to_cpup((__be16 *)st->rx) >> 3) & 0x1FFF;
> 734 *val = ((*val) << (sizeof(*val) * 8 - 13)) >>
> ^^^^^^^^^^^^^^^^^^^^^^^
> 735 (sizeof(*val) * 8 - 13);
> ^^^^^^^^^^^^^^^^^^^^^^^
>
> This code works, but it relies on undefined behavior of left shift
> overflow and it's very unsatisfying. Pretty sure there is a UBSan
> warning for this at runtime.
Thanks Dan. Looks like a slightly odd variant on open coded sign_extend32()
Should be fine to replace with
*val = sign_extend32(*val, 13);
What can I say, I wrote this a long time ago when I was young and stupid :)
Anyhow, I'll roll a patch to do that shortly.
Jonathan
>
> 736 } else {
> 737 /* get the temperature when available */
> 738 ret = sca3000_read_data_short(st,
> 739 SCA3000_REG_TEMP_MSB_ADDR,
> 740 2);
> 741 if (ret < 0) {
> 742 mutex_unlock(&st->lock);
> 743 return ret;
> 744 }
> 745 *val = ((st->rx[0] & 0x3F) << 3) |
> 746 ((st->rx[1] & 0xE0) >> 5);
> 747 }
> 748 mutex_unlock(&st->lock);
> 749 return IIO_VAL_INT;
>
> regards,
> dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [bug report] staging:iio:sca3000 extract old event handling and move to poll for events from buffer
2021-06-03 16:38 ` Jonathan Cameron
@ 2021-06-04 6:45 ` Dan Carpenter
2021-06-04 17:11 ` Andy Shevchenko
1 sibling, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2021-06-04 6:45 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: jic23, linux-iio
On Thu, Jun 03, 2021 at 05:38:42PM +0100, Jonathan Cameron wrote:
> On Fri, 28 May 2021 15:00:52 +0300
> Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> > Hello Jonathan Cameron,
> >
> > The patch 25888dc51163: "staging:iio:sca3000 extract old event
> > handling and move to poll for events from buffer" from May 18, 2011,
> > leads to the following static checker warning:
> >
> > drivers/iio/accel/sca3000.c:734 sca3000_read_raw()
> > warn: no-op. '((*val) << 19) >> 19'
> >
> > drivers/iio/accel/sca3000.c
> > 709 static int sca3000_read_raw(struct iio_dev *indio_dev,
> > 710 struct iio_chan_spec const *chan,
> > 711 int *val,
> > 712 int *val2,
> > 713 long mask)
> > 714 {
> > 715 struct sca3000_state *st = iio_priv(indio_dev);
> > 716 int ret;
> > 717 u8 address;
> > 718
> > 719 switch (mask) {
> > 720 case IIO_CHAN_INFO_RAW:
> > 721 mutex_lock(&st->lock);
> > 722 if (chan->type == IIO_ACCEL) {
> > 723 if (st->mo_det_use_count) {
> > 724 mutex_unlock(&st->lock);
> > 725 return -EBUSY;
> > 726 }
> > 727 address = sca3000_addresses[chan->address][0];
> > 728 ret = sca3000_read_data_short(st, address, 2);
> > 729 if (ret < 0) {
> > 730 mutex_unlock(&st->lock);
> > 731 return ret;
> > 732 }
> > 733 *val = (be16_to_cpup((__be16 *)st->rx) >> 3) & 0x1FFF;
> > 734 *val = ((*val) << (sizeof(*val) * 8 - 13)) >>
> > ^^^^^^^^^^^^^^^^^^^^^^^
> > 735 (sizeof(*val) * 8 - 13);
> > ^^^^^^^^^^^^^^^^^^^^^^^
> >
> > This code works, but it relies on undefined behavior of left shift
> > overflow and it's very unsatisfying. Pretty sure there is a UBSan
> > warning for this at runtime.
>
> Thanks Dan. Looks like a slightly odd variant on open coded sign_extend32()
> Should be fine to replace with
>
> *val = sign_extend32(*val, 13);
>
> What can I say, I wrote this a long time ago when I was young and stupid :)
I didn't think of sign_extend32()... I was forced to look at some of my
2009 code the other day and it was *so* bad. :P
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [bug report] staging:iio:sca3000 extract old event handling and move to poll for events from buffer
2021-06-03 16:38 ` Jonathan Cameron
2021-06-04 6:45 ` Dan Carpenter
@ 2021-06-04 17:11 ` Andy Shevchenko
1 sibling, 0 replies; 4+ messages in thread
From: Andy Shevchenko @ 2021-06-04 17:11 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: Dan Carpenter, jic23, linux-iio
On Thu, Jun 3, 2021 at 7:38 PM Jonathan Cameron <jic23@kernel.org> wrote:
> On Fri, 28 May 2021 15:00:52 +0300
> Dan Carpenter <dan.carpenter@oracle.com> wrote:
...
> > 733 *val = (be16_to_cpup((__be16 *)st->rx) >> 3) & 0x1FFF;
> > 734 *val = ((*val) << (sizeof(*val) * 8 - 13)) >>
> > ^^^^^^^^^^^^^^^^^^^^^^^
> > 735 (sizeof(*val) * 8 - 13);
> > ^^^^^^^^^^^^^^^^^^^^^^^
> >
> > This code works, but it relies on undefined behavior of left shift
> > overflow and it's very unsatisfying. Pretty sure there is a UBSan
> > warning for this at runtime.
>
> Thanks Dan. Looks like a slightly odd variant on open coded sign_extend32()
> Should be fine to replace with
>
> *val = sign_extend32(*val, 13);
Oh, indeed.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-06-04 17:12 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-28 12:00 [bug report] staging:iio:sca3000 extract old event handling and move to poll for events from buffer Dan Carpenter
2021-06-03 16:38 ` Jonathan Cameron
2021-06-04 6:45 ` Dan Carpenter
2021-06-04 17:11 ` Andy Shevchenko
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.