| Message ID | 20250309182100.1351128-2-jic23@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | IIO: Introduce iio_push_to_buffers_with_ts() taking an input buffer length argument. | expand |
On Sun, 2025-03-09 at 18:20 +0000, Jonathan Cameron wrote: > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Check that total_len argument against iio_dev->scan_bytes. > > The size needs to be at least as big as the scan. It can be larger, > which is typical if only part of fixed sized storage is used due to > a subset of channels being enabled. > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > include/linux/iio/buffer.h | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/include/linux/iio/buffer.h b/include/linux/iio/buffer.h > index 3b8d618bb3df..75d5e58b646b 100644 > --- a/include/linux/iio/buffer.h > +++ b/include/linux/iio/buffer.h > @@ -45,6 +45,19 @@ static inline int iio_push_to_buffers_with_timestamp(struct > iio_dev *indio_dev, > return iio_push_to_buffers(indio_dev, data); > } > > +static inline int iio_push_to_buffers_with_ts(struct iio_dev *indio_dev, > + void *data, size_t total_len, > + int64_t timestamp) > +{ Kind of a nitpick but what about data_len as the size relate to *data? > + if (total_len < indio_dev->scan_bytes) { Given this is to be called on a fastpath and that the above is clearly a bug, what do you think about unlikely(total_len < indio_dev->scan_bytes) for some micro optimization? Anyways, this looks like a nice API improvement to me! - Nuno Sá >
On Mon, 10 Mar 2025 08:34:50 +0000 Nuno Sá <noname.nuno@gmail.com> wrote: > On Sun, 2025-03-09 at 18:20 +0000, Jonathan Cameron wrote: > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > Check that total_len argument against iio_dev->scan_bytes. > > > > The size needs to be at least as big as the scan. It can be larger, > > which is typical if only part of fixed sized storage is used due to > > a subset of channels being enabled. > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > --- > > include/linux/iio/buffer.h | 13 +++++++++++++ > > 1 file changed, 13 insertions(+) > > > > diff --git a/include/linux/iio/buffer.h b/include/linux/iio/buffer.h > > index 3b8d618bb3df..75d5e58b646b 100644 > > --- a/include/linux/iio/buffer.h > > +++ b/include/linux/iio/buffer.h > > @@ -45,6 +45,19 @@ static inline int iio_push_to_buffers_with_timestamp(struct > > iio_dev *indio_dev, > > return iio_push_to_buffers(indio_dev, data); > > } > > > > +static inline int iio_push_to_buffers_with_ts(struct iio_dev *indio_dev, > > + void *data, size_t total_len, > > + int64_t timestamp) > > +{ > > Kind of a nitpick but what about data_len as the size relate to *data? Maybe data_total_len? I kind of want to make people wonder what the total is about and read the docs. Which I forgot to actually add :( This has always been a quirky, non obvious interface. > > > + if (total_len < indio_dev->scan_bytes) { > > Given this is to be called on a fastpath and that the above is clearly a bug, > what do you think about unlikely(total_len < indio_dev->scan_bytes) for some > micro optimization? Fair point. Probably never matters as any branch predictor ought to get this one quickly but it also acts as documentation so sure. > > Anyways, this looks like a nice API improvement to me! > > - Nuno Sá > >
On Mon, 2025-03-10 at 20:04 +0000, Jonathan Cameron wrote: > On Mon, 10 Mar 2025 08:34:50 +0000 > Nuno Sá <noname.nuno@gmail.com> wrote: > > > On Sun, 2025-03-09 at 18:20 +0000, Jonathan Cameron wrote: > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > > > Check that total_len argument against iio_dev->scan_bytes. > > > > > > The size needs to be at least as big as the scan. It can be larger, > > > which is typical if only part of fixed sized storage is used due to > > > a subset of channels being enabled. > > > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > --- > > > include/linux/iio/buffer.h | 13 +++++++++++++ > > > 1 file changed, 13 insertions(+) > > > > > > diff --git a/include/linux/iio/buffer.h b/include/linux/iio/buffer.h > > > index 3b8d618bb3df..75d5e58b646b 100644 > > > --- a/include/linux/iio/buffer.h > > > +++ b/include/linux/iio/buffer.h > > > @@ -45,6 +45,19 @@ static inline int iio_push_to_buffers_with_timestamp(struct > > > iio_dev *indio_dev, > > > return iio_push_to_buffers(indio_dev, data); > > > } > > > > > > +static inline int iio_push_to_buffers_with_ts(struct iio_dev *indio_dev, > > > + void *data, size_t total_len, > > > + int64_t timestamp) > > > +{ > > > > Kind of a nitpick but what about data_len as the size relate to *data? > Maybe data_total_len? I kind of want to make people wonder what the total Fine by me... It is starting to get a bit verbose but I guess still on the acceptable side of things. - Nuno Sá
diff --git a/include/linux/iio/buffer.h b/include/linux/iio/buffer.h index 3b8d618bb3df..75d5e58b646b 100644 --- a/include/linux/iio/buffer.h +++ b/include/linux/iio/buffer.h @@ -45,6 +45,19 @@ static inline int iio_push_to_buffers_with_timestamp(struct iio_dev *indio_dev, return iio_push_to_buffers(indio_dev, data); } +static inline int iio_push_to_buffers_with_ts(struct iio_dev *indio_dev, + void *data, size_t total_len, + int64_t timestamp) +{ + if (total_len < indio_dev->scan_bytes) { + dev_err(&indio_dev->dev, + "Undersized storage pushed to buffer\n"); + return -ENOSPC; + } + + return iio_push_to_buffers_with_timestamp(indio_dev, data, timestamp); +} + int iio_push_to_buffers_with_ts_unaligned(struct iio_dev *indio_dev, const void *data, size_t data_sz, int64_t timestamp);