| Message ID | e986b4562ca663e19ea30b81d15221c15bd87227.1695727471.git.mazziesaccount@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Support ROHM BM1390 pressure sensor | expand |
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?.. > + * 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. > + */
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; }