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

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

[Junjie Cao]

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@vger.kernel.org
Signed-off-by: Junjie Cao <junjie.cao@intel.com>
---
 drivers/gpio/gpio-pch.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/gpio/gpio-pch.c b/drivers/gpio/gpio-pch.c
index 4ffa0955a9e3..07a5617b314b 100644
--- a/drivers/gpio/gpio-pch.c
+++ b/drivers/gpio/gpio-pch.c
@@ -96,7 +96,7 @@ struct pch_gpio {
 	struct pch_gpio_reg_data pch_gpio_reg;
 	int irq_base;
 	enum pch_type_t ioh;
-	spinlock_t spinlock;
+	raw_spinlock_t spinlock;
 };
 
 static int pch_gpio_set(struct gpio_chip *gpio, unsigned int nr, int val)
@@ -105,7 +105,7 @@ static int pch_gpio_set(struct gpio_chip *gpio, unsigned int nr, int val)
 	struct pch_gpio *chip =	gpiochip_get_data(gpio);
 	unsigned long flags;
 
-	spin_lock_irqsave(&chip->spinlock, flags);
+	raw_spin_lock_irqsave(&chip->spinlock, flags);
 	reg_val = ioread32(&chip->reg->po);
 	if (val)
 		reg_val |= BIT(nr);
@@ -113,7 +113,7 @@ static int pch_gpio_set(struct gpio_chip *gpio, unsigned int nr, int val)
 		reg_val &= ~BIT(nr);
 
 	iowrite32(reg_val, &chip->reg->po);
-	spin_unlock_irqrestore(&chip->spinlock, flags);
+	raw_spin_unlock_irqrestore(&chip->spinlock, flags);
 
 	return 0;
 }
@@ -133,7 +133,7 @@ static int pch_gpio_direction_output(struct gpio_chip *gpio, unsigned int nr,
 	u32 reg_val;
 	unsigned long flags;
 
-	spin_lock_irqsave(&chip->spinlock, flags);
+	raw_spin_lock_irqsave(&chip->spinlock, flags);
 
 	reg_val = ioread32(&chip->reg->po);
 	if (val)
@@ -147,7 +147,7 @@ static int pch_gpio_direction_output(struct gpio_chip *gpio, unsigned int nr,
 	pm |= BIT(nr);
 	iowrite32(pm, &chip->reg->pm);
 
-	spin_unlock_irqrestore(&chip->spinlock, flags);
+	raw_spin_unlock_irqrestore(&chip->spinlock, flags);
 
 	return 0;
 }
@@ -158,12 +158,12 @@ static int pch_gpio_direction_input(struct gpio_chip *gpio, unsigned int nr)
 	u32 pm;
 	unsigned long flags;
 
-	spin_lock_irqsave(&chip->spinlock, flags);
+	raw_spin_lock_irqsave(&chip->spinlock, flags);
 	pm = ioread32(&chip->reg->pm);
 	pm &= BIT(gpio_pins[chip->ioh]) - 1;
 	pm &= ~BIT(nr);
 	iowrite32(pm, &chip->reg->pm);
-	spin_unlock_irqrestore(&chip->spinlock, flags);
+	raw_spin_unlock_irqrestore(&chip->spinlock, flags);
 
 	return 0;
 }
@@ -265,7 +265,7 @@ static int pch_irq_type(struct irq_data *d, unsigned int type)
 		return 0;
 	}
 
-	spin_lock_irqsave(&chip->spinlock, flags);
+	raw_spin_lock_irqsave(&chip->spinlock, flags);
 
 	/* Set interrupt mode */
 	im = ioread32(im_reg) & ~(PCH_IM_MASK << (im_pos * 4));
@@ -277,7 +277,7 @@ static int pch_irq_type(struct irq_data *d, unsigned int type)
 	else if (type & IRQ_TYPE_EDGE_BOTH)
 		irq_set_handler_locked(d, handle_edge_irq);
 
-	spin_unlock_irqrestore(&chip->spinlock, flags);
+	raw_spin_unlock_irqrestore(&chip->spinlock, flags);
 	return 0;
 }
 
@@ -374,7 +374,7 @@ static int pch_gpio_probe(struct pci_dev *pdev,
 	chip->ioh = id->driver_data;
 	chip->reg = chip->base;
 	pci_set_drvdata(pdev, chip);
-	spin_lock_init(&chip->spinlock);
+	raw_spin_lock_init(&chip->spinlock);
 	pch_gpio_setup(chip);
 
 	ret = devm_gpiochip_add_data(dev, &chip->gpio, chip);
@@ -407,9 +407,9 @@ static int pch_gpio_suspend(struct device *dev)
 	struct pch_gpio *chip = dev_get_drvdata(dev);
 	unsigned long flags;
 
-	spin_lock_irqsave(&chip->spinlock, flags);
+	raw_spin_lock_irqsave(&chip->spinlock, flags);
 	pch_gpio_save_reg_conf(chip);
-	spin_unlock_irqrestore(&chip->spinlock, flags);
+	raw_spin_unlock_irqrestore(&chip->spinlock, flags);
 
 	return 0;
 }
@@ -419,11 +419,11 @@ static int pch_gpio_resume(struct device *dev)
 	struct pch_gpio *chip = dev_get_drvdata(dev);
 	unsigned long flags;
 
-	spin_lock_irqsave(&chip->spinlock, flags);
+	raw_spin_lock_irqsave(&chip->spinlock, flags);
 	iowrite32(0x01, &chip->reg->reset);
 	iowrite32(0x00, &chip->reg->reset);
 	pch_gpio_restore_reg_conf(chip);
-	spin_unlock_irqrestore(&chip->spinlock, flags);
+	raw_spin_unlock_irqrestore(&chip->spinlock, flags);
 
 	return 0;
 }
-- 
2.43.0

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@intel.com> 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@vger.kernel.org

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@intel.com>

Reviewed-by: Linus Walleij <linusw@kernel.org>

Yours,
Linus Walleij

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

[Junjie Cao]

Hi Linus,

Thanks for the review!

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

No -- it is indeed not a regression: the lock has been a plain
spinlock_t since IRQ support landed in 2011, so it never worked on
PREEMPT_RT rather than regressing. I Cc'd stable because current LTS
kernels with PREEMPT_RT hit this sleeping-in-atomic BUG today, like
the two fixes I referenced (286533cb14a3, 90f0109019e6, both Cc'd
stable); Bartosz has since applied it with the tag kept, so I'm happy
to defer to whatever you and he prefer.

Many thanks,
Junjie

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

[Linus Walleij]

On Mon, Jul 27, 2026 at 5:19 PM Junjie Cao <junjie.cao@intel.com> wrote:

> No -- it is indeed not a regression: the lock has been a plain
> spinlock_t since IRQ support landed in 2011, so it never worked on
> PREEMPT_RT rather than regressing. I Cc'd stable because current LTS
> kernels with PREEMPT_RT hit this sleeping-in-atomic BUG today, like
> the two fixes I referenced (286533cb14a3, 90f0109019e6, both Cc'd
> stable); Bartosz has since applied it with the tag kept, so I'm happy
> to defer to whatever you and he prefer.

It's no big deal, the stable people pick a lot of random stuff I don't
even understand anyway. If they want it, they will take it.

Yours,
Linus Walleij

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

[Bartosz Golaszewski]

On Thu, 23 Jul 2026 09:41:29 +0800, Junjie Cao 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.
> 
> [...]

Applied, thanks!

[1/1] gpio: pch: use raw_spinlock_t for the register lock
      https://git.kernel.org/brgl/c/a02b8950d619123da64f69b70fe1dadef217dfe4

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>