Re: [PATCH v4 1/5] tools: iio: iio_generic_buffer ensure alignment

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 9/30/23 19:34, Jonathan Cameron wrote:
On Wed, 27 Sep 2023 11:26:07 +0300
Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote:

The iio_generic_buffer can return garbage values when the total size of
scan data is not a multiple of the largest element in the scan. This can be
demonstrated by reading a scan, consisting, for example of one 4-byte and
one 2-byte element, where the 4-byte element is first in the buffer.

The IIO generic buffer code does not take into account the last two
padding bytes that are needed to ensure that the 4-byte data for next
scan is correctly aligned.

Add the padding bytes required to align the next sample with the scan size.

Signed-off-by: Matti Vaittinen <mazziesaccount@xxxxxxxxx>

---
I think the whole alignment code could be revised here, but I am unsure
what kind of alignment is expected, and if it actually depends on the
architecture. Anyways, I'll quote myself from another mail to explain
how this patch handles things:

For non power of2 sizes, the alignment code will result strange alignments.
For example, scan consisting of two 6-byte elements would be packed -
meaning the second element would probably break the alignment rules by
starting from address '6'. I think that on most architectures the proper
access would require 2 padding bytes to be added at the end of the first
sample. Current code wouldn't do that.

If we allow only power of 2 sizes - I would expect a scan consisting of a
8 byte element followed by a 16 byte element to be tightly packed. I'd
assume that for the 16 byte data, it'd be enough to ensure 8 byte alignment.
Current code would however add 8 bytes of padding at the end of the first
8 byte element to make the 16 byte scan element to be aligned at 16 byte
address. To my uneducated mind this is not needed - but maybe I just don't
know what I am writing about :)

Revision history
v3 => v4:
  - drop extra print and TODO coment
  - add comment clarifying alignment sizes
---
  tools/iio/iio_generic_buffer.c | 18 +++++++++++++++++-
  1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/tools/iio/iio_generic_buffer.c b/tools/iio/iio_generic_buffer.c
index 44bbf80f0cfd..c07c49397b19 100644
--- a/tools/iio/iio_generic_buffer.c
+++ b/tools/iio/iio_generic_buffer.c
@@ -54,9 +54,12 @@ enum autochan {
  static unsigned int size_from_channelarray(struct iio_channel_info *channels, int num_channels)
  {
  	unsigned int bytes = 0;
-	int i = 0;
+	int i = 0, max = 0;
+	unsigned int misalignment;
while (i < num_channels) {
+		if (channels[i].bytes > max)
+			max = channels[i].bytes;
  		if (bytes % channels[i].bytes == 0)
  			channels[i].location = bytes;
  		else
@@ -66,6 +69,19 @@ static unsigned int size_from_channelarray(struct iio_channel_info *channels, in
  		bytes = channels[i].location + channels[i].bytes;
  		i++;
  	}
+	/*
+	 * We wan't the data in next sample to also be properly aligned so
+	 * we'll add padding at the end if needed.
+	 *
+	 * Please note, this code does ensure alignment to maximum channel
+	 * size. It works only as long as the channel sizes are 1, 2, 4 or 8
+	 * bytes. Also, on 32 bit platforms it might be enough to align also
+	 * the 8 byte elements to 4 byte boundary - which this code is not
+	 * doing.
Very much not!  We need to present same data alignment to userspace
indpendent of what architecture is running.

It's annoyingly inconsistent how 8 byte elements are handled on 32 bit
architectures as some have optimized aligned access routines and others
will read as 2 32 bit fields.  Hence we just stick to 8 byte value is
8 byte aligned which is always fine but wastes a bit of space on x86 32
bit - which I don't care about ;)

Please drop this last bit of the comment as we should just say what it
does, not conjecture what it might do!

Ok. The comment was more to catch the reviewers' attention ;) I'll just note the alignment works for power of 2 sample sizes and aligns according to the max sized sample, even if it was bigger than 8.

Thanks!

-- Matti


--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux