[PATCH 0/2] gpio: use raw spinlocks in irq startup paths

[PATCH 0/2] gpio: use raw spinlocks in irq startup paths

[Runyu Xiao]

This 2-patch series fixes two GPIO irqchip paths where IRQ startup or
unmask can update controller state under a regular spinlock.  On
PREEMPT_RT, that lock can sleep while irq_startup() is running in a
non-sleepable context.

Both issues were found by our static analysis tool and then manually
reviewed against the current tree.  The grounded PoCs kept the
request_threaded_irq() -> __setup_irq() -> irq_startup() carriers and
Lockdep reported "BUG: sleeping function called from invalid context" on
the corresponding driver update helpers.

Convert the affected register locks to raw_spinlock_t.  The locked
sections only serialize MMIO register access and irqchip state updates,
so they should remain non-sleeping.  The conversion does not move any
sleepable operation under a raw lock; it preserves the existing short
register-critical sections while making their non-sleeping requirement
explicit for PREEMPT_RT.

Runyu Xiao (2):
  gpio: sch: use raw_spinlock_t in the irq startup path
  gpio: eic-sprd: use raw_spinlock_t in the irq startup path

 drivers/gpio/gpio-eic-sprd.c |  8 ++++----
 drivers/gpio/gpio-sch.c      | 32 ++++++++++++++++----------------
 2 files changed, 20 insertions(+), 20 deletions(-)

-- 
2.34.1

[PATCH 1/2] gpio: sch: use raw_spinlock_t in the irq startup path

[Runyu Xiao]

sch_irq_unmask() enables the GPIO IRQ and then updates the controller
state through sch_irq_mask_unmask(), which takes sch->lock with
spin_lock_irqsave().  The callback can be reached from irq_startup()
while setting up a requested IRQ.  That path is not sleepable, but on
PREEMPT_RT a regular spinlock_t becomes a sleeping lock.

This issue was found by our static analysis tool and then manually
reviewed against the current tree.

The grounded PoC kept the request_threaded_irq() -> __setup_irq() ->
irq_startup() -> sch_irq_unmask() -> sch_irq_mask_unmask() carrier and
used the original spin_lock_irqsave(&sch->lock) edge.  Lockdep reported:

  BUG: sleeping function called from invalid context
  hardirqs last disabled at ... __setup_irq.constprop.0 ... [vuln_msv]
  sch_rt_spin_lock_irqsave+0x1c/0x30 [vuln_msv]
  sch_irq_mask_unmask.constprop.0+0x31/0x70 [vuln_msv]
  __setup_irq.constprop.0+0xd/0x30 [vuln_msv]

Convert the SCH controller lock to raw_spinlock_t.  The same lock is
also used by the GPIO direction and value callbacks, but those critical
sections only update MMIO-backed GPIO registers and do not 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.

Fixes: 7a81638485c1 ("gpio: sch: Add edge event support")
Cc: stable@vger.kernel.org
Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
---
 drivers/gpio/gpio-sch.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c
index 966d16a6d515..5e361742a11a 100644
--- a/drivers/gpio/gpio-sch.c
+++ b/drivers/gpio/gpio-sch.c
@@ -39,7 +39,7 @@
 struct sch_gpio {
 	struct gpio_chip chip;
 	void __iomem *regs;
-	spinlock_t lock;
+	raw_spinlock_t lock;
 	unsigned short resume_base;
 
 	/* GPE handling */
@@ -104,9 +104,9 @@ static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned int gpio_num)
 	struct sch_gpio *sch = gpiochip_get_data(gc);
 	unsigned long flags;
 
-	spin_lock_irqsave(&sch->lock, flags);
+	raw_spin_lock_irqsave(&sch->lock, flags);
 	sch_gpio_reg_set(sch, gpio_num, GIO, 1);
-	spin_unlock_irqrestore(&sch->lock, flags);
+	raw_spin_unlock_irqrestore(&sch->lock, flags);
 	return 0;
 }
 
@@ -122,9 +122,9 @@ static int sch_gpio_set(struct gpio_chip *gc, unsigned int gpio_num, int val)
 	struct sch_gpio *sch = gpiochip_get_data(gc);
 	unsigned long flags;
 
-	spin_lock_irqsave(&sch->lock, flags);
+	raw_spin_lock_irqsave(&sch->lock, flags);
 	sch_gpio_reg_set(sch, gpio_num, GLV, val);
-	spin_unlock_irqrestore(&sch->lock, flags);
+	raw_spin_unlock_irqrestore(&sch->lock, flags);
 
 	return 0;
 }
@@ -135,9 +135,9 @@ static int sch_gpio_direction_out(struct gpio_chip *gc, unsigned int gpio_num,
 	struct sch_gpio *sch = gpiochip_get_data(gc);
 	unsigned long flags;
 
-	spin_lock_irqsave(&sch->lock, flags);
+	raw_spin_lock_irqsave(&sch->lock, flags);
 	sch_gpio_reg_set(sch, gpio_num, GIO, 0);
-	spin_unlock_irqrestore(&sch->lock, flags);
+	raw_spin_unlock_irqrestore(&sch->lock, flags);
 
 	/*
 	 * according to the datasheet, writing to the level register has no
@@ -196,14 +196,14 @@ static int sch_irq_type(struct irq_data *d, unsigned int type)
 		return -EINVAL;
 	}
 
-	spin_lock_irqsave(&sch->lock, flags);
+	raw_spin_lock_irqsave(&sch->lock, flags);
 
 	sch_gpio_reg_set(sch, gpio_num, GTPE, rising);
 	sch_gpio_reg_set(sch, gpio_num, GTNE, falling);
 
 	irq_set_handler_locked(d, handle_edge_irq);
 
-	spin_unlock_irqrestore(&sch->lock, flags);
+	raw_spin_unlock_irqrestore(&sch->lock, flags);
 
 	return 0;
 }
@@ -215,9 +215,9 @@ static void sch_irq_ack(struct irq_data *d)
 	irq_hw_number_t gpio_num = irqd_to_hwirq(d);
 	unsigned long flags;
 
-	spin_lock_irqsave(&sch->lock, flags);
+	raw_spin_lock_irqsave(&sch->lock, flags);
 	sch_gpio_reg_set(sch, gpio_num, GTS, 1);
-	spin_unlock_irqrestore(&sch->lock, flags);
+	raw_spin_unlock_irqrestore(&sch->lock, flags);
 }
 
 static void sch_irq_mask_unmask(struct gpio_chip *gc, irq_hw_number_t gpio_num, int val)
@@ -225,9 +225,9 @@ static void sch_irq_mask_unmask(struct gpio_chip *gc, irq_hw_number_t gpio_num,
 	struct sch_gpio *sch = gpiochip_get_data(gc);
 	unsigned long flags;
 
-	spin_lock_irqsave(&sch->lock, flags);
+	raw_spin_lock_irqsave(&sch->lock, flags);
 	sch_gpio_reg_set(sch, gpio_num, GGPE, val);
-	spin_unlock_irqrestore(&sch->lock, flags);
+	raw_spin_unlock_irqrestore(&sch->lock, flags);
 }
 
 static void sch_irq_mask(struct irq_data *d)
@@ -268,12 +268,12 @@ static u32 sch_gpio_gpe_handler(acpi_handle gpe_device, u32 gpe, void *context)
 	int offset;
 	u32 ret;
 
-	spin_lock_irqsave(&sch->lock, flags);
+	raw_spin_lock_irqsave(&sch->lock, flags);
 
 	core_status = ioread32(sch->regs + CORE_BANK_OFFSET + GTS);
 	resume_status = ioread32(sch->regs + RESUME_BANK_OFFSET + GTS);
 
-	spin_unlock_irqrestore(&sch->lock, flags);
+	raw_spin_unlock_irqrestore(&sch->lock, flags);
 
 	pending = (resume_status << sch->resume_base) | core_status;
 	for_each_set_bit(offset, &pending, sch->chip.ngpio)
@@ -343,7 +343,7 @@ static int sch_gpio_probe(struct platform_device *pdev)
 
 	sch->regs = regs;
 
-	spin_lock_init(&sch->lock);
+	raw_spin_lock_init(&sch->lock);
 	sch->chip = sch_gpio_chip;
 	sch->chip.label = dev_name(dev);
 	sch->chip.parent = dev;
-- 
2.34.1

Re: [PATCH 1/2] gpio: sch: use raw_spinlock_t in the irq startup path

[Andy Shevchenko]

On Wed, Jun 17, 2026 at 11:40:34PM +0800, Runyu Xiao wrote:
> sch_irq_unmask() enables the GPIO IRQ and then updates the controller
> state through sch_irq_mask_unmask(), which takes sch->lock with
> spin_lock_irqsave().  The callback can be reached from irq_startup()
> while setting up a requested IRQ.  That path is not sleepable, but on
> PREEMPT_RT a regular spinlock_t becomes a sleeping lock.
> 
> This issue was found by our static analysis tool and then manually
> reviewed against the current tree.
> 
> The grounded PoC kept the request_threaded_irq() -> __setup_irq() ->
> irq_startup() -> sch_irq_unmask() -> sch_irq_mask_unmask() carrier and
> used the original spin_lock_irqsave(&sch->lock) edge.  Lockdep reported:
> 
>   BUG: sleeping function called from invalid context
>   hardirqs last disabled at ... __setup_irq.constprop.0 ... [vuln_msv]
>   sch_rt_spin_lock_irqsave+0x1c/0x30 [vuln_msv]
>   sch_irq_mask_unmask.constprop.0+0x31/0x70 [vuln_msv]
>   __setup_irq.constprop.0+0xd/0x30 [vuln_msv]
> 
> Convert the SCH controller lock to raw_spinlock_t.  The same lock is
> also used by the GPIO direction and value callbacks, but those critical
> sections only update MMIO-backed GPIO registers and do not 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.

Okay, no objection.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

Bart, you can take it to your branch directly in case it's not too late
for getting into v7.2-rc1, otherwise I can take via my branch and then PR
somewhere near -rc2.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] gpio: sch: use raw_spinlock_t in the irq startup path

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Read-Modify-Write cycle on the Write-1-to-Clear (W1C) status register causes lost interrupts.
--

commit 088444fdf19ac3925d66a59ae9e74b003c72c066
Author: Runyu Xiao <runyu.xiao@seu.edu.cn>

gpio: sch: use raw_spinlock_t in the irq startup path

This patch converts the SCH controller lock from spinlock_t to
raw_spinlock_t to prevent a sleep-in-invalid-context bug on PREEMPT_RT.
The lock is acquired in the non-sleepable IRQ startup path.

> diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c
> index 966d16a6d5157..5e361742a11ad 100644
> --- a/drivers/gpio/gpio-sch.c
> +++ b/drivers/gpio/gpio-sch.c

[ ... ]

> @@ -215,9 +215,9 @@ static void sch_irq_ack(struct irq_data *d)
>  	irq_hw_number_t gpio_num = irqd_to_hwirq(d);
>  	unsigned long flags;
>  
> -	spin_lock_irqsave(&sch->lock, flags);
> +	raw_spin_lock_irqsave(&sch->lock, flags);
>  	sch_gpio_reg_set(sch, gpio_num, GTS, 1);

[Severity: High]
This isn't a bug introduced by this patch, but does this cause lost
interrupts due to a read-modify-write on a write-1-to-clear register?

sch_irq_ack() acknowledges a GPIO interrupt by calling
sch_gpio_reg_set() for the GTS register. 

Looking at sch_gpio_reg_set():

drivers/gpio/gpio-sch.c:sch_gpio_reg_set() {
    ...
    reg_val = ioread8(sch->regs + offset);

    if (val)
        reg_val |= BIT(bit);
    else
        reg_val &= ~BIT(bit);

    iowrite8(reg_val, sch->regs + offset);
}

And sch_gpio_gpe_handler() demonstrates that GTS returns the status of
pending interrupts when read:

drivers/gpio/gpio-sch.c:sch_gpio_gpe_handler() {
    ...
    core_status = ioread32(sch->regs + CORE_BANK_OFFSET + GTS);
    ...
}

Because GTS is a write-1-to-clear status register, the ioread8() in
sch_gpio_reg_set() returns 1s for all currently pending interrupts.

When those 1s are written back via iowrite8(), won't it inadvertently
clear all pending interrupts in that bank, instead of just the specific
gpio_num being acknowledged?

> -	spin_unlock_irqrestore(&sch->lock, flags);
> +	raw_spin_unlock_irqrestore(&sch->lock, flags);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260617154035.1199948-1-runyu.xiao@seu.edu.cn?part=1

[PATCH 2/2] gpio: eic-sprd: use raw_spinlock_t in the irq startup path

[Runyu Xiao]

sprd_eic_irq_unmask() enables the GPIO IRQ and then updates controller
state through sprd_eic_update(), which takes sprd_eic->lock with
spin_lock_irqsave().  The callback can be reached from irq_startup()
while setting up a requested IRQ.  That path is not sleepable, but on
PREEMPT_RT a regular spinlock_t becomes a sleeping lock.

This issue was found by our static analysis tool and then manually
reviewed against the current tree.

The grounded PoC kept the request_threaded_irq() -> __setup_irq() ->
irq_startup() -> sprd_eic_irq_unmask() -> sprd_eic_update() carrier and
used the original spin_lock_irqsave(&sprd_eic->lock) edge.  Lockdep
reported:

  BUG: sleeping function called from invalid context
  hardirqs last disabled at ... __setup_irq.constprop.0 ... [vuln_msv]
  sprd_rt_spin_lock_irqsave+0x1c/0x30 [vuln_msv]
  sprd_eic_update.constprop.0+0x48/0x90 [vuln_msv]
  sprd_eic_irq_unmask.constprop.0+0x35/0x50 [vuln_msv]
  __setup_irq.constprop.0+0xd/0x30 [vuln_msv]

Convert the Spreadtrum EIC controller lock to raw_spinlock_t.  The
locked section only serializes MMIO register updates and does not contain
sleepable operations, so keeping it non-sleeping is appropriate for the
irqchip callbacks.

Fixes: 25518e024e3a ("gpio: Add Spreadtrum EIC driver support")
Cc: stable@vger.kernel.org
Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
---
 drivers/gpio/gpio-eic-sprd.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpio-eic-sprd.c b/drivers/gpio/gpio-eic-sprd.c
index 50fafeda8d7e..3b7ebcf12fe7 100644
--- a/drivers/gpio/gpio-eic-sprd.c
+++ b/drivers/gpio/gpio-eic-sprd.c
@@ -95,7 +95,7 @@ struct sprd_eic {
 	struct notifier_block irq_nb;
 	void __iomem *base[SPRD_EIC_MAX_BANK];
 	enum sprd_eic_type type;
-	spinlock_t lock;
+	raw_spinlock_t lock;
 	int irq;
 };
 
@@ -149,7 +149,7 @@ static void sprd_eic_update(struct gpio_chip *chip, unsigned int offset,
 	unsigned long flags;
 	u32 tmp;
 
-	spin_lock_irqsave(&sprd_eic->lock, flags);
+	raw_spin_lock_irqsave(&sprd_eic->lock, flags);
 	tmp = readl_relaxed(base + reg);
 
 	if (val)
@@ -158,7 +158,7 @@ static void sprd_eic_update(struct gpio_chip *chip, unsigned int offset,
 		tmp &= ~BIT(SPRD_EIC_BIT(offset));
 
 	writel_relaxed(tmp, base + reg);
-	spin_unlock_irqrestore(&sprd_eic->lock, flags);
+	raw_spin_unlock_irqrestore(&sprd_eic->lock, flags);
 }
 
 static int sprd_eic_read(struct gpio_chip *chip, unsigned int offset, u16 reg)
@@ -628,7 +628,7 @@ static int sprd_eic_probe(struct platform_device *pdev)
 	if (!sprd_eic)
 		return -ENOMEM;
 
-	spin_lock_init(&sprd_eic->lock);
+	raw_spin_lock_init(&sprd_eic->lock);
 	sprd_eic->type = pdata->type;
 
 	sprd_eic->irq = platform_get_irq(pdev, 0);
-- 
2.34.1

Re: [PATCH 2/2] gpio: eic-sprd: use raw_spinlock_t in the irq startup path

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Potential NULL pointer dereference in `sprd_eic_toggle_trigger` for unmapped or spurious hardware interrupts.
- [High] `sprd_eic_toggle_trigger` fails to properly toggle polarity for `SPRD_EIC_LATCH` because `sprd_eic_get` returns `-ENOTSUPP`.
- [High] Potential hardirq livelock and console spam due to `dev_warn` inside a retry loop in `sprd_eic_toggle_trigger`.
- [High] Unbinding one EIC instance breaks all other EIC instances because they share the same parent IRQ chained handler.
- [High] sprd_eic_toggle_trigger modifies interrupt masks unconditionally without holding desc->lock, creating a race condition with disable_irq().
--

commit 69e3dda6e9ba1a662342eec6f6b0ec04beeadc64
Author: Runyu Xiao <runyu.xiao@seu.edu.cn>

gpio: eic-sprd: use raw_spinlock_t in the irq startup path

sprd_eic_irq_unmask() enables the GPIO IRQ and then updates controller
state through sprd_eic_update(), which takes sprd_eic->lock with
spin_lock_irqsave(). The callback can be reached from irq_startup()
while setting up a requested IRQ, which is not sleepable on PREEMPT_RT.
This patch converts the lock to raw_spinlock_t to avoid sleeping.

> diff --git a/drivers/gpio/gpio-eic-sprd.c b/drivers/gpio/gpio-eic-sprd.c
> index 50fafeda8d7e2..3b7ebcf12fe7f 100644
> --- a/drivers/gpio/gpio-eic-sprd.c
> +++ b/drivers/gpio/gpio-eic-sprd.c
[ ... ]
> @@ -149,7 +149,7 @@ static void sprd_eic_update(struct gpio_chip *chip, unsigned int offset,
>  	unsigned long flags;
>  	u32 tmp;
>  
> -	spin_lock_irqsave(&sprd_eic->lock, flags);
> +	raw_spin_lock_irqsave(&sprd_eic->lock, flags);
>  	tmp = readl_relaxed(base + reg);
>  
>  	if (val)
> @@ -158,7 +158,7 @@ static void sprd_eic_update(struct gpio_chip *chip, unsigned int offset,
>  		tmp &= ~BIT(SPRD_EIC_BIT(offset));
>  
>  	writel_relaxed(tmp, base + reg);
> -	spin_unlock_irqrestore(&sprd_eic->lock, flags);
> +	raw_spin_unlock_irqrestore(&sprd_eic->lock, flags);
>  }

[Severity: Critical]
This is a pre-existing issue, but can sprd_eic_handle_one_type() cause a
NULL pointer dereference for unmapped or spurious hardware interrupts?

If an interrupt is unmapped, irq_find_mapping() returns 0:

drivers/gpio/gpio-eic-sprd.c:sprd_eic_handle_one_type() {
    ...
    girq = irq_find_mapping(chip->irq.domain, offset);
    generic_handle_irq(girq);
    sprd_eic_toggle_trigger(chip, girq, offset);
}

The girq value 0 is then passed into sprd_eic_toggle_trigger():

drivers/gpio/gpio-eic-sprd.c:sprd_eic_toggle_trigger() {
    ...
    struct irq_data *data = irq_get_irq_data(irq);
    u32 trigger = irqd_get_trigger_type(data);
    ...
}

Calling irq_get_irq_data(0) returns NULL, which causes irqd_get_trigger_type()
to dereference NULL. Can this path be triggered by the bootloader leaving an
interrupt unmasked?

[Severity: High]
This is a pre-existing issue, but does sprd_eic_toggle_trigger() race with
disable_irq() since it alters the hardware mask without holding the
descriptor lock?

Looking at sprd_eic_toggle_trigger(), it unconditionally calls
sprd_eic_irq_unmask() after returning from an interrupt handler:

drivers/gpio/gpio-eic-sprd.c:sprd_eic_toggle_trigger() {
    ...
    sprd_eic_irq_mask(data);
    ...
    sprd_eic_irq_unmask(data);
}

If a concurrent thread calls disable_irq(), which holds desc->lock and masks
the hardware, sprd_eic_toggle_trigger() could execute immediately afterward and
unmask it again without checking irqd_irq_disabled(data). Can this leave the
system vulnerable to unhandled interrupt storms?

[Severity: High]
This is a pre-existing issue, but does sprd_eic_toggle_trigger() have a
potential hardirq livelock and console spam issue?

If the line state changes, the code logs a warning and retries endlessly:

drivers/gpio/gpio-eic-sprd.c:sprd_eic_toggle_trigger() {
    ...
retry:
    ...
    post_state = sprd_eic_get(chip, offset);
    if (state != post_state) {
        dev_warn(chip->parent, "EIC level was changed.\n");
        state = post_state;
        goto retry;
    }
}

Since this runs in the context of a cascaded interrupt handler, if the input
line oscillates rapidly, will the dev_warn() overhead inside a tight loop
cause lockups and watchdog panics without any retry bounds?

[Severity: High]
This is a pre-existing issue, but does SPRD_EIC_LATCH fail to toggle polarity?

In sprd_eic_toggle_trigger(), the state is used to set the latch polarity:

drivers/gpio/gpio-eic-sprd.c:sprd_eic_toggle_trigger() {
    ...
    state = sprd_eic_get(chip, offset);
    ...
    case SPRD_EIC_LATCH:
        if (state)
            sprd_eic_update(chip, offset, SPRD_EIC_LATCH_INTPOL, 0);
        else
            sprd_eic_update(chip, offset, SPRD_EIC_LATCH_INTPOL, 1);
        break;
}

However, sprd_eic_get() doesn't implement reading for SPRD_EIC_LATCH
and returns -ENOTSUPP. This negative value evaluates as true, meaning the
code will always execute the true branch and never toggle to 1. Will this
cause LATCH EICs to become permanently stuck?

[ ... ]
> @@ -628,7 +628,7 @@ static int sprd_eic_probe(struct platform_device *pdev)
>  	if (!sprd_eic)
>  		return -ENOMEM;
>  
> -	spin_lock_init(&sprd_eic->lock);
> +	raw_spin_lock_init(&sprd_eic->lock);
>  	sprd_eic->type = pdata->type;
>  
>  	sprd_eic->irq = platform_get_irq(pdev, 0);

[Severity: High]
This is a pre-existing issue, but will unbinding one EIC instance break all
other EIC instances?

During probe, each instance sets its parent handler to sprd_eic_irq_handler
on the shared parent IRQ:

drivers/gpio/gpio-eic-sprd.c:sprd_eic_probe() {
    ...
    irq->parent_handler = sprd_eic_irq_handler;
    irq->parent_handler_data = sprd_eic;
    irq->num_parents = 1;
    irq->parents = &sprd_eic->irq;
    ...
}

If any single EIC instance is unbound, gpiolib's cleanup routine calls
irq_set_chained_handler_and_data(parent, NULL, NULL), clearing the shared
parent handler entirely. Will this leave the remaining EIC instances
completely non-functional?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260617154035.1199948-1-runyu.xiao@seu.edu.cn?part=2