Re: [PATCH v3 1/6] tools: iio: iio_generic_buffer ensure alignment

[Matti Vaittinen <mazziesaccount@gmail.com>]

On 9/25/23 16:16, Jonathan Cameron wrote:
> On Mon, 25 Sep 2023 10:01:09 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> On 9/24/23 18:57, Jonathan Cameron wrote:
>>> On Fri, 22 Sep 2023 14:16:08 +0300
>>> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>>>    
>>>> The iio_generic_buffer can return garbage values when the total size of
>>>> scan data is not a multiple of 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 elemnt is first in the buffer.
>>>>
>>>> The IIO generic buffert code does not take into accunt the last two
>>>> padding bytes that are needed to ensure that the 4byte data for next
>>>> scan is correctly aligned.
>>>>
>>>> Add padding bytes required to align the next sample into the scan size.
>>>>
>>>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>>>> ---
>>>> Please note, This one could have RFC in subject.:
>>>> I attempted to write the fix so that the alignment is done based on the
>>>> biggest channel data. This may be wrong. Maybe a fixed 8 byte alignment
>>>> should be used instead? This patch can be dropped from the series if the
>>>> fix is not correct / agreed.
>>>>
>>>>    tools/iio/iio_generic_buffer.c | 15 ++++++++++++++-
>>>>    1 file changed, 14 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/tools/iio/iio_generic_buffer.c b/tools/iio/iio_generic_buffer.c
>>>> index 44bbf80f0cfd..fc562799a109 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,16 @@ 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. TODO: should we use fixed
>>>> +	 * 8 byte alignment instead of the size of the biggest samnple?
>>>> +	 */
>>>
>>> Should be aligned to max size seen in the scan.
>>
>> Or, maybe it should be
>> min(max_size_in_scan, 8);
>> ?
> 
> Definitely not.   If you are grabbing just one channel of 8 bit data,
> we want it to be tightly packed.

I think that in this case the max_size_in_scan would be 1, and min(1, 8) 
would be 1 as well, resulting a tightly packed data. I am just wondering 
if we should use 8 as maximum alignment - eg, if our scan has 16 bytes 
data + 1 byte data, we would add 7 bytes of padding, not 15 bytes of 
padding. I am not sure what is the right thing to do.

> If we have a bug that already made that true then we might be stuck
> with it, but I'm fairly sure we don't.
>>
>> I think my suggestion above may yield undesirable effects should the
>> scan elements be greater than 8 bytes. (Don't know if this is supported
>> though)
> 
> It is supported in theory, in practice not seen one yet.

So, whether to unconditionally use largest scan element sized alignment 
- or largest scan element up to 8 bytes - is a question we haven't hit 
yet :)

Actually, more I stare at the alignment code here, less sure I am it is 
correct - but maybe I don't understand how the data should be aligned.

I think it works if allowed data sizes are 1, 2, 4, and 8. However, I 
suspect it breaks for other sizes.

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 :)

In any case, the patch here should fix things when allowed scan element 
sizes are 1, 2, 4 and 8 and we have to add padding after last scan 
element. It won't work for other sizes, but as I wrote, I suspect the 
whole alignment code here may be broken for other sizes so things 
shouldn't at least get worse with this patch... I think this should be 
revised if we see samples of other sizes - and in any case, this might 
at least warrant a comment here :) (I reserve a right to be wrong. 
Haven't been sleeping too well lately and my head is humming...)

>>>> +	misalignment = bytes % max;
>>>> +	if (misalignment) {
>>>> +		printf("Misalignment %u. Adding Padding %u\n", misalignment,  max - misalignment);
>>>
>>> No print statement as this is correct behaviour (well the tool is buggy but the kernel generates it
>>> correctly I believe).  Fine to add a comment though!
>>
>> Oh, indeed. The print was forgotten from my test runs. Thanks for
>> pointing it out!
>>
>>>    
>>>> +		bytes += max - misalignment;
>>>> +	}
>>>>    
>>>>    	return bytes;
>>>>    }
>>>    

Yours,
	-- Matti

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

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