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.
>>> 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,
>>> };
>>>
>>
>>
