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