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

Ju Nan posted 1 patch 1 month, 4 weeks ago
There is a newer version of this series
drivers/pinctrl/stm32/pinctrl-stm32.c | 53 ++++++++++++++-------------
1 file changed, 28 insertions(+), 25 deletions(-)
[PATCH v2] pinctrl: stm32: program the EXTI mux from .alloc instead of .activate
Posted by Ju Nan 1 month, 4 weeks ago
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
Posted by sashiko-bot@kernel.org 1 month, 4 weeks ago
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