iio_compute_scan_bytes does not seem to account for alignment if first channel uses more storagebits than its successors

iio_compute_scan_bytes does not seem to account for alignment if first channel uses more storagebits than its successors

[Lars Möllendorf]

[-- Attachment #1: Type: text/plain, Size: 4160 bytes --]

Hi,

I have written a custom kernel module implementing the IIO device API
backed by an IIO triggered buffer.

My IIO device provides 3 channels + timestamp. The sizes of the channels are

index  | iio_chan_spec.scan_type.storagebits
-------|------------------------------------------------
   0   |  32
   1   |  16
   2   |  16

If I select channel 0 (32bit) and one of channel 1 or 2 (16bit)
indio_dev.scan_bytes and iio_buffer.bytes_per_datum have a value of 6
Byte which does not account for any alignment.


After having a closer look at  `iio_compute_scan_bytes` which is
responsible for calculating both, `indio_dev.scan_bytes` and
`iio_buffer.bytes_per_datum` it seems to me that the order of channels
matter:

```c
	/* How much space will the demuxed element take? */
	for_each_set_bit(i, mask,
			 indio_dev->masklength) {
		length = iio_storage_bytes_for_si(indio_dev, i);
		bytes = ALIGN(bytes, length);
		bytes += length;
	}
```

I understand that in case the length of each scan element is smaller
then the length of the successive scan elements, this algorithm works
because it aligns the current element to its own length. But if, as in
my case, the length of channel 0's scan elements  is greater then the
size of the samples of the consecutive channels no alignment seems to be
taken into account. Do I miss something here?

To make the algorithm work for any case the greatest length of all
enabled scan elements could be used for alignment, e.g.:

```diff
diff --git a/drivers/iio/industrialio-buffer.c
b/drivers/iio/industrialio-buffer.c
index 5d05c38c4ba9..3d2c4e26d818 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -546,16 +546,18 @@ static int iio_compute_scan_bytes(struct iio_dev
*indio_dev,
 				const unsigned long *mask, bool timestamp)
 {
 	unsigned bytes = 0;
-	int length, i;
+	int length, i, align = 0, count = 0;

 	/* How much space will the demuxed element take? */
 	for_each_set_bit(i, mask,
 			 indio_dev->masklength) {
 		length = iio_storage_bytes_for_si(indio_dev, i);
-		bytes = ALIGN(bytes, length);
-		bytes += length;
+		align = max(align, length);
+		count++;
 	}

+	bytes = count * align;
+
 	if (timestamp) {
 		length = iio_storage_bytes_for_timestamp(indio_dev);
 		bytes = ALIGN(bytes, length);

```

And if the storage bytes for timestamp have to be taken into account for
the alignment of the scan elements (what seems not to be the case as it
is currently implemented), then:

```diff
diff --git a/drivers/iio/industrialio-buffer.c
b/drivers/iio/industrialio-buffer.c
index 5d05c38c4ba9..59aee2ea4e19 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -546,21 +546,23 @@ static int iio_compute_scan_bytes(struct iio_dev
*indio_dev,
 				const unsigned long *mask, bool timestamp)
 {
 	unsigned bytes = 0;
-	int length, i;
+	int length, i, align = 0, count = 0;
+
+	if (timestamp) {
+		align = iio_storage_bytes_for_timestamp(indio_dev);
+		count++;
+	}

 	/* How much space will the demuxed element take? */
 	for_each_set_bit(i, mask,
 			 indio_dev->masklength) {
 		length = iio_storage_bytes_for_si(indio_dev, i);
-		bytes = ALIGN(bytes, length);
-		bytes += length;
+		align = max(align, length);
+		count++;
 	}

-	if (timestamp) {
-		length = iio_storage_bytes_for_timestamp(indio_dev);
-		bytes = ALIGN(bytes, length);
-		bytes += length;
-	}
+	bytes = count * align;
+
 	return bytes;
 }
```

But in my case the latter would bloat the buffer from 16 Byte to 4*16 =
64 Byte per scan if all channels are selected and timestamp is active.

For now, I will work around this by using 32 storagebits for all my
channels. This gives my 4 Bytes of overhead per scan if all elements are
selected and additional 2 Byte if timestamp is active.

In "Why do you align the buffer pointer to a multiple of the size of the
current scan element in iio_buffer_foreach_sample()?" on
https://github.com/analogdevicesinc/libiio/issues/324 I have been
pointed to this mailing list.

Regards,
Lars M.

[-- Attachment #2: pEpkey.asc --]
[-- Type: application/pgp-keys, Size: 1809 bytes --]

Re: iio_compute_scan_bytes does not seem to account for alignment if first channel uses more storagebits than its successors

[Lars-Peter Clausen]

On 11/29/19 3:30 PM, Lars Möllendorf wrote:
> Hi,
> 
> I have written a custom kernel module implementing the IIO device API
> backed by an IIO triggered buffer.
> 
> My IIO device provides 3 channels + timestamp. The sizes of the channels are
> 
> index  | iio_chan_spec.scan_type.storagebits
> -------|------------------------------------------------
>    0   |  32
>    1   |  16
>    2   |  16
> 
> If I select channel 0 (32bit) and one of channel 1 or 2 (16bit)
> indio_dev.scan_bytes and iio_buffer.bytes_per_datum have a value of 6
> Byte which does not account for any alignment.
> 
> 
> After having a closer look at  `iio_compute_scan_bytes` which is
> responsible for calculating both, `indio_dev.scan_bytes` and
> `iio_buffer.bytes_per_datum` it seems to me that the order of channels
> matter:
> 
> ```c
> 	/* How much space will the demuxed element take? */
> 	for_each_set_bit(i, mask,
> 			 indio_dev->masklength) {
> 		length = iio_storage_bytes_for_si(indio_dev, i);
> 		bytes = ALIGN(bytes, length);
> 		bytes += length;
> 	}
> ```
> 
> I understand that in case the length of each scan element is smaller
> then the length of the successive scan elements, this algorithm works
> because it aligns the current element to its own length. But if, as in
> my case, the length of channel 0's scan elements  is greater then the
> size of the samples of the consecutive channels no alignment seems to be
> taken into account. Do I miss something here?
[...]
> But in my case the latter would bloat the buffer from 16 Byte to 4*16 =
> 64 Byte per scan if all channels are selected and timestamp is active.
> 
> For now, I will work around this by using 32 storagebits for all my
> channels. This gives my 4 Bytes of overhead per scan if all elements are
> selected and additional 2 Byte if timestamp is active.
> 
> In "Why do you align the buffer pointer to a multiple of the size of the
> current scan element in iio_buffer_foreach_sample()?" on
> https://github.com/analogdevicesinc/libiio/issues/324 I have been
> pointed to this mailing list.

Hi Lars,

The way this is supposed to work is that each element is aligned to its
own natural alignment. What seems to be missing at the moment is that
the total length is also aligned to the size of the first element, so
that alignment is preserved for multiple consecutive samples. I feel
like we had that at some point, but maybe I'm misremembering.

E.g. putting something like

 unsigned int first_index = find_first_bit(mask, indio_dev->masklength);
 length = iio_storage_bytes_for_si(indio_dev, first_index);
 bytes = ALIGN(bytes, length);

below the loop should do the trick I believe.

- Lars

Re: iio_compute_scan_bytes does not seem to account for alignment if first channel uses more storagebits than its successors

[Jonathan Cameron]

On Fri, 29 Nov 2019 18:23:37 +0100
Lars-Peter Clausen <lars@metafoo.de> wrote:

> On 11/29/19 3:30 PM, Lars Möllendorf wrote:
> > Hi,
> > 
> > I have written a custom kernel module implementing the IIO device API
> > backed by an IIO triggered buffer.
> > 
> > My IIO device provides 3 channels + timestamp. The sizes of the channels are
> > 
> > index  | iio_chan_spec.scan_type.storagebits
> > -------|------------------------------------------------
> >    0   |  32
> >    1   |  16
> >    2   |  16
> > 
> > If I select channel 0 (32bit) and one of channel 1 or 2 (16bit)
> > indio_dev.scan_bytes and iio_buffer.bytes_per_datum have a value of 6
> > Byte which does not account for any alignment.
> > 
> > 
> > After having a closer look at  `iio_compute_scan_bytes` which is
> > responsible for calculating both, `indio_dev.scan_bytes` and
> > `iio_buffer.bytes_per_datum` it seems to me that the order of channels
> > matter:
> > 
> > ```c
> > 	/* How much space will the demuxed element take? */
> > 	for_each_set_bit(i, mask,
> > 			 indio_dev->masklength) {
> > 		length = iio_storage_bytes_for_si(indio_dev, i);
> > 		bytes = ALIGN(bytes, length);
> > 		bytes += length;
> > 	}
> > ```
> > 
> > I understand that in case the length of each scan element is smaller
> > then the length of the successive scan elements, this algorithm works
> > because it aligns the current element to its own length. But if, as in
> > my case, the length of channel 0's scan elements  is greater then the
> > size of the samples of the consecutive channels no alignment seems to be
> > taken into account. Do I miss something here?  
> [...]
> > But in my case the latter would bloat the buffer from 16 Byte to 4*16 =
> > 64 Byte per scan if all channels are selected and timestamp is active.
> > 
> > For now, I will work around this by using 32 storagebits for all my
> > channels. This gives my 4 Bytes of overhead per scan if all elements are
> > selected and additional 2 Byte if timestamp is active.
> > 
> > In "Why do you align the buffer pointer to a multiple of the size of the
> > current scan element in iio_buffer_foreach_sample()?" on
> > https://github.com/analogdevicesinc/libiio/issues/324 I have been
> > pointed to this mailing list.  
> 
> Hi Lars,
> 
> The way this is supposed to work is that each element is aligned to its
> own natural alignment. What seems to be missing at the moment is that
> the total length is also aligned to the size of the first element, so
> that alignment is preserved for multiple consecutive samples. I feel
> like we had that at some point, but maybe I'm misremembering.
> 
> E.g. putting something like
> 
>  unsigned int first_index = find_first_bit(mask, indio_dev->masklength);
>  length = iio_storage_bytes_for_si(indio_dev, first_index);
>  bytes = ALIGN(bytes, length);
> 
> below the loop should do the trick I believe.

Good find by the way.  Not sure how we never hit this before as there
are definitely sensors out there with similar mixes to you have.
Maybe they are always used with the timestamp enabled?  Anyhow, no
matter it's clearly wrong.

Lars trick doesn't work either (I think) as we can have

u16
u16 (might be padding or real channel)
u32
u16

which should be 4 byte aligned.

I think we need to keep track of the largest element present during
the loop and use that length afterwards to pad out the end, but only
the end rather than every element.  In theory we can also have
larger elements than the timestamp, so should not do special handling
for that, it's just another element.

I think that ends up effectively a combination of the two suggestions?

Jonathan

> 
> - Lars
> 

Re: iio_compute_scan_bytes does not seem to account for alignment if first channel uses more storagebits than its successors

[Jonathan Cameron]

On Fri, 29 Nov 2019 18:23:37 +0100
Lars-Peter Clausen <lars@metafoo.de> wrote:

> On 11/29/19 3:30 PM, Lars Möllendorf wrote:
> > Hi,
> > 
> > I have written a custom kernel module implementing the IIO device API
> > backed by an IIO triggered buffer.
> > 
> > My IIO device provides 3 channels + timestamp. The sizes of the channels are
> > 
> > index  | iio_chan_spec.scan_type.storagebits
> > -------|------------------------------------------------
> >    0   |  32
> >    1   |  16
> >    2   |  16
> > 
> > If I select channel 0 (32bit) and one of channel 1 or 2 (16bit)
> > indio_dev.scan_bytes and iio_buffer.bytes_per_datum have a value of 6
> > Byte which does not account for any alignment.
> > 
> > 
> > After having a closer look at  `iio_compute_scan_bytes` which is
> > responsible for calculating both, `indio_dev.scan_bytes` and
> > `iio_buffer.bytes_per_datum` it seems to me that the order of channels
> > matter:
> > 
> > ```c
> > 	/* How much space will the demuxed element take? */
> > 	for_each_set_bit(i, mask,
> > 			 indio_dev->masklength) {
> > 		length = iio_storage_bytes_for_si(indio_dev, i);
> > 		bytes = ALIGN(bytes, length);
> > 		bytes += length;
> > 	}
> > ```
> > 
> > I understand that in case the length of each scan element is smaller
> > then the length of the successive scan elements, this algorithm works
> > because it aligns the current element to its own length. But if, as in
> > my case, the length of channel 0's scan elements  is greater then the
> > size of the samples of the consecutive channels no alignment seems to be
> > taken into account. Do I miss something here?  
> [...]
> > But in my case the latter would bloat the buffer from 16 Byte to 4*16 =
> > 64 Byte per scan if all channels are selected and timestamp is active.
> > 
> > For now, I will work around this by using 32 storagebits for all my
> > channels. This gives my 4 Bytes of overhead per scan if all elements are
> > selected and additional 2 Byte if timestamp is active.
> > 
> > In "Why do you align the buffer pointer to a multiple of the size of the
> > current scan element in iio_buffer_foreach_sample()?" on
> > https://github.com/analogdevicesinc/libiio/issues/324 I have been
> > pointed to this mailing list.  
> 
> Hi Lars,
I managed to crash my email client whilst sending a first reply to this
so no idea if anyone got it!
> 
> The way this is supposed to work is that each element is aligned to its
> own natural alignment. What seems to be missing at the moment is that
> the total length is also aligned to the size of the first element, so
> that alignment is preserved for multiple consecutive samples. I feel
> like we had that at some point, but maybe I'm misremembering.

It's more than possible we broke this in a cleanup at some stage
and a bit surprising it hasn't caused problems before as we definitely
have other sensors with similar channel patterns. I guess maybe they
always have the timestamp enabled.

Good spot by the way.

> 
> E.g. putting something like
> 
>  unsigned int first_index = find_first_bit(mask, indio_dev->masklength);
>  length = iio_storage_bytes_for_si(indio_dev, first_index);
>  bytes = ALIGN(bytes, length);
> 
> below the loop should do the trick I believe.
It would work for Lars case, but I think a combination of both approaches
is needed to handle cases like

u16
u16 (may be padding)
u32
u16

We need to realign to the largest sized element whereever it occurs in the
channels list.  So find that whilst computing the individual alignments,
and apply it only once at the end.

Thanks,

Jonathan

> 
> - Lars
> 

Re: iio_compute_scan_bytes does not seem to account for alignment if first channel uses more storagebits than its successors

[Lars-Peter Clausen]

On 12/1/19 1:10 PM, Jonathan Cameron wrote:
>>
>> E.g. putting something like
>>
>>  unsigned int first_index = find_first_bit(mask, indio_dev->masklength);
>>  length = iio_storage_bytes_for_si(indio_dev, first_index);
>>  bytes = ALIGN(bytes, length);
>>
>> below the loop should do the trick I believe.
> It would work for Lars case, but I think a combination of both approaches
> is needed to handle cases like
> 
> u16
> u16 (may be padding)
> u32
> u16
> 
> We need to realign to the largest sized element whereever it occurs in the
> channels list.  So find that whilst computing the individual alignments,
> and apply it only once at the end.
> 

Ah, yes. We need the same rules as for the alignment or padding of a
struct, which mean align the size to size of the largest element.

[PATCH 1/3] iio: buffer: align the size of scan bytes to size of the largest element

[Lars Möllendorf]

Signed-off-by: Lars Möllendorf <lars.moellendorf@plating.de>
---
 drivers/iio/industrialio-buffer.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 5d05c38c4ba9..2f037cd59d53 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -546,7 +546,7 @@ static int iio_compute_scan_bytes(struct iio_dev *indio_dev,
 				const unsigned long *mask, bool timestamp)
 {
 	unsigned bytes = 0;
-	int length, i;
+	int length, i, largest = 0;

 	/* How much space will the demuxed element take? */
 	for_each_set_bit(i, mask,
@@ -554,13 +554,17 @@ static int iio_compute_scan_bytes(struct iio_dev *indio_dev,
 		length = iio_storage_bytes_for_si(indio_dev, i);
 		bytes = ALIGN(bytes, length);
 		bytes += length;
+		largest = max(largest, length);
 	}

 	if (timestamp) {
 		length = iio_storage_bytes_for_timestamp(indio_dev);
 		bytes = ALIGN(bytes, length);
 		bytes += length;
+		largest = max(largest, length);
 	}
+
+	bytes = ALIGN(bytes, largest);
 	return bytes;
 }

--
2.23.0

Re: [PATCH 1/3] iio: buffer: align the size of scan bytes to size of the largest element

[Ardelean, Alexandru]

On Mon, 2019-12-02 at 14:01 +0100, Lars Möllendorf wrote:
> 

Hey Lars,

Thank you for the patch.

Could you add a description of the problem in the commit description?
You did a really great job on describing it via email earlier, and it would
be great to have it in the git history as well.

Also, this patch is marked 1/3 ; curios: are there 2 more patches in a set?
Sometimes, some patches get lost via email clients/servers.

Maybe Jonathan [or someone else] has some more points to this.

Thanks
Alex

> Signed-off-by: Lars Möllendorf <lars.moellendorf@plating.de>
> ---
>  drivers/iio/industrialio-buffer.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/industrialio-buffer.c
> b/drivers/iio/industrialio-buffer.c
> index 5d05c38c4ba9..2f037cd59d53 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -546,7 +546,7 @@ static int iio_compute_scan_bytes(struct iio_dev
> *indio_dev,
>  				const unsigned long *mask, bool timestamp)
>  {
>  	unsigned bytes = 0;
> -	int length, i;
> +	int length, i, largest = 0;
> 
>  	/* How much space will the demuxed element take? */
>  	for_each_set_bit(i, mask,
> @@ -554,13 +554,17 @@ static int iio_compute_scan_bytes(struct iio_dev
> *indio_dev,
>  		length = iio_storage_bytes_for_si(indio_dev, i);
>  		bytes = ALIGN(bytes, length);
>  		bytes += length;
> +		largest = max(largest, length);
>  	}
> 
>  	if (timestamp) {
>  		length = iio_storage_bytes_for_timestamp(indio_dev);
>  		bytes = ALIGN(bytes, length);
>  		bytes += length;
> +		largest = max(largest, length);
>  	}
> +
> +	bytes = ALIGN(bytes, largest);
>  	return bytes;
>  }
> 
> --
> 2.23.0
> 

[PATCH] iio: buffer: align the size of scan bytes to size of the largest element

[Lars Möllendorf]

Previous versions of `iio_compute_scan_bytes` only aligned the current
element to its own length (i.e. its own natural alignment). This worked
in case the length of each channel's scan elements had been smaller then
the length of the successive channel's scan elements.

E.g.

u16
u32 <- aligned to its natural alignment

But if the length of a channel's scan elements is greater then the
length of scan elements of the consecutive channel no alignment had
been taken into account. This is however needed to preserve the
alignment for multiple consecutive sets of scan elements.

u32 <- alignment is off by two byte for the second set of scan elements
u16 <- no alignment takes place

This commit fixes this by aligning the scan bytes to the size of the
largest scan element.

Signed-off-by: Lars Möllendorf <lars.moellendorf@plating.de>
---
 drivers/iio/industrialio-buffer.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 5d05c38c4ba9..2f037cd59d53 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -546,7 +546,7 @@ static int iio_compute_scan_bytes(struct iio_dev *indio_dev,
 				const unsigned long *mask, bool timestamp)
 {
 	unsigned bytes = 0;
-	int length, i;
+	int length, i, largest = 0;
 
 	/* How much space will the demuxed element take? */
 	for_each_set_bit(i, mask,
@@ -554,13 +554,17 @@ static int iio_compute_scan_bytes(struct iio_dev *indio_dev,
 		length = iio_storage_bytes_for_si(indio_dev, i);
 		bytes = ALIGN(bytes, length);
 		bytes += length;
+		largest = max(largest, length);
 	}
 
 	if (timestamp) {
 		length = iio_storage_bytes_for_timestamp(indio_dev);
 		bytes = ALIGN(bytes, length);
 		bytes += length;
+		largest = max(largest, length);
 	}
+
+	bytes = ALIGN(bytes, largest);
 	return bytes;
 }
 
-- 
2.23.0

Re: [PATCH 1/3] iio: buffer: align the size of scan bytes to size of the largest element

[Lars Möllendorf]

[-- Attachment #1: Type: text/plain, Size: 4345 bytes --]

On 02.12.19 14:37, Ardelean, Alexandru wrote:
> On Mon, 2019-12-02 at 14:01 +0100, Lars Möllendorf wrote:
>>
> 
> Hey Lars,
> 
> Thank you for the patch.
> 
> Could you add a description of the problem in the commit description?
> You did a really great job on describing it via email earlier, and it would
> be great to have it in the git history as well.

Is the description in my latest patch ok?


> Also, this patch is marked 1/3 ; curios: are there 2 more patches in a set?
> Sometimes, some patches get lost via email clients/servers.

No, there is only one patch. I just did not use `git format-patch`
correctly in my first attempt.

> Maybe Jonathan [or someone else] has some more points to this.

Anything else I can do to improve the patch? It is the first time I am
trying to submit a patch to the kernel. Would be nice to know if it is
accepted and if not, why. So I can learn from my mistakes.

> 
> Thanks
> Alex
> 
>> Signed-off-by: Lars Möllendorf <lars.moellendorf@plating.de>
>> ---
>>  drivers/iio/industrialio-buffer.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iio/industrialio-buffer.c
>> b/drivers/iio/industrialio-buffer.c
>> index 5d05c38c4ba9..2f037cd59d53 100644
>> --- a/drivers/iio/industrialio-buffer.c
>> +++ b/drivers/iio/industrialio-buffer.c
>> @@ -546,7 +546,7 @@ static int iio_compute_scan_bytes(struct iio_dev
>> *indio_dev,
>>  				const unsigned long *mask, bool timestamp)
>>  {
>>  	unsigned bytes = 0;
>> -	int length, i;
>> +	int length, i, largest = 0;
>>
>>  	/* How much space will the demuxed element take? */
>>  	for_each_set_bit(i, mask,
>> @@ -554,13 +554,17 @@ static int iio_compute_scan_bytes(struct iio_dev
>> *indio_dev,
>>  		length = iio_storage_bytes_for_si(indio_dev, i);
>>  		bytes = ALIGN(bytes, length);
>>  		bytes += length;
>> +		largest = max(largest, length);
>>  	}
>>
>>  	if (timestamp) {
>>  		length = iio_storage_bytes_for_timestamp(indio_dev);
>>  		bytes = ALIGN(bytes, length);
>>  		bytes += length;
>> +		largest = max(largest, length);
>>  	}
>> +
>> +	bytes = ALIGN(bytes, largest);
>>  	return bytes;
>>  }
>>
>> --
>> 2.23.0
>>

-- 

Lars Möllendorf, B. Eng.


Tel.:    +49 (0) 7641 93500-425
Fax:     +49 (0) 7641 93500-999
E-Mail:  lars.moellendorf@plating.de <mailto:lars.moellendorf@plating.de>
Website: www.plating.de <http://www.plating.de>

--------------------------------
plating electronic GmbH - Amtsgericht Freiburg - HRB Nr. 260 592 /
Geschäftsführer Karl Rieder / Rheinstraße 4 – 79350 Sexau – Tel.:+49 (0)
7641 – 93500-0

--------------------------------
Der Inhalt dieser E-Mail ist vertraulich und ausschließlich für den
bezeichneten Adressaten bestimmt. Wenn Sie nicht der vorgesehene
Adressat dieser E-Mail oder dessen Vertreter sein sollten, so beachten
Sie bitte, dass jede Form der Kenntnisnahme, Veröffentlichung,
Vervielfältigung oder Weitergabe des Inhalts dieser E-Mail unzulässig
ist. Wir bitten Sie, sich in diesem Fall mit dem Absender der E-Mail in
Verbindung zu setzen. Aussagen gegenüber  dem Adressaten unterliegen den
Regelungen des zugrundeliegenden Angebotes bzw. Auftrags, insbesondere
den Allgemeinen Geschäftsbedingungen und der individuellen
Haftungsvereinbarung. Der Inhalt der E-Mail ist nur rechtsverbindlich,
wenn er unsererseits durch einen Brief oder ein Telefax entsprechend
bestaetigt wird.

The information contained in this email is confidential. It is intended
solely for the addressee. Access to this email by anyone else is
unauthorized. If you are not the intended recipient, any form of
disclosure, reproduction, distribution or any action taken or refrained
from in reliance on it, is prohibited and may be unlawful. Please notify
the sender immediately. All statements of opinion or advice directed via
this email to our clients are subject to the terms and conditions
expressed in the governing client engagement letter. The content of this
email is not legally binding unless confirmed by letter or fax.

Although plating electronic GmbH attempts to sweep e-mail and
attachments for viruses, it does not guarantee that either are
virus-free and accepts no liability for any damage sustained as a result
of viruses.


[-- Attachment #2: pEpkey.asc --]
[-- Type: application/pgp-keys, Size: 1809 bytes --]

Re: [PATCH 1/3] iio: buffer: align the size of scan bytes to size of the largest element

[Ardelean, Alexandru]

On Wed, 2019-12-04 at 10:24 +0100, Lars Möllendorf wrote:
> On 02.12.19 14:37, Ardelean, Alexandru wrote:
> > On Mon, 2019-12-02 at 14:01 +0100, Lars Möllendorf wrote:
> > 
> > Hey Lars,
> > 
> > Thank you for the patch.
> > 
> > Could you add a description of the problem in the commit description?
> > You did a really great job on describing it via email earlier, and it
> > would
> > be great to have it in the git history as well.
> 
> Is the description in my latest patch ok?

Looks good to me.

I won't add any formal tags (Reviewed-by), since I don't understand the
full-scope of the issue.
I was just replying to the lack of description the commit.

> 
> 
> > Also, this patch is marked 1/3 ; curios: are there 2 more patches in a
> > set?
> > Sometimes, some patches get lost via email clients/servers.
> 
> No, there is only one patch. I just did not use `git format-patch`
> correctly in my first attempt.
> 
> > Maybe Jonathan [or someone else] has some more points to this.
> 
> Anything else I can do to improve the patch? It is the first time I am
> trying to submit a patch to the kernel. Would be nice to know if it is
> accepted and if not, why. So I can learn from my mistakes.
> 

So, Jonathan may come back to this and reply.
I have nothing more to add.

Typically he does that during weekends; but sometimes he replies during the
week.
If you don't get a reply from him in 1-2 weeks, maybe send a ping-email.
But, if he gets around it this week[end], then at least your patch has the
description part now :)


> > Thanks
> > Alex
> > 
> > > Signed-off-by: Lars Möllendorf <lars.moellendorf@plating.de>
> > > ---
> > >  drivers/iio/industrialio-buffer.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/iio/industrialio-buffer.c
> > > b/drivers/iio/industrialio-buffer.c
> > > index 5d05c38c4ba9..2f037cd59d53 100644
> > > --- a/drivers/iio/industrialio-buffer.c
> > > +++ b/drivers/iio/industrialio-buffer.c
> > > @@ -546,7 +546,7 @@ static int iio_compute_scan_bytes(struct iio_dev
> > > *indio_dev,
> > >  				const unsigned long *mask, bool timestamp)
> > >  {
> > >  	unsigned bytes = 0;
> > > -	int length, i;
> > > +	int length, i, largest = 0;
> > > 
> > >  	/* How much space will the demuxed element take? */
> > >  	for_each_set_bit(i, mask,
> > > @@ -554,13 +554,17 @@ static int iio_compute_scan_bytes(struct
> > > iio_dev
> > > *indio_dev,
> > >  		length = iio_storage_bytes_for_si(indio_dev, i);
> > >  		bytes = ALIGN(bytes, length);
> > >  		bytes += length;
> > > +		largest = max(largest, length);
> > >  	}
> > > 
> > >  	if (timestamp) {
> > >  		length = iio_storage_bytes_for_timestamp(indio_dev);
> > >  		bytes = ALIGN(bytes, length);
> > >  		bytes += length;
> > > +		largest = max(largest, length);
> > >  	}
> > > +
> > > +	bytes = ALIGN(bytes, largest);
> > >  	return bytes;
> > >  }
> > > 
> > > --
> > > 2.23.0
> > > 

Re: [PATCH] iio: buffer: align the size of scan bytes to size of the largest element

[Jonathan Cameron]

On Mon,  2 Dec 2019 15:27:14 +0100
Lars Möllendorf <lars.moellendorf@plating.de> wrote:

Even when fixing a patch send issue as you did here, please mark clearly
with v2 and add a change log saying that's what you did.  Also
please don't send in reply to the earlier thread, but instead as a
completely new thread.   The reply to thread thing seems sensible for
simple cases, but when we get to v20 (it happens ;) things get really
confusing :)

> Previous versions of `iio_compute_scan_bytes` only aligned the current
> element to its own length (i.e. its own natural alignment). This worked
> in case the length of each channel's scan elements had been smaller then
> the length of the successive channel's scan elements.

I'd modify this description a little bit.  The key thing was the length of the
last element - didn't matter what happened before that.

u32
u16
u32
would be aligned correctly I hope?

> 
> E.g.
> 
> u16
> u32 <- aligned to its natural alignment
> 
> But if the length of a channel's scan elements is greater then the
> length of scan elements of the consecutive channel no alignment had
> been taken into account. This is however needed to preserve the
> alignment for multiple consecutive sets of scan elements.
> 
> u32 <- alignment is off by two byte for the second set of scan elements
> u16 <- no alignment takes place
> 
> This commit fixes this by aligning the scan bytes to the size of the
> largest scan element.
> 
> Signed-off-by: Lars Möllendorf <lars.moellendorf@plating.de>
Code looks good to me, so just a v3 tidying up the message.

Thanks for sorting this out!  Ideally we'd add a specific
fixes tag as well for when this was introduced but I fear
it goes back nearly all the way (at least before the move
out of staging) so we can probably leave that vague for this
one unless you fancy some archaeology. 

Jonathan

> ---
>  drivers/iio/industrialio-buffer.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 5d05c38c4ba9..2f037cd59d53 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -546,7 +546,7 @@ static int iio_compute_scan_bytes(struct iio_dev *indio_dev,
>  				const unsigned long *mask, bool timestamp)
>  {
>  	unsigned bytes = 0;
> -	int length, i;
> +	int length, i, largest = 0;
>  
>  	/* How much space will the demuxed element take? */
>  	for_each_set_bit(i, mask,
> @@ -554,13 +554,17 @@ static int iio_compute_scan_bytes(struct iio_dev *indio_dev,
>  		length = iio_storage_bytes_for_si(indio_dev, i);
>  		bytes = ALIGN(bytes, length);
>  		bytes += length;
> +		largest = max(largest, length);
>  	}
>  
>  	if (timestamp) {
>  		length = iio_storage_bytes_for_timestamp(indio_dev);
>  		bytes = ALIGN(bytes, length);
>  		bytes += length;
> +		largest = max(largest, length);
>  	}
> +
> +	bytes = ALIGN(bytes, largest);
>  	return bytes;
>  }
>