Question: SPEAr PLGPIO irq_enable on PREEMPT_RT and regmap updates

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

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

[Viresh Kumar]

+ 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.

-- 
viresh

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

[Herve Codina]

Hi,

On Thu, 18 Jun 2026 13:10:31 +0530
Viresh Kumar <viresh.kumar@linaro.org> 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é

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

[Sebastian Andrzej Siewior]

On 2026-06-18 10:34:18 [+0800], Runyu Xiao wrote:
> Hi,
Hi,

…
> 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.

Not sure how this will look like, but okay. I was looking at making the
a lock a raw_spinlock_t for fast_io. Since it is just a read and write
it shouldn't be a problem. But then there is the regcache and the sync
of many registers might be painful. The actual problem is the type MAPLE
and RBTREE which have an allocation in their write callback. That is a
no but the FLAT ones should work since there is just one alloc during
init. Well, wouldn't it be for the lock that is acquired during the
callback.

I don't think this is required given that it is init time so
holding the lock shouldn't be required. This was introduced in commit
fd4ebc07b4dff ("regmap: Hold the regmap lock when allocating and freeing
the cache"). This change broke gpio-104-idio-16.c, pio-pci-idio-16.c,
pio-pcie-idio-24, gpio-ws16c48.c and pinctrl-apple-gpio.c.

So unless there is something that I miss…

> 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

Sebastian

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

[Runyu Xiao]

Hi,

Thanks everyone for the feedback.

I will not send the irq_bus_sync_unlock/shadow-state patch for now. From
Sebastian's comments, it sounds like the more important question is
whether this should be handled at the regmap locking/cache level, or by
using a raw lock only where the regmap path is known to be safe.

Herve, your point about other GPIO controllers is fair. I should not
treat PLGPIO as special without checking the wider pattern. I will look
at other irq_enable/irq_disable users that combine irqchip callbacks,
driver spinlocks and regmap_update_bits(), and compare whether they are
using MMIO/flat cache/raw regmap locking or a sleepable regmap path.

For the current PLGPIO draft, I will hold it back until I can answer
whether the right fix belongs in this driver or in the common regmap/GPIO
pattern.

Thanks,
Runyu