Re: [bug report] iio: pressure: bmp280: Add triggered buffer support

[Vasileios Amoiridis <vassilisamir@gmail.com>]

On Sat, Aug 17, 2024 at 03:48:09PM +0100, Jonathan Cameron wrote:
> On Thu, 15 Aug 2024 14:22:12 +0300
> Dan Carpenter <dan.carpenter@linaro.org> wrote:
> 
> > Hello Vasileios Amoiridis,
> > 
> > Commit 80cd23f43ddc ("iio: pressure: bmp280: Add triggered buffer
> > support") from Jun 28, 2024 (linux-next), leads to the following (UNPUBLISHED)
> > Smatch static checker warning:
> > 
> > 	drivers/iio/pressure/bmp280-core.c:2206 bmp580_trigger_handler()
> > 	warn: not copying enough bytes for '&data->sensor_data[1]' (4 vs 3 bytes)
> > 
> 
> This is a fun one. 
> 
> The data is little endian whatever happens (that is advertised to userspace
> which gets to unwind that if it wants to, or just store the data as is),
> I think the code is functionally correct, but we shouldn't really be using
> s32 for sensor_data (can't use __le32 either though as it's actually __le24
> + 1 byte of padding.  As it stands the code splats a s64 over some of the s32
> entries anyway so there is size confusion going on as well.
> 
> Right option is probably to make it a u8 buffer that is 4 times larger
> and fix up the code to multiple current index by 4.
> 
> That will get away from any pretence that this is a 32 bit cpu endian
> value.
> 
> Vasileios, if you agree with that analysis then please spin a suitable
> patch.
> 
> Jonathan
> 
> 

Hi Dan, Jonathan,

The code was not tested on a big-endian machine so I cannot say with huge
confidence but not only it is shown to userspace as LE, I think I have seen
similar practices in other drivers as well (maybe these are bugs though...).

I totally understand why we should make it a u8 buf, and I feel it is going
to be quite trivial as well. I will prepare a patch before the weekend.

Cheers,
Vasilis

> > drivers/iio/pressure/bmp280-core.c
> >     2188 static irqreturn_t bmp580_trigger_handler(int irq, void *p)
> >     2189 {
> >     2190         struct iio_poll_func *pf = p;
> >     2191         struct iio_dev *indio_dev = pf->indio_dev;
> >     2192         struct bmp280_data *data = iio_priv(indio_dev);
> >     2193         int ret;
> >     2194 
> >     2195         guard(mutex)(&data->lock);
> >     2196 
> >     2197         /* Burst read data registers */
> >     2198         ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB,
> >     2199                                data->buf, BMP280_BURST_READ_BYTES);
> >     2200         if (ret) {
> >     2201                 dev_err(data->dev, "failed to burst read sensor data\n");
> >     2202                 goto out;
> >     2203         }
> >     2204 
> >     2205         /* Temperature calculations */
> > --> 2206         memcpy(&data->sensor_data[1], &data->buf[0], 3);  
> >                          ^^^^^^^^^^^^^^^^^^^^                 ^
> > sensor_data is an s32 type.  We're copying 3 bytes to it.  This can't  be
> > correct from an endian perspective.
> > 
> >     2207 
> >     2208         /* Pressure calculations */
> >     2209         memcpy(&data->sensor_data[0], &data->buf[3], 3);
> >                         ^^^^^^^^^^^^^^^^^^^^^                 ^
> > Same
> > 
> >     2210 
> >     2211         iio_push_to_buffers_with_timestamp(indio_dev, &data->sensor_data,
> >     2212                                            iio_get_time_ns(indio_dev));
> >     2213 
> >     2214 out:
> >     2215         iio_trigger_notify_done(indio_dev->trig);
> >     2216 
> >     2217         return IRQ_HANDLED;
> >     2218 }
> > 
> > regards,
> > dan carpenter
> > 
>