On 12/23/19 6:05 PM, Jonathan Cameron wrote:
> On Mon, 16 Dec 2019 08:51:27 +0100
> Lars-Peter Clausen <lars@xxxxxxxxxx> wrote:
>
>> On 12/15/19 10:09 PM, Lars Möllendorf wrote:
>>> -----Ursprüngliche Nachricht-----
>>>> Von: Lars Möllendorf <lars.moellendorf@xxxxxxxxxx>
>>>> Gesendet: Freitag 13 Dezember 2019 14:58
>>>> An: Jonathan Cameron <jic23@xxxxxxxxxx>; Hartmut Knaack <knaack.h@xxxxxx>; Lars-Peter Clausen <lars@xxxxxxxxxx>; Peter Meerwald-Stadler <pmeerw@xxxxxxxxxx>; linux-iio@xxxxxxxxxxxxxxx
>>>> CC: Lars Möllendorf <lars.moellendorf@xxxxxxxxxx>
>>>> 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.
Reviewed-by: Lars-Peter Clausen <lars@xxxxxxxxxx>
>
> Thanks,
>
> Jonathan
>
>
>>>
>>>
>>>> Fixes: 959d2952d124 ("staging:iio: make iio_sw_buffer_preenable much more
>>>> general.")
>>>> Signed-off-by: Lars Möllendorf <lars.moellendorf@xxxxxxxxxx>
>>>> ---
>>>> 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
>>>>
>>
>
