Re: Questions: iio: accel: kionix-kx022a: timestamp when using the data-rdy trigger?

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Fri, 17 Feb 2023 07:56:22 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> Hi Mehdi,
> 
> On 2/16/23 22:22, Mehdi Djait wrote:
> > Hi all,
> > 
> > DISCLAIMER: I'm new to kernel development.
> > 
> > I'm currently working on extending the kionix-kx022a driver to support
> > kionix-kx132.  
> 
> Thanks for working with this :) Support for the kx132, kx122 etc. is 
> very welcome!
> 
> > My question is about the timestamp pushed in the trigger
> > handler. The kionix-kx022a supports both FIFO and triggered buffer
> > mode, for my questions the triggered buffer mode is used.
> > 
> > Before asking the question: I tried to read every documentation
> > available, the kernel code and I found the Threads [1] [2] [3]
> > 
> > To better explain my question here are the two relevant setup functions:
> > A.  devm_iio_triggered_buffer_setup_ext(dev, idev,
> >                                          &iio_pollfunc_store_time,
> >                                          kx022a_trigger_handler,
> >                                          IIO_BUFFER_DIRECTION_IN,
> >                                          &kx022a_buffer_ops,
> >                                          kx022a_fifo_attributes)
> > 
> > B. devm_request_threaded_irq(data->dev, irq, kx022a_irq_handler,
> >                               &kx022a_irq_thread_handler,
> >                               IRQF_ONESHOT, name, idev);
> > 
> > 
> > And here are the relevant steps after an IRQ occurs :
> > 1. IRQ context --> kx022a_irq_handler() --> gets the current timestamp
> > with "data->timestamp = iio_get_time_ns(idev);" and returns
> > IRQ_WAKE_THREAD
> > 
> > 2. kx022a_irq_thread_handler() -> checks that the trigger is enabled  
> > --> iio_trigger_poll_chained() --> handle_nested_irq(): which will only  
> > call the bottom half of the pollfuncs  
> 
> I don't get the kx022a at my hands until next week to test this, but it 
> seems to me your reasoning is right. iio_pollfunc_store_time() is 
> probably not called. I just wonder why I didn't see zero timestamps when 
> testing this. (OTOH, I had somewhat peculiar IRQ handling at first - 
> maybe I broke this along the way).

This is a common problem.  So far we've always solved it in the driver
by using the pf->timestamp only if it's been set - otherwise fallback
to grabbing a new one to pass into iio_push_to_buffer_with_timestamp()
in the threaded handler.

It might be possible to solve in a generic fashion but it's a bit
fiddly so I don't think anyone has ever looked at it.

> 
> > 3. kx022a_trigger_handler() --> iio_push_to_buffers_with_timestamp(idev,
> > data->buffer, pf->timestamp)
> > 
> > 
> > My questions are:
> > Question 1: Is iio_push_to_buffers_with_timestamp(idev, data->buffer,
> > data->timestamp) instead of "pf->timestamp" better in the
> > trigger_handler ?  
> 
> I don't see any "technical reasons" why it would be better. I think it 
> is more standard looking though - but seems like it is plain wrong here 
> as you pointed out.

Agreed. That looks like a bug.

> 
> > I was first concerned that it would be racy with the
> > irq_handler, but the IRQF_ONESHOT flag is used, which means that the irq
> > line is disabled until the threaded handler has been run, i.e. until
> > kx022a_trigger_handler runs and retruns IRQ_HANDLED (right?).  
> 
> Yes. This is the purpose of IRQF_ONESHOT. (Well, AFAICS the IRQs are 
> re-enabled even if some other value is returned unless the IRQ_NONE is 
> returned repeatedly).
> 
> > Question 2: If the change proposed in question 1 is wrong, would this
> > one be better iio_push_to_buffers_with_timestamp(idev, data->buffer,
> > iio_get_time_ns(idev)). There is some delay between the IRQ occuring
> > and trigger_handler being called but that is better than getting all 0
> > timestamps like suggested in [2]  
> 
> Please, use the data->timestamp as you suggested.

I'd suggest a bit of both.  If you have a timestamp from the irq handler
use it. If it's not available then grab one locally in the threaded handler.

> 
> > I hope that I'm understating this correctly or at least not totally
> > off :) If yes, I will send a patch.  
> 
> Thanks Mehdi! I think this was a great catch! Maybe - while at it - you 
> could also send a patch adding a small kerneldoc to the 
> iio_trigger_poll_chained() mentioning this particular issue. Yes, I 
> guess it should be obvious just by reading the function name *_chained() 
> - but I did fall on this trap (and according to your reference [2] so 
> has someone else).
> 
> > [1] https://lore.kernel.org/linux-iio/4FDB33CD.2090805@metafoo.de/
> > [2] https://lore.kernel.org/linux-iio/20201205182659.7cd23d5b@archlinux/
> > [3] https://lore.kernel.org/linux-iio/20220126191606.00003f37@Huawei.com/  
> 
> Yours,
> 	-- Matti
>