On Fri, 17 Feb 2023 07:56:22 +0200 Matti Vaittinen <mazziesaccount@xxxxxxxxx> 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@xxxxxxxxxx/ > > [2] https://lore.kernel.org/linux-iio/20201205182659.7cd23d5b@archlinux/ > > [3] https://lore.kernel.org/linux-iio/20220126191606.00003f37@xxxxxxxxxx/ > > Yours, > -- Matti >
