Re: [PATCH 1/5] iio:common: introduce st_sensors_buffer_preenable/predisable functions

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 1 Aug 2019 08:24:10 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> On Wed, 2019-07-31 at 14:52 -0700, Denis Ciocca wrote:
> > [External]
> > 
> > This patch is introducing preenable/postdisable in the common
> > st_sensors_buffer code in order to remove the memory allocation /
> > de-allocation from each single st driver.
> >   
> 
> Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> 
> > Signed-off-by: Denis Ciocca <denis.ciocca@st.com>

As a rework, this is clearly reasonable, however, if we are going to
touch this code at all, there are a few things I would like to tidy
up about it.

Firstly it's one of relatively few drivers that actually touch scan_bytes
in the first place.  That is supposed to be internal state to the core
subsystem and not used by drivers (see INTERN marking in iio.h).
It bled across the boundary in too many places where I wasn't looking.

Secondly these allocations are small.  You would be better off just
making them part of the main state structure and not dynamically allocated
at all.

So move buffer_data to the end of struct st_sensor_data and make it
whatever the maximum size needed is - I'm thinking probably 32 bytes
but haven't checked.

You call the bulk regmap API against it so you also need to ensure
it's in it's own cacheline.  Use the __cacheline_aligned magic
to enforce that.  The iio_priv region is always aligned appropriately
to allow iio_priv accessed structures to pull this trick.

That way we don't need to do any memory handling on demand at all.
We may or many not save memory as will depend on exactly how big
that structure already is and what mood the allocator is in.

I don't think I'm missing a reason we can't take the approach of
embedding the buffer and it definitely makes the code simpler.

Thanks,

Jonathan


> > ---
> >  .../iio/common/st_sensors/st_sensors_buffer.c | 22 +++++++++++++++++++
> >  include/linux/iio/common/st_sensors.h         |  2 ++
> >  2 files changed, 24 insertions(+)
> > 
> > diff --git a/drivers/iio/common/st_sensors/st_sensors_buffer.c b/drivers/iio/common/st_sensors/st_sensors_buffer.c
> > index eee30130ae23..9da1c8104883 100644
> > --- a/drivers/iio/common/st_sensors/st_sensors_buffer.c
> > +++ b/drivers/iio/common/st_sensors/st_sensors_buffer.c
> > @@ -81,6 +81,28 @@ irqreturn_t st_sensors_trigger_handler(int irq, void *p)
> >  }
> >  EXPORT_SYMBOL(st_sensors_trigger_handler);
> >  
> > +int st_sensors_buffer_preenable(struct iio_dev *indio_dev)
> > +{
> > +	struct st_sensor_data *sdata = iio_priv(indio_dev);
> > +
> > +	sdata->buffer_data = kmalloc(indio_dev->scan_bytes,
> > +				     GFP_DMA | GFP_KERNEL);
> > +	if (!sdata->buffer_data)
> > +		return -ENOMEM;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(st_sensors_buffer_preenable);
> > +
> > +int st_sensors_buffer_postdisable(struct iio_dev *indio_dev)
> > +{
> > +	struct st_sensor_data *sdata = iio_priv(indio_dev);
> > +
> > +	kfree(sdata->buffer_data);
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(st_sensors_buffer_postdisable);
> > +
> >  MODULE_AUTHOR("Denis Ciocca <denis.ciocca@st.com>");
> >  MODULE_DESCRIPTION("STMicroelectronics ST-sensors buffer");
> >  MODULE_LICENSE("GPL v2");
> > diff --git a/include/linux/iio/common/st_sensors.h b/include/linux/iio/common/st_sensors.h
> > index 28fc1f9fa7d5..c66ebb236a15 100644
> > --- a/include/linux/iio/common/st_sensors.h
> > +++ b/include/linux/iio/common/st_sensors.h
> > @@ -254,6 +254,8 @@ struct st_sensor_data {
> >  
> >  #ifdef CONFIG_IIO_BUFFER
> >  irqreturn_t st_sensors_trigger_handler(int irq, void *p);
> > +int st_sensors_buffer_preenable(struct iio_dev *indio_dev);
> > +int st_sensors_buffer_postdisable(struct iio_dev *indio_dev);
> >  #endif
> >  
> >  #ifdef CONFIG_IIO_TRIGGER