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@xxxxxxxxxx> 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
