Re: [PATCH v13 01/15] asm-generic: barrier: Add smp_cond_load_relaxed_timeout()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Logic Error / Timeout Multiplication on ARM64
- [Medium] Macro Hygiene / Variable Shadowing with Dual Evaluation Scope
--

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

asm-generic: barrier: Add smp_cond_load_relaxed_timeout()

This commit adds the smp_cond_load_relaxed_timeout() macro to allow
waiting for a condition variable to change while enforcing a timeout.
The implementation uses cpu_poll_relax() to slow down the busy-wait
and periodically evaluates the timeout condition.

> diff --git a/include/asm-generic/barrier.h b/include/asm-generic/barrier.h
> index d4f581c1e21da..ecdaddbbd7b64 100644
> --- a/include/asm-generic/barrier.h
> +++ b/include/asm-generic/barrier.h

[ ... ]

> +#ifndef smp_cond_load_relaxed_timeout
> +#define smp_cond_load_relaxed_timeout(ptr, cond_expr,			\
> +				      time_expr_ns, timeout_ns)		\
> +({									\
> +	typeof(ptr) __PTR = (ptr);					\
> +	__unqual_scalar_typeof(*(ptr)) VAL;				\
> +	u32 __count = 0, __spin = SMP_TIMEOUT_POLL_COUNT;		\
> +	s64 __timeout = (s64)timeout_ns;				\
> +	s64 __time_now, __time_end = 0;					\

[Severity: Medium]
Could these generic local variable names cause shadowing issues?

If a caller references a variable named __timeout or __count in their
cond_expr, the macro will evaluate its own loop-mutated local variable
instead of the caller's variable.

Additionally, if a wrapper like smp_cond_load_acquire_timeout evaluates
cond_expr both inside and outside the loop, the same condition expression
could evaluate differently in the two scopes. Using more unique prefixes
(e.g., ___smp_cond_timeout) might prevent this.

> +									\
> +	for (;;) {							\
> +		VAL = READ_ONCE(*__PTR);				\
> +		if (cond_expr)						\
> +			break;						\
> +		cpu_poll_relax(__PTR, VAL, (u64)__timeout);		\
> +		if (++__count < __spin)					\
> +			continue;					\
> +		__time_now = (s64)(time_expr_ns);			\
> +		if (unlikely(__time_end == 0))				\
> +			__time_end = __time_now + __timeout;		\

[Severity: High]
Does this sequence result in doubling the expected timeout duration on some
architectures?

On platforms like ARM64 supporting WFET, cpu_poll_relax() can be mapped
to instruct the CPU to sleep for the provided __timeout duration. Since the
absolute deadline __time_end is not initialized until after this first
cpu_poll_relax() call, the initial sleep is excluded from the timeout window.

When the macro then computes the new deadline:
    __time_end = __time_now + __timeout;

and loops again, it could wait for another full timeout_ns, effectively
doubling the wait time.

> +		__timeout = __time_end - __time_now;			\
> +		if (__time_now <= 0 || __timeout <= 0) {		\
> +			VAL = READ_ONCE(*__PTR);			\
> +			break;						\
> +		}							\
> +		__count = 0;						\
> +	}								\
> +	(typeof(*(ptr)))VAL;						\
> +})
> +#endif

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