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

[Matti Vaittinen <mazziesaccount@gmail.com>]

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).

> 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.

> 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 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

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~