Re: [PATCH] gpio: pch: use raw_spinlock_t for the register lock

[Linus Walleij]

Hi Junjie,

thanks for your patch!

On Thu, Jul 23, 2026 at 3:42 AM Junjie Cao <junjie.cao@xxxxxxxxx> wrote:

> pch_irq_type() is registered as the irq_chip .irq_set_type callback and
> takes chip->spinlock with spin_lock_irqsave().  This callback is reached
> from __setup_irq() -> __irq_set_trigger() -> chip->irq_set_type() while
> the caller holds desc->lock, a raw_spinlock_t, with hardirqs disabled.
> That context is not sleepable, but on PREEMPT_RT a regular spinlock_t is
> an rtmutex-backed sleeping lock, so acquiring it there is invalid.
>
> This was confirmed on a PREEMPT_RT kernel with lockdep
> (PROVE_RAW_LOCK_NESTING and DEBUG_ATOMIC_SLEEP).  A grounded PoC mirrored
> pch_irq_type()'s locking and drove it through the real genirq carrier
> irq_set_irq_type() -> __irq_set_trigger() -> chip->irq_set_type(), i.e.
> the same __irq_set_trigger() edge that __setup_irq() takes for a
> requested IRQ.  With the original spin_lock_irqsave() edge lockdep
> reported an invalid wait context, immediately followed by:
>
>   BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:48
>   in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 95, name: insmod
>   hardirqs last disabled at (3784): _raw_spin_lock_irqsave+0x4f/0x60
>    rt_spin_lock+0x3a/0x1c0
>    repro_irq_set_type+0x64/0xa0 [pch_repro]
>    __irq_set_trigger+0x69/0x140
>    irq_set_irq_type+0x78/0xd0
>
> Switching the mirrored lock to raw_spinlock_t made both splats go away.
>
> Convert the register lock to raw_spinlock_t.  The same lock also
> serializes the GPIO direction/value callbacks and the suspend/resume
> register save/restore, but all of those critical sections only perform
> MMIO register accesses (ioread32()/iowrite32()) and
> irq_set_handler_locked(); none of them contain sleepable operations.
> Keeping this register lock non-sleeping is therefore appropriate for the
> irqchip callbacks and does not change the GPIO-side locking contract.
>
> This is the same class of issue and fix as recently addressed for other
> GPIO controllers, e.g. commit 286533cb14a3 ("gpio: sch: use raw_spinlock_t
> in the irq startup path") and commit 90f0109019e6 ("gpio: eic-sprd: use
> raw_spinlock_t in the irq startup path").
>
> Fixes: 38eb18a6f92d ("gpio-pch: Support interrupt function")
> Cc: stable@xxxxxxxxxxxxxxx

Really? Why to stable? Is this a regression of something that used
to work before? Using RT is fringe IMO.

> Signed-off-by: Junjie Cao <junjie.cao@xxxxxxxxx>

Reviewed-by: Linus Walleij <linusw@xxxxxxxxxx>

Yours,
Linus Walleij