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

[Jonathan Cameron <jic23@kernel.org>]

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


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