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