[PATCH] pinctrl: stm32: use a raw spinlock regmap to program the EXTI mux

[PATCH] pinctrl: stm32: use a raw spinlock regmap to program the EXTI mux

[Ju Nan]

stm32_gpio_domain_activate() programs the EXTI interrupt multiplexer
with regmap_field_write(), on a regmap the driver obtains from the generic
syscon driver via syscon_regmap_lookup_by_phandle(np, "st,syscfg").

An irq_domain .activate callback is by contract called from raw atomic
context: __setup_irq() takes the raw desc->lock and calls
irq_activate() while holding it.

That regmap is created by syscon with regmap_init_mmio(). The regmap-mmio
bus sets .fast_io = true, and syscon does not set use_raw_spinlock, so
__regmap_init() protects the regmap with a spinlock_t.

On !RT a spinlock_t only ever spins and nothing bad happens at runtime,
which is why this has gone unnoticed. On PREEMPT_RT spinlock_t is a
sleeping lock, and taking it under the raw desc->lock is a sleep in atomic
context. lockdep's wait-context checker catches this ahead of time — with
CONFIG_PROVE_RAW_LOCK_NESTING=y the driver splats on boot as soon as
anything requests a GPIO interrupt (here: an sii902x HDMI bridge):

BUG: Invalid wait context
...
(&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

i.e. a wait type 3 (LD_WAIT_CONFIG, sleeping-on-RT) lock is acquired while
the raw desc->lock has already limited the context to wait type 2
(LD_WAIT_SPIN).

Note the driver is already aware that it runs in atomic context here: it uses
the _in_atomic() hwspinlock primitives around this very same register
access. The syscon lock is the one lock in that section it does not control.

The fix:

Register a regmap with use_raw_spinlock = true for the node through
of_syscon_register_regmap() before looking it up, so the syscon layer hands
out that one instead of instantiating its default. It has to go through the
syscon layer rather than staying private to the driver, because both pinctrl
instances of an STM32MP1 (pinctrl and pinctrl_z) reference the same node —
private regmaps would give them one lock each and no mutual exclusion on the
mux registers. Same pattern as drivers/soc/samsung/exynos-pmu.c.

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>
---
 drivers/pinctrl/stm32/pinctrl-stm32.c | 64 +++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c
index 6a99708a5a23..39df117489d1 100644
--- a/drivers/pinctrl/stm32/pinctrl-stm32.c
+++ b/drivers/pinctrl/stm32/pinctrl-stm32.c
@@ -1757,6 +1757,68 @@ static struct irq_domain *stm32_pctrl_get_irq_domain(struct platform_device *pde
 	return domain;
 }
 
+/*
+ * The interrupt mux registers are written from stm32_gpio_domain_activate(),
+ * which the irq core calls with the raw desc->lock held. The regmap the
+ * generic syscon driver hands out is protected by a spinlock_t, which may
+ * sleep on PREEMPT_RT and therefore must not be taken from there.
+ *
+ * Publish a raw spinlock regmap for the node before looking it up, so that
+ * all of its users keep sharing one regmap, and one lock.
+ */
+static const struct regmap_config stm32_pctrl_syscfg_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.use_raw_spinlock = true,
+};
+
+static void stm32_pctrl_publish_syscfg_regmap(struct device_node *np)
+{
+	struct regmap_config config = stm32_pctrl_syscfg_regmap_config;
+	struct device_node *syscfg_np;
+	struct regmap *regmap;
+	void __iomem *base;
+	struct resource res;
+
+	syscfg_np = of_parse_phandle(np, "st,syscfg", 0);
+	if (!syscfg_np)
+		return;
+
+	if (of_address_to_resource(syscfg_np, 0, &res) ||
+	    resource_size(&res) < config.reg_stride)
+		goto out_put;
+
+	config.max_register = resource_size(&res) - config.reg_stride;
+
+	base = of_iomap(syscfg_np, 0);
+	if (!base)
+		goto out_put;
+
+	/*
+	 * The regmap is handed over to the syscon layer, which never releases
+	 * it, so it must outlive this driver: no device managed allocation
+	 * here, and no device to attach it to either.
+	 */
+	regmap = regmap_init_mmio(NULL, base, &config);
+	if (IS_ERR(regmap)) {
+		iounmap(base);
+		goto out_put;
+	}
+
+	/*
+	 * A regmap is already registered for that node, most likely by the
+	 * other pinctrl instance sharing it. Drop ours and use that one.
+	 */
+	if (of_syscon_register_regmap(syscfg_np, regmap)) {
+		regmap_exit(regmap);
+		iounmap(base);
+	}
+
+out_put:
+	of_node_put(syscfg_np);
+}
+
 static int stm32_pctrl_dt_setup_irq(struct platform_device *pdev,
 			   struct stm32_pinctrl *pctl)
 {
@@ -1766,6 +1828,8 @@ static int stm32_pctrl_dt_setup_irq(struct platform_device *pdev,
 	int offset, ret, i;
 	int mask, mask_width;
 
+	stm32_pctrl_publish_syscfg_regmap(np);
+
 	pctl->regmap = syscon_regmap_lookup_by_phandle(np, "st,syscfg");
 	if (IS_ERR(pctl->regmap))
 		return PTR_ERR(pctl->regmap);
-- 
2.55.0

Re: [PATCH] pinctrl: stm32: use a raw spinlock regmap to program the EXTI mux

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Bypassing standard syscon regmap initialization leaves the syscfg clock disabled, leading to hardware bus faults on platforms where the syscfg node requires a clock.
- [High] If another driver sharing the syscon node probes before pinctrl, the generic syscon layer will create a default regmap without `use_raw_spinlock`, causing the PREEMPT_RT sleeping-in-atomic bug to persist.
--

--- Patch [1]: [PATCH] pinctrl: stm32: use a raw spinlock regmap to program the EXTI mux ---
commit e662d8a81eaff2488e3427af7c289ca0a5b94fde
Author: Ju Nan <junan76@163.com>
Subject: pinctrl: stm32: use a raw spinlock regmap to program the EXTI mux

This commit addresses an invalid wait context issue on PREEMPT_RT by manually
publishing a regmap with use_raw_spinlock = true for the st,syscfg node. It
attempts to preempt the generic syscon layer from instantiating a default
regmap protected by a sleeping spinlock. The interrupt multiplexer is then
programmed using this raw-spinlock-protected regmap.

Closes: https://lore.kernel.org/all/20220202174430.pf37tt6lua2op3gc@pengutronix.de/

> diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c
> index 6a99708a5a23c..39df117489d12 100644
> --- a/drivers/pinctrl/stm32/pinctrl-stm32.c
> +++ b/drivers/pinctrl/stm32/pinctrl-stm32.c
[ ... ]
> @@ -1757,6 +1757,68 @@ static struct irq_domain *stm32_pctrl_get_irq_domain(struct platform_device *pde
[ ... ]
> +static void stm32_pctrl_publish_syscfg_regmap(struct device_node *np)
> +{
> +	struct regmap_config config = stm32_pctrl_syscfg_regmap_config;
> +	struct device_node *syscfg_np;
> +	struct regmap *regmap;
> +	void __iomem *base;
> +	struct resource res;
[ ... ]
> +	/*
> +	 * The regmap is handed over to the syscon layer, which never releases
> +	 * it, so it must outlive this driver: no device managed allocation
> +	 * here, and no device to attach it to either.
> +	 */
> +	regmap = regmap_init_mmio(NULL, base, &config);
> +	if (IS_ERR(regmap)) {
> +		iounmap(base);
> +		goto out_put;
> +	}

[Severity: High]
Does bypassing the standard syscon regmap initialization leave the syscfg
clock disabled?

The generic syscon handling normally calls of_clk_get() and 
regmap_mmio_attach_clk(). Since this manual initialization skips that, the
clock is never attached to the regmap.

On platforms like STM32F4/F7/H7 where the syscfg node explicitly requires a 
clock, could writing to the EXTI mux registers in stm32_gpio_domain_activate()
with the clock disabled trigger an imprecise external abort (bus fault)?

> +
> +	/*
> +	 * A regmap is already registered for that node, most likely by the
> +	 * other pinctrl instance sharing it. Drop ours and use that one.
> +	 */
> +	if (of_syscon_register_regmap(syscfg_np, regmap)) {
> +		regmap_exit(regmap);
> +		iounmap(base);
> +	}

[Severity: High]
If another driver sharing the st,syscfg node (like Ethernet MAC, I2C, or USB)
probes before pinctrl, won't the generic syscon layer have already
instantiated a default regmap without use_raw_spinlock?

In that scenario, of_syscon_register_regmap() would return an error and the
driver silently discards its raw regmap. Since the driver then falls back to
the already registered sleeping regmap, does this cause the PREEMPT_RT 
sleeping-in-atomic bug to persist when stm32_gpio_domain_activate() writes to
the regmap under the raw desc->lock?

> +
> +out_put:
> +	of_node_put(syscfg_np);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803061718.43210-1-junan76@163.com?part=1

Re: [PATCH] pinctrl: stm32: use a raw spinlock regmap to program the EXTI mux

[Ju Nan]

On 8/3/26 2:34 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Bypassing standard syscon regmap initialization leaves the syscfg clock disabled, leading to hardware bus faults on platforms where the syscfg node requires a clock.
> - [High] If another driver sharing the syscon node probes before pinctrl, the generic syscon layer will create a default regmap without `use_raw_spinlock`, causing the PREEMPT_RT sleeping-in-atomic bug to persist.

> [Severity: High]
> Does bypassing the standard syscon regmap initialization leave the syscfg
> clock disabled?
> 
> The generic syscon handling normally calls of_clk_get() and 
> regmap_mmio_attach_clk(). Since this manual initialization skips that, the
> clock is never attached to the regmap.
> 
> On platforms like STM32F4/F7/H7 where the syscfg node explicitly requires a 
> clock, could writing to the EXTI mux registers in stm32_gpio_domain_activate()
> with the clock disabled trigger an imprecise external abort (bus fault)?

That's right for STM32F4/F7, I have checked related dts files and the syscfg
node does require a clock.

>> +
>> +	/*
>> +	 * A regmap is already registered for that node, most likely by the
>> +	 * other pinctrl instance sharing it. Drop ours and use that one.
>> +	 */
>> +	if (of_syscon_register_regmap(syscfg_np, regmap)) {
>> +		regmap_exit(regmap);
>> +		iounmap(base);
>> +	}
> 
> [Severity: High]
> If another driver sharing the st,syscfg node (like Ethernet MAC, I2C, or USB)
> probes before pinctrl, won't the generic syscon layer have already
> instantiated a default regmap without use_raw_spinlock?
> 
> In that scenario, of_syscon_register_regmap() would return an error and the
> driver silently discards its raw regmap. Since the driver then falls back to
> the already registered sleeping regmap, does this cause the PREEMPT_RT 
> sleeping-in-atomic bug to persist when stm32_gpio_domain_activate() writes to
> the regmap under the raw desc->lock?
> 


True again for STM32F4/H7. But the syscfg node is shared between pinctrl and
pinctrl_z only on STM32MP series SoC, and that's the platform on which I tested
this patch.

Thanks sashiko-bot :P, I will send a V2 later.

>> +
>> +out_put:
>> +	of_node_put(syscfg_np);
>> +}
> 

[PATCH v2] pinctrl: stm32: program the EXTI mux from .alloc instead of .activate

[Ju Nan]

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().

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 v2:

- remove stm32_gpio_domain_activate callback
- program mux register in stm32_gpio_domain_alloc instead

v1: https://lore.kernel.org/all/20260803061718.43210-1-junan76@163.com/
---
 drivers/pinctrl/stm32/pinctrl-stm32.c | 53 ++++++++++++++-------------
 1 file changed, 28 insertions(+), 25 deletions(-)

diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c
index 6a99708a5a23..4332ac93a6ff 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)
@@ -653,6 +629,27 @@ static int stm32_gpio_domain_alloc(struct irq_domain *d,
 	if (ret)
 		return ret;
 
+	/*
+	 * Now that the line is reserved, point its mux at this bank. Doing it
+	 * here rather than from .activate() keeps the access out of the raw
+	 * atomic section the irq core runs .activate() in; nothing needs it to
+	 * be reprogrammed at startup time, and the resume path rewrites it on
+	 * its own.
+	 */
+	if (pctl->hwlock) {
+		ret = hwspin_lock_timeout_in_atomic(pctl->hwlock,
+						    HWSPNLCK_TIMEOUT);
+		if (ret) {
+			dev_err(pctl->dev, "Can't get hwspinlock\n");
+			goto err_free_mux;
+		}
+	}
+
+	regmap_field_write(pctl->irqmux[hwirq], bank->bank_ioport_nr);
+
+	if (pctl->hwlock)
+		hwspin_unlock_in_atomic(pctl->hwlock);
+
 	parent_fwspec.fwnode = d->parent->fwnode;
 	parent_fwspec.param_count = 2;
 	parent_fwspec.param[0] = fwspec->param[0];
@@ -662,6 +659,13 @@ static int stm32_gpio_domain_alloc(struct irq_domain *d,
 				      bank);
 
 	return irq_domain_alloc_irqs_parent(d, virq, nr_irqs, &parent_fwspec);
+
+err_free_mux:
+	spin_lock_irqsave(&pctl->irqmux_lock, flags);
+	pctl->irqmux_map &= ~BIT(hwirq);
+	spin_unlock_irqrestore(&pctl->irqmux_lock, flags);
+
+	return ret;
 }
 
 static void stm32_gpio_domain_free(struct irq_domain *d, unsigned int virq,
@@ -683,7 +687,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

Re: [PATCH v2] pinctrl: stm32: program the EXTI mux from .alloc instead of .activate

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] hwspin_lock_timeout_in_atomic() is called in a sleepable context and held across a sleepable operation, leading to hardware lock corruption and co-processor lockups.
--

--- Patch [1]: [PATCH v2] pinctrl: stm32: program the EXTI mux from .alloc instead of .activate ---
commit e4c0724053fe56b6dc864fd8cb1577c252a8ed5b
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 mux and its associated hardware
spinlock from the .activate callback (an atomic context) to the .alloc
callback (a sleepable context) to resolve invalid wait context bugs on
PREEMPT_RT.

Closes: https://lore.kernel.org/all/20220202174430.pf37tt6lua2op3gc@pengutronix.de/

> diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c
> --- a/drivers/pinctrl/stm32/pinctrl-stm32.c
> +++ b/drivers/pinctrl/stm32/pinctrl-stm32.c

[ ... ]

> @@ -653,6 +629,27 @@ static int stm32_gpio_domain_alloc(struct irq_domain *d,
>  	if (ret)
>  		return ret;
>  
> +	/*
> +	 * Now that the line is reserved, point its mux at this bank. Doing it
> +	 * here rather than from .activate() keeps the access out of the raw
> +	 * atomic section the irq core runs .activate() in; nothing needs it to
> +	 * be reprogrammed at startup time, and the resume path rewrites it on
> +	 * its own.
> +	 */
> +	if (pctl->hwlock) {
> +		ret = hwspin_lock_timeout_in_atomic(pctl->hwlock,
> +						    HWSPNLCK_TIMEOUT);

[Severity: High]
Since stm32_gpio_domain_alloc() runs in a sleepable context, could using the
_in_atomic variant of the hwspinlock lead to lock corruption?

The _in_atomic variant skips local locking, meaning it does not disable
preemption or local IRQs. Without preemption disabled, can two Linux threads
concurrently attempt to acquire the lock and corrupt its state?

> +		if (ret) {
> +			dev_err(pctl->dev, "Can't get hwspinlock\n");
> +			goto err_free_mux;
> +		}
> +	}
> +
> +	regmap_field_write(pctl->irqmux[hwirq], bank->bank_ioport_nr);

[Severity: High]
Can this sequence cause a deadlock with co-processors on PREEMPT_RT systems?

Hardware spinlocks are cross-core resources. Because this is now in a 
sleepable context, regmap_field_write() can sleep on PREEMPT_RT while
contending for its lock. If the thread is preempted or sleeps while holding 
the hardware spinlock, will co-processors attempting to acquire the lock 
spin forever and hang?

> +
> +	if (pctl->hwlock)
> +		hwspin_unlock_in_atomic(pctl->hwlock);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260804032227.35017-3-junan76@163.com?part=1

[PATCH v3] pinctrl: stm32: program the EXTI mux from .alloc instead of .activate

[Ju Nan]

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

Re: [PATCH v3] pinctrl: stm32: program the EXTI mux from .alloc instead of .activate

[sashiko-bot]

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