On Sat, 1 May 2021 22:25:55 +0300
Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
> On Sat, May 1, 2021 at 8:28 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> >
> > From: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> >
> > 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);
> > +}
>
