Re: [PATCH 3/4] iopoll: Reorder the timeout handling in poll_timeout_us()

[Jani Nikula]

On Thu, 03 Jul 2025, Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> Currently poll_timeout_us() evaluates 'op' and 'cond' twice
> within the loop, once at the start, and a second time after
> the timeout check. While it's probably not a big deal to do
> it twice almost back to back, it does make the macro a bit messy.
>
> Simplify the implementation to evaluate the timeout at the
> very start, then follow up with 'op'/'cond', and finally
> check if the timeout did in fact happen or not.
>
> For good measure throw in a compiler barrier between the timeout
> and 'op'/'cond' evaluations to make sure the compiler can't reoder
> the operations (which could cause false positive timeouts).
> The similar i915 __wait_for() macro already has the barrier, though
> there it is between the 'op' and 'cond' evaluations, which seems
> like it could still allow 'op' and the timeout evaluations to get
> reordered incorrectly. I suppose the ktime_get() might itself act
> as a sufficient barrier here, but better safe than sorry I guess.
>
> Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> Cc: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> Cc: Dibin Moolakadan Subrahmanian <dibin.moolakadan.subrahmanian@xxxxxxxxx>
> Cc: Imre Deak <imre.deak@xxxxxxxxx>
> Cc: David Laight <david.laight.linux@xxxxxxxxx>
> Cc: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> Cc: Matt Wagantall <mattw@xxxxxxxxxxxxxx>
> Cc: Dejin Zheng <zhengdejin5@xxxxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: intel-xe@xxxxxxxxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>

Reviewed-by: Jani Nikula <jani.nikula@xxxxxxxxx>

> ---
>  include/linux/iopoll.h | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/include/linux/iopoll.h b/include/linux/iopoll.h
> index 69296e6adbf3..0e0940a60fdb 100644
> --- a/include/linux/iopoll.h
> +++ b/include/linux/iopoll.h
> @@ -41,18 +41,17 @@
>  	if ((sleep_before_op) && __sleep_us) \
>  		usleep_range((__sleep_us >> 2) + 1, __sleep_us); \
>  	for (;;) { \
> +		bool __expired = __timeout_us && \
> +			ktime_compare(ktime_get(), __timeout) > 0; \
> +		/* guarantee 'op' and 'cond' are evaluated after timeout expired */ \
> +		barrier(); \
>  		op; \
>  		if (cond) { \
>  			___ret = 0; \
>  			break; \
>  		} \
> -		if (__timeout_us && \
> -		    ktime_compare(ktime_get(), __timeout) > 0) { \
> -			op; \
> -			if (cond) \
> -				___ret = 0; \
> -			else \
> -				___ret = -ETIMEDOUT; \
> +		if (__expired) { \
> +			___ret = -ETIMEDOUT; \
>  			break; \
>  		} \
>  		if (__sleep_us) \
> @@ -97,17 +96,16 @@
>  			__left_ns -= __delay_ns; \
>  	} \
>  	for (;;) { \
> +		bool __expired = __timeout_us && __left_ns < 0; \
> +		/* guarantee 'op' and 'cond' are evaluated after timeout expired */ \
> +		barrier(); \
>  		op; \
>  		if (cond) { \
>  			___ret = 0; \
>  			break; \
>  		} \
> -		if (__timeout_us && __left_ns < 0) { \
> -			op; \
> -			if (cond) \
> -				___ret = 0; \
> -			else \
> -				___ret = -ETIMEDOUT; \
> +		if (__expired) { \
> +			___ret = -ETIMEDOUT; \
>  			break; \
>  		} \
>  		if (__delay_us) { \

-- 
Jani Nikula, Intel