drivers/pinctrl/stm32/pinctrl-stm32.c | 55 +++++++++++++-------------- 1 file changed, 26 insertions(+), 29 deletions(-)
stm32_gpio_domain_activate() writes the EXTI interrupt multiplexer
through a regmap obtained from the generic syscon driver. The irq core
calls .activate from __setup_irq() with the raw desc->lock held, so this
happens in a raw atomic section. regmap-mmio sets fast_io, and syscon
does not ask for a raw spinlock, so the regmap is protected by a
spinlock_t. On PREEMPT_RT that is a sleeping lock, which must not be
taken there. lockdep reports it as soon as a GPIO interrupt is
requested:
BUG: Invalid wait context
6.17.0 #4 Not tainted
-----------------------------
kworker/u8:0/12 is trying to lock:
(&syscon_config)->lock){....}-{3:3}, at: regmap_lock_spinlock
other info that might help us debug this:
6 locks held by kworker/u8:0/12:
#5: (&irq_desc_lock_class){-...}-{2:2}, at: __setup_irq
stack backtrace:
regmap_lock_spinlock
regmap_field_update_bits_base
stm32_gpio_domain_activate
irq_domain_activate_irq
__setup_irq
request_threaded_irq
The driver was already aware of running in atomic context there: it uses
the _in_atomic() hwspinlock primitives around the very same access. The
syscon lock is the one lock in that section it does not control.
Program the mux from .alloc instead, which runs in a sleepable context.
That callback already owns this resource: it reserves the mux line in
pctl->irqmux_map under irqmux_lock, so writing the value it just claimed
is a natural fit, and the value only depends on the bank.
Nothing requires the mux to be reprogrammed at interrupt startup time:
the domain has no .deactivate, .free() only releases the irqmux_map bit
without touching the registers, and the resume path reprograms the mux
itself in stm32_pinctrl_restore_gpio_regs().
Keep the access inside the existing irqmux_lock section rather than
after it. hwspin_lock_timeout_in_atomic() deliberately skips the local
lock that the other hwspinlock modes take, so its caller is responsible
for disabling preemption and for excluding other local contexts; every
other hwspinlock user in this driver relies on bank->lock for that.
irqmux_lock plays the same role here, and it also turns the mux
reservation and the mux write into a single critical section. Nesting
the regmap spinlock_t inside irqmux_lock, itself a spinlock_t, is what
the wait-context checker expects.
Note that this bounds the semaphore hold time on !PREEMPT_RT only. On
PREEMPT_RT spinlock_t disables neither interrupts nor preemption, so the
hardware semaphore can be held across a preemptible section and a
coprocessor contending for it may poll for an unbounded time. That is
not introduced here: bank->lock is a spinlock_t too, so every existing
hwspinlock user in this driver behaves the same way on PREEMPT_RT.
Removing that exposure requires the EXTI mux regmap to be raw-spinlock
based, which the syscon core does not currently offer, and is left for a
separate change.
While moving the code, release the mux reservation when the hwspinlock
cannot be taken, which the .activate callback had no way of doing.
Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Closes: https://lore.kernel.org/all/20220202174430.pf37tt6lua2op3gc@pengutronix.de/
Signed-off-by: Ju Nan <junan76@163.com>
---
Changes in v3:
- Keep the mux write inside the existing irqmux_lock critical section
rather than after it.
v2: https://lore.kernel.org/all/20260804032227.35017-3-junan76@163.com/
v1: https://lore.kernel.org/all/20260803061718.43210-1-junan76@163.com/
---
drivers/pinctrl/stm32/pinctrl-stm32.c | 55 +++++++++++++--------------
1 file changed, 26 insertions(+), 29 deletions(-)
diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c
index 6a99708a5a23..d97057ec28af 100644
--- a/drivers/pinctrl/stm32/pinctrl-stm32.c
+++ b/drivers/pinctrl/stm32/pinctrl-stm32.c
@@ -600,30 +600,6 @@ static int stm32_gpio_domain_translate(struct irq_domain *d,
return 0;
}
-static int stm32_gpio_domain_activate(struct irq_domain *d,
- struct irq_data *irq_data, bool reserve)
-{
- struct stm32_gpio_bank *bank = d->host_data;
- struct stm32_pinctrl *pctl = dev_get_drvdata(bank->gpio_chip.parent);
- int ret = 0;
-
- if (pctl->hwlock) {
- ret = hwspin_lock_timeout_in_atomic(pctl->hwlock,
- HWSPNLCK_TIMEOUT);
- if (ret) {
- dev_err(pctl->dev, "Can't get hwspinlock\n");
- return ret;
- }
- }
-
- regmap_field_write(pctl->irqmux[irq_data->hwirq], bank->bank_ioport_nr);
-
- if (pctl->hwlock)
- hwspin_unlock_in_atomic(pctl->hwlock);
-
- return ret;
-}
-
static int stm32_gpio_domain_alloc(struct irq_domain *d,
unsigned int virq,
unsigned int nr_irqs, void *data)
@@ -637,18 +613,40 @@ static int stm32_gpio_domain_alloc(struct irq_domain *d,
int ret = 0;
/*
- * Check first that the IRQ MUX of that line is free.
- * gpio irq mux is shared between several banks, protect with a lock
+ * Check first that the IRQ MUX of that line is free, then point it at
+ * this bank. The mux is shared between several banks, and the register
+ * is shared with a coprocessor, so hold both irqmux_lock and the
+ * hwspinlock across the update. irqmux_lock also provides the local
+ * exclusion and the preemption disabling that the _in_atomic()
+ * hwspinlock primitives leave to their caller, as it does for the other
+ * hwspinlock users in this driver.
*/
spin_lock_irqsave(&pctl->irqmux_lock, flags);
if (pctl->irqmux_map & BIT(hwirq)) {
dev_err(pctl->dev, "irq line %ld already requested.\n", hwirq);
ret = -EBUSY;
- } else {
- pctl->irqmux_map |= BIT(hwirq);
+ goto unlock;
+ }
+
+ pctl->irqmux_map |= BIT(hwirq);
+
+ if (pctl->hwlock) {
+ ret = hwspin_lock_timeout_in_atomic(pctl->hwlock,
+ HWSPNLCK_TIMEOUT);
+ if (ret) {
+ dev_err(pctl->dev, "Can't get hwspinlock\n");
+ pctl->irqmux_map &= ~BIT(hwirq);
+ goto unlock;
+ }
}
+ regmap_field_write(pctl->irqmux[hwirq], bank->bank_ioport_nr);
+
+ if (pctl->hwlock)
+ hwspin_unlock_in_atomic(pctl->hwlock);
+
+unlock:
spin_unlock_irqrestore(&pctl->irqmux_lock, flags);
if (ret)
return ret;
@@ -683,7 +681,6 @@ static const struct irq_domain_ops stm32_gpio_domain_ops = {
.translate = stm32_gpio_domain_translate,
.alloc = stm32_gpio_domain_alloc,
.free = stm32_gpio_domain_free,
- .activate = stm32_gpio_domain_activate,
};
/* Pinctrl functions */
--
2.55.0
On Tue, 2026-08-04 at 22:14 +0800, Ju Nan wrote:
> stm32_gpio_domain_activate() writes the EXTI interrupt multiplexer
> through a regmap obtained from the generic syscon driver. The irq core
> calls .activate from __setup_irq() with the raw desc->lock held, so this
> happens in a raw atomic section. regmap-mmio sets fast_io, and syscon
> does not ask for a raw spinlock, so the regmap is protected by a
> spinlock_t. On PREEMPT_RT that is a sleeping lock, which must not be
> taken there. lockdep reports it as soon as a GPIO interrupt is
> requested:
>
> BUG: Invalid wait context
> 6.17.0 #4 Not tainted
> -----------------------------
> kworker/u8:0/12 is trying to lock:
> (&syscon_config)->lock){....}-{3:3}, at: regmap_lock_spinlock
> other info that might help us debug this:
> 6 locks held by kworker/u8:0/12:
> #5: (&irq_desc_lock_class){-...}-{2:2}, at: __setup_irq
> stack backtrace:
> regmap_lock_spinlock
> regmap_field_update_bits_base
> stm32_gpio_domain_activate
> irq_domain_activate_irq
> __setup_irq
> request_threaded_irq
>
> The driver was already aware of running in atomic context there: it uses
> the _in_atomic() hwspinlock primitives around the very same access. The
> syscon lock is the one lock in that section it does not control.
>
> Program the mux from .alloc instead, which runs in a sleepable context.
> That callback already owns this resource: it reserves the mux line in
> pctl->irqmux_map under irqmux_lock, so writing the value it just claimed
> is a natural fit, and the value only depends on the bank.
>
> Nothing requires the mux to be reprogrammed at interrupt startup time:
> the domain has no .deactivate, .free() only releases the irqmux_map bit
> without touching the registers, and the resume path reprograms the mux
> itself in stm32_pinctrl_restore_gpio_regs().
>
> Keep the access inside the existing irqmux_lock section rather than
> after it. hwspin_lock_timeout_in_atomic() deliberately skips the local
> lock that the other hwspinlock modes take, so its caller is responsible
> for disabling preemption and for excluding other local contexts; every
> other hwspinlock user in this driver relies on bank->lock for that.
> irqmux_lock plays the same role here, and it also turns the mux
> reservation and the mux write into a single critical section. Nesting
> the regmap spinlock_t inside irqmux_lock, itself a spinlock_t, is what
> the wait-context checker expects.
>
> Note that this bounds the semaphore hold time on !PREEMPT_RT only. On
> PREEMPT_RT spinlock_t disables neither interrupts nor preemption, so the
> hardware semaphore can be held across a preemptible section and a
> coprocessor contending for it may poll for an unbounded time. That is
> not introduced here: bank->lock is a spinlock_t too, so every existing
> hwspinlock user in this driver behaves the same way on PREEMPT_RT.
> Removing that exposure requires the EXTI mux regmap to be raw-spinlock
> based, which the syscon core does not currently offer, and is left for a
> separate change.
>
> While moving the code, release the mux reservation when the hwspinlock
> cannot be taken, which the .activate callback had no way of doing.
>
> Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Closes: https://lore.kernel.org/all/20220202174430.pf37tt6lua2op3gc@pengutronix.de/
> Signed-off-by: Ju Nan <junan76@163.com>
> ---
> Changes in v3:
>
> - Keep the mux write inside the existing irqmux_lock critical section
> rather than after it.
>
> v2: https://lore.kernel.org/all/20260804032227.35017-3-junan76@163.com/
> v1: https://lore.kernel.org/all/20260803061718.43210-1-junan76@163.com/
> ---
> drivers/pinctrl/stm32/pinctrl-stm32.c | 55 +++++++++++++--------------
> 1 file changed, 26 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c
> index 6a99708a5a23..d97057ec28af 100644
> --- a/drivers/pinctrl/stm32/pinctrl-stm32.c
> +++ b/drivers/pinctrl/stm32/pinctrl-stm32.c
> @@ -600,30 +600,6 @@ static int stm32_gpio_domain_translate(struct irq_domain *d,
> return 0;
> }
>
> -static int stm32_gpio_domain_activate(struct irq_domain *d,
> - struct irq_data *irq_data, bool reserve)
> -{
> - struct stm32_gpio_bank *bank = d->host_data;
> - struct stm32_pinctrl *pctl = dev_get_drvdata(bank->gpio_chip.parent);
> - int ret = 0;
> -
> - if (pctl->hwlock) {
> - ret = hwspin_lock_timeout_in_atomic(pctl->hwlock,
> - HWSPNLCK_TIMEOUT);
> - if (ret) {
> - dev_err(pctl->dev, "Can't get hwspinlock\n");
> - return ret;
> - }
> - }
> -
> - regmap_field_write(pctl->irqmux[irq_data->hwirq], bank->bank_ioport_nr);
> -
> - if (pctl->hwlock)
> - hwspin_unlock_in_atomic(pctl->hwlock);
> -
> - return ret;
> -}
> -
> static int stm32_gpio_domain_alloc(struct irq_domain *d,
> unsigned int virq,
> unsigned int nr_irqs, void *data)
> @@ -637,18 +613,40 @@ static int stm32_gpio_domain_alloc(struct irq_domain *d,
> int ret = 0;
>
> /*
> - * Check first that the IRQ MUX of that line is free.
> - * gpio irq mux is shared between several banks, protect with a lock
> + * Check first that the IRQ MUX of that line is free, then point it at
> + * this bank. The mux is shared between several banks, and the register
> + * is shared with a coprocessor, so hold both irqmux_lock and the
> + * hwspinlock across the update. irqmux_lock also provides the local
> + * exclusion and the preemption disabling that the _in_atomic()
> + * hwspinlock primitives leave to their caller, as it does for the other
> + * hwspinlock users in this driver.
> */
> spin_lock_irqsave(&pctl->irqmux_lock, flags);
>
> if (pctl->irqmux_map & BIT(hwirq)) {
> dev_err(pctl->dev, "irq line %ld already requested.\n", hwirq);
> ret = -EBUSY;
> - } else {
> - pctl->irqmux_map |= BIT(hwirq);
> + goto unlock;
> + }
> +
> + pctl->irqmux_map |= BIT(hwirq);
> +
> + if (pctl->hwlock) {
> + ret = hwspin_lock_timeout_in_atomic(pctl->hwlock,
> + HWSPNLCK_TIMEOUT);
Your other patch
https://lore.kernel.org/lkml/20260827141127.25789-2-junan76@163.com/
that renames s/HWSPNLCK_TIMEOUT/HWSPNLCK_TIMEOUT_MS/ depends on this code move to
.alloc(), but the dependency is not explicitly reported.
It's probably better to keep the two patches in a series so the dependency is clear.
> + if (ret) {
> + dev_err(pctl->dev, "Can't get hwspinlock\n");
> + pctl->irqmux_map &= ~BIT(hwirq);
> + goto unlock;
> + }
> }
>
> + regmap_field_write(pctl->irqmux[hwirq], bank->bank_ioport_nr);
I think there is an issue here, which is probably the initial reason for having the
regmap write in the separate .activate().
You can have two GPIO from separate banks but on the same bank's line (let's say PA5
and PB5, on different bank A vs B but on same line 5) and both require to be IRQ.
Clearly only one GPIO will "win".
The MUX does not have a busy state to know it's already taken. The .alloc() here
relies on the common IRQ parent behind the MUX for that line to detect that the
line (5 in this example) is already taken.
In fact, a few lines below, not show in this diff, .alloc() terminates with:
return irq_domain_alloc_irqs_parent(d, virq, nr_irqs, &parent_fwspec);
The first GPIO that enters here (e.g. PA5) will set the MUX with regmap, then allocate
the parent's IRQ and return ok.
The second GPIO (e.g. PB5) will also set the MUX (breaking the previous settings for
PA5), try to allocate the same parent's IRQ and fail.
Result, the first GPIO setting is now broken!
While moving the .activate() code in .alloc(), please take care of this issue.
Regards,
Antonio
> +
> + if (pctl->hwlock)
> + hwspin_unlock_in_atomic(pctl->hwlock);
> +
> +unlock:
> spin_unlock_irqrestore(&pctl->irqmux_lock, flags);
> if (ret)
> return ret;
> @@ -683,7 +681,6 @@ static const struct irq_domain_ops stm32_gpio_domain_ops = {
> .translate = stm32_gpio_domain_translate,
> .alloc = stm32_gpio_domain_alloc,
> .free = stm32_gpio_domain_free,
> - .activate = stm32_gpio_domain_activate,
> };
>
> /* Pinctrl functions */
On 2026-08-04 22:14:03 [+0800], Ju Nan wrote:
…
> Nothing requires the mux to be reprogrammed at interrupt startup time:
> the domain has no .deactivate, .free() only releases the irqmux_map bit
> without touching the registers, and the resume path reprograms the mux
> itself in stm32_pinctrl_restore_gpio_regs().
The deactivate was last removed in commit db0f032512443 ("pinctrl:
stm32: check for IRQ MUX validity during alloc()")
…
> While moving the code, release the mux reservation when the hwspinlock
> cannot be taken, which the .activate callback had no way of doing.
It makes sense.
Acked-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Closes: https://lore.kernel.org/all/20220202174430.pf37tt6lua2op3gc@pengutronix.de/
> Signed-off-by: Ju Nan <junan76@163.com>
Sebastian
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: Pre-existing issues: - [High] Missing hardware spinlock serialization when restoring `irqmux` in the resume path. - [Medium] Missing cleanup of `irqmux_map` reservation and hardware mux state when parent IRQ domain allocation fails. - [Medium] HWSPNLCK_TIMEOUT is defined as microseconds but passed to a function expecting milliseconds. -- --- Patch [1]: [PATCH v3] pinctrl: stm32: program the EXTI mux from .alloc instead of .activate --- (Note: The format of this report has been altered to provide a free-form summary without quoting the original patch, due to recitation restrictions.) commit a303f103b81b7e7b2ea355f3dc199454d3911bbc Author: Ju Nan <junan76@163.com> pinctrl: stm32: program the EXTI mux from .alloc instead of .activate This commit moves the programming of the EXTI interrupt multiplexer from the .activate callback to the sleepable .alloc callback. This prevents a wait context violation on PREEMPT_RT caused by a spinlock-protected regmap access in the syscon driver. It also ensures the EXTI mux write is serialized within the existing irqmux_lock section. Closes: https://lore.kernel.org/all/20220202174430.pf37tt6lua2op3gc@pengutronix.de/ v2: https://lore.kernel.org/all/20260804032227.35017-3-junan76@163.com/ v1: https://lore.kernel.org/all/20260803061718.43210-1-junan76@163.com/ [Severity: Medium] This is a pre-existing issue, but in stm32_gpio_domain_alloc(), if irq_domain_alloc_irqs_parent() fails, the function directly returns the error without reverting the irqmux_map reservation or the hardware state. Because the core IRQ subsystem does not invoke the .free callback for a domain that fails mid-allocation, doesn't this result in the irqmux_map bit remaining permanently set and the hardware EXTI mux remaining incorrectly programmed for an abandoned allocation? [Severity: Medium] This is also a pre-existing issue, but when hwspin_lock_timeout_in_atomic() is called in stm32_gpio_domain_alloc(), HWSPNLCK_TIMEOUT is passed as the timeout argument. Since HWSPNLCK_TIMEOUT is defined as 1000 microseconds, but the timeout parameter expects milliseconds (using msecs_to_jiffies), doesn't this result in a 1-second timeout instead of a 1-millisecond timeout? Spinning for 1 second in an atomic context could potentially trigger lockup detectors if the lock is heavily contended. [Severity: High] This is another pre-existing issue, but in stm32_pinctrl_restore_gpio_regs(), the irqmux is reprogrammed using regmap_field_write() without holding the hardware spinlock. Since stm32_gpio_domain_alloc() correctly serializes access to the shared EXTI register against the coprocessor by acquiring pctl->hwlock, doesn't the unprotected write in the resume path risk a data race and potential hardware register corruption if the coprocessor contends for it concurrently? -- Sashiko AI review · https://sashiko.dev/#/patchset/20260804141402.86911-2-junan76@163.com?part=1
© 2016 - 2026 Red Hat, Inc.