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

[sashiko-bot@kernel.org]

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