[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