[RFC PATCH 0/4] IIO: Alignment fixes part 4 - bounce buffers for the hard cases.

[RFC PATCH 0/4] IIO: Alignment fixes part 4 - bounce buffers for the hard cases.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

I finally got around to do a manual audit of all the calls to
iio_push_to_buffers_with_timestamp() which has the somewhat odd requirements
of:
1. 8 byte alignment of the provided buffer.
2. space for an 8 byte naturally aligned timestamp to be inserted at the
   end.

Unfortunately there were rather a lot of these left, but time to bite the bullet
and clean them up.

As discussed previous in
https://lore.kernel.org/linux-iio/20200920112742.170751-1-jic23@kernel.org/
it is not easy to fix the alignment issue without requiring a bounce buffer.
This final part of the 4 sets of fixes is concerned with the cases where
bounce buffers are the proposed solutions.

In these cases we have hardware that reads a prefix that we wish to
drop. That makes it hard to directly read the data into the correct location.

Rather than implementing bouce buffers in each case, this set provides some
magic in the core to handle them via a new function.
iio_push_to_buffers_with_ts_na() - non aligned

Note this is totally untested as I don't have suitable hardware or emulation.
I can fake something up in the dummy driver or via QEMU but I definitely want
both eyes and testing on this series!

Cc: Jan Kiszka <jan.kiszka@siemens.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Nuno Sa <nuno.sa@analog.com>

Jonathan Cameron (4):
  iio: core: Introduce iio_push_to_buffers_with_ts_na() for non aligned
    case.
  iio: adc: ti-adc108s102: Fix alignment of buffer pushed to iio
    buffers.
  iio: gyro: mpu3050: Fix alignment and size issues with buffers.
  iio: imu: adis16400: Fix buffer alignment requirements.

 drivers/iio/adc/ti-adc108s102.c   | 11 +++++-----
 drivers/iio/gyro/mpu3050-core.c   | 24 ++++++++++-----------
 drivers/iio/imu/adis16400.c       | 20 ++++++++++++-----
 drivers/iio/industrialio-buffer.c | 36 +++++++++++++++++++++++++++++++
 include/linux/iio/buffer.h        |  4 ++++
 include/linux/iio/iio-opaque.h    |  4 ++++
 6 files changed, 76 insertions(+), 23 deletions(-)

-- 
2.31.1

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

[Jonathan Cameron]

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);
+	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;
-- 
2.31.1

[RFC PATCH 2/4] iio: adc: ti-adc108s102: Fix alignment of buffer pushed to iio buffers.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Use the newly introduce iio_push_to_buffers_with_ts_na() function
to ensure a bounce buffer is used to provide the required alignment and
space padding needed by the IIO core which requires the timestamp
is naturally aligned.  There will be a performance cost to this change
but it will ensure the driver works on platforms that do not support
unaligned 8 byte assignements, and with consumer drivers that may
assume natural alignment of the timestamp.

Issue found as part of an audit of all calls to
iio_push_to_buffers_with_timestamp()

Fixes: 7e87d11c9bda ("iio: adc: Add support for TI ADC108S102 and ADC128S102")
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Jan Kiszka <jan.kiszka@siemens.com>
---
 drivers/iio/adc/ti-adc108s102.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/adc/ti-adc108s102.c b/drivers/iio/adc/ti-adc108s102.c
index 183b2245e89b..7aeed3503829 100644
--- a/drivers/iio/adc/ti-adc108s102.c
+++ b/drivers/iio/adc/ti-adc108s102.c
@@ -75,9 +75,9 @@ struct adc108s102_state {
 	 *  rx_buf: |XX|R0|R1|R2|R3|R4|R5|R6|R7|tt|tt|tt|tt|
 	 *
 	 *  tx_buf: 8 channel read commands, plus 1 dummy command
-	 *  rx_buf: 1 dummy response, 8 channel responses, plus 64-bit timestamp
+	 *  rx_buf: 1 dummy response, 8 channel responses
 	 */
-	__be16				rx_buf[13] ____cacheline_aligned;
+	__be16				rx_buf[9] ____cacheline_aligned;
 	__be16				tx_buf[9] ____cacheline_aligned;
 };
 
@@ -149,9 +149,10 @@ static irqreturn_t adc108s102_trigger_handler(int irq, void *p)
 		goto out_notify;
 
 	/* Skip the dummy response in the first slot */
-	iio_push_to_buffers_with_timestamp(indio_dev,
-					   (u8 *)&st->rx_buf[1],
-					   iio_get_time_ns(indio_dev));
+	iio_push_to_buffers_with_ts_na(indio_dev,
+				       &st->rx_buf[1],
+				       st->ring_xfer.len - sizeof(st->rx_buf[1]),
+				       iio_get_time_ns(indio_dev));
 
 out_notify:
 	iio_trigger_notify_done(indio_dev->trig);
-- 
2.31.1

[RFC PATCH 3/4] iio: gyro: mpu3050: Fix alignment and size issues with buffers.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Fix a set of closely related issues.
1. When using fifo_values() there was not enough space for the timestamp to
   be inserted by iio_push_to_buffers_with_timestamp()
2. fifo_values() did not meet the alignment requirement of
   iio_push_to_buffers_with_timestamp()
3. hw_values did not meet the alignment requirement either.

1 and 2 fixed by using new iio_push_to_buffers_with_ts_na() which has
no alignment or space padding requirements.
3 fixed by introducing a structure that makes the space and alignment
requirements explicit.

Fixes: 3904b28efb2c ("iio: gyro: Add driver for the MPU-3050 gyroscope")
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/iio/gyro/mpu3050-core.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/iio/gyro/mpu3050-core.c b/drivers/iio/gyro/mpu3050-core.c
index ac90be03332a..a7fdfe811258 100644
--- a/drivers/iio/gyro/mpu3050-core.c
+++ b/drivers/iio/gyro/mpu3050-core.c
@@ -462,13 +462,10 @@ static irqreturn_t mpu3050_trigger_handler(int irq, void *p)
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct mpu3050 *mpu3050 = iio_priv(indio_dev);
 	int ret;
-	/*
-	 * Temperature 1*16 bits
-	 * Three axes 3*16 bits
-	 * Timestamp 64 bits (4*16 bits)
-	 * Sum total 8*16 bits
-	 */
-	__be16 hw_values[8];
+	struct {
+		__be16 chans[4];
+		s64 timestamp __aligned(8);
+	} scan;
 	s64 timestamp;
 	unsigned int datums_from_fifo = 0;
 
@@ -563,9 +560,10 @@ static irqreturn_t mpu3050_trigger_handler(int irq, void *p)
 				fifo_values[4]);
 
 			/* Index past the footer (fifo_values[0]) and push */
-			iio_push_to_buffers_with_timestamp(indio_dev,
-							   &fifo_values[1],
-							   timestamp);
+			iio_push_to_buffers_with_ts_na(indio_dev,
+						       &fifo_values[1],
+						       sizeof(__be16) * 4,
+						       timestamp);
 
 			fifocnt -= toread;
 			datums_from_fifo++;
@@ -623,15 +621,15 @@ static irqreturn_t mpu3050_trigger_handler(int irq, void *p)
 		goto out_trigger_unlock;
 	}
 
-	ret = regmap_bulk_read(mpu3050->map, MPU3050_TEMP_H, &hw_values,
-			       sizeof(hw_values));
+	ret = regmap_bulk_read(mpu3050->map, MPU3050_TEMP_H, scan.chans,
+			       sizeof(scan.chans));
 	if (ret) {
 		dev_err(mpu3050->dev,
 			"error reading axis data\n");
 		goto out_trigger_unlock;
 	}
 
-	iio_push_to_buffers_with_timestamp(indio_dev, hw_values, timestamp);
+	iio_push_to_buffers_with_timestamp(indio_dev, &scan, timestamp);
 
 out_trigger_unlock:
 	mutex_unlock(&mpu3050->lock);
-- 
2.31.1

[RFC PATCH 4/4] iio: imu: adis16400: Fix buffer alignment requirements.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

iio_push_to_buffers_with_timestamp() requires that the buffer
is 8 byte alignment to ensure an inserted timestamp is naturally aligned.

This requirement was not met here when burst mode is in use beause
of a leading u16. Use the new iio_push_to_buffers_with_ts_na() function
that has more relaxed requirements.

It is somewhat complex to access that actual data length, but a
safe bound can be found by using scan_bytes - sizeof(timestamp) so that
is used in this path.

More efficient approaches exist, but this ensure correctness at the
cost of using a bounce buffer.

Fixes: 5075e0720d93 ("iio: imu: adis: generalize burst mode support")
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/imu/adis16400.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/imu/adis16400.c b/drivers/iio/imu/adis16400.c
index 768aa493a1a6..c6d03a37373b 100644
--- a/drivers/iio/imu/adis16400.c
+++ b/drivers/iio/imu/adis16400.c
@@ -663,13 +663,23 @@ static irqreturn_t adis16400_trigger_handler(int irq, void *p)
 		spi_setup(st->adis.spi);
 	}
 
-	if (st->variant->flags & ADIS16400_BURST_DIAG_STAT)
+	if (st->variant->flags & ADIS16400_BURST_DIAG_STAT) {
 		buffer = adis->buffer + sizeof(u16);
-	else
-		buffer = adis->buffer;
+		/*
+		 * The size here is always larger than, or equal to the true
+		 * size of the channel data. This may result in a larger copy
+		 * than necessary, but as the target buffer will be
+		 * buffer->scan_bytes this will be safe.
+		 */
+		iio_push_to_buffers_with_ts_na(indio_dev, buffer,
+					       indio_dev->scan_bytes - sizeof(pf->timestamp),
+					       pf->timestamp);
+	} else {
+		iio_push_to_buffers_with_timestamp(indio_dev,
+						   adis->buffer,
+						   pf->timestamp);
+	}
 
-	iio_push_to_buffers_with_timestamp(indio_dev, buffer,
-		pf->timestamp);
 
 	iio_trigger_notify_done(indio_dev->trig);
 
-- 
2.31.1

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

[Andy Shevchenko]

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);
> +}

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

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;
> > +       }

-- 
With Best Regards,
Andy Shevchenko

RE: [RFC PATCH 4/4] iio: imu: adis16400: Fix buffer alignment requirements.

[Sa, Nuno]

> 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>; Sa, Nuno
> <Nuno.Sa@analog.com>
> Subject: [RFC PATCH 4/4] iio: imu: adis16400: Fix buffer alignment
> requirements.
> 
> 
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> iio_push_to_buffers_with_timestamp() requires that the buffer
> is 8 byte alignment to ensure an inserted timestamp is naturally
> aligned.
> 
> This requirement was not met here when burst mode is in use beause
> of a leading u16. Use the new iio_push_to_buffers_with_ts_na()
> function
> that has more relaxed requirements.
> 
> It is somewhat complex to access that actual data length, but a
> safe bound can be found by using scan_bytes - sizeof(timestamp) so
> that
> is used in this path.
> 
> More efficient approaches exist, but this ensure correctness at the
> cost of using a bounce buffer.
> 
> Fixes: 5075e0720d93 ("iio: imu: adis: generalize burst mode support")
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Nuno Sa <nuno.sa@analog.com>
> ---
>  drivers/iio/imu/adis16400.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/imu/adis16400.c b/drivers/iio/imu/adis16400.c
> index 768aa493a1a6..c6d03a37373b 100644
> --- a/drivers/iio/imu/adis16400.c
> +++ b/drivers/iio/imu/adis16400.c
> @@ -663,13 +663,23 @@ static irqreturn_t
> adis16400_trigger_handler(int irq, void *p)
>  		spi_setup(st->adis.spi);
>  	}
> 
> -	if (st->variant->flags & ADIS16400_BURST_DIAG_STAT)
> +	if (st->variant->flags & ADIS16400_BURST_DIAG_STAT) {
>  		buffer = adis->buffer + sizeof(u16);
> -	else
> -		buffer = adis->buffer;
> +		/*
> +		 * The size here is always larger than, or equal to the
> true
> +		 * size of the channel data. This may result in a larger
> copy
> +		 * than necessary, but as the target buffer will be
> +		 * buffer->scan_bytes this will be safe.
> +		 */
> +		iio_push_to_buffers_with_ts_na(indio_dev, buffer,
> +					       indio_dev->scan_bytes -
> sizeof(pf->timestamp),
> +					       pf->timestamp);
> +	} else {
> +		iio_push_to_buffers_with_timestamp(indio_dev,
> +						   adis->buffer,
> +						   pf->timestamp);
> +	}
> 

Hi Jonathan,

This looks good to me although I think there's some stuff to care in
' iio_push_to_buffers_with_ts_na()'. However, for this patch alone:

Reviewed-by: Nuno Sá <nuno.sa@analog.com>

I will also check if I find any HW to test this...

- Nuno Sá

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

[Sa, Nuno]

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

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

[Jonathan Cameron]

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;
> > > +       }  
> 

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

[Jonathan Cameron]

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);
> > +}  
> 

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

[Jonathan Cameron]

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

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

[Sa, Nuno]

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

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

[Sa, Nuno]

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

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

[Andy Shevchenko]

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.



-- 
With Best Regards,
Andy Shevchenko

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

[Jonathan Cameron]

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á

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

[Sa, Nuno]

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

Re: [RFC PATCH 3/4] iio: gyro: mpu3050: Fix alignment and size issues with buffers.

[Linus Walleij]

On Sat, May 1, 2021 at 7:26 PM Jonathan Cameron <jic23@kernel.org> wrote:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> Fix a set of closely related issues.
> 1. When using fifo_values() there was not enough space for the timestamp to
>    be inserted by iio_push_to_buffers_with_timestamp()
> 2. fifo_values() did not meet the alignment requirement of
>    iio_push_to_buffers_with_timestamp()
> 3. hw_values did not meet the alignment requirement either.
>
> 1 and 2 fixed by using new iio_push_to_buffers_with_ts_na() which has
> no alignment or space padding requirements.
> 3 fixed by introducing a structure that makes the space and alignment
> requirements explicit.
>
> Fixes: 3904b28efb2c ("iio: gyro: Add driver for the MPU-3050 gyroscope")
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>

Again a very nice fix:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Again I wonder how we can help driver authors to always get this right.

Yours,
Linus Walleij