Message ID | e986b4562ca663e19ea30b81d15221c15bd87227.1695727471.git.mazziesaccount@gmail.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:cae8:0:b0:403:3b70:6f57 with SMTP id r8csp2683778vqu; Wed, 27 Sep 2023 07:52:39 -0700 (PDT) X-Google-Smtp-Source: AGHT+IEtEgkq4WBA2NX71SDYUYBKGyQ3FqDDWZNUD43O0bMtEGDZdUp789hQfBPMku2X9Fg27BVU X-Received: by 2002:a17:902:82c1:b0:1c6:d1f:514d with SMTP id u1-20020a17090282c100b001c60d1f514dmr1929504plz.45.1695826358881; Wed, 27 Sep 2023 07:52:38 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1695826358; cv=none; d=google.com; s=arc-20160816; b=zY0NUNlqHx/8g8eWQECl4wAAS1qW7wrIjr523bpgLorJ1FXtD05oC/GdVhNO/Cp2bQ 36OOcXz9+4B+UzwaF3LskKgn06fj9cdgXzYFmrYm3oBPIIxv9OpGsm/D3BMGmgb8n3ii CCjXIqlAZunNRli9xhCk2XS/NZZ15+xWdCOmB59Mk5xalkgFRQ294gDa2JU6NdmHi+Ko tFHzjQ7/a3z6fgBO02TJLJk58lHM63uQYKiUWalZFpEFU8W9j6d8dGAVD83bhKx1sKAc rrQOeAOMSmW38ZaCThSNz4iOmtAdVDOwEzHwOsec6N2qBDWaGioRTKJlF7f6+dlfTVpA XY6w== 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=3M+fhkSU75ZwJTVy/9LEZES58n3Sr/w+Y0+8L0nomxI=; fh=UdmbWaEkiZjiE5aS9fW0BpH9MhEg6tNeE+OQ6eH7yzc=; b=hzxDgFIqTP1hdZaUJp5r8P83nq9pvrwhXUlDndx4s8CDynRdsHNFarUEJxVH/kLJIr HmCUKkfF3ql88rjwsBKRuw01dNQigbpGFQYCAR9hu9iwJVsUOC3Q5eXqAnZy7EwGxs6+ KXTehtvP3NdZYgYAAJs6C/1x5ZHAk/8R5pLP57NzBEqFJu9pOzaVvvVd0za1oMps52g6 TNBymk+AIymxHFGxed42o5fHNcK24vn4iCPSThqht25futQi4ydNQnnwUrutiGT1n6Bv NYtxVFXkGAIx1XTUhuUcXgwWx5HNsUHZCw7kbkhlmaxdXjKiTUSY2Xvptp6N6RLUQNAT 07WQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20230601 header.b=S6bnlg6i; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 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 agentk.vger.email (agentk.vger.email. [2620:137:e000::3:2]) by mx.google.com with ESMTPS id l12-20020a170903244c00b001b801044467si16952758pls.3.2023.09.27.07.52.38 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 27 Sep 2023 07:52:38 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20230601 header.b=S6bnlg6i; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 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 agentk.vger.email (Postfix) with ESMTP id BAB8281B4551; Wed, 27 Sep 2023 01:27:22 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at agentk.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230262AbjI0I07 (ORCPT <rfc822;ruipengqi7@gmail.com> + 23 others); Wed, 27 Sep 2023 04:26:59 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:47828 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230380AbjI0I01 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 27 Sep 2023 04:26:27 -0400 Received: from mail-lf1-x12b.google.com (mail-lf1-x12b.google.com [IPv6:2a00:1450:4864:20::12b]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0A732CE9; Wed, 27 Sep 2023 01:26:23 -0700 (PDT) Received: by mail-lf1-x12b.google.com with SMTP id 2adb3069b0e04-503397ee920so16761852e87.1; Wed, 27 Sep 2023 01:26:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1695803182; x=1696407982; 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=3M+fhkSU75ZwJTVy/9LEZES58n3Sr/w+Y0+8L0nomxI=; b=S6bnlg6iw3CoIgvx97RpA9I+w/dZH1ssIGt+8PXtX2ZW1ICm+Tcuh2HwsUN7rPaAWq a9DF0IFLKSw1QzovG0nPPrExbNyyqc4POBE+lZznDUIqb5HzHN6OstWvg7lfOp1EAuAT cc2xXpxvVv5E8sOGM9T/y/nepfh0lud54ANJwqe3ZN7KIVFpZXmQdwlKQq5EZrlkQv4c Mfe50mhAk58KE14xYi/gH1y8F+EY3H2dIYcZnC+xbgcorO9IDqouYinD9dXTLrXtg6Ce r//3j1lnMEXVDyAwisflX3gLMzLM2n4/0r/NC0kuenDmq70MHYbi+ZFHjptMb1G7yNQb kyOA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695803182; x=1696407982; 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=3M+fhkSU75ZwJTVy/9LEZES58n3Sr/w+Y0+8L0nomxI=; b=MWQ0GEcHmkY+kdQ2UmT65ZkiaTpzPqOn1Z6e9Kec1srAWOgww6G6MiwZLOKinDqvGk 75uE7PnD6qzNQUH6SGdUcjkZbGxFdYEWoc13w4ZMUoOPf5StBf4Qnq1aAmimt+ATWzqu U9KY3xAXrC8ZrSpcqDyC3AwYH6IYAdlV9DPLj70jQ1A0KPC1NkA0ci0hAaKEf5cpv8hS kL5xz/sGuWqGKfjTlhHYqZ+WZAPuY/Boqp2UVe4a0rbVMPXzSXfqx5iDQjiB3k2kpzEq 4BPJy/VlogUdA3ZmNiMiuL4jJeKzuYUX7fBK1HInrUW9RH+7FVxfXCgvr+x006xkix3E SIgg== X-Gm-Message-State: AOJu0Yyevn2YS4lSq5rEj8Kh2G15OPuxzS7cTH5HjbiRB9VAfJjx9GAe M4qWfxuUySyLs9UHQuwBii/ngh/dJT4= X-Received: by 2002:a05:6512:11c6:b0:4fa:5e76:7ad4 with SMTP id h6-20020a05651211c600b004fa5e767ad4mr983405lfr.10.1695803181724; Wed, 27 Sep 2023 01:26:21 -0700 (PDT) Received: from fedora ([213.255.186.46]) by smtp.gmail.com with ESMTPSA id h18-20020a197012000000b0050309ea3a62sm2479646lfc.277.2023.09.27.01.26.20 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 27 Sep 2023 01:26:20 -0700 (PDT) Date: Wed, 27 Sep 2023 11:26:07 +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>, Benjamin Bara <bbara93@gmail.com>, Christophe JAILLET <christophe.jaillet@wanadoo.fr>, linux-iio@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH v4 1/5] tools: iio: iio_generic_buffer ensure alignment Message-ID: <e986b4562ca663e19ea30b81d15221c15bd87227.1695727471.git.mazziesaccount@gmail.com> References: <cover.1695727471.git.mazziesaccount@gmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="FUsvzO4N/DXR6Pi1" Content-Disposition: inline In-Reply-To: <cover.1695727471.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 agentk.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 (agentk.vger.email [0.0.0.0]); Wed, 27 Sep 2023 01:27:22 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1778199046015420231 X-GMAIL-MSGID: 1778202820420193650 |
Series |
Support ROHM BM1390 pressure sensor
|
|
Commit Message
Matti Vaittinen
Sept. 27, 2023, 8:26 a.m. UTC
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@gmail.com> --- 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(-)
Comments
On 9/27/23 15:27, Andy Shevchenko wrote: > On Wed, Sep 27, 2023 at 11:26:07AM +0300, Matti Vaittinen 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. > > ... > >> + /* >> + * We wan't the data in next sample to also be properly aligned so > > Pardon me, won't or want, I didn't get?.. Both :D Well, purpose was to say want, but as I try to explain we get what we want only in some case - in other cases we won't ;) Anyways, this is something that should be fixed - thanks :) > >> + * 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 > > 32-bit > >> + * the 8 byte elements to 4 byte boundary - which this code is not > > 8-byte > 4-byte > >> + * doing. >> + */ > Thanks! Yours, -- Matti
On Wed, 27 Sep 2023 11:26:07 +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 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@gmail.com> > > --- > 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! > + */ > + misalignment = bytes % max; > + if (misalignment) > + bytes += max - misalignment; > > return bytes; > }
On 9/30/23 19:34, Jonathan Cameron wrote: > On Wed, 27 Sep 2023 11:26:07 +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 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@gmail.com> >> >> --- >> 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
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. + */ + misalignment = bytes % max; + if (misalignment) + bytes += max - misalignment; return bytes; }