Question: SPEAr PLGPIO irq_enable on PREEMPT_RT and regmap updates

[Runyu Xiao]

Hi,

While auditing GPIO/pinctrl irqchip callbacks, our static analysis tool
flagged the SPEAr PLGPIO irq_enable path, and we manually reviewed it
against the current tree.

The path is:

  irq_startup()
    -> plgpio_irq_enable()
       -> gpiochip_enable_irq()
       -> spin_lock_irqsave(&plgpio->lock)
       -> plgpio_reg_reset()
       -> regmap_update_bits()

On PREEMPT_RT, plgpio->lock is a regular spinlock_t and can become a
sleeping lock.  Since irq_enable/irq_disable can be called from IRQ
management paths while the IRQ descriptor raw lock is held, taking that
regular spinlock there looks unsafe.

A minimal Lockdep reproducer preserving this irq_chip::irq_enable carrier
reports:

  BUG: sleeping function called from invalid context
  irqs_disabled(): 1
  plgpio_rt_spin_lock_irqsave
  plgpio_irq_enable
  request_threaded_irq_probe_path

My first thought was to convert the PLGPIO register lock to
raw_spinlock_t.  However, that does not seem sufficient because the IE/EIT
updates go through regmap_update_bits()/regmap_read()/regmap_write().  For
the syscon/MMIO regmap used here, regmap may still take its own regular
fast-IO lock unless the regmap was created with use_raw_spinlock.  So a
raw_spinlock_t conversion in the PLGPIO driver alone may just move the
PREEMPT_RT problem one level down into regmap.

The repair I am considering is to keep the gpiolib resource updates in
the fast irq_enable/irq_disable callbacks, but defer the actual PLGPIO
IE/EIT register writes to irq_bus_sync_unlock(), after the IRQ core has
dropped desc->lock.  The driver would keep per-line shadow state for:

  - IRQ disabled/enabled state
  - pending IE update
  - edge direction state
  - pending EIT update

and then synchronize those shadow updates from irq_bus_sync_unlock()
under a mutex.

In other words, the fast callbacks would only update local shadow state
and call gpiochip_enable_irq()/gpiochip_disable_irq(), while the sleepable
regmap writes would be batched into the irq bus sync phase.

Does that sound like an acceptable direction for SPEAr PLGPIO, or would
you prefer a different fix, such as changing the underlying syscon regmap
locking model or handling only the IE register path?

The draft patch I have locally is roughly:

  pinctrl: spear: defer PLGPIO IRQ updates to bus sync

and it changes only drivers/pinctrl/spear/pinctrl-plgpio.c.

Thanks,
Runyu