| Message ID | 20210501172515.513486-2-jic23@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | IIO: Alignment fixes part 4 - bounce buffers for the hard cases. | expand |
On Sat, May 1, 2021 at 8:28 PM Jonathan Cameron <jic23@kernel.org> wrote: > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Whilst it is almost always possible to arrange for scan data to be > read directly into a buffer that is suitable for passing to > iio_push_to_buffers_with_timestamp(), there are a few places where > leading data needs to be skipped over. > > For these cases introduce a function that will allocate an appropriate > sized and aligned bounce buffer (if not already allocated) and copy > the unaligned data into that before calling > iio_push_to_buffers_with_timestamp() on the bounce buffer. > We tie the lifespace of this buffer to that of the iio_dev.dev > which should ensure no memory leaks occur. ... > +/** > + * iio_push_to_buffers_with_ts_na() - push to registered buffer, > + * no alignment or space requirements. > + * @indio_dev: iio_dev structure for device. > + * @data: channel data excluding the timestamp. > + * @data_sz: size of data. > + * @timestamp: timestamp for the sample data. > + * > + * This special variant of iio_push_to_buffers_with_timetamp() does > + * not require space for the timestamp, or 8 byte alignment of data. > + * It does however require an allocation on first call and additional > + * coppies on all calls, so should be avoided if possible copies > + */ I do not like the _na part in the name (My first impression was with a Timestamp that was not available, what?!). Can we spell it better? > +int iio_push_to_buffers_with_ts_na(struct iio_dev *indio_dev, > + const void *data, > + size_t data_sz, > + int64_t timestamp) > +{ > + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev); > + > + data_sz = min_t(size_t, indio_dev->scan_bytes, data_sz); > + if (iio_dev_opaque->bounce_buffer_size != indio_dev->scan_bytes) { > + iio_dev_opaque->bounce_buffer = > + devm_krealloc(&indio_dev->dev, > + iio_dev_opaque->bounce_buffer, Oh la la, foo = realloc(foo, ...) is 101 type of mistakes. Please, don't do this. > + indio_dev->scan_bytes, GFP_KERNEL); > + if (!iio_dev_opaque) > + return -ENOMEM; > + } > + memcpy(iio_dev_opaque->bounce_buffer, data, data_sz); > + return iio_push_to_buffers_with_timestamp(indio_dev, > + iio_dev_opaque->bounce_buffer, > + timestamp); > +}
On Sat, May 1, 2021 at 10:25 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Sat, May 1, 2021 at 8:28 PM Jonathan Cameron <jic23@kernel.org> wrote: ... > > + iio_dev_opaque->bounce_buffer = > > + devm_krealloc(&indio_dev->dev, > > + iio_dev_opaque->bounce_buffer, > > Oh la la, foo = realloc(foo, ...) is 101 type of mistakes. > Please, don't do this. > > > + indio_dev->scan_bytes, GFP_KERNEL); > > + if (!iio_dev_opaque) And this check... > > + return -ENOMEM; > > + }
> From: Jonathan Cameron <jic23@kernel.org> > Sent: Saturday, May 1, 2021 7:25 PM > To: linux-iio@vger.kernel.org > Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Subject: [RFC PATCH 1/4] iio: core: Introduce > iio_push_to_buffers_with_ts_na() for non aligned case. > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Whilst it is almost always possible to arrange for scan data to be > read directly into a buffer that is suitable for passing to > iio_push_to_buffers_with_timestamp(), there are a few places where > leading data needs to be skipped over. > > For these cases introduce a function that will allocate an appropriate > sized and aligned bounce buffer (if not already allocated) and copy > the unaligned data into that before calling > iio_push_to_buffers_with_timestamp() on the bounce buffer. > We tie the lifespace of this buffer to that of the iio_dev.dev > which should ensure no memory leaks occur. > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/iio/industrialio-buffer.c | 36 > +++++++++++++++++++++++++++++++ > include/linux/iio/buffer.h | 4 ++++ > include/linux/iio/iio-opaque.h | 4 ++++ > 3 files changed, 44 insertions(+) > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio- > buffer.c > index 9a8e16c7e9af..818dfaa73665 100644 > --- a/drivers/iio/industrialio-buffer.c > +++ b/drivers/iio/industrialio-buffer.c > @@ -1727,6 +1727,42 @@ int iio_push_to_buffers(struct iio_dev > *indio_dev, const void *data) > } > EXPORT_SYMBOL_GPL(iio_push_to_buffers); > > +/** > + * iio_push_to_buffers_with_ts_na() - push to registered buffer, > + * no alignment or space requirements. > + * @indio_dev: iio_dev structure for device. > + * @data: channel data excluding the timestamp. > + * @data_sz: size of data. > + * @timestamp: timestamp for the sample data. > + * > + * This special variant of iio_push_to_buffers_with_timetamp() does > + * not require space for the timestamp, or 8 byte alignment of data. > + * It does however require an allocation on first call and additional > + * coppies on all calls, so should be avoided if possible > + */ > +int iio_push_to_buffers_with_ts_na(struct iio_dev *indio_dev, > + const void *data, > + size_t data_sz, > + int64_t timestamp) > +{ > + struct iio_dev_opaque *iio_dev_opaque = > to_iio_dev_opaque(indio_dev); > + > + data_sz = min_t(size_t, indio_dev->scan_bytes, data_sz); I'm not really sure about this. Is it really a good idea to silently truncate the data if some erroneous driver calls this with data_sz > scan_bytes? (I don't think it's ever valid for data_sz to be bigger than scan_bytes) We might be discarding data that userland actually requested. So, I'm not sure if it would not be better to be strict here and return error, so that driver developers could catch this early on... data_sz < scan_bytes is also probably not valid as that might happen if some drivers call this without the scan elements properly aligned which will definitely be problematic... Naturally in this case we would have to take into account timestamp possible alignment fill bytes as it could be possible (and likely) that data_sz won't include those bits... Alternatively, we could be consistent with 'iio_push_to_buffers_with_timestamp()' and assume drivers play along in terms of alignment and size (so, removing the data_sz parameter)... My point is, if we are taking the data_sz parameter, maybe we should be pedantic about it... > + if (iio_dev_opaque->bounce_buffer_size != indio_dev- > >scan_bytes) { Where do we set ' bounce_buffer_size '? I guess we do not want to get here all the time... > + iio_dev_opaque->bounce_buffer = > + devm_krealloc(&indio_dev->dev, > + iio_dev_opaque->bounce_buffer, > + indio_dev->scan_bytes, > GFP_KERNEL); I'm also not a big fan of realloc so I prefer to avoid it unless really needed... Here, maybe we could just allocate this once during device/buffer initialization and allocate it for the maximum possible size, i.e all scan elements enabled... That could also make this API simpler to the point of actually be inlined... - Nuno Sá
On Sat, 1 May 2021 22:27:22 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Sat, May 1, 2021 at 10:25 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Sat, May 1, 2021 at 8:28 PM Jonathan Cameron <jic23@kernel.org> wrote: > > ... > > > > + iio_dev_opaque->bounce_buffer = > > > + devm_krealloc(&indio_dev->dev, > > > + iio_dev_opaque->bounce_buffer, > > > > Oh la la, foo = realloc(foo, ...) is 101 type of mistakes. > > Please, don't do this. > > > > > + indio_dev->scan_bytes, GFP_KERNEL); > > > + if (!iio_dev_opaque) > > And this check... Yup. Thoroughly broken. Hohum, but basic idea conveyed however bad the rfc was :( > > > > + return -ENOMEM; > > > + } >
On Sat, 1 May 2021 22:25:55 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Sat, May 1, 2021 at 8:28 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > Whilst it is almost always possible to arrange for scan data to be > > read directly into a buffer that is suitable for passing to > > iio_push_to_buffers_with_timestamp(), there are a few places where > > leading data needs to be skipped over. > > > > For these cases introduce a function that will allocate an appropriate > > sized and aligned bounce buffer (if not already allocated) and copy > > the unaligned data into that before calling > > iio_push_to_buffers_with_timestamp() on the bounce buffer. > > We tie the lifespace of this buffer to that of the iio_dev.dev > > which should ensure no memory leaks occur. > > ... > > > +/** > > + * iio_push_to_buffers_with_ts_na() - push to registered buffer, > > + * no alignment or space requirements. > > + * @indio_dev: iio_dev structure for device. > > + * @data: channel data excluding the timestamp. > > + * @data_sz: size of data. > > + * @timestamp: timestamp for the sample data. > > + * > > + * This special variant of iio_push_to_buffers_with_timetamp() does > > + * not require space for the timestamp, or 8 byte alignment of data. > > + * It does however require an allocation on first call and additional > > + * coppies on all calls, so should be avoided if possible > > copies One day I'll remember to actually spell check *sigh* > > > + */ > > I do not like the _na part in the name (My first impression was with a > Timestamp that was not available, what?!). Can we spell it better? I struggled with the naming. Ideally we'd have started with this as the iio_push_to_buffers_with_timestamp() and had an _aligned version for the existing case. Perhaps spend the characters and just make it _with_ts_unaligned() > > > +int iio_push_to_buffers_with_ts_na(struct iio_dev *indio_dev, > > + const void *data, > > + size_t data_sz, > > + int64_t timestamp) > > +{ > > + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev); > > + > > + data_sz = min_t(size_t, indio_dev->scan_bytes, data_sz); > > + if (iio_dev_opaque->bounce_buffer_size != indio_dev->scan_bytes) { > > > + iio_dev_opaque->bounce_buffer = > > + devm_krealloc(&indio_dev->dev, > > + iio_dev_opaque->bounce_buffer, > > Oh la la, foo = realloc(foo, ...) is 101 type of mistakes. > Please, don't do this. For realloc I'd agree because if the new allocation fails we'd just have lost the pointer, but with a managed case, I think we'll leave the original pointer alone from the point of view of the devm_ cleanup. The only exit paths of interest in devm_krealloc() are the ones where we are trying to allocate a new larger object (otherwise it either does nothing or it is just a call to devm_kmalloc(). The one on failure to find the original managed resource, so can't loose it because it wasn't there. Am I missing something? > > > + indio_dev->scan_bytes, GFP_KERNEL); > > + if (!iio_dev_opaque) > > + return -ENOMEM; As you observed this is clearly garbage. I should have sat on this patch for a day and at least reread it or ideally done some testing. Failing to set the bounce_buffer_size() definitely doesn't help either.. > > + } > > + memcpy(iio_dev_opaque->bounce_buffer, data, data_sz); > > + return iio_push_to_buffers_with_timestamp(indio_dev, > > + iio_dev_opaque->bounce_buffer, > > + timestamp); > > +} >
On Sun, 2 May 2021 09:10:56 +0000 "Sa, Nuno" <Nuno.Sa@analog.com> wrote: > > From: Jonathan Cameron <jic23@kernel.org> > > Sent: Saturday, May 1, 2021 7:25 PM > > To: linux-iio@vger.kernel.org > > Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Subject: [RFC PATCH 1/4] iio: core: Introduce > > iio_push_to_buffers_with_ts_na() for non aligned case. > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > Whilst it is almost always possible to arrange for scan data to be > > read directly into a buffer that is suitable for passing to > > iio_push_to_buffers_with_timestamp(), there are a few places where > > leading data needs to be skipped over. > > > > For these cases introduce a function that will allocate an appropriate > > sized and aligned bounce buffer (if not already allocated) and copy > > the unaligned data into that before calling > > iio_push_to_buffers_with_timestamp() on the bounce buffer. > > We tie the lifespace of this buffer to that of the iio_dev.dev > > which should ensure no memory leaks occur. > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > --- > > drivers/iio/industrialio-buffer.c | 36 > > +++++++++++++++++++++++++++++++ > > include/linux/iio/buffer.h | 4 ++++ > > include/linux/iio/iio-opaque.h | 4 ++++ > > 3 files changed, 44 insertions(+) > > > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio- > > buffer.c > > index 9a8e16c7e9af..818dfaa73665 100644 > > --- a/drivers/iio/industrialio-buffer.c > > +++ b/drivers/iio/industrialio-buffer.c > > @@ -1727,6 +1727,42 @@ int iio_push_to_buffers(struct iio_dev > > *indio_dev, const void *data) > > } > > EXPORT_SYMBOL_GPL(iio_push_to_buffers); > > > > +/** > > + * iio_push_to_buffers_with_ts_na() - push to registered buffer, > > + * no alignment or space requirements. > > + * @indio_dev: iio_dev structure for device. > > + * @data: channel data excluding the timestamp. > > + * @data_sz: size of data. > > + * @timestamp: timestamp for the sample data. > > + * > > + * This special variant of iio_push_to_buffers_with_timetamp() does > > + * not require space for the timestamp, or 8 byte alignment of data. > > + * It does however require an allocation on first call and additional > > + * coppies on all calls, so should be avoided if possible > > + */ > > +int iio_push_to_buffers_with_ts_na(struct iio_dev *indio_dev, > > + const void *data, > > + size_t data_sz, > > + int64_t timestamp) > > +{ > > + struct iio_dev_opaque *iio_dev_opaque = > > to_iio_dev_opaque(indio_dev); > > + > > + data_sz = min_t(size_t, indio_dev->scan_bytes, data_sz); > I'm not really sure about this. Is it really a good idea to silently truncate > the data if some erroneous driver calls this with data_sz > scan_bytes? > (I don't think it's ever valid for data_sz to be bigger than scan_bytes) It's not probably not. This is just a fairly nasty way of papering over anyone doing that. > We might be discarding data that userland actually requested. So, I'm > not sure if it would not be better to be strict here and return error, so that > driver developers could catch this early on... I'm fairly sure we'll never be discarding data userspace requested because scan_bytes will be the maximum possible and is the size of the kfifo element. Any more data that comes in is going to be dropped anyway assuming it ends up in a kfifo_put() I wondered if we wanted to enforce this? It would not seem totally silly for a driver that wasn't bothering to track the size of it's enabled channels to provide data_sz == maximum size it ever needs. Docs should definitely say if that's valid however, and it's definitely less than optimal because we may copy garbage that will then be overwritten by the timestamp. > > data_sz < scan_bytes is also probably not valid as that might happen > if some drivers call this without the scan elements properly aligned > which will definitely be problematic... Naturally in this case we would > have to take into account timestamp possible alignment fill bytes as it > could be possible (and likely) that data_sz won't include those bits... data_sz < scan_bytes is intentionally valid (and indeed I made one of the drivers do this). Given we are bouncing anyway, why insist the driver provides space for padding + timestamp? That can be done for the bounce buffer. > > Alternatively, we could be consistent with 'iio_push_to_buffers_with_timestamp()' > and assume drivers play along in terms of alignment and size > (so, removing the data_sz parameter)... > > My point is, if we are taking the data_sz parameter, maybe we should be pedantic > about it... > > + if (iio_dev_opaque->bounce_buffer_size != indio_dev- > > >scan_bytes) { > > Where do we set ' bounce_buffer_size '? I guess we do not want to > get here all the time... :( oops > > + iio_dev_opaque->bounce_buffer = > > + devm_krealloc(&indio_dev->dev, > > + iio_dev_opaque->bounce_buffer, > > + indio_dev->scan_bytes, > > GFP_KERNEL); > > I'm also not a big fan of realloc so I prefer to avoid it unless really needed... > Here, maybe we could just allocate this once during device/buffer initialization > and allocate it for the maximum possible size, i.e all scan elements enabled... We can but to do that requires more invasive changes. Perhaps not horrible ones, but we'd need to add stuff to caller drivers to configure the bounce buffer, or some flag to see they needed to enable support for this. Only 3 drivers out of all those in IIO are currently using this code (might be a few others that could take advantage but not many - so need to keep it localized). I suppose we could have an iio_unaligned_push_to_buffers_prepare() to be called from the update_scan_mode() callback (typed to be useable as that callback if nothing else to be done in the relevant drivers. > That could also make this API simpler to the point of actually be inlined... Can't make it inline without making the bounce buffer visible to the driver and I don't want the driver to be able to see it as it adds another place for things to be done wrongly in drivers. Any driver doing this is having to deal with a bounce buffer anyway, so the indirection doesn't worry me too much. > > - Nuno Sá Thanks, Jonathan
> -----Original Message----- > From: Jonathan Cameron <jic23@kernel.org> > Sent: Sunday, May 2, 2021 6:09 PM > To: Sa, Nuno <Nuno.Sa@analog.com> > Cc: linux-iio@vger.kernel.org; Jonathan Cameron > <Jonathan.Cameron@huawei.com> > Subject: Re: [RFC PATCH 1/4] iio: core: Introduce > iio_push_to_buffers_with_ts_na() for non aligned case. > > > On Sun, 2 May 2021 09:10:56 +0000 > "Sa, Nuno" <Nuno.Sa@analog.com> wrote: > > > > From: Jonathan Cameron <jic23@kernel.org> > > > Sent: Saturday, May 1, 2021 7:25 PM > > > To: linux-iio@vger.kernel.org > > > Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > Subject: [RFC PATCH 1/4] iio: core: Introduce > > > iio_push_to_buffers_with_ts_na() for non aligned case. > > > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > > > Whilst it is almost always possible to arrange for scan data to be > > > read directly into a buffer that is suitable for passing to > > > iio_push_to_buffers_with_timestamp(), there are a few places > where > > > leading data needs to be skipped over. > > > > > > For these cases introduce a function that will allocate an > appropriate > > > sized and aligned bounce buffer (if not already allocated) and copy > > > the unaligned data into that before calling > > > iio_push_to_buffers_with_timestamp() on the bounce buffer. > > > We tie the lifespace of this buffer to that of the iio_dev.dev > > > which should ensure no memory leaks occur. > > > > > > Signed-off-by: Jonathan Cameron > <Jonathan.Cameron@huawei.com> > > > --- > > > drivers/iio/industrialio-buffer.c | 36 > > > +++++++++++++++++++++++++++++++ > > > include/linux/iio/buffer.h | 4 ++++ > > > include/linux/iio/iio-opaque.h | 4 ++++ > > > 3 files changed, 44 insertions(+) > > > > > > diff --git a/drivers/iio/industrialio-buffer.c > b/drivers/iio/industrialio- > > > buffer.c > > > index 9a8e16c7e9af..818dfaa73665 100644 > > > --- a/drivers/iio/industrialio-buffer.c > > > +++ b/drivers/iio/industrialio-buffer.c > > > @@ -1727,6 +1727,42 @@ int iio_push_to_buffers(struct iio_dev > > > *indio_dev, const void *data) > > > } > > > EXPORT_SYMBOL_GPL(iio_push_to_buffers); > > > > > > +/** > > > + * iio_push_to_buffers_with_ts_na() - push to registered buffer, > > > + * no alignment or space requirements. > > > + * @indio_dev: iio_dev structure for device. > > > + * @data: channel data excluding the timestamp. > > > + * @data_sz: size of data. > > > + * @timestamp: timestamp for the sample data. > > > + * > > > + * This special variant of iio_push_to_buffers_with_timetamp() > does > > > + * not require space for the timestamp, or 8 byte alignment of > data. > > > + * It does however require an allocation on first call and additional > > > + * coppies on all calls, so should be avoided if possible > > > + */ > > > +int iio_push_to_buffers_with_ts_na(struct iio_dev *indio_dev, > > > + const void *data, > > > + size_t data_sz, > > > + int64_t timestamp) > > > +{ > > > + struct iio_dev_opaque *iio_dev_opaque = > > > to_iio_dev_opaque(indio_dev); > > > + > > > + data_sz = min_t(size_t, indio_dev->scan_bytes, data_sz); > > I'm not really sure about this. Is it really a good idea to silently > truncate > > the data if some erroneous driver calls this with data_sz > > scan_bytes? > > (I don't think it's ever valid for data_sz to be bigger than scan_bytes) > > It's not probably not. This is just a fairly nasty way of papering over > anyone doing that. > > > We might be discarding data that userland actually requested. So, I'm > > not sure if it would not be better to be strict here and return error, > so that > > driver developers could catch this early on... > > I'm fairly sure we'll never be discarding data userspace requested > because scan_bytes will be the maximum possible and is the size of > the > kfifo element. Any more data that comes in is going to be dropped > anyway > assuming it ends up in a kfifo_put() I think it depends. Think on a totally erroneous driver that does not track enabled elements and just pushes all elements to IIO buffers. If userspace just requested, let's say, the last 2 elements, I believe those would be definitely discarded by this... More subtle things could also happen I guess... In this case, we can arguably say that the driver is completely broken and while I'm fairly sure such a scenario would never pass through revision, I think that if we make things more strict, we might minimize the chances of such patches to appear in the first place (unless someone is sending patches without testing them at all). > I wondered if we wanted to enforce this? It would not seem totally > silly for a driver that wasn't bothering to track the size of it's enabled > channels to provide data_sz == maximum size it ever needs. > Docs should definitely say if that's valid however, and it's definitely less > than optimal because we may copy garbage that will then be > overwritten by > the timestamp. > > > > > data_sz < scan_bytes is also probably not valid as that might happen > > if some drivers call this without the scan elements properly aligned > > which will definitely be problematic... Naturally in this case we would > > have to take into account timestamp possible alignment fill bytes as it > > could be possible (and likely) that data_sz won't include those bits... > > data_sz < scan_bytes is intentionally valid (and indeed I made one > of the drivers do this). Given we are bouncing anyway, why insist the > driver provides space for padding + timestamp? That can be done for > the bounce buffer. What I meant was that if data_sz < scan_bytes - timestamp - padding, then the driver is most likely not guaranteeing properly aligned elements. I was not that clear :) Anyways, I still feel that if we are taking the data_sz parameter on the API, we should handle it more strictly. But I do not feel too strong about this (as you said, It's not expected to exist too much users of this) and in the end of the day it still behaves the same way as 'iio_push_to_buffers_with_timestamp()'... > > > > Alternatively, we could be consistent with > 'iio_push_to_buffers_with_timestamp()' > > and assume drivers play along in terms of alignment and size > > (so, removing the data_sz parameter)... > > > > My point is, if we are taking the data_sz parameter, maybe we > should be pedantic > > about it... > > > + if (iio_dev_opaque->bounce_buffer_size != indio_dev- > > > >scan_bytes) { > > > > Where do we set ' bounce_buffer_size '? I guess we do not want to > > get here all the time... > > :( oops > > > > + iio_dev_opaque->bounce_buffer = > > > + devm_krealloc(&indio_dev->dev, > > > + iio_dev_opaque->bounce_buffer, > > > + indio_dev->scan_bytes, > > > GFP_KERNEL); > > > > I'm also not a big fan of realloc so I prefer to avoid it unless really > needed... > > Here, maybe we could just allocate this once during device/buffer > initialization > > and allocate it for the maximum possible size, i.e all scan elements > enabled... > > We can but to do that requires more invasive changes. Perhaps not > horrible ones, > but we'd need to add stuff to caller drivers to configure the bounce > buffer, or > some flag to see they needed to enable support for this. Only 3 > drivers out of > all those in IIO are currently using this code (might be a few others that > could > take advantage but not many - so need to keep it localized). I was not thinking that way. I was just thinking in always allocating the bounce buffer. But I do agree that it is less than ideal to allocate something that will almost never be used... > I suppose we could have an > iio_unaligned_push_to_buffers_prepare() to be called from the > update_scan_mode() > callback (typed to be useable as that callback if nothing else to be done > in > the relevant drivers. It could also be an option... If we want to not use realloc, we would have to manually control the lifespan of the buffer and free it in the disable path. But I guess that might feel like too much handling... Anyways, maybe sticking with realloc, we could just use a more normal pattern like: tmp = devm_realloc(bounce_fuffer, ...); if (!tmp) error; bounce_buffer = tmp; - Nuno Sá
> -----Original Message----- > From: Sa, Nuno <Nuno.Sa@analog.com> > Sent: Monday, May 3, 2021 9:15 AM > To: Jonathan Cameron <jic23@kernel.org> > Cc: linux-iio@vger.kernel.org; Jonathan Cameron > <Jonathan.Cameron@huawei.com> > Subject: RE: [RFC PATCH 1/4] iio: core: Introduce > iio_push_to_buffers_with_ts_na() for non aligned case. > > > > > > -----Original Message----- > > From: Jonathan Cameron <jic23@kernel.org> > > Sent: Sunday, May 2, 2021 6:09 PM > > To: Sa, Nuno <Nuno.Sa@analog.com> > > Cc: linux-iio@vger.kernel.org; Jonathan Cameron > > <Jonathan.Cameron@huawei.com> > > Subject: Re: [RFC PATCH 1/4] iio: core: Introduce > > iio_push_to_buffers_with_ts_na() for non aligned case. > > > > > > On Sun, 2 May 2021 09:10:56 +0000 > > "Sa, Nuno" <Nuno.Sa@analog.com> wrote: > > > > > > From: Jonathan Cameron <jic23@kernel.org> > > > > Sent: Saturday, May 1, 2021 7:25 PM > > > > To: linux-iio@vger.kernel.org > > > > Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > Subject: [RFC PATCH 1/4] iio: core: Introduce > > > > iio_push_to_buffers_with_ts_na() for non aligned case. > > > > > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > > > > > Whilst it is almost always possible to arrange for scan data to be > > > > read directly into a buffer that is suitable for passing to > > > > iio_push_to_buffers_with_timestamp(), there are a few places > > where > > > > leading data needs to be skipped over. > > > > > > > > For these cases introduce a function that will allocate an > > appropriate > > > > sized and aligned bounce buffer (if not already allocated) and > copy > > > > the unaligned data into that before calling > > > > iio_push_to_buffers_with_timestamp() on the bounce buffer. > > > > We tie the lifespace of this buffer to that of the iio_dev.dev > > > > which should ensure no memory leaks occur. > > > > > > > > Signed-off-by: Jonathan Cameron > > <Jonathan.Cameron@huawei.com> > > > > --- > > > > drivers/iio/industrialio-buffer.c | 36 > > > > +++++++++++++++++++++++++++++++ > > > > include/linux/iio/buffer.h | 4 ++++ > > > > include/linux/iio/iio-opaque.h | 4 ++++ > > > > 3 files changed, 44 insertions(+) > > > > > > > > diff --git a/drivers/iio/industrialio-buffer.c > > b/drivers/iio/industrialio- > > > > buffer.c > > > > index 9a8e16c7e9af..818dfaa73665 100644 > > > > --- a/drivers/iio/industrialio-buffer.c > > > > +++ b/drivers/iio/industrialio-buffer.c > > > > @@ -1727,6 +1727,42 @@ int iio_push_to_buffers(struct iio_dev > > > > *indio_dev, const void *data) > > > > } > > > > EXPORT_SYMBOL_GPL(iio_push_to_buffers); > > > > > > > > +/** > > > > + * iio_push_to_buffers_with_ts_na() - push to registered > buffer, > > > > + * no alignment or space requirements. > > > > + * @indio_dev: iio_dev structure for device. > > > > + * @data: channel data excluding the timestamp. > > > > + * @data_sz: size of data. > > > > + * @timestamp: timestamp for the sample data. > > > > + * > > > > + * This special variant of iio_push_to_buffers_with_timetamp() > > does > > > > + * not require space for the timestamp, or 8 byte alignment of > > data. > > > > + * It does however require an allocation on first call and > additional > > > > + * coppies on all calls, so should be avoided if possible > > > > + */ > > > > +int iio_push_to_buffers_with_ts_na(struct iio_dev *indio_dev, > > > > + const void *data, > > > > + size_t data_sz, > > > > + int64_t timestamp) > > > > +{ > > > > + struct iio_dev_opaque *iio_dev_opaque = > > > > to_iio_dev_opaque(indio_dev); > > > > + > > > > + data_sz = min_t(size_t, indio_dev->scan_bytes, data_sz); > > > I'm not really sure about this. Is it really a good idea to silently > > truncate > > > the data if some erroneous driver calls this with data_sz > > > scan_bytes? > > > (I don't think it's ever valid for data_sz to be bigger than > scan_bytes) > > > > It's not probably not. This is just a fairly nasty way of papering over > > anyone doing that. > > > > > We might be discarding data that userland actually requested. So, > I'm > > > not sure if it would not be better to be strict here and return error, > > so that > > > driver developers could catch this early on... > > > > I'm fairly sure we'll never be discarding data userspace requested > > because scan_bytes will be the maximum possible and is the size of > > the > > kfifo element. Any more data that comes in is going to be dropped > > anyway > > assuming it ends up in a kfifo_put() > > I think it depends. Think on a totally erroneous driver that does not > track > enabled elements and just pushes all elements to IIO buffers. If > userspace > just requested, let's say, the last 2 elements, I believe those would be > definitely discarded by this... More subtle things could also happen I > guess... > In this case, we can arguably say that the driver is completely broken > and while > I'm fairly sure such a scenario would never pass through revision, I > think that > if we make things more strict, we might minimize the chances of such > patches > to appear in the first place (unless someone is sending patches without > testing them at all). > > > I wondered if we wanted to enforce this? It would not seem totally > > silly for a driver that wasn't bothering to track the size of it's enabled > > channels to provide data_sz == maximum size it ever needs. > > Docs should definitely say if that's valid however, and it's definitely > less > > than optimal because we may copy garbage that will then be > > overwritten by > > the timestamp. > > > > > > > > data_sz < scan_bytes is also probably not valid as that might > happen > > > if some drivers call this without the scan elements properly aligned > > > which will definitely be problematic... Naturally in this case we > would > > > have to take into account timestamp possible alignment fill bytes as > it > > > could be possible (and likely) that data_sz won't include those > bits... > > > > data_sz < scan_bytes is intentionally valid (and indeed I made one > > of the drivers do this). Given we are bouncing anyway, why insist > the > > driver provides space for padding + timestamp? That can be done > for > > the bounce buffer. > > What I meant was that if data_sz < scan_bytes - timestamp - padding, > then > the driver is most likely not guaranteeing properly aligned elements. I > was > not that clear :) > > Anyways, I still feel that if we are taking the data_sz parameter on the > API, > we should handle it more strictly. But I do not feel too strong about > this (as > you said, It's not expected to exist too much users of this) and in the > end > of the day it still behaves the same way as > 'iio_push_to_buffers_with_timestamp()'... > > > > > > > Alternatively, we could be consistent with > > 'iio_push_to_buffers_with_timestamp()' > > > and assume drivers play along in terms of alignment and size > > > (so, removing the data_sz parameter)... > > > > > > My point is, if we are taking the data_sz parameter, maybe we > > should be pedantic > > > about it... > > > > + if (iio_dev_opaque->bounce_buffer_size != indio_dev- > > > > >scan_bytes) { > > > > > > Where do we set ' bounce_buffer_size '? I guess we do not want > to > > > get here all the time... > > > > :( oops > > > > > > + iio_dev_opaque->bounce_buffer = > > > > + devm_krealloc(&indio_dev->dev, > > > > + iio_dev_opaque->bounce_buffer, > > > > + indio_dev->scan_bytes, > > > > GFP_KERNEL); > > > > > > I'm also not a big fan of realloc so I prefer to avoid it unless really > > needed... > > > Here, maybe we could just allocate this once during device/buffer > > initialization > > > and allocate it for the maximum possible size, i.e all scan elements > > enabled... > > > > We can but to do that requires more invasive changes. Perhaps not > > horrible ones, > > but we'd need to add stuff to caller drivers to configure the bounce > > buffer, or > > some flag to see they needed to enable support for this. Only 3 > > drivers out of > > all those in IIO are currently using this code (might be a few others > that > > could > > take advantage but not many - so need to keep it localized). > > I was not thinking that way. I was just thinking in always allocating the > bounce > buffer. But I do agree that it is less than ideal to allocate something > that will > almost never be used... > > > I suppose we could have an > > iio_unaligned_push_to_buffers_prepare() to be called from the > > update_scan_mode() > > callback (typed to be useable as that callback if nothing else to be > done > > in > > the relevant drivers. > > It could also be an option... If we want to not use realloc, we would > have to manually > control the lifespan of the buffer and free it in the disable path. But I > guess that might feel > like too much handling... > > Anyways, maybe sticking with realloc, we could just use a more normal > pattern like: > > tmp = devm_realloc(bounce_fuffer, ...); > if (!tmp) > error; > > bounce_buffer = tmp; > I just read your reply to Andy about this and I think you are right... If the new allocation fails, the older pointer is still preserved. However, I find the above form a bit more readable and straight :) - Nuno Sá
On Mon, May 3, 2021 at 10:47 AM Sa, Nuno <Nuno.Sa@analog.com> wrote: > > From: Sa, Nuno <Nuno.Sa@analog.com> > > Sent: Monday, May 3, 2021 9:15 AM ... > > Anyways, maybe sticking with realloc, we could just use a more normal > > pattern like: > > > > tmp = devm_realloc(bounce_fuffer, ...); > > if (!tmp) > > error; > > > > bounce_buffer = tmp; > > > > I just read your reply to Andy about this and I think you are right... If the > new allocation fails, the older pointer is still preserved. However, I find > the above form a bit more readable and straight :) You beat me up to it. While devm_realloc() may actually be safer, the pattern is that: we should not rely on specifics of the custom implementation, it will really confuse new contributors or people who may take this as a "good' (which is not) pattern.
On Mon, 3 May 2021 07:46:49 +0000 "Sa, Nuno" <Nuno.Sa@analog.com> wrote: > > -----Original Message----- > > From: Sa, Nuno <Nuno.Sa@analog.com> > > Sent: Monday, May 3, 2021 9:15 AM > > To: Jonathan Cameron <jic23@kernel.org> > > Cc: linux-iio@vger.kernel.org; Jonathan Cameron > > <Jonathan.Cameron@huawei.com> > > Subject: RE: [RFC PATCH 1/4] iio: core: Introduce > > iio_push_to_buffers_with_ts_na() for non aligned case. > > > > > > > > > > > -----Original Message----- > > > From: Jonathan Cameron <jic23@kernel.org> > > > Sent: Sunday, May 2, 2021 6:09 PM > > > To: Sa, Nuno <Nuno.Sa@analog.com> > > > Cc: linux-iio@vger.kernel.org; Jonathan Cameron > > > <Jonathan.Cameron@huawei.com> > > > Subject: Re: [RFC PATCH 1/4] iio: core: Introduce > > > iio_push_to_buffers_with_ts_na() for non aligned case. > > > > > > > > > On Sun, 2 May 2021 09:10:56 +0000 > > > "Sa, Nuno" <Nuno.Sa@analog.com> wrote: > > > > > > > > From: Jonathan Cameron <jic23@kernel.org> > > > > > Sent: Saturday, May 1, 2021 7:25 PM > > > > > To: linux-iio@vger.kernel.org > > > > > Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > > Subject: [RFC PATCH 1/4] iio: core: Introduce > > > > > iio_push_to_buffers_with_ts_na() for non aligned case. > > > > > > > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > > > > > > > Whilst it is almost always possible to arrange for scan data to be > > > > > read directly into a buffer that is suitable for passing to > > > > > iio_push_to_buffers_with_timestamp(), there are a few places > > > where > > > > > leading data needs to be skipped over. > > > > > > > > > > For these cases introduce a function that will allocate an > > > appropriate > > > > > sized and aligned bounce buffer (if not already allocated) and > > copy > > > > > the unaligned data into that before calling > > > > > iio_push_to_buffers_with_timestamp() on the bounce buffer. > > > > > We tie the lifespace of this buffer to that of the iio_dev.dev > > > > > which should ensure no memory leaks occur. > > > > > > > > > > Signed-off-by: Jonathan Cameron > > > <Jonathan.Cameron@huawei.com> > > > > > --- > > > > > drivers/iio/industrialio-buffer.c | 36 > > > > > +++++++++++++++++++++++++++++++ > > > > > include/linux/iio/buffer.h | 4 ++++ > > > > > include/linux/iio/iio-opaque.h | 4 ++++ > > > > > 3 files changed, 44 insertions(+) > > > > > > > > > > diff --git a/drivers/iio/industrialio-buffer.c > > > b/drivers/iio/industrialio- > > > > > buffer.c > > > > > index 9a8e16c7e9af..818dfaa73665 100644 > > > > > --- a/drivers/iio/industrialio-buffer.c > > > > > +++ b/drivers/iio/industrialio-buffer.c > > > > > @@ -1727,6 +1727,42 @@ int iio_push_to_buffers(struct iio_dev > > > > > *indio_dev, const void *data) > > > > > } > > > > > EXPORT_SYMBOL_GPL(iio_push_to_buffers); > > > > > > > > > > +/** > > > > > + * iio_push_to_buffers_with_ts_na() - push to registered > > buffer, > > > > > + * no alignment or space requirements. > > > > > + * @indio_dev: iio_dev structure for device. > > > > > + * @data: channel data excluding the timestamp. > > > > > + * @data_sz: size of data. > > > > > + * @timestamp: timestamp for the sample data. > > > > > + * > > > > > + * This special variant of iio_push_to_buffers_with_timetamp() > > > does > > > > > + * not require space for the timestamp, or 8 byte alignment of > > > data. > > > > > + * It does however require an allocation on first call and > > additional > > > > > + * coppies on all calls, so should be avoided if possible > > > > > + */ > > > > > +int iio_push_to_buffers_with_ts_na(struct iio_dev *indio_dev, > > > > > + const void *data, > > > > > + size_t data_sz, > > > > > + int64_t timestamp) > > > > > +{ > > > > > + struct iio_dev_opaque *iio_dev_opaque = > > > > > to_iio_dev_opaque(indio_dev); > > > > > + > > > > > + data_sz = min_t(size_t, indio_dev->scan_bytes, data_sz); > > > > I'm not really sure about this. Is it really a good idea to silently > > > truncate > > > > the data if some erroneous driver calls this with data_sz > > > > scan_bytes? > > > > (I don't think it's ever valid for data_sz to be bigger than > > scan_bytes) > > > > > > It's not probably not. This is just a fairly nasty way of papering over > > > anyone doing that. > > > > > > > We might be discarding data that userland actually requested. So, > > I'm > > > > not sure if it would not be better to be strict here and return error, > > > so that > > > > driver developers could catch this early on... > > > > > > I'm fairly sure we'll never be discarding data userspace requested > > > because scan_bytes will be the maximum possible and is the size of > > > the > > > kfifo element. Any more data that comes in is going to be dropped > > > anyway > > > assuming it ends up in a kfifo_put() > > > > I think it depends. Think on a totally erroneous driver that does not > > track > > enabled elements and just pushes all elements to IIO buffers. If > > userspace > > just requested, let's say, the last 2 elements, I believe those would be > > definitely discarded by this... More subtle things could also happen I > > guess... > > In this case, we can arguably say that the driver is completely broken > > and while > > I'm fairly sure such a scenario would never pass through revision, I > > think that > > if we make things more strict, we might minimize the chances of such > > patches > > to appear in the first place (unless someone is sending patches without > > testing them at all). Like me ;) *laughs* Agreed there is nothing stopping at least adding a warning print here and it might help weed out gross bugs even if it is imprecise. > > > > > I wondered if we wanted to enforce this? It would not seem totally > > > silly for a driver that wasn't bothering to track the size of it's enabled > > > channels to provide data_sz == maximum size it ever needs. > > > Docs should definitely say if that's valid however, and it's definitely > > less > > > than optimal because we may copy garbage that will then be > > > overwritten by > > > the timestamp. > > > > > > > > > > > data_sz < scan_bytes is also probably not valid as that might > > happen > > > > if some drivers call this without the scan elements properly aligned > > > > which will definitely be problematic... Naturally in this case we > > would > > > > have to take into account timestamp possible alignment fill bytes as > > it > > > > could be possible (and likely) that data_sz won't include those > > bits... > > > > > > data_sz < scan_bytes is intentionally valid (and indeed I made one > > > of the drivers do this). Given we are bouncing anyway, why insist > > the > > > driver provides space for padding + timestamp? That can be done > > for > > > the bounce buffer. > > > > What I meant was that if data_sz < scan_bytes - timestamp - padding, > > then > > the driver is most likely not guaranteeing properly aligned elements. I > > was > > not that clear :) If you hit that condition I think it would be a bug as not enough data provided. We could easily enforce data_sz < scan_bytes - timstamp, but the padding would need to be stashed somewhere. + I've been lazy in one of the drivers because I couldn't figure it out in a trivial fashion - perhaps I should take another look at that one and see if I can make the check accurate rather than a worst case bound. > > > > Anyways, I still feel that if we are taking the data_sz parameter on the > > API, > > we should handle it more strictly. But I do not feel too strong about > > this (as > > you said, It's not expected to exist too much users of this) and in the > > end > > of the day it still behaves the same way as > > 'iio_push_to_buffers_with_timestamp()'... > > > > > > > > > > Alternatively, we could be consistent with > > > 'iio_push_to_buffers_with_timestamp()' > > > > and assume drivers play along in terms of alignment and size > > > > (so, removing the data_sz parameter)... > > > > > > > > My point is, if we are taking the data_sz parameter, maybe we > > > should be pedantic > > > > about it... > > > > > + if (iio_dev_opaque->bounce_buffer_size != indio_dev- > > > > > >scan_bytes) { > > > > > > > > Where do we set ' bounce_buffer_size '? I guess we do not want > > to > > > > get here all the time... > > > > > > :( oops > > > > > > > > + iio_dev_opaque->bounce_buffer = > > > > > + devm_krealloc(&indio_dev->dev, > > > > > + iio_dev_opaque->bounce_buffer, > > > > > + indio_dev->scan_bytes, > > > > > GFP_KERNEL); > > > > > > > > I'm also not a big fan of realloc so I prefer to avoid it unless really > > > needed... > > > > Here, maybe we could just allocate this once during device/buffer > > > initialization > > > > and allocate it for the maximum possible size, i.e all scan elements > > > enabled... > > > > > > We can but to do that requires more invasive changes. Perhaps not > > > horrible ones, > > > but we'd need to add stuff to caller drivers to configure the bounce > > > buffer, or > > > some flag to see they needed to enable support for this. Only 3 > > > drivers out of > > > all those in IIO are currently using this code (might be a few others > > that > > > could > > > take advantage but not many - so need to keep it localized). > > > > I was not thinking that way. I was just thinking in always allocating the > > bounce > > buffer. But I do agree that it is less than ideal to allocate something > > that will > > almost never be used... Ah got you. We could do that, but I think it wants to be opt in just because I'd feel bad adding an extra allocation to every driver. > > > > > I suppose we could have an > > > iio_unaligned_push_to_buffers_prepare() to be called from the > > > update_scan_mode() > > > callback (typed to be useable as that callback if nothing else to be > > done > > > in > > > the relevant drivers. > > > > It could also be an option... If we want to not use realloc, we would > > have to manually > > control the lifespan of the buffer and free it in the disable path. But I > > guess that might feel > > like too much handling... > > > > Anyways, maybe sticking with realloc, we could just use a more normal > > pattern like: > > > > tmp = devm_realloc(bounce_fuffer, ...); > > if (!tmp) > > error; > > > > bounce_buffer = tmp; > > > > I just read your reply to Andy about this and I think you are right... If the > new allocation fails, the older pointer is still preserved. However, I find > the above form a bit more readable and straight :) > Understood. Given it's causing confusion (and I had to think about it in the first place), simply have the more standard pattern is probably wise even if strictly speaking unnecessary! Give people what they expect to see rather than being 'clever' :) > - Nuno Sá
> -----Original Message----- > From: Jonathan Cameron <jic23@kernel.org> > Sent: Monday, May 3, 2021 12:40 PM > To: Sa, Nuno <Nuno.Sa@analog.com> > Cc: linux-iio@vger.kernel.org; Jonathan Cameron > <Jonathan.Cameron@huawei.com> > Subject: Re: [RFC PATCH 1/4] iio: core: Introduce > iio_push_to_buffers_with_ts_na() for non aligned case. > > > On Mon, 3 May 2021 07:46:49 +0000 > "Sa, Nuno" <Nuno.Sa@analog.com> wrote: > > > > -----Original Message----- > > > From: Sa, Nuno <Nuno.Sa@analog.com> > > > Sent: Monday, May 3, 2021 9:15 AM > > > To: Jonathan Cameron <jic23@kernel.org> > > > Cc: linux-iio@vger.kernel.org; Jonathan Cameron > > > <Jonathan.Cameron@huawei.com> > > > Subject: RE: [RFC PATCH 1/4] iio: core: Introduce > > > iio_push_to_buffers_with_ts_na() for non aligned case. > > > > > > > > > > > > > > > > -----Original Message----- > > > > From: Jonathan Cameron <jic23@kernel.org> > > > > Sent: Sunday, May 2, 2021 6:09 PM > > > > To: Sa, Nuno <Nuno.Sa@analog.com> > > > > Cc: linux-iio@vger.kernel.org; Jonathan Cameron > > > > <Jonathan.Cameron@huawei.com> > > > > Subject: Re: [RFC PATCH 1/4] iio: core: Introduce > > > > iio_push_to_buffers_with_ts_na() for non aligned case. > > > > > > > > > > > > On Sun, 2 May 2021 09:10:56 +0000 > > > > "Sa, Nuno" <Nuno.Sa@analog.com> wrote: > > > > > > > > > > From: Jonathan Cameron <jic23@kernel.org> > > > > > > Sent: Saturday, May 1, 2021 7:25 PM > > > > > > To: linux-iio@vger.kernel.org > > > > > > Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > > > Subject: [RFC PATCH 1/4] iio: core: Introduce > > > > > > iio_push_to_buffers_with_ts_na() for non aligned case. > > > > > > > > > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > > > > > > > > > Whilst it is almost always possible to arrange for scan data to > be > > > > > > read directly into a buffer that is suitable for passing to > > > > > > iio_push_to_buffers_with_timestamp(), there are a few > places > > > > where > > > > > > leading data needs to be skipped over. > > > > > > > > > > > > For these cases introduce a function that will allocate an > > > > appropriate > > > > > > sized and aligned bounce buffer (if not already allocated) and > > > copy > > > > > > the unaligned data into that before calling > > > > > > iio_push_to_buffers_with_timestamp() on the bounce > buffer. > > > > > > We tie the lifespace of this buffer to that of the iio_dev.dev > > > > > > which should ensure no memory leaks occur. > > > > > > > > > > > > Signed-off-by: Jonathan Cameron > > > > <Jonathan.Cameron@huawei.com> > > > > > > --- > > > > > > drivers/iio/industrialio-buffer.c | 36 > > > > > > +++++++++++++++++++++++++++++++ > > > > > > include/linux/iio/buffer.h | 4 ++++ > > > > > > include/linux/iio/iio-opaque.h | 4 ++++ > > > > > > 3 files changed, 44 insertions(+) > > > > > > > > > > > > diff --git a/drivers/iio/industrialio-buffer.c > > > > b/drivers/iio/industrialio- > > > > > > buffer.c > > > > > > index 9a8e16c7e9af..818dfaa73665 100644 > > > > > > --- a/drivers/iio/industrialio-buffer.c > > > > > > +++ b/drivers/iio/industrialio-buffer.c > > > > > > @@ -1727,6 +1727,42 @@ int iio_push_to_buffers(struct > iio_dev > > > > > > *indio_dev, const void *data) > > > > > > } > > > > > > EXPORT_SYMBOL_GPL(iio_push_to_buffers); > > > > > > > > > > > > +/** > > > > > > + * iio_push_to_buffers_with_ts_na() - push to registered > > > buffer, > > > > > > + * no alignment or space requirements. > > > > > > + * @indio_dev: iio_dev structure for device. > > > > > > + * @data: channel data excluding the timestamp. > > > > > > + * @data_sz: size of data. > > > > > > + * @timestamp: timestamp for the sample data. > > > > > > + * > > > > > > + * This special variant of > iio_push_to_buffers_with_timetamp() > > > > does > > > > > > + * not require space for the timestamp, or 8 byte alignment > of > > > > data. > > > > > > + * It does however require an allocation on first call and > > > additional > > > > > > + * coppies on all calls, so should be avoided if possible > > > > > > + */ > > > > > > +int iio_push_to_buffers_with_ts_na(struct iio_dev > *indio_dev, > > > > > > + const void *data, > > > > > > + size_t data_sz, > > > > > > + int64_t timestamp) > > > > > > +{ > > > > > > + struct iio_dev_opaque *iio_dev_opaque = > > > > > > to_iio_dev_opaque(indio_dev); > > > > > > + > > > > > > + data_sz = min_t(size_t, indio_dev->scan_bytes, > data_sz); > > > > > I'm not really sure about this. Is it really a good idea to silently > > > > truncate > > > > > the data if some erroneous driver calls this with data_sz > > > > > scan_bytes? > > > > > (I don't think it's ever valid for data_sz to be bigger than > > > scan_bytes) > > > > > > > > It's not probably not. This is just a fairly nasty way of papering > over > > > > anyone doing that. > > > > > > > > > We might be discarding data that userland actually requested. > So, > > > I'm > > > > > not sure if it would not be better to be strict here and return > error, > > > > so that > > > > > driver developers could catch this early on... > > > > > > > > I'm fairly sure we'll never be discarding data userspace requested > > > > because scan_bytes will be the maximum possible and is the size > of > > > > the > > > > kfifo element. Any more data that comes in is going to be > dropped > > > > anyway > > > > assuming it ends up in a kfifo_put() > > > > > > I think it depends. Think on a totally erroneous driver that does not > > > track > > > enabled elements and just pushes all elements to IIO buffers. If > > > userspace > > > just requested, let's say, the last 2 elements, I believe those would > be > > > definitely discarded by this... More subtle things could also happen > I > > > guess... > > > In this case, we can arguably say that the driver is completely > broken > > > and while > > > I'm fairly sure such a scenario would never pass through revision, I > > > think that > > > if we make things more strict, we might minimize the chances of > such > > > patches > > > to appear in the first place (unless someone is sending patches > without > > > testing them at all). > > Like me ;) *laughs* Well, at least you warned everyone about it :) > Agreed there is nothing stopping at least adding a warning print here > and it might help weed out gross bugs even if it is imprecise. > > > > > > > > I wondered if we wanted to enforce this? It would not seem > totally > > > > silly for a driver that wasn't bothering to track the size of it's > enabled > > > > channels to provide data_sz == maximum size it ever needs. > > > > Docs should definitely say if that's valid however, and it's > definitely > > > less > > > > than optimal because we may copy garbage that will then be > > > > overwritten by > > > > the timestamp. > > > > > > > > > > > > > > data_sz < scan_bytes is also probably not valid as that might > > > happen > > > > > if some drivers call this without the scan elements properly > aligned > > > > > which will definitely be problematic... Naturally in this case we > > > would > > > > > have to take into account timestamp possible alignment fill > bytes as > > > it > > > > > could be possible (and likely) that data_sz won't include those > > > bits... > > > > > > > > data_sz < scan_bytes is intentionally valid (and indeed I made > one > > > > of the drivers do this). Given we are bouncing anyway, why insist > > > the > > > > driver provides space for padding + timestamp? That can be > done > > > for > > > > the bounce buffer. > > > > > > What I meant was that if data_sz < scan_bytes - timestamp - > padding, > > > then > > > the driver is most likely not guaranteeing properly aligned > elements. I > > > was > > > not that clear :) > > If you hit that condition I think it would be a bug as not enough data > provided. > We could easily enforce data_sz < scan_bytes - timstamp, but the > padding would > need to be stashed somewhere. + I've been lazy in one of the drivers > because I think with data_sz we can easily calculate the needed padding for the timestamp... Naturally we would need to clearly state in the docs (probably done already) that data_sz should not contain any bits related with the timestamp element... > I couldn't figure it out in a trivial fashion - perhaps I should take another > look at that one and see if I can make the check accurate rather than a > worst > case bound. > > > > > > > Anyways, I still feel that if we are taking the data_sz parameter on > the > > > API, > > > we should handle it more strictly. But I do not feel too strong about > > > this (as > > > you said, It's not expected to exist too much users of this) and in > the > > > end > > > of the day it still behaves the same way as > > > 'iio_push_to_buffers_with_timestamp()'... > > > > > > > > > > > > > Alternatively, we could be consistent with > > > > 'iio_push_to_buffers_with_timestamp()' > > > > > and assume drivers play along in terms of alignment and size > > > > > (so, removing the data_sz parameter)... > > > > > > > > > > My point is, if we are taking the data_sz parameter, maybe we > > > > should be pedantic > > > > > about it... > > > > > > + if (iio_dev_opaque->bounce_buffer_size != > indio_dev- > > > > > > >scan_bytes) { > > > > > > > > > > Where do we set ' bounce_buffer_size '? I guess we do not > want > > > to > > > > > get here all the time... > > > > > > > > :( oops > > > > > > > > > > + iio_dev_opaque->bounce_buffer = > > > > > > + devm_krealloc(&indio_dev->dev, > > > > > > + iio_dev_opaque- > >bounce_buffer, > > > > > > + indio_dev->scan_bytes, > > > > > > GFP_KERNEL); > > > > > > > > > > I'm also not a big fan of realloc so I prefer to avoid it unless > really > > > > needed... > > > > > Here, maybe we could just allocate this once during > device/buffer > > > > initialization > > > > > and allocate it for the maximum possible size, i.e all scan > elements > > > > enabled... > > > > > > > > We can but to do that requires more invasive changes. Perhaps > not > > > > horrible ones, > > > > but we'd need to add stuff to caller drivers to configure the > bounce > > > > buffer, or > > > > some flag to see they needed to enable support for this. Only 3 > > > > drivers out of > > > > all those in IIO are currently using this code (might be a few > others > > > that > > > > could > > > > take advantage but not many - so need to keep it localized). > > > > > > I was not thinking that way. I was just thinking in always allocating > the > > > bounce > > > buffer. But I do agree that it is less than ideal to allocate something > > > that will > > > almost never be used... > > Ah got you. We could do that, but I think it wants to be opt in just > because > I'd feel bad adding an extra allocation to every driver. Agreed... - Nuno Sá
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c index 9a8e16c7e9af..818dfaa73665 100644 --- a/drivers/iio/industrialio-buffer.c +++ b/drivers/iio/industrialio-buffer.c @@ -1727,6 +1727,42 @@ int iio_push_to_buffers(struct iio_dev *indio_dev, const void *data) } EXPORT_SYMBOL_GPL(iio_push_to_buffers); +/** + * iio_push_to_buffers_with_ts_na() - push to registered buffer, + * no alignment or space requirements. + * @indio_dev: iio_dev structure for device. + * @data: channel data excluding the timestamp. + * @data_sz: size of data. + * @timestamp: timestamp for the sample data. + * + * This special variant of iio_push_to_buffers_with_timetamp() does + * not require space for the timestamp, or 8 byte alignment of data. + * It does however require an allocation on first call and additional + * coppies on all calls, so should be avoided if possible + */ +int iio_push_to_buffers_with_ts_na(struct iio_dev *indio_dev, + const void *data, + size_t data_sz, + int64_t timestamp) +{ + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev); + + data_sz = min_t(size_t, indio_dev->scan_bytes, data_sz); + if (iio_dev_opaque->bounce_buffer_size != indio_dev->scan_bytes) { + iio_dev_opaque->bounce_buffer = + devm_krealloc(&indio_dev->dev, + iio_dev_opaque->bounce_buffer, + indio_dev->scan_bytes, GFP_KERNEL); + if (!iio_dev_opaque) + return -ENOMEM; + } + memcpy(iio_dev_opaque->bounce_buffer, data, data_sz); + return iio_push_to_buffers_with_timestamp(indio_dev, + iio_dev_opaque->bounce_buffer, + timestamp); +} +EXPORT_SYMBOL_GPL(iio_push_to_buffers_with_ts_na); + /** * iio_buffer_release() - Free a buffer's resources * @ref: Pointer to the kref embedded in the iio_buffer struct diff --git a/include/linux/iio/buffer.h b/include/linux/iio/buffer.h index b6928ac5c63d..4ea9027ad2bf 100644 --- a/include/linux/iio/buffer.h +++ b/include/linux/iio/buffer.h @@ -38,6 +38,10 @@ static inline int iio_push_to_buffers_with_timestamp(struct iio_dev *indio_dev, return iio_push_to_buffers(indio_dev, data); } +int iio_push_to_buffers_with_ts_na(struct iio_dev *indio_dev, + const void *data, size_t data_sz, + int64_t timestamp); + bool iio_validate_scan_mask_onehot(struct iio_dev *indio_dev, const unsigned long *mask); diff --git a/include/linux/iio/iio-opaque.h b/include/linux/iio/iio-opaque.h index 32addd5e790e..70ca58e66c0a 100644 --- a/include/linux/iio/iio-opaque.h +++ b/include/linux/iio/iio-opaque.h @@ -19,6 +19,8 @@ * @groupcounter: index of next attribute group * @legacy_scan_el_group: attribute group for legacy scan elements attribute group * @legacy_buffer_group: attribute group for legacy buffer attributes group + * @bounce_buffer: for devices that call iio_push_to_buffers_with_timestamp_na + * @bounce_buffer_size: size of currently allocate bounce buffer * @debugfs_dentry: device specific debugfs dentry * @cached_reg_addr: cached register address for debugfs reads * @read_buf: read buffer to be used for the initial reg read @@ -38,6 +40,8 @@ struct iio_dev_opaque { int groupcounter; struct attribute_group legacy_scan_el_group; struct attribute_group legacy_buffer_group; + void *bounce_buffer; + size_t bounce_buffer_size; #if defined(CONFIG_DEBUG_FS) struct dentry *debugfs_dentry; unsigned cached_reg_addr;