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

["Lars Möllendorf" <lars.moellendorf@plating.de>]

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



On 26.12.19 22:16, Lars-Peter Clausen wrote:
> On 12/23/19 6:05 PM, Jonathan Cameron wrote:
>> On Mon, 16 Dec 2019 08:51:27 +0100
>> Lars-Peter Clausen <lars@metafoo.de> wrote:
>>
>>> On 12/15/19 10:09 PM, Lars Möllendorf wrote:
>>>> -----Ursprüngliche Nachricht-----  
>>>>> Von: Lars Möllendorf <lars.moellendorf@plating.de>
>>>>> Gesendet: Freitag 13 Dezember 2019 14:58
>>>>> An: Jonathan Cameron <jic23@kernel.org>; Hartmut Knaack <knaack.h@gmx.de>; Lars-Peter Clausen <lars@metafoo.de>; Peter Meerwald-Stadler <pmeerw@pmeerw.net>; linux-iio@vger.kernel.org
>>>>> CC: Lars Möllendorf <lars.moellendorf@plating.de>
>>>>> Betreff: [PATCH v3] iio: buffer: align the size of scan bytes to size of the largest element
>>>>>
>>>>> Previous versions of `iio_compute_scan_bytes` only aligned each element
>>>>> to its own length (i.e. its own natural alignment). Because multiple
>>>>> consecutive sets of scan elements are buffered this does not work in
>>>>> case the computed scan bytes do not align with the natural alignment of
>>>>> the first scan element in the set.
>>>>>
>>>>> This commit fixes this by aligning the scan bytes to the natural
>>>>> alignment of the largest scan element in the set.  
>>>>
>>>>
>>>>
>>>> After re-reading my commit message, I come to the conclusion that it really is sufficient to align the scan bytes to the natural alignment of the *first* element. This would save us the `max()` comparisons for each bit. At the moment I am not at my workstation, but I could submit a v4 next Friday.
>>>>   
>>>
>>> I thought so too in the beginning, but as Jonathan pointed out, it does
>>> not work for all cases. Lets say you have u16,u16,u32,u16. If all
>>> channels are enabled the size is aligned to the first element, but the
>>> u32 would not be aligned in the second dataset.
>>>
>>
>> I'm sitting on this at the moment... Can I confirm we have consensus
>> that this patch is the correct fix?
>>
>> Lars and Lars?
> 
> Current version looks good to me.

I agree that the current implementation will fix the aforementioned cases.

> Reviewed-by: Lars-Peter Clausen <lars@metafoo.de>
> 
>>
>> Thanks,
>>
>> Jonathan
>>
>>
>>>>
>>>>   
>>>>> Fixes: 959d2952d124 ("staging:iio: make iio_sw_buffer_preenable much more
>>>>> general.")
>>>>> Signed-off-by: Lars Möllendorf <lars.moellendorf@plating.de>
>>>>> ---
>>>>> v3:
>>>>>   - Fix the problem description in the commit message
>>>>>   - Add "Fixes" tag
>>>>>
>>>>> v2:
>>>>>   - Fix subject of patch which marked it the first in a set of three.
>>>>>   - Add a description of the problem in the commit message
>>>>>
>>>>> ---
>>>>>  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 --]