On Fri, 2018-11-09 at 11:44 +0100, Fabrice Gasnier wrote:
> On 11/9/18 11:03 AM, Ardelean, Alexandru wrote:
> > On Sat, 2018-11-03 at 18:19 +0000, Jonathan Cameron wrote:
> > > On Fri, 22 Jun 2018 16:53:22 +0300
> > > Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx> wrote:
> >
> > Hey,
> >
> > Thanks for taking the time for this, Jonathan.
> >
> > >
> > > > All devices using a triggered buffer need to attach and detach the
> > > > trigger
> > > > to the device in order to properly work. Instead of doing this in
> > > > each
> > > > and
> > > > every driver by hand move this into the core.
> > > >
> > > > Signed-off-by: Lars-Peter Clausen <lars@xxxxxxxxxx>
> > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx>
> > > > ---
> > > >
> > > > Note: `Signed-off-by` is also Lars-Peter Clausen because he is the
> > > > original author of this patch [on an older kernel].
> > > > The patch has been updated since the original patch from Lars.
> > > >
> > > > There has been a (small) discussion about whether such a patch
> > > > makes
> > > > sense to implement (for reducing code duplication), or whether it's
> > > > too
> > > > risky to do it.
> > > >
> > > > The reason for the risky-ness is that there is no consistent way in
> > > > which
> > > > drivers attach/detach the poll function.
> > > > i.e.
> > > > 1. some drivers call `iio_triggered_buffer_postenable()` before
> > > > doing HW
> > > > stuff, some after (for attaching the poll func)
> > > > 2. similarly, for `iio_triggered_buffer_predisable()` some
> > > > drivers
> > > > do it
> > > > before HW stuff, some after (for detaching the poll func)
> > > >
> > > > Common sense would dictate that (in the case of
> > > > `iio_triggered_buffer_postenable()`) there would normally be HW
> > > > setup
> > > > first
> > > > and then attach the poll func.
> > > > And the reverse done for `iio_triggered_buffer_predisable()`.
> > >
> > > I'm sure I once had the flow around this all well laid out, but I
> > > really
> > > can't recall what it was and it was all tied around trying to do the
> > > buffer enable as lockless as possible. Years ago Lars pointed out
> > > that
> > > was impossible so a lot of the logic has been messed up since then.
> > >
> > > Anyhow, I've put this off long enough (sorry about that - just new it
> > > was going to involve some head scratching!) Crucially I have another
> > > plan. Figure out which drivers this actually makes a difference to
> > > and thankfully in most cases the author is still active :)
> > >
> > >
> > > >
> > > > However, it's unclear whether this reasoning is completely sound
> > > > for
> > > > all
> > > > drivers.
> > > >
> > > > Hence this RFC.
> > >
> > > I've culled the cases that are obvious. Either just the defaults or
> > > where we do the calls at the end of enable and start of disable.
> > >
> > > In those there is no ordering change.
> > >
> > > There are some odd ones
> > > gp2ap020a00f does it under a lock. Which makes no difference.
> > > isl29125 totally missbalanced so the preenable does things undone in
> > > the
> > > predisable. I wonder if anyone has one to allow us to test
> > > cleaning
> > > that up. No flow change due to this patch though so fine.
> > > tcs3414 is much the same as the isl29125.
> > > lmp91000?? Is about all I can say on that. It detaches a poll
> > > function
> > > that
> > > was never attached.
> > >
> > > Anyhow, so what's left (hopefully we haven't had any crazies since
> > > you posted this patch. I'll check at somepoint)
> > >
> > > > drivers/iio/adc/ad_sigma_delta.c | 5 ----
> > >
> > > I'll assume you are fine with that one ;)
> > >
> > > > drivers/iio/adc/dln2-adc.c | 4 +--
> > >
> > > + CC Jack Anderson (worst comes to the worst I have one of these and
> > > can
> > > run basic tests).
> > >
> > > > drivers/iio/adc/stm32-adc.c | 11 --------
> > >
> > > + CC Fabrice
>
> Hi Ardelean, Jonathan,
>
> Thanks for CC'ing me. Please find my comments here here after.
>
> > >
> > > > drivers/iio/adc/vf610_adc.c | 6 +----
> > >
> > > + CC Sanchayan who might still have access to one of these
> > > + Bhuvanchanda who fixed a bug not so long ago...
> > >
> > > > drivers/iio/chemical/atlas-ph-sensor.c | 8 ------
> > >
> > > + CC Matt who is definitely still contactable as I met him last week
> > > ;)
> > >
> > > > drivers/iio/potentiostat/lmp91000.c | 1 -
> > >
> > > Another of Matt's. On this one, I'm not sure who it attaches
> > > the pollfunc in the first place. I may be going crazy however, so
> > > over to Matt to point out what I'm missing.
> > >
> > > So the actual change that matters is:
> > >
> > > > diff --git a/drivers/iio/industrialio-buffer.c
> > > > b/drivers/iio/industrialio-buffer.c
> > > > index cd5bfe39591b..d8eb534a9544 100644
> > > > --- a/drivers/iio/industrialio-buffer.c
> > > > +++ b/drivers/iio/industrialio-buffer.c
> > > > @@ -24,6 +24,7 @@
> > > >
> > > > #include <linux/iio/iio.h>
> > > > #include "iio_core.h"
> > > > +#include "iio_core_trigger.h"
> > > > #include <linux/iio/sysfs.h>
> > > > #include <linux/iio/buffer.h>
> > > > #include <linux/iio/buffer_impl.h>
> > > > @@ -961,6 +962,13 @@ static int iio_enable_buffers(struct iio_dev
> > > > *indio_dev,
> > > > }
> > > > }
> > > >
> > > > + if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
> > > > + ret = iio_trigger_attach_poll_func(indio_dev->trig,
> > > > + indio_dev-
> > > > >pollfunc);
> > > > + if (ret)
> > > > + goto err_disable_buffers;
> > > > + }
> > > > +
> > >
> > > This is immediately after the postenable call.
> > >
> >
> > I'm vague here about your comment.
> > Do I need to change something ?
> >
>
> I have some concern about the ordering here as well, regarding
> stm32-adc.c: I think this is same case as ad_sigma_delta.c (I haven't
> checked the others).
>
> With this patch, in stm32-adc case, the hardware gets started (e.g.
> getting data with interrupts or dma) before the handler has been
> attached:
> -> iio_triggered_buffer_postenable
> -> iio_trigger_attach_poll_func
> -> request_threaded_irq
>
> I haven't tested it, but I think this is racy. I feel like
> iio_trigger_attach_poll_func should happen before postenable call
> (rather than after), see after stm32-adc.c.
>
In our internal tree it seems to be done in the way that you mention.
(i.e. attach poll_func, then call post_enable)
See:
https://github.com/analogdevicesinc/linux/commit/eee97d12665fef8cf429a1e5035b23ae969705b8#diff-0a87744ce945d2c1c89ea19f21fb35bbR722
I can't remember the details of the discussion we had [internally] about
it, and when I sent the patch, I did it the other way around.
I guess I was confused about this being done one way or the other, and I
probably thought that this may be the good approach (when I sent the RFC
patch).
Mind you, I'm not yet that familiar yet with IIO internals/subtleties.
But, your explanation makes sense. And confirms the initial patch that Lars
did in our tree.
Maybe one way to look at this is that, since it's been working in our tree
for a few years now, it could be the good approach.
And it's good that it's confirmed about someone else.
Thanks
Alex
> > > > return 0;
> > > >
> > > > err_disable_buffers:
> > > > @@ -987,6 +995,11 @@ static int iio_disable_buffers(struct iio_dev
> > > > *indio_dev)
> > > > if (list_empty(&indio_dev->buffer_list))
> > > > return 0;
> > > >
> > >
> > > This is immediately before the predisable call.
> >
> > Same here as above:
> > ```
> > I'm vague here about your comment.
> > Do I need to change something ?
> > ```
> >
>
> Same here (at least for stm32-adc case): the handler gets unregistered
> but the hardware is still running (e.g. getting data with interrupts or
> dma).
>
> I'd prefer the other way :-):
> - upon enable: attach the poll func, then start the HW
> - upon disable: stop the HW, detach the poll func
>
> > >
> > > > + if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
> > > > + iio_trigger_detach_poll_func(indio_dev->trig,
> > > > + indio_dev->pollfunc);
> > > > + }
> > > > +
> > > > /*
> > > > * If things go wrong at some step in disable we still need
> > > > to
> > > > continue
> > > > * to perform the other steps, otherwise we leave the
> > > > device in a
> > > > diff --git a/drivers/iio/industrialio-trigger.c
> > > > b/drivers/iio/industrialio-trigger.c
> > > > index ce66699c7fcc..a4dacb638a72 100644
> > > > --- a/drivers/iio/industrialio-trigger.c
> > > > +++ b/drivers/iio/industrialio-trigger.c
> > > > @@ -242,8 +242,8 @@ static void iio_trigger_put_irq(struct
> > > > iio_trigger
> > > > *trig, int irq)
> > > > * the relevant function is in there may be the best option.
> > > > */
> > > > /* Worth protecting against double additions? */
> > > > -static int iio_trigger_attach_poll_func(struct iio_trigger *trig,
> > > > - struct iio_poll_func *pf)
> > > > +int iio_trigger_attach_poll_func(struct iio_trigger *trig,
> > > > + struct iio_poll_func *pf)
> > > > {
> > > > int ret = 0;
> > > > bool notinuse
> > > > @@ -290,8 +290,8 @@ static int iio_trigger_attach_poll_func(struct
> > > > iio_trigger *trig,
> > > > return ret;
> > > > }
> > > >
> > > > -static int iio_trigger_detach_poll_func(struct iio_trigger *trig,
> > > > - struct iio_poll_func *pf)
> > > > +int iio_trigger_detach_poll_func(struct iio_trigger *trig,
> > > > + struct iio_poll_func *pf)
> > > > {
> > > > int ret = 0;
> > > > bool no_other_users
> > > > @@ -758,17 +758,3 @@ void
> > > > iio_device_unregister_trigger_consumer(struct
> > > > iio_dev *indio_dev)
> > > > if (indio_dev->trig)
> > > > iio_trigger_put(indio_dev->trig);
> > > > }
> > > > -
> > > > -int iio_triggered_buffer_postenable(struct iio_dev *indio_dev)
> > > > -{
> > > > - return iio_trigger_attach_poll_func(indio_dev->trig,
> > > > - indio_dev->pollfunc);
> > > > -}
> > > > -EXPORT_SYMBOL(iio_triggered_buffer_postenable);
> > > > -
> > > > -int iio_triggered_buffer_predisable(struct iio_dev *indio_dev)
> > > > -{
> > > > - return iio_trigger_detach_poll_func(indio_dev->trig,
> > > > - indio_dev->pollfunc);
> > > > -}
> > > > -EXPORT_SYMBOL(iio_triggered_buffer_predisable);
> > >
> > >
> > >
> > >
> > > > diff --git a/drivers/iio/adc/ad_sigma_delta.c
> > > > b/drivers/iio/adc/ad_sigma_delta.c
> > > > index cf1b048b0665..ef046277bf7b 100644
> > > > --- a/drivers/iio/adc/ad_sigma_delta.c
> > > > +++ b/drivers/iio/adc/ad_sigma_delta.c
> > > > @@ -338,10 +338,6 @@ static int ad_sd_buffer_postenable(struct
> > > > iio_dev
> > > > *indio_dev)
> > > > unsigned int channel;
> > > > int ret;
> > > >
> > > > - ret = iio_triggered_buffer_postenable(indio_dev);
> > > > - if (ret < 0)
> > > > - return ret;
> > > > -
> > >
> > > Here is one where the ordering actually changes. I'm going to hope
> > > you
> > > concluded
> > > this one was fine ;)
> > >
> >
> > Yep, this patch should be fine (for the ad_sigma_delta.c change).
> > It's been in our tree for a while.
> >
> > > > channel = find_first_bit(indio_dev->active_scan_mask,
> > > > indio_dev->masklength);
> > > > ret = ad_sigma_delta_set_channel(sigma_delta,
> > > > @@ -426,7 +422,6 @@ static irqreturn_t ad_sd_trigger_handler(int
> > > > irq,
> > > > void *p)
> > > >
> > > > static const struct iio_buffer_setup_ops ad_sd_buffer_setup_ops =
> > > > {
> > > > .postenable = &ad_sd_buffer_postenable,
> > > > - .predisable = &iio_triggered_buffer_predisable,
> > > > .postdisable = &ad_sd_buffer_postdisable,
> > > > .validate_scan_mask = &iio_validate_scan_mask_onehot,
> > > > };
> > > > diff --git a/drivers/iio/adc/dln2-adc.c b/drivers/iio/adc/dln2-
> > > > adc.c
> > > > index c64c6675cae6..51135e7c0d4f 100644
> > > > --- a/drivers/iio/adc/dln2-adc.c
> > > > +++ b/drivers/iio/adc/dln2-adc.c
> > > > @@ -560,7 +560,7 @@ static int
> > > > dln2_adc_triggered_buffer_postenable(struct iio_dev *indio_dev)
> > > > mutex_unlock(&dln2->mutex);
> > > > }
> > > >
> > > > - return iio_triggered_buffer_postenable(indio_dev);
> > > > + return 0;
> > > > }
> > > >
> > > > static int dln2_adc_triggered_buffer_predisable(struct iio_dev
> > > > *indio_dev)
> > > > @@ -585,7 +585,7 @@ static int
> > > > dln2_adc_triggered_buffer_predisable(struct iio_dev *indio_dev)
> > > > return ret;
> > > > }
> > > >
> > > > - return iio_triggered_buffer_predisable(indio_dev);
> > >
> > > An unbalanced one. Postenable is fine but presdisable. Who knows..
> > >
> > > > + return 0;
> > > > }
> > > >
> > > > diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-
> > > > adc.c
> > > > index 378411853d75..ce0d17c03d7e 100644
> > > > --- a/drivers/iio/adc/stm32-adc.c
> > > > +++ b/drivers/iio/adc/stm32-adc.c
> > > > @@ -1482,10 +1482,6 @@ static int
> > > > stm32_adc_buffer_postenable(struct
> > > > iio_dev *indio_dev)
> > > > goto err_clr_trig;
> > > > }
> > > >
> > > > - ret = iio_triggered_buffer_postenable(indio_dev);
> > > > - if (ret < 0)
> > > > - goto err_stop_dma;
> > > > -
>
> This is where the ordering changes.
> I'd rather prefer to call "iio_trigger_attach_poll_func" before calling
> the .postenable routine, and the reverse order when disabling.
>
> Thanks,
> Best regards,
> Fabrice
>
> > > > /* Reset adc buffer index */
> > > > adc->bufi = 0;
> > > >
> > > > @@ -1496,9 +1492,6 @@ static int stm32_adc_buffer_postenable(struct
> > > > iio_dev *indio_dev)
> > > >
> > > > return 0;
> > > >
> > > > -err_stop_dma:
> > > > - if (adc->dma_chan)
> > > > - dmaengine_terminate_all(adc->dma_chan);
> > > > err_clr_trig:
> > > > stm32_adc_set_trig(indio_dev, NULL);
> > > > err_unprepare:
> > > > @@ -1517,10 +1510,6 @@ static int
> > > > stm32_adc_buffer_predisable(struct
> > > > iio_dev *indio_dev)
> > > > if (!adc->dma_chan)
> > > > stm32_adc_conv_irq_disable(adc);
> > > >
> > > > - ret = iio_triggered_buffer_predisable(indio_dev);
> > > > - if (ret < 0)
> > > > - dev_err(&indio_dev->dev, "predisable failed\n");
> > > > -
> > > > if (adc->dma_chan)
> > > > dmaengine_terminate_all(adc->dma_chan);
> > > >
> > > > diff --git a/drivers/iio/adc/vf610_adc.c
> > > > b/drivers/iio/adc/vf610_adc.c
> > > > index bbcb7a4d7edf..3a15b98cfd39 100644
> > > > --- a/drivers/iio/adc/vf610_adc.c
> > > > +++ b/drivers/iio/adc/vf610_adc.c
> > > > @@ -740,10 +740,6 @@ static int vf610_adc_buffer_postenable(struct
> > > > iio_dev *indio_dev)
> > > > int ret;
> > > > int val;
> > > >
> > > > - ret = iio_triggered_buffer_postenable(indio_dev);
> > > > - if (ret)
> > > > - return ret;
> > > > -
> > > > val = readl(info->regs + VF610_REG_ADC_GC);
> > > > val |= VF610_ADC_ADCON;
> > > > writel(val, info->regs + VF610_REG_ADC_GC);
> > > > @@ -774,7 +770,7 @@ static int vf610_adc_buffer_predisable(struct
> > > > iio_dev *indio_dev)
> > > >
> > > > writel(hc_cfg, info->regs + VF610_REG_ADC_HC0);
> > > >
> > > > - return iio_triggered_buffer_predisable(indio_dev);
> > > > + return 0;
> > > > }
> > > >
> > > > static const struct iio_buffer_setup_ops
> > > > iio_triggered_buffer_setup_ops = {
> > >
> > >
> > > > diff --git a/drivers/iio/chemical/atlas-ph-sensor.c
> > > > b/drivers/iio/chemical/atlas-ph-sensor.c
> > > > index a406ad31b096..8fed75f9e95d 100644
> > > > --- a/drivers/iio/chemical/atlas-ph-sensor.c
> > > > +++ b/drivers/iio/chemical/atlas-ph-sensor.c
> > > > @@ -305,10 +305,6 @@ static int atlas_buffer_postenable(struct
> > > > iio_dev
> > > > *indio_dev)
> > > > struct atlas_data *data = iio_priv(indio_dev);
> > > > int ret;
> > > >
> > > > - ret = iio_triggered_buffer_postenable(indio_dev);
> > > > - if (ret)
> > > > - return ret;
> > > > -
> > > > ret = pm_runtime_get_sync(&data->client->dev);
> > > > if (ret < 0) {
> > > > pm_runtime_put_noidle(&data->client->dev);
> > > > @@ -323,10 +319,6 @@ static int atlas_buffer_predisable(struct
> > > > iio_dev
> > > > *indio_dev)
> > > > struct atlas_data *data = iio_priv(indio_dev);
> > > > int ret;
> > > >
> > > > - ret = iio_triggered_buffer_predisable(indio_dev);
> > > > - if (ret)
> > > > - return ret;
> > > > -
> > > > ret = atlas_set_interrupt(data, false);
> > > > if (ret)
> > > > return ret;
> > > > diff --git a/drivers/iio/potentiostat/lmp91000.c
> > > > b/drivers/iio/potentiostat/lmp91000.c
> > > > index 85714055cc74..84f9105cbb37 100644
> > > > --- a/drivers/iio/potentiostat/lmp91000.c
> > > > +++ b/drivers/iio/potentiostat/lmp91000.c
> > > > @@ -295,7 +295,6 @@ static int lmp91000_buffer_predisable(struct
> > > > iio_dev *indio_dev)
> > > >
> > > > static const struct iio_buffer_setup_ops lmp91000_buffer_setup_ops
> > > > = {
> > > > .preenable = lmp91000_buffer_preenable,
> > > > - .postenable = iio_triggered_buffer_postenable,
> > >
> > > A resounding ???
> > > > .predisable = lmp91000_buffer_predisable,
> > > > };
> > > >
> > >
> > >
>
>
