iio: buffer: align the size of scan bytes to size of the largest element
diff mbox series

Message ID 20191202142714.12030-1-lars.moellendorf@plating.de
State New
Headers show
Series
  • iio: buffer: align the size of scan bytes to size of the largest element
Related show

Commit Message

Lars Möllendorf Dec. 2, 2019, 2:27 p.m. UTC
Previous versions of `iio_compute_scan_bytes` only aligned the current
element to its own length (i.e. its own natural alignment). This worked
in case the length of each channel's scan elements had been smaller then
the length of the successive channel's scan elements.

E.g.

u16
u32 <- aligned to its natural alignment

But if the length of a channel's scan elements is greater then the
length of scan elements of the consecutive channel no alignment had
been taken into account. This is however needed to preserve the
alignment for multiple consecutive sets of scan elements.

u32 <- alignment is off by two byte for the second set of scan elements
u16 <- no alignment takes place

This commit fixes this by aligning the scan bytes to the size of the
largest scan element.

Signed-off-by: Lars Möllendorf <lars.moellendorf@plating.de>
---
 drivers/iio/industrialio-buffer.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Jonathan Cameron Dec. 6, 2019, 5:34 p.m. UTC | #1
On Mon,  2 Dec 2019 15:27:14 +0100
Lars Möllendorf <lars.moellendorf@plating.de> wrote:

Even when fixing a patch send issue as you did here, please mark clearly
with v2 and add a change log saying that's what you did.  Also
please don't send in reply to the earlier thread, but instead as a
completely new thread.   The reply to thread thing seems sensible for
simple cases, but when we get to v20 (it happens ;) things get really
confusing :)

> Previous versions of `iio_compute_scan_bytes` only aligned the current
> element to its own length (i.e. its own natural alignment). This worked
> in case the length of each channel's scan elements had been smaller then
> the length of the successive channel's scan elements.

I'd modify this description a little bit.  The key thing was the length of the
last element - didn't matter what happened before that.

u32
u16
u32
would be aligned correctly I hope?

> 
> E.g.
> 
> u16
> u32 <- aligned to its natural alignment
> 
> But if the length of a channel's scan elements is greater then the
> length of scan elements of the consecutive channel no alignment had
> been taken into account. This is however needed to preserve the
> alignment for multiple consecutive sets of scan elements.
> 
> u32 <- alignment is off by two byte for the second set of scan elements
> u16 <- no alignment takes place
> 
> This commit fixes this by aligning the scan bytes to the size of the
> largest scan element.
> 
> Signed-off-by: Lars Möllendorf <lars.moellendorf@plating.de>
Code looks good to me, so just a v3 tidying up the message.

Thanks for sorting this out!  Ideally we'd add a specific
fixes tag as well for when this was introduced but I fear
it goes back nearly all the way (at least before the move
out of staging) so we can probably leave that vague for this
one unless you fancy some archaeology. 

Jonathan

> ---
>  drivers/iio/industrialio-buffer.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 5d05c38c4ba9..2f037cd59d53 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -546,7 +546,7 @@ static int iio_compute_scan_bytes(struct iio_dev *indio_dev,
>  				const unsigned long *mask, bool timestamp)
>  {
>  	unsigned bytes = 0;
> -	int length, i;
> +	int length, i, largest = 0;
>  
>  	/* How much space will the demuxed element take? */
>  	for_each_set_bit(i, mask,
> @@ -554,13 +554,17 @@ static int iio_compute_scan_bytes(struct iio_dev *indio_dev,
>  		length = iio_storage_bytes_for_si(indio_dev, i);
>  		bytes = ALIGN(bytes, length);
>  		bytes += length;
> +		largest = max(largest, length);
>  	}
>  
>  	if (timestamp) {
>  		length = iio_storage_bytes_for_timestamp(indio_dev);
>  		bytes = ALIGN(bytes, length);
>  		bytes += length;
> +		largest = max(largest, length);
>  	}
> +
> +	bytes = ALIGN(bytes, largest);
>  	return bytes;
>  }
>

Patch
diff mbox series

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 5d05c38c4ba9..2f037cd59d53 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -546,7 +546,7 @@  static int iio_compute_scan_bytes(struct iio_dev *indio_dev,
 				const unsigned long *mask, bool timestamp)
 {
 	unsigned bytes = 0;
-	int length, i;
+	int length, i, largest = 0;
 
 	/* How much space will the demuxed element take? */
 	for_each_set_bit(i, mask,
@@ -554,13 +554,17 @@  static int iio_compute_scan_bytes(struct iio_dev *indio_dev,
 		length = iio_storage_bytes_for_si(indio_dev, i);
 		bytes = ALIGN(bytes, length);
 		bytes += length;
+		largest = max(largest, length);
 	}
 
 	if (timestamp) {
 		length = iio_storage_bytes_for_timestamp(indio_dev);
 		bytes = ALIGN(bytes, length);
 		bytes += length;
+		largest = max(largest, length);
 	}
+
+	bytes = ALIGN(bytes, largest);
 	return bytes;
 }