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
>
> > 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 ?
> > 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 ?
```
>
> > + 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;
> > -
> > /* 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,
> > };
> >
>
>
