Re: [bug report] staging:iio:sca3000 extract old event handling and move to poll for events from buffer

[Dan Carpenter <dan.carpenter@oracle.com>]

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