Re: [PATCH v13 04/15] arm64: support WFET in smp_cond_load_relaxed_timeout()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Double timeout duration due to late deadline initialization in smp_cond_load_relaxed_timeout().
- [Medium] Linker errors for loadable kernel modules using generic barrier timeout APIs.
- [Medium] Integer overflow in NSECS_TO_CYCLES() degrades WFET to a busy-wait loop for large timeouts.
--

commit e5b7a3dd71128bb8e09cbcac08762647dcc92dd1
Author: Ankur Arora <ankur.a.arora@oracle.com>

arm64: support WFET in smp_cond_load_relaxed_timeout()
    
To handle WFET use __cmpwait_timeout() similarly to __cmpwait(). These
call out to the respective __cmpwait_case_timeout_##sz(),
__cmpwait_case_##sz() functions.

> diff --git a/arch/arm64/include/asm/barrier.h b/arch/arm64/include/asm/barrier.h
> index 6190e178db51f..fbd71cd4ef4e5 100644
> --- a/arch/arm64/include/asm/barrier.h
> +++ b/arch/arm64/include/asm/barrier.h

[ ... ]

> @@ -234,7 +234,9 @@ extern bool arch_timer_evtstrm_available(void);
>  #define SMP_TIMEOUT_POLL_COUNT	1
>  
>  #define cpu_poll_relax(ptr, val, timeout_ns) do {			\
> -	if (arch_timer_evtstrm_available())				\
> +	if (alternative_has_cap_unlikely(ARM64_HAS_WFXT))		\
> +		__cmpwait_relaxed_timeout(ptr, val, timeout_ns);	\

[Severity: High]
Does this sequence double the intended timeout duration?

Looking at how the generic smp_cond_load_relaxed_timeout() is implemented
in include/asm-generic/barrier.h, it calculates the absolute deadline
(__time_end = __time_now + __timeout) only after the first iteration of the
loop. 

Because SMP_TIMEOUT_POLL_COUNT is 1, the very first iteration immediately
invokes cpu_poll_relax(), which will block natively via WFET for the full
timeout_ns. When it wakes up, __time_end is calculated as now + timeout_ns,
effectively adding the timeout again and allowing the wait to last up to
twice the requested duration.

> +	else if (arch_timer_evtstrm_available())			\
>  		__cmpwait_relaxed(ptr, val);				\
>  	else								\
>  		cpu_relax();						\
> diff --git a/arch/arm64/include/asm/cmpxchg.h b/arch/arm64/include/asm/cmpxchg.h
> index 6cf3cd6873f5f..9e4cdc9e41d1b 100644
> --- a/arch/arm64/include/asm/cmpxchg.h
> +++ b/arch/arm64/include/asm/cmpxchg.h

[ ... ]

> @@ -235,20 +237,52 @@ __CMPWAIT_CASE( ,  , 64);
>  
>  #undef __CMPWAIT_CASE
>  
> +#define __CMPWAIT_TIMEOUT_CASE(w, sfx, sz)				\
> +static inline void __cmpwait_case_timeout_##sz(volatile void *ptr,	\
> +					       unsigned long val,	\
> +					       u64 timeout_ns)		\
> +{									\
> +	unsigned long tmp;						\
> +	u64 ecycles = __delay_cycles() +				\

[Severity: Medium]
Will loadable kernel modules using the generic barrier timeout APIs fail to
link here?

The inline function __cmpwait_case_timeout_##sz() unconditionally calls
__delay_cycles(). If __delay_cycles() is not exported to normal modules
(e.g., if it is only exported via EXPORT_SYMBOL_IF_KUNIT), any module using
smp_cond_load_relaxed_timeout() will hit undefined symbol linker errors.

> +			NSECS_TO_CYCLES(timeout_ns);			\

[Severity: Medium]
Could a large timeout_ns value overflow here, causing WFET to degrade to a
busy-wait loop?

If NSECS_TO_CYCLES() is called with a large timeout_ns (for example, greater
than 153 seconds on a standard 24MHz generic timer), the 64-bit unsigned
calculation could wrap around. This would pass a very small cycle increment
to WFET, causing it to wake immediately and consume excess power by spinning.

> +	asm volatile(							\
> +	"	sevl\n"							\
> +	"	wfe\n"							\
> +	"	ldxr" #sfx "\t%" #w "[tmp], %[v]\n"			\
> +	"	eor	%" #w "[tmp], %" #w "[tmp], %" #w "[val]\n"	\
> +	"	cbnz	%" #w "[tmp], 2f\n"				\
> +	"	msr s0_3_c1_c0_0, %[ecycles]\n"				\
> +	"2:"								\
> +	: [tmp] "=&r" (tmp), [v] "+Q" (*(u##sz *)ptr)			\
> +	: [val] "r" (val), [ecycles] "r" (ecycles));			\
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702013334.140905-1-ankur.a.arora@oracle.com?part=4