Re: [PATCH] irqchip/stm32mp-exti: fix the unit of the hwspinlock timeout

[Thomas Gleixner <tglx@kernel.org>]

On Sat, Aug 15 2026 at 10:19, Radu Rendec wrote:
> On Sat, 2026-08-15 at 10:08 -0400, Radu Rendec wrote:
>> On Wed, 2026-08-05 at 11:21 +0800, Ju Nan wrote:
>> > HWSPNLCK_TIMEOUT is passed to hwspin_lock_timeout_in_atomic(), whose
>> > timeout argument is in milliseconds, not microseconds:
>> > 
>> >   atomic_delay += HWSPINLOCK_RETRY_DELAY_US;
>> >   if (atomic_delay > to * 1000)
>> >           return -ETIMEDOUT;
>> > 
>> > So stm32mp_exti_set_type() asks for a 1 second timeout where the comment
>> > next to the macro says it wants 1 millisecond. The semaphore is polled
>> > with udelay() from a section that holds chip_data->rlock, a
>> > raw_spinlock_t, so preemption stays disabled for the whole wait on every
>> > configuration, PREEMPT_RT included.
>> > 
>> > The hwspinlock core documents this explicitly:
>> > 
>> >   If the mode is HWLOCK_IN_ATOMIC (called from an atomic context) the
>> >   timeout is handled with busy-waiting delays, hence shall not exceed
>> >   few msecs.
>> > 
>> > Pass the value the comment always described. The core retries every
>> > HWSPINLOCK_RETRY_DELAY_US (100 us), so the semaphore is still polled ten
>> > times before giving up, which is far longer than any plausible hold time
>> > on the coprocessor side. A timeout is reported with pr_err() and fails
>> > the trigger type configuration, so shortening it degrades gracefully.
>> > 
>> > Signed-off-by: Ju Nan <junan76@163.com>
>> > ---
>> >  drivers/irqchip/irq-stm32mp-exti.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> > 
>> > diff --git a/drivers/irqchip/irq-stm32mp-exti.c b/drivers/irqchip/irq-stm32mp-exti.c
>> > index a24f4f1a4..f5f0109bf 100644
>> > --- a/drivers/irqchip/irq-stm32mp-exti.c
>> > +++ b/drivers/irqchip/irq-stm32mp-exti.c
>> > @@ -23,7 +23,7 @@
>> >  
>> >  #define IRQS_PER_BANK			32
>> >  
>> > -#define HWSPNLCK_TIMEOUT		1000 /* usec */
>> > +#define HWSPNLCK_TIMEOUT		1 /* msec */
>> >  
>> >  #define EXTI_EnCIDCFGR(n)		(0x180 + (n) * 4)
>> >  #define EXTI_HWCFGR1			0x3f0
>> 
>> Reviewed-by: Radu Rendec <radu@rendec.net>
>
> Oops! Hit the "send" button too soon. The patch is OK, so the r-b tag
> stays. But it also needs this:
>
> Fixes: 5257169ade8c ("irqchip/stm32-exti: Use the hwspin_lock_timeout_in_atomic() API")

That's correct, but the real problem with that culprit commit is that it
just used the existing HWSPNLCK_TIMEOUT define without looking what the
units are, which in turn is a stupidity of the original code which
picked the most generic naming convention for that define. That should
have been:

#define HWSPNLCK_TIMEOUT_US		1000

which would have made it entirely clear what the unit is without the
stupid tail comment.

Now this "fix" just proliferates the same stupidity instead of changing
the define to:

#define HWSPNLCK_TIMEOUT_MS		1

No?

Ju, please send a V3 to that effect.

Thanks,

        tglx