diff mbox series

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

Message ID 20210501171352.512953-2-jic23@kernel.org (mailing list archive)
State New, archived
Headers show
Series IIO: Alignment fixes part 3 - __aligned(8) used to ensure alignment | expand

Commit Message

Jonathan Cameron May 1, 2021, 5:13 p.m. UTC 
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(-)

Comments

Eugen Hristev May 26, 2021, 5:23 p.m. UTC | #1 
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 ?

Thanks,
Eugen

>          /*
>           * lock to prevent concurrent 'single conversion' requests through
>           * sysfs.
> --
> 2.31.1
>
Jonathan Cameron May 27, 2021, 8:48 a.m. UTC | #2 
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
> >   
>
diff mbox series

Patch

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);
 	/*
 	 * lock to prevent concurrent 'single conversion' requests through
 	 * sysfs.