[PATCH] iio: ad_sigma_delta: allocate data dynamically for samples

[PATCH] iio: ad_sigma_delta: allocate data dynamically for samples

[Alexandru Ardelean]

This change has a few parts:
1. Remove the buffer from the trigger handler's stack
2. Having it dynamically allocated means it should be cache-aligned
3. The buffer would now adapt to the actual number of bytes needed,
   whether it's more than 16 bytes, or less
4. Having it in the heap somewhere, allows it to work with DMA

This is a fix + enhancement in one.

Fixes: af3008485ea03 ("iio: ad_sigma_delta: allocate data dynamically for samples")
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/adc/ad_sigma_delta.c       | 12 ++++++++++--
 include/linux/iio/adc/ad_sigma_delta.h |  2 ++
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
index ec0e38566ece..91d5dda53d29 100644
--- a/drivers/iio/adc/ad_sigma_delta.c
+++ b/drivers/iio/adc/ad_sigma_delta.c
@@ -360,6 +360,11 @@ static int ad_sd_buffer_postenable(struct iio_dev *indio_dev)
 	if (ret)
 		return ret;
 
+	kfree(sigma_delta->buf_data);
+	sigma_delta->buf_data = kzalloc(indio_dev->scan_bytes, GFP_KERNEL);
+	if (!sigma_delta->buf_data)
+		return -ENOMEM;
+
 	spi_bus_lock(sigma_delta->spi->master);
 	sigma_delta->bus_locked = true;
 	sigma_delta->keep_cs_asserted = true;
@@ -403,12 +408,12 @@ static irqreturn_t ad_sd_trigger_handler(int irq, void *p)
 	struct iio_poll_func *pf = p;
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct ad_sigma_delta *sigma_delta = iio_device_get_drvdata(indio_dev);
+	uint8_t *data = sigma_delta->buf_data;
 	unsigned int reg_size;
 	unsigned int data_reg;
-	uint8_t data[16];
 	int ret;
 
-	memset(data, 0x00, 16);
+	memset(data, 0x00, indio_dev->scan_bytes);
 
 	reg_size = indio_dev->channels[0].scan_type.realbits +
 			indio_dev->channels[0].scan_type.shift;
@@ -568,6 +573,9 @@ EXPORT_SYMBOL_GPL(ad_sd_setup_buffer_and_trigger);
  */
 void ad_sd_cleanup_buffer_and_trigger(struct iio_dev *indio_dev)
 {
+	struct ad_sigma_delta *sigma_delta = iio_device_get_drvdata(indio_dev);
+
+	kfree(sigma_delta->buf_data);
 	ad_sd_remove_trigger(indio_dev);
 	iio_triggered_buffer_cleanup(indio_dev);
 }
diff --git a/include/linux/iio/adc/ad_sigma_delta.h b/include/linux/iio/adc/ad_sigma_delta.h
index 6e9fb1932dde..36dc49b8dfd5 100644
--- a/include/linux/iio/adc/ad_sigma_delta.h
+++ b/include/linux/iio/adc/ad_sigma_delta.h
@@ -75,6 +75,8 @@ struct ad_sigma_delta {
 
 	const struct ad_sigma_delta_info *info;
 
+	uint8_t			*buf_data;
+
 	/*
 	 * DMA (thus cache coherency maintenance) requires the
 	 * transfer buffers to live in their own cache lines.
-- 
2.20.1

Re: [PATCH] iio: ad_sigma_delta: allocate data dynamically for samples

[Jonathan Cameron]

On Thu, 30 May 2019 10:59:11 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> This change has a few parts:
> 1. Remove the buffer from the trigger handler's stack
> 2. Having it dynamically allocated means it should be cache-aligned
> 3. The buffer would now adapt to the actual number of bytes needed,
>    whether it's more than 16 bytes, or less
> 4. Having it in the heap somewhere, allows it to work with DMA
> 
> This is a fix + enhancement in one.
> 
> Fixes: af3008485ea03 ("iio: ad_sigma_delta: allocate data dynamically for samples")
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

Comment inline.  This is using an element of iio_dev that is marked
INTERN in the header...

> ---
>  drivers/iio/adc/ad_sigma_delta.c       | 12 ++++++++++--
>  include/linux/iio/adc/ad_sigma_delta.h |  2 ++
>  2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
> index ec0e38566ece..91d5dda53d29 100644
> --- a/drivers/iio/adc/ad_sigma_delta.c
> +++ b/drivers/iio/adc/ad_sigma_delta.c
> @@ -360,6 +360,11 @@ static int ad_sd_buffer_postenable(struct iio_dev *indio_dev)
>  	if (ret)
>  		return ret;
>  
> +	kfree(sigma_delta->buf_data);
> +	sigma_delta->buf_data = kzalloc(indio_dev->scan_bytes, GFP_KERNEL);
Hmm. Another thing that I'm trying to stop drivers doing is accessing elements
of the struct iio_dev that have been marked in the header as INTERN.

That really restricts the ability to refactor that code.

The argument has always been that a driver ought to be able to 'tell' how
large a buffer it can conceivably need and often it will actually use
a 'superset' of that to avoid the need to do additional allocations.

It's a it trickier when there is a generic layer inbetween the driver and
the core like we have here. 

If the below 16 is correct, you are almost certain to not actually see any
advantage in a separate allocation.  So two options come to mind.

1) Ensure ad_sigma_delta is appropriately aligned in the priv data
of each driver, then use ___cacheline_aligned on a fixed 16 byte buffer
in ad_sigma_delta.
2) Do the separate allocation, but stick to 16 bytes (you will always
get more than that anyway as always at least a cacheline - otherwise
same DMA issue might occur).

> +	if (!sigma_delta->buf_data)
> +		return -ENOMEM;
> +
>  	spi_bus_lock(sigma_delta->spi->master);
>  	sigma_delta->bus_locked = true;
>  	sigma_delta->keep_cs_asserted = true;
> @@ -403,12 +408,12 @@ static irqreturn_t ad_sd_trigger_handler(int irq, void *p)
>  	struct iio_poll_func *pf = p;
>  	struct iio_dev *indio_dev = pf->indio_dev;
>  	struct ad_sigma_delta *sigma_delta = iio_device_get_drvdata(indio_dev);
> +	uint8_t *data = sigma_delta->buf_data;
>  	unsigned int reg_size;
>  	unsigned int data_reg;
> -	uint8_t data[16];
>  	int ret;
>  
> -	memset(data, 0x00, 16);
> +	memset(data, 0x00, indio_dev->scan_bytes);
>  
>  	reg_size = indio_dev->channels[0].scan_type.realbits +
>  			indio_dev->channels[0].scan_type.shift;
> @@ -568,6 +573,9 @@ EXPORT_SYMBOL_GPL(ad_sd_setup_buffer_and_trigger);
>   */
>  void ad_sd_cleanup_buffer_and_trigger(struct iio_dev *indio_dev)
>  {
> +	struct ad_sigma_delta *sigma_delta = iio_device_get_drvdata(indio_dev);
> +
> +	kfree(sigma_delta->buf_data);
>  	ad_sd_remove_trigger(indio_dev);
>  	iio_triggered_buffer_cleanup(indio_dev);
>  }
> diff --git a/include/linux/iio/adc/ad_sigma_delta.h b/include/linux/iio/adc/ad_sigma_delta.h
> index 6e9fb1932dde..36dc49b8dfd5 100644
> --- a/include/linux/iio/adc/ad_sigma_delta.h
> +++ b/include/linux/iio/adc/ad_sigma_delta.h
> @@ -75,6 +75,8 @@ struct ad_sigma_delta {
>  
>  	const struct ad_sigma_delta_info *info;
>  
> +	uint8_t			*buf_data;
> +
>  	/*
>  	 * DMA (thus cache coherency maintenance) requires the
>  	 * transfer buffers to live in their own cache lines.

Re: [PATCH] iio: ad_sigma_delta: allocate data dynamically for samples

[Ardelean, Alexandru]

On Sat, 2019-06-08 at 15:21 +0100, Jonathan Cameron wrote:
> [External]
> 
> 
> On Thu, 30 May 2019 10:59:11 +0300
> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> 
> > This change has a few parts:
> > 1. Remove the buffer from the trigger handler's stack
> > 2. Having it dynamically allocated means it should be cache-aligned
> > 3. The buffer would now adapt to the actual number of bytes needed,
> >    whether it's more than 16 bytes, or less
> > 4. Having it in the heap somewhere, allows it to work with DMA
> > 
> > This is a fix + enhancement in one.
> > 
> > Fixes: af3008485ea03 ("iio: ad_sigma_delta: allocate data dynamically for samples")
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> 
> Comment inline.  This is using an element of iio_dev that is marked
> INTERN in the header...
> 
> > ---
> >  drivers/iio/adc/ad_sigma_delta.c       | 12 ++++++++++--
> >  include/linux/iio/adc/ad_sigma_delta.h |  2 ++
> >  2 files changed, 12 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
> > index ec0e38566ece..91d5dda53d29 100644
> > --- a/drivers/iio/adc/ad_sigma_delta.c
> > +++ b/drivers/iio/adc/ad_sigma_delta.c
> > @@ -360,6 +360,11 @@ static int ad_sd_buffer_postenable(struct iio_dev *indio_dev)
> >       if (ret)
> >               return ret;
> > 
> > +     kfree(sigma_delta->buf_data);
> > +     sigma_delta->buf_data = kzalloc(indio_dev->scan_bytes, GFP_KERNEL);
> Hmm. Another thing that I'm trying to stop drivers doing is accessing elements
> of the struct iio_dev that have been marked in the header as INTERN.
> 
> That really restricts the ability to refactor that code.
> 
> The argument has always been that a driver ought to be able to 'tell' how
> large a buffer it can conceivably need and often it will actually use
> a 'superset' of that to avoid the need to do additional allocations.
> 
> It's a it trickier when there is a generic layer inbetween the driver and
> the core like we have here.
> 
> If the below 16 is correct, you are almost certain to not actually see any
> advantage in a separate allocation.  So two options come to mind.
> 
> 1) Ensure ad_sigma_delta is appropriately aligned in the priv data
> of each driver, then use ___cacheline_aligned on a fixed 16 byte buffer
> in ad_sigma_delta.
> 2) Do the separate allocation, but stick to 16 bytes (you will always
> get more than that anyway as always at least a cacheline - otherwise
> same DMA issue might occur).

Hmmm.
I think I got the gist of it.

I'll take a look and come back.

Thanks
Alex

> 
> > +     if (!sigma_delta->buf_data)
> > +             return -ENOMEM;
> > +
> >       spi_bus_lock(sigma_delta->spi->master);
> >       sigma_delta->bus_locked = true;
> >       sigma_delta->keep_cs_asserted = true;
> > @@ -403,12 +408,12 @@ static irqreturn_t ad_sd_trigger_handler(int irq, void *p)
> >       struct iio_poll_func *pf = p;
> >       struct iio_dev *indio_dev = pf->indio_dev;
> >       struct ad_sigma_delta *sigma_delta = iio_device_get_drvdata(indio_dev);
> > +     uint8_t *data = sigma_delta->buf_data;
> >       unsigned int reg_size;
> >       unsigned int data_reg;
> > -     uint8_t data[16];
> >       int ret;
> > 
> > -     memset(data, 0x00, 16);
> > +     memset(data, 0x00, indio_dev->scan_bytes);
> > 
> >       reg_size = indio_dev->channels[0].scan_type.realbits +
> >                       indio_dev->channels[0].scan_type.shift;
> > @@ -568,6 +573,9 @@ EXPORT_SYMBOL_GPL(ad_sd_setup_buffer_and_trigger);
> >   */
> >  void ad_sd_cleanup_buffer_and_trigger(struct iio_dev *indio_dev)
> >  {
> > +     struct ad_sigma_delta *sigma_delta = iio_device_get_drvdata(indio_dev);
> > +
> > +     kfree(sigma_delta->buf_data);
> >       ad_sd_remove_trigger(indio_dev);
> >       iio_triggered_buffer_cleanup(indio_dev);
> >  }
> > diff --git a/include/linux/iio/adc/ad_sigma_delta.h b/include/linux/iio/adc/ad_sigma_delta.h
> > index 6e9fb1932dde..36dc49b8dfd5 100644
> > --- a/include/linux/iio/adc/ad_sigma_delta.h
> > +++ b/include/linux/iio/adc/ad_sigma_delta.h
> > @@ -75,6 +75,8 @@ struct ad_sigma_delta {
> > 
> >       const struct ad_sigma_delta_info *info;
> > 
> > +     uint8_t                 *buf_data;
> > +
> >       /*
> >        * DMA (thus cache coherency maintenance) requires the
> >        * transfer buffers to live in their own cache lines.