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

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 27 Dec 2019 10:45:46 +0100
Lars Möllendorf <lars.moellendorf@plating.de> wrote:

> 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.
Applied to the fixes-togreg branch of iio.git.  Hopefully
we won't have anyone relying on the previously broken alignment...

*crosses fingers*.

Thanks,

Jonathan

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