On Thu, 15 Aug 2024 14:22:12 +0300
Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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
>
