Message ID | 029b4e3e18c76b330b606f5b14699e5ee4e5ed35.1695380366.git.mazziesaccount@gmail.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:612c:172:b0:3f2:4152:657d with SMTP id h50csp5493653vqi; Fri, 22 Sep 2023 04:30:44 -0700 (PDT) X-Google-Smtp-Source: AGHT+IF2ymWAD4XDV3pHU1W5SgHt9sXgtFfXMX9MDzTegmM5A/Vcyr9B5RDVOU9pR377iIw7/l8L X-Received: by 2002:a17:90a:648e:b0:274:8949:d834 with SMTP id h14-20020a17090a648e00b002748949d834mr6844582pjj.49.1695382244157; Fri, 22 Sep 2023 04:30:44 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1695382244; cv=none; d=google.com; s=arc-20160816; b=gpppetqx8W7wkKOY90Jqz/iDO3z6TfPJcP/9svVP+F0P3l9hV2Ok3KX97PAWpR01Xv WrV5TNVa7Ow+l4nTi6/TB8oLUeWP4/CoGO/OgCORl4M6Y9FD8eVxkUpWBPQ2nH9kHZIg UgZ5ReCmyH18zarDC0Xkx98zCOD1d1bVucsqe2Ofhn3hhvanUNk2EQYxcN4d40nMy05N qUBhwbzQb7TqRNUd2AMldoss7A9mLjE3YsgrNefZTF2By9bV8BC8q/dFMgaHKV4E/Lgj 2rWrezBrXO1UekW/giKLlAXiszD/PeRPR4II4ruMaW2euu2rRNnfH5K8eijn/dckQLIn RaYw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:in-reply-to:content-disposition:mime-version :references:message-id:subject:cc:to:from:date:dkim-signature; bh=9THroEikiL60Yi8fcuQ2zOCWbKMjxLA15+wfPD4+rk8=; fh=+XkJcXOKJ4Dl6AYTln3UzvhZHjSiM5yCnDKdoCP8e7U=; b=KH/Q051/AxiEY+ZM2QGNkCC6+mLzL5F3TZva71BcMIpv30/4g/uW+NnktpRaN4eY34 URwhmDMACoATrQ7r43wHduf2UhMVCXDgyNXkawnVxtJSE4BhyhuhjY5SYOTQqTS5TXQB 8aEbqSkyG21dTSJJZPLwwhQH2Q/Wm0sJZ/viF131eXcK3Omcd7BRvtDzJ16AZKr8EAyg /Z3rPo8faDrs1it4O++IcH7umXx5Bm86gSaZxyL68eUHtVs2ZoydzLt5KX+gZMnhBUt/ bN4qlA6DoWxsJFIXreqo9gPGWZHiuJ04Fib4QH+WjMWV27LJLF+oMorQOmgvln3G4gCm xF/w== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20230601 header.b=XWcrk36L; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: from morse.vger.email (morse.vger.email. [23.128.96.31]) by mx.google.com with ESMTPS id t2-20020a17090aba8200b00273ce975beesi3774473pjr.114.2023.09.22.04.30.43 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 22 Sep 2023 04:30:44 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20230601 header.b=XWcrk36L; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by morse.vger.email (Postfix) with ESMTP id 8866F80AC37B; Fri, 22 Sep 2023 04:16:35 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at morse.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233540AbjIVLQ1 (ORCPT <rfc822;chrisfriedt@gmail.com> + 30 others); Fri, 22 Sep 2023 07:16:27 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46226 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230156AbjIVLQZ (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 22 Sep 2023 07:16:25 -0400 Received: from mail-lf1-x12a.google.com (mail-lf1-x12a.google.com [IPv6:2a00:1450:4864:20::12a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9F4A1AC; Fri, 22 Sep 2023 04:16:19 -0700 (PDT) Received: by mail-lf1-x12a.google.com with SMTP id 2adb3069b0e04-502e7d66c1eso3322084e87.1; Fri, 22 Sep 2023 04:16:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1695381378; x=1695986178; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=9THroEikiL60Yi8fcuQ2zOCWbKMjxLA15+wfPD4+rk8=; b=XWcrk36LErSz9FQfU5G3Qc8MV4yZvGckoRZGh0S5w4xy76sNGCEz9OTMARfeVwu4+T pbXCXTQobWxq6sJGFMgjE1wnGNNUpKrUhTB8CExDTBhz97vzFZTweAuMEYwJNgrx03HF huW8XhcnCIGeZAf0X2LfD24C27B7F0GN65q1c2wVgJ/JhzE83yJTyLxYpdAjfO4Dyv1D IEdJOCv07HOPs7ZvnV40+ZwUbVPkMdzNEkKPfZUOKUD8hu/3X6KHwvlVgITzrR3md8xa FrA35w7+YLjMkTZpS/Fvh8Ubn4svsbFrOZYDq01IFiNo23VGZaikSTd0DVQUKxWizNom RKJA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695381378; x=1695986178; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=9THroEikiL60Yi8fcuQ2zOCWbKMjxLA15+wfPD4+rk8=; b=VpZMGa06c5YjCpGs1pbpgYG5IjoKOY0dEINyAF1elZ612vDwLrQ7WYgNC3daQ4WVip aZCcKIc/U7/HjEbnAFCoDigBAYL+v9+Ud9gcbt31RD5jxBADr3gvw6AmcOsOI0RNHyJm Q2jstiYvlb8ypr2ouL9019gBXk56y1q/+sJW0HQTN6Q5LiZ/PDyLAFztUqycm//q7x/j 2PrxysASKIbWioO/97cdkmSYPGuBT8w+coDWx2frylb8aBpzoyZoSlOO753fgkp/JdD9 Eq3oJCiEWCj9QZK/4hxnw/4ZLszxSytujEDR6FrllFyPj7V/gBMPbOmqgx5ffqVGIMH7 6EtQ== X-Gm-Message-State: AOJu0Yz5GueauLAMVVwy+T6q23G6L3OlIaA/ZufqjWNCufgR/SfIPnIv FZ9bjsocEW9+xhLwhZi266M= X-Received: by 2002:a05:6512:2013:b0:502:ffdf:b098 with SMTP id a19-20020a056512201300b00502ffdfb098mr6463433lfb.6.1695381377612; Fri, 22 Sep 2023 04:16:17 -0700 (PDT) Received: from dc78bmyyyyyyyyyyyyyyt-3.rev.dnainternet.fi (dc78bmyyyyyyyyyyyyyyt-3.rev.dnainternet.fi. [2001:14ba:16f8:1500::1]) by smtp.gmail.com with ESMTPSA id eq25-20020a056512489900b004fe1f1c0ee4sm696297lfb.82.2023.09.22.04.16.16 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 22 Sep 2023 04:16:16 -0700 (PDT) Date: Fri, 22 Sep 2023 14:16:08 +0300 From: Matti Vaittinen <mazziesaccount@gmail.com> To: Matti Vaittinen <mazziesaccount@gmail.com>, Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> Cc: Jonathan Cameron <jic23@kernel.org>, Lars-Peter Clausen <lars@metafoo.de>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, Matti Vaittinen <mazziesaccount@gmail.com>, Andy Shevchenko <andriy.shevchenko@linux.intel.com>, Angel Iglesias <ang.iglesiasg@gmail.com>, Andreas Klinger <ak@it-klinger.de>, Christophe JAILLET <christophe.jaillet@wanadoo.fr>, Benjamin Bara <bbara93@gmail.com>, linux-iio@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH v3 1/6] tools: iio: iio_generic_buffer ensure alignment Message-ID: <029b4e3e18c76b330b606f5b14699e5ee4e5ed35.1695380366.git.mazziesaccount@gmail.com> References: <cover.1695380366.git.mazziesaccount@gmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="ubfT4fawWH07RnEo" Content-Disposition: inline In-Reply-To: <cover.1695380366.git.mazziesaccount@gmail.com> X-Spam-Status: No, score=-0.6 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on morse.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (morse.vger.email [0.0.0.0]); Fri, 22 Sep 2023 04:16:35 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1777737131772376322 X-GMAIL-MSGID: 1777737131772376322 |
Series |
Support ROHM BM1390 pressure sensor
|
|
Commit Message
Matti Vaittinen
Sept. 22, 2023, 11:16 a.m. UTC
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(-)
Comments
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. > + 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! > + bytes += max - misalignment; > + } > > return bytes; > }
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); ? 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) > >> + 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
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
On Tue, 26 Sep 2023 13:29:02 +0300 Matti Vaittinen <mazziesaccount@gmail.com> wrote: > 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. Ah I read that backwards as you've noticed. We don't have any such big channels, so indeed have some flexibility here. I think we stick to naturally aligned so 16 bytes case would be 16 byte aligned - mostly because I don't expect to see one any time soon and because it makes the docs simpler. > > > 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. Indeed - it's meant to be power of 2 only. More than possible we don't check that rigorously enough or have it clearly documented though. The aim is for the data to be padded for efficient accesses + because it is a pain to deal with arbitrary padding so we restrict it to power of 2 naturally aligned only. One relaxation we've talked about in the past is packing multiple channels per byte (for logic analyser cases). Not done it yet though. > > 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 :) 16 byte alignement is probably not needed - but who knows for future architecture. Note this has been really non obvious in the past and is why we force alignment for timestamp elements in lots of drivers. Most architectures align an s64 in a c structure to an 8 byte boundary but x86 32 bit doesn't. Hence we have to force it all over the place. Fixing that (because userspace ABI data placement should not vary across architectures) was a pain. I don't want to do it again! > > 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...) Might be worth a power of 2 only comment. J
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? + */ + misalignment = bytes % max; + if (misalignment) { + printf("Misalignment %u. Adding Padding %u\n", misalignment, max - misalignment); + bytes += max - misalignment; + } return bytes; }