Re: [PATCH 01/11] iio: adc: at91-sama5d2: Fix buffer alignment in iio_push_to_buffers_with_timestamp()

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Wed, 26 May 2021 17:23:11 +0000
<Eugen.Hristev@microchip.com> wrote:

> On 5/1/21 8:13 PM, Jonathan Cameron wrote:
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > To make code more readable, use a structure to express the channel
> > layout and ensure the timestamp is 8 byte aligned.
> > 
> > Found during an audit of all calls of this function.
> > 
> > Fixes: 5e1a1da0f8c9 ("iio: adc: at91-sama5d2_adc: add hw trigger and buffer support")
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Eugen Hristev <eugen.hristev@microchip.com>
> > ---
> >   drivers/iio/adc/at91-sama5d2_adc.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> > index a7826f097b95..d356b515df09 100644
> > --- a/drivers/iio/adc/at91-sama5d2_adc.c
> > +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> > @@ -403,7 +403,8 @@ struct at91_adc_state {
> >          struct at91_adc_dma             dma_st;
> >          struct at91_adc_touch           touch_st;
> >          struct iio_dev                  *indio_dev;
> > -       u16                             buffer[AT91_BUFFER_MAX_HWORDS];
> > +       /* Ensure naturally aligned timestamp */
> > +       u16                             buffer[AT91_BUFFER_MAX_HWORDS] __aligned(8);  
> 
> Hello Jonathan,
> 
> I am preparing to change this buffer to a dynamically allocated 
> buffer... because we want to support several versions of the ADC with 
> this driver, having an arbitrary number of channels..
> 
> You think it's possible to have this alignment when I move to a 
> devm_kzalloc call ?

Hmm. You should be fine, but looking through the docs, guaranteed alignment for
devm_kmalloc is unsigned long long.  Sounds worryingly vague, but c99 (which kernel
uses) guarantees at least 64 bits so that's fine.

However, unless your maximum size is very large, I'd be cynical and just leave it
where it is.  Chances are it won't actually make any difference to the real amount
of memory allocated for at91_adc_state and it'll make your code simpler.

No one minds a buffer being too big :)

Jonathan

> 
> Thanks,
> Eugen
> 
> >          /*
> >           * lock to prevent concurrent 'single conversion' requests through
> >           * sysfs.
> > --
> > 2.31.1
> >   
>