On Tue, 2019-07-30 at 16:08 +0000, Denis CIOCCA wrote:
> [External]
>
> Hi Alexandru,
>
> > -----Original Message-----
> > From: Ardelean, Alexandru <alexandru.Ardelean@xxxxxxxxxx>
> > Sent: Tuesday, July 30, 2019 2:02 AM
> > To: jic23@xxxxxxxxxx; Denis CIOCCA <denis.ciocca@xxxxxx>; linux-
> > iio@xxxxxxxxxxxxxxx
> > Subject: Re: [PATCH 4/4] iio:pressure: preenable/postenable/predisable
> > fixup for ST press buffer
> >
> > On Mon, 2019-07-29 at 17:03 -0700, Denis Ciocca wrote:
> > > [External]
> > >
> > > This patch is trying to cleanup for good the buffers operation functions.
> > > There is no need of using preenable, all can be done into postenable.
> > > Let's also use logical sequence of operations as already done in accel
> > > driver.
> > > Finally also rename the goto label using operation to perform and not
> > > where it fails.
> > >
> >
> > Reviewed-by: Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx>
> >
> >
> > Unrelated to this patch, I was thinking that it would be a neat idea to move
> > the `buffer_data` allocation in
> > `drivers/iio/common/st_sensors/st_sensors_buffer.c`
> >
> > This would remove some duplication of this alloc + free in drivers.
> >
> > Maybe in st_sensors_trigger_handler() something like:
> >
> > if (!sdata->buffer_data) {
> > sdata->buffer_data = devm_kmalloc()
> > if (!sdata->buffer_data) {
> > dev_err(indio_dev->dev, "Failed to allocate buffer data\n");
> > goto st_sensors_get_buffer_element_error;
> > }
> > }
> >
> > Using devm_kmalloc() or a similar devm_ variant would be neat, since it gets
> > free'd when the device gets removed.
>
> Not sure about the solution proposed.
> Memory allocation is actually related to how many channels are enabled,
> one possibility could be to allocate the maximum but not sure it's worth...
> Moreover the memory allocation there could let driver miss the first sample I guess.
>
I see.
Would [then] moving the allocation in preenable work? and the free in postdisable?
Thing is: when iio_triggered_buffer_postenable() and iio_triggered_buffer_predisable() get removed (and logic moved into
core), then the order changes, and the attach pollfunc gets called before the allocation.
The final destination for these changes is this commit:
https://github.com/analogdevicesinc/linux/commit/eee97d12665fef8c#diff-0a87744ce945d2c1c89ea19f21fb35bbR722
[this an older form]
In this commit, the attach/detach of the poll_func is moved into core, since that is all that the 2 functions do
(iio_triggered_buffer_postenable() and iio_triggered_buffer_predisable())
When the move happens [for the ST drivers in particular] the poll_func is attached, and then the allocation is done, and
the free is done first, and then the poll_func is detached.
>
> > Thanks
> > Alex
> >
> > > Signed-off-by: Denis Ciocca <denis.ciocca@xxxxxx>
> > > ---
> > > drivers/iio/pressure/st_pressure_buffer.c | 32
> > > ++++++++---------------
> > > 1 file changed, 11 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/drivers/iio/pressure/st_pressure_buffer.c
> > > b/drivers/iio/pressure/st_pressure_buffer.c
> > > index f21b630abaa0..54823cfcfab5 100644
> > > --- a/drivers/iio/pressure/st_pressure_buffer.c
> > > +++ b/drivers/iio/pressure/st_pressure_buffer.c
> > > @@ -29,53 +29,43 @@ int st_press_trig_set_state(struct iio_trigger *trig,
> > bool state)
> > > return st_sensors_set_dataready_irq(indio_dev, state); }
> > >
> > > -static int st_press_buffer_preenable(struct iio_dev *indio_dev) -{
> > > - return st_sensors_set_enable(indio_dev, true);
> > > -}
> > > -
> > > static int st_press_buffer_postenable(struct iio_dev *indio_dev) {
> > > - int err;
> > > struct st_sensor_data *press_data = iio_priv(indio_dev);
> > > + int err;
> > >
> > > press_data->buffer_data = kmalloc(indio_dev->scan_bytes,
> > > GFP_DMA | GFP_KERNEL);
> > > - if (press_data->buffer_data == NULL) {
> > > - err = -ENOMEM;
> > > - goto allocate_memory_error;
> > > - }
> > > + if (!press_data->buffer_data)
> > > + return -ENOMEM;
> > >
> > > err = iio_triggered_buffer_postenable(indio_dev);
> > > if (err < 0)
> > > - goto st_press_buffer_postenable_error;
> > > + goto st_press_free_buffer;
> > >
> > > - return err;
> > > + return st_sensors_set_enable(indio_dev, true);
> > >
> > > -st_press_buffer_postenable_error:
> > > +st_press_free_buffer:
> > > kfree(press_data->buffer_data);
> > > -allocate_memory_error:
> > > return err;
> > > }
> > >
> > > static int st_press_buffer_predisable(struct iio_dev *indio_dev)
> > > {
> > > - int err;
> > > struct st_sensor_data *press_data = iio_priv(indio_dev);
> > > -
> > > - err = iio_triggered_buffer_predisable(indio_dev);
> > > - if (err < 0)
> > > - goto st_press_buffer_predisable_error;
> > > + int err, err2;
> > >
> > > err = st_sensors_set_enable(indio_dev, false);
> > >
> > > -st_press_buffer_predisable_error:
> > > + err2 = iio_triggered_buffer_predisable(indio_dev);
> > > + if (!err)
> > > + err = err2;
> > > +
> > > kfree(press_data->buffer_data);
> > > return err;
> > > }
> > >
> > > static const struct iio_buffer_setup_ops st_press_buffer_setup_ops = {
> > > - .preenable = &st_press_buffer_preenable,
> > > .postenable = &st_press_buffer_postenable,
> > > .predisable = &st_press_buffer_predisable,
> > > };
