Linux-IIO Archive on lore.kernel.org
 help / color / Atom feed
* iio_compute_scan_bytes does not seem to account for alignment if first channel uses more storagebits than its successors
@ 2019-11-29 14:30 Lars Möllendorf
  2019-11-29 17:23 ` Lars-Peter Clausen
  0 siblings, 1 reply; 10+ messages in thread
From: Lars Möllendorf @ 2019-11-29 14:30 UTC (permalink / raw)
  To: linux-iio

[-- 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 --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: iio_compute_scan_bytes does not seem to account for alignment if first channel uses more storagebits than its successors
  2019-11-29 14:30 iio_compute_scan_bytes does not seem to account for alignment if first channel uses more storagebits than its successors Lars Möllendorf
@ 2019-11-29 17:23 ` Lars-Peter Clausen
  2019-12-01 12:10   ` Jonathan Cameron
  0 siblings, 1 reply; 10+ messages in thread
From: Lars-Peter Clausen @ 2019-11-29 17:23 UTC (permalink / raw)
  To: Lars Möllendorf, linux-iio

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: iio_compute_scan_bytes does not seem to account for alignment if first channel uses more storagebits than its successors
  2019-11-29 17:23 ` Lars-Peter Clausen
@ 2019-12-01 12:10   ` Jonathan Cameron
  2019-12-01 12:29     ` Lars-Peter Clausen
  0 siblings, 1 reply; 10+ messages in thread
From: Jonathan Cameron @ 2019-12-01 12:10 UTC (permalink / raw)
  To: Lars-Peter Clausen; +Cc: Lars Möllendorf, linux-iio

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
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: iio_compute_scan_bytes does not seem to account for alignment if first channel uses more storagebits than its successors
  2019-12-01 12:10   ` Jonathan Cameron
@ 2019-12-01 12:29     ` Lars-Peter Clausen
  2019-12-02 13:01       ` [PATCH 1/3] iio: buffer: align the size of scan bytes to size of the largest element Lars Möllendorf
  0 siblings, 1 reply; 10+ messages in thread
From: Lars-Peter Clausen @ 2019-12-01 12:29 UTC (permalink / raw)
  To: Jonathan Cameron; +Cc: Lars Möllendorf, linux-iio

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.


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/3] iio: buffer: align the size of scan bytes to size of the largest element
  2019-12-01 12:29     ` Lars-Peter Clausen
@ 2019-12-02 13:01       ` Lars Möllendorf
  2019-12-02 13:37         ` Ardelean, Alexandru
  0 siblings, 1 reply; 10+ messages in thread
From: Lars Möllendorf @ 2019-12-02 13:01 UTC (permalink / raw)
  To: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, linux-iio
  Cc: 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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/3] iio: buffer: align the size of scan bytes to size of the largest element
  2019-12-02 13:01       ` [PATCH 1/3] iio: buffer: align the size of scan bytes to size of the largest element Lars Möllendorf
@ 2019-12-02 13:37         ` Ardelean, Alexandru
  2019-12-02 14:27           ` [PATCH] " Lars Möllendorf
  2019-12-04  9:24           ` [PATCH 1/3] " Lars Möllendorf
  0 siblings, 2 replies; 10+ messages in thread
From: Ardelean, Alexandru @ 2019-12-02 13:37 UTC (permalink / raw)
  To: linux-iio, lars.moellendorf, jic23, knaack.h, lars, pmeerw

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
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH] iio: buffer: align the size of scan bytes to size of the largest element
  2019-12-02 13:37         ` Ardelean, Alexandru
@ 2019-12-02 14:27           ` " Lars Möllendorf
  2019-12-06 17:34             ` Jonathan Cameron
  2019-12-04  9:24           ` [PATCH 1/3] " Lars Möllendorf
  1 sibling, 1 reply; 10+ messages in thread
From: Lars Möllendorf @ 2019-12-02 14:27 UTC (permalink / raw)
  To: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, linux-iio
  Cc: 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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/3] iio: buffer: align the size of scan bytes to size of the largest element
  2019-12-02 13:37         ` Ardelean, Alexandru
  2019-12-02 14:27           ` [PATCH] " Lars Möllendorf
@ 2019-12-04  9:24           ` " Lars Möllendorf
  2019-12-04 11:12             ` Ardelean, Alexandru
  1 sibling, 1 reply; 10+ messages in thread
From: Lars Möllendorf @ 2019-12-04  9:24 UTC (permalink / raw)
  To: Ardelean, Alexandru, linux-iio, jic23, knaack.h, lars, pmeerw

[-- 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 --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/3] iio: buffer: align the size of scan bytes to size of the largest element
  2019-12-04  9:24           ` [PATCH 1/3] " Lars Möllendorf
@ 2019-12-04 11:12             ` Ardelean, Alexandru
  0 siblings, 0 replies; 10+ messages in thread
From: Ardelean, Alexandru @ 2019-12-04 11:12 UTC (permalink / raw)
  To: lars.moellendorf, jic23, knaack.h, linux-iio, pmeerw, lars

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] iio: buffer: align the size of scan bytes to size of the largest element
  2019-12-02 14:27           ` [PATCH] " Lars Möllendorf
@ 2019-12-06 17:34             ` Jonathan Cameron
  0 siblings, 0 replies; 10+ messages in thread
From: Jonathan Cameron @ 2019-12-06 17:34 UTC (permalink / raw)
  To: Lars Möllendorf
  Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio

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;
>  }
>  


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, back to index

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-29 14:30 iio_compute_scan_bytes does not seem to account for alignment if first channel uses more storagebits than its successors Lars Möllendorf
2019-11-29 17:23 ` Lars-Peter Clausen
2019-12-01 12:10   ` Jonathan Cameron
2019-12-01 12:29     ` Lars-Peter Clausen
2019-12-02 13:01       ` [PATCH 1/3] iio: buffer: align the size of scan bytes to size of the largest element Lars Möllendorf
2019-12-02 13:37         ` Ardelean, Alexandru
2019-12-02 14:27           ` [PATCH] " Lars Möllendorf
2019-12-06 17:34             ` Jonathan Cameron
2019-12-04  9:24           ` [PATCH 1/3] " Lars Möllendorf
2019-12-04 11:12             ` Ardelean, Alexandru

Linux-IIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iio/0 linux-iio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-iio linux-iio/ https://lore.kernel.org/linux-iio \
		linux-iio@vger.kernel.org
	public-inbox-index linux-iio

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-iio


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git