[RFC] pinctrl: intel: Stop setting IRQF_NO_THREAD ?

[Hans de Goede <hansg@kernel.org>]

Hi,

While debugging the following lockdep report:

=============================
 [ BUG: Invalid wait context ]
 ...
 swapper/10/0 is trying to lock:
 ffff88819c271888 (&tp->xfer_wait){....}-{3:3},
  at: __wake_up (kernel/sched/wait.c:106 kernel/sched/wait.c:127)
 ...
 Call Trace:
 <IRQ>
 ...
 __raw_spin_lock_irqsave (./include/linux/spinlock_api_smp.h:111)
 __wake_up (kernel/sched/wait.c:106 kernel/sched/wait.c:127)
 vsc_tp_isr (drivers/misc/mei/vsc-tp.c:110) mei_vsc_hw
 __handle_irq_event_percpu (kernel/irq/handle.c:158)
 handle_irq_event (kernel/irq/handle.c:195 kernel/irq/handle.c:210)
 handle_edge_irq (kernel/irq/chip.c:833)
 ...
 </IRQ>

I realized after a while that the root-cause here is the IRQF_NO_THREAD
usage in pinctrl-intel.c. This was introduced in 1a7d1cb81eb2 ("pinctrl:
intel: Prevent force threading of the interrupt handler") to avoid problems
caused by using request_irq() for what should be a chained irq handler
(which itself is a workaround because of a shared IRQ on some platforms).

Generally speaking using IRQF_NO_THREAD is undesirable for 2 reasons:

1. It introduces extra latency on PREEMPT-RT kernels
2. Setting IRQF_NO_THREAD requires all interrupt handlers for GPIO
   interrupts to use raw-spinlocks only since normal spinlocks can
   sleep in PREEMPT-RT kernels and with IRQF_NO_THREAD the interrupt
   handlers will run in an atomic context

2. is what is causing the lockdep report above, by simply using a
wake_up(&wq_head) call in an interrupt handler, since wait-queues
use normal spinlocks not raw spinlocks.

I've tried just removing the IRQF_NO_THREAD flag and that fixes
the lockdep report. I've also tried reproducing the problem for
which the flag was added in commit 1a7d1cb81eb2 by using a kernel
with CONFIG_IRQ_FORCED_THREADING and "threadirqs" on the kernel
commandline. And the problem not reproduce. I'm not sure this is
100% proof that the flag is no longer necessary though ...

So 2 questions:

1. Should we maybe just drop the flag ?
2. Or should we have 2 different code-paths for GPIO controllers
with/without shared IRQs and use a chained-irq approach for the
not shared case, to at least reduce the usage of the flag ?

Regards,

Hans


p.s. For the 2 different code paths approach I believe we can
use intel_pinctrl_probe_by_uid() to identify platforms which use
a shared IRQ. AFAICT on all platforms which use
intel_pinctrl_probe_by_hid() there will only be a single GPIO
controller instance.