RE: [RFC PATCH 1/4] iio: core: Introduce iio_push_to_buffers_with_ts_na() for non aligned case.

["Sa, Nuno" <Nuno.Sa@analog.com>]

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