Re: Question: SPEAr PLGPIO irq_enable on PREEMPT_RT and regmap updates

Hi,

On Thu, 18 Jun 2026 13:10:31 +0530
Viresh Kumar <[email protected]> wrote:

> + Herve (the last guy to work on this driver).
> 
> On 18-06-26, 10:34, Runyu Xiao wrote:
> > 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.  
> 
> I haven't worked on this for a very long time now (15 yrs). There are some
> people who use this hardware, and so it is not removed until now.
> 
> Also I am not sure if RT kernel is a valid use case here for this SoC family.
> 

I know some users and they don't use RT kernel.

But well, isn't the pattern present in some other gpio controller drivers ?
How it is done in others ?

What is specific in this controller compare to others ?
We take and release spinlock in gpio chip .irq_enable(). I think we can
find other drivers doing that and probably drivers using a regmap as well.

Best regards,
Hervé