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@xxxxxxxxxx/
[2] https://lore.kernel.org/linux-iio/20201205182659.7cd23d5b@archlinux/
[3] https://lore.kernel.org/linux-iio/20220126191606.00003f37@xxxxxxxxxx/
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
