Re: [PATCH 1/2] iio: hrtimer-trigger: Mark hrtimer to expire in hard interrupt context

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

On 2020-08-13 11:46:30 [+0200], Lars-Peter Clausen wrote:
> 
> It should not affect the IRQ handlers of individual drivers. The hrtimer
> triggers acts like an IRQ chip and will call generic_handle_irq() to
> multiplex the interrupt handling onto all consumers. As far as I understand
> it there is a requirement that generic_handle_irq() is called in hard irq
> context, even with PREEMT_RT=y.

That is correct.

> If you are running with forced IRQ threads the only thing that will then
> happen in the actual hard IRQ context is the launching of the IRQ threads.
> Th e IRQ handler of the device driver will run in a threaded IRQ.

So if it is really just the wakeup of the IRQ-thread then it should be
okay.
One thing: iio_trigger_poll() may invoke iio_trigger_notify_done(). This
would invoke trig->ops->try_reenable callback if available.
I grepped and found
- bma180_trig_try_reen() 
  It appears to perform i2c_smbus_read_byte_data() and smbus sounds
  sleeping. I don't know if it attempts to acquire any spinlock_t but it
  will be wrong on RT.

- bmc150_accel_trig_try_reen()
  This one has mutex_lock() which is wrong even on !RT in this context
  (unless the previous `if' saves us).

- mxc4005_trigger_try_reen()
  This one uses regmap_write(). regmap internally uses a lock and the
  config does not disable / provide a lock. This means
  regmap_lock_mutex() is used (or regmap_lock_spinlock() in case of
  bus->fast_io but I doubt it with i2c).

Am I looking somehow wrong at this or did just nobody try the
combination of one of the three drivers here together with the hrtimer
trigger?

> > 
> > Has this change (including the second patch in thread) been tested on RT
> > in terms of locking and latency?
> 
> It has not been tested in terms of latency. But like I said if you are
> running with forced IRQ threads the effect should be minimal.
> 
> Without this patch there is an correctness issue when PREEMT_RT=y since
> generic_handle_irq() runs with interrupts on which breaks its internal
> assumptions.

I'm trying to understand the scope of the change. As I said above, if it
is just wakeup of the thread, then it is fine. I have memory of people
running iio drivers (or triggers) in hardirq-context for $reason and try
avoid something like this.

Sebastian