* [bug report] iio: pressure: bmp280: Add triggered buffer support
@ 2024-08-15 11:22 Dan Carpenter
2024-08-17 14:48 ` Jonathan Cameron
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2024-08-15 11:22 UTC (permalink / raw)
To: Vasileios Amoiridis; +Cc: linux-iio
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)
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
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [bug report] iio: pressure: bmp280: Add triggered buffer support 2024-08-15 11:22 [bug report] iio: pressure: bmp280: Add triggered buffer support Dan Carpenter @ 2024-08-17 14:48 ` Jonathan Cameron 2024-08-19 20:34 ` Vasileios Amoiridis 0 siblings, 1 reply; 3+ messages in thread From: Jonathan Cameron @ 2024-08-17 14:48 UTC (permalink / raw) To: Dan Carpenter; +Cc: Vasileios Amoiridis, linux-iio 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 > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] iio: pressure: bmp280: Add triggered buffer support 2024-08-17 14:48 ` Jonathan Cameron @ 2024-08-19 20:34 ` Vasileios Amoiridis 0 siblings, 0 replies; 3+ messages in thread From: Vasileios Amoiridis @ 2024-08-19 20:34 UTC (permalink / raw) To: Jonathan Cameron; +Cc: Dan Carpenter, Vasileios Amoiridis, linux-iio 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 > > > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-08-19 20:34 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-08-15 11:22 [bug report] iio: pressure: bmp280: Add triggered buffer support Dan Carpenter 2024-08-17 14:48 ` Jonathan Cameron 2024-08-19 20:34 ` Vasileios Amoiridis
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox