Re: [PATCH] iio: at91-sama5d2_adc: fix iio_triggered_buffer_{predisable,postenable} positions

[Ludovic Desroches]

On Tue, Dec 03, 2019 at 10:49:58AM +0100, Eugen Hristev - M18282 wrote:
> 
> 
> On 29.11.2019 09:02, Ardelean, Alexandru wrote:
> 
> > On Thu, 2019-11-28 at 15:19 +0000, Eugen.Hristev@xxxxxxxxxxxxx wrote:
> >>
> > 
> > Hey,
> > 
> > Sorry for the late reply.
> > I'm also juggling a few things.
> > 
> >>
> >> On 28.11.2019 10:36, Eugen.Hristev@xxxxxxxxxxxxx wrote:
> >>
> >>> On 25.11.2019 17:03, Ardelean, Alexandru wrote:
> >>>> On Wed, 2019-10-23 at 11:25 +0300, Alexandru Ardelean wrote:
> >>>>> The iio_triggered_buffer_{predisable,postenable} functions
> >>>>> attach/detach
> >>>>> poll functions.
> >>>>>
> >>>>> The iio_triggered_buffer_postenable() should be called first to
> >>>>> attach
> >>>>> the
> >>>>> poll function, and then the driver can init the data to be
> >>>>> triggered.
> >>>>>
> >>>>> Similarly, iio_triggered_buffer_predisable() should be called last
> >>>>> to
> >>>>> first
> >>>>> disable the data (to be triggered) and then the poll function
> >>>>> should be
> >>>>> detached.
> >>>
> >>> Hi Alexandru,
> >>>
> >>> Sorry for this late reply,
> >>>
> >>> I remember that by adding specific at91_adc code for
> >>> predisable/postenable , I was replacing the existing standard callback
> >>> with my own, and have my specific at91 code before postenable and then
> >>> calling the subsystem postenable,
> >>> and in similar way, for predisable, first call the subsystem predisable
> >>> then doing my predisable code (in reverse order as in postenable)
> >>>
> >>> If you say the order should be reversed (basically have the
> >>> pollfunction
> >>> first), how is current code working ?
> >>> Should current code fail if the poll function is not attached in time ?
> >>> Or there is a race between triggered data and the attachment of the
> >>> pollfunc ?
> >>>
> >>> I am thinking that attaching the pollfunc later makes it work because
> >>> the DMA is not started yet. What happens if we have the pollfunc
> >>> attached but DMA is not started (basically the trigger is not started)
> >>> ,
> >>> can this lead to unexpected behavior ? Like the pollfunc polling but no
> >>> trigger started/no DMA started.
> >>
> >> I looked a bit more into the code and in DMA case, using postenable
> >> first will lead to calling attach pollfunc, which will also enable the
> >> trigger, but the DMA is not yet started.
> >> Is this the desired effect ?
> > 
> > Yes.
> 
> How is this correct ? We start the trigger but have no buffer to carry 
> to... what happens with the data ? -> I think we both have an answer to 
> that, as you state below
> 
> > 
> >> Normally when using DMA I would say we
> >> would need to enable DMA first to be ready to carry data (and coherent
> >> area etc.) and then enable the trigger.
> > 
> > So, there is a change in our tree [from some time ago].
> > See here:
> > https://github.com/analogdevicesinc/linux/commit/eee97d12665fef8cf429a1e5035b23ae969705b8
> > 
> > Particularly, what's interesting is around line:
> > https://github.com/analogdevicesinc/linux/commit/eee97d12665fef8cf429a1e5035b23ae969705b8#diff-0a87744ce945d2c1c89ea19f21fb35bbR722
> > And you may need to expand some stuff to see more of the function-body.
> > And some things may have changed in upstream IIO since that change.
> > 
> > The change is to make the pollfunc attach/detach become part of the IIO
> > framework, because plenty of drivers just call
> > iio_triggered_buffer_postenable() & iio_triggered_buffer_predisable() to
> > manually attach/detach the pollfunc for triggered buffers.
> 
> Okay, I understand this. at91-sama5d2_adc does not manually 
> attach/detach the pollfunc. So why do we need to change anything here ?
> 
> 
> > 
> > That change is from 2015, and since then, some drivers were added that just
> > manually attach/detach the pollfunc [and do nothing more with the
> > postenable/predisable hooks].
> > 
> > I tried to upstream a more complete version of that patch a while ago [u1].
> > https://patchwork.kernel.org/patch/10482167/
> > https://patchwork.kernel.org/patch/10737291/
> > 
> > The conclusion was to first fix the attach/detach pollfunc order in all IIO
> > drivers, so that when patch [u1] is applied, there is no more discussion
> > about the correct order for attach/detach pollfunc.
> 
> Allright, what is required to be fixed regarding the order, in this 
> specific case? We enable the DMA, and then we do the normal 'postenable' 
> that was called anyway if we did not override the 'postenable' in the 
> ops. Do you want to move this code to 'preenable' and keep 'postenable' 
> to the standard subsystem one ?
> 
> The same applies to the predisable, we first call the subsystem 
> 'predisable' then do the specific at91 stuff. You want to move this to 
> the 'postdisable' ?
> 
> I think reverting the order inside the functions themselves is not good 
> as we replace the order of starting trigger/DMA setup.
> So, coming to your question below...
> 
> > 
> > Coming back here [and to your question], my answer is: I don't know if the
> > at91 DMA needs to be enabled/disabled before/after the pollfunc
> > attach/detach.
> > This sounds like specific stuff for at91 [which is fine].
> > 
> > It could be that some other hooks may need to used to enable DMA
> > before/after the attach/detach pollfunc. Maybe preenable()/postdisable() ?
> > 
> > In any case, what I would like [with this discussion], is to resolve a
> > situation where we can get closer to moving the attach/pollfunc code to IIO
> > core. So, if AT91 requires a different ordering, I think you would be more
> > appropriate to tell me, and propose an alternative to this patch.
> 
> ... yes, this looks more appropriate, to move things to 
> 'preenable/postdisable', if you feel like 'postenable/predisable' is not 
> the proper place to put them.
> But the order itself, first enable DMA then trigger, and disable in 
> reverse order, I do not think there is anything wrong with that? Am I 
> misunderstanding ?
> 
> If Jonathan or Ludovic have a different idea, please let me know.
> 

I didn't chime in because I am not sure that I really get the issue. I see
the order of the sequence which enables the DMA first and for me it's safe
in this way and I also have doubt it works well if DMA is enabled after
but I didn't do the test.

Regards

Ludovic

> Also, I can test your patch to see if everything is fine.
> 
> Thanks,
> Eugen
> 
> > 
> > Thanks :)
> > Alex
> > 
> >>
> >>>>> For this driver, the predisable & postenable hooks are also need to
> >>>>> take
> >>>>> into consideration the touchscreen, so the hooks need to be put in
> >>>>> places
> >>>>> that avoid the code for that cares about it.
> >>>>>
> >>>>
> >>>> ping here
> >>>>
> >>>>> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx>
> >>>>> ---
> >>>>>     drivers/iio/adc/at91-sama5d2_adc.c | 19 ++++++++++---------
> >>>>>     1 file changed, 10 insertions(+), 9 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c
> >>>>> b/drivers/iio/adc/at91-
> >>>>> sama5d2_adc.c
> >>>>> index e1850f3d5cf3..ac3e5c4c9840 100644
> >>>>> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> >>>>> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> >>>>> @@ -889,20 +889,24 @@ static int at91_adc_buffer_postenable(struct
> >>>>> iio_dev *indio_dev)
> >>>>>          if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES))
> >>>>>                  return -EINVAL;
> >>>>>
> >>>>> +     ret = iio_triggered_buffer_postenable(indio_dev);
> >>>>> +     if (ret)
> >>>>> +             return ret;
> >>>>> +
> >>>>>          /* we continue with the triggered buffer */
> >>>>>          ret = at91_adc_dma_start(indio_dev);
> >>>>>          if (ret) {
> >>>>>                  dev_err(&indio_dev->dev, "buffer postenable
> >>>>> failed\n");
> >>>>> +             iio_triggered_buffer_predisable(indio_dev);
> >>>>>                  return ret;
> >>>>>          }
> >>>>>
> >>>>> -     return iio_triggered_buffer_postenable(indio_dev);
> >>>>> +     return 0;
> >>>>>     }
> >>>>>
> >>>>>     static int at91_adc_buffer_predisable(struct iio_dev *indio_dev)
> >>>>>     {
> >>>>>          struct at91_adc_state *st = iio_priv(indio_dev);
> >>>>> -     int ret;
> >>>>>          u8 bit;
> >>>>>
> >>>>>          /* check if we are disabling triggered buffer or the
> >>>>> touchscreen */
> >>>>> @@ -916,13 +920,8 @@ static int at91_adc_buffer_predisable(struct
> >>>>> iio_dev
> >>>>> *indio_dev)
> >>>>>          if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES))
> >>>>>                  return -EINVAL;
> >>>>>
> >>>>> -     /* continue with the triggered buffer */
> >>>>> -     ret = iio_triggered_buffer_predisable(indio_dev);
> >>>>> -     if (ret < 0)
> >>>>> -             dev_err(&indio_dev->dev, "buffer predisable
> >>>>> failed\n");
> >>>>> -
> >>>>>          if (!st->dma_st.dma_chan)
> >>>>> -             return ret;
> >>>>> +             goto out;
> >>>>>
> >>>>>          /* if we are using DMA we must clear registers and end DMA
> >>>>> */
> >>>>>          dmaengine_terminate_sync(st->dma_st.dma_chan);
> >>>>> @@ -949,7 +948,9 @@ static int at91_adc_buffer_predisable(struct
> >>>>> iio_dev
> >>>>> *indio_dev)
> >>>>>
> >>>>>          /* read overflow register to clear possible overflow status
> >>>>> */
> >>>>>          at91_adc_readl(st, AT91_SAMA5D2_OVER);
> >>>>> -     return ret;
> >>>>> +
> >>>>> +out:
> >>>
> >>> I would prefer if this label is named with a function name prefix,
> >>> otherwise 'out' is pretty generic and can collide with other things in
> >>> the file... I want to avoid having an out2 , out3 later if code
> >>> changes.
> >>>
> > 
> > Sure.
> > Will do that.
> > 
> > I did not bother much with these labels, because after applying [u1], some
> > of them [maybe all] should go away.
> > 
> > 
> >>> Thanks for the patch,
> >>> Eugen
> >>>
> >>>>> +     return iio_triggered_buffer_predisable(indio_dev);
> >>>>>     }
> >>>>>
> >>>>>     static const struct iio_buffer_setup_ops at91_buffer_setup_ops =
> >>>>> {
> >>>> _______________________________________________
> >>>> linux-arm-kernel mailing list
> >>>> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> >>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >>>>
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >