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

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 <[email protected]>

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/[email protected]/
v2: https://lore.kernel.org/all/[email protected]/
v1: https://lore.kernel.org/all/[email protected]/

[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/[email protected]?part=1