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

+ 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