From: Ville Syrjälä <[email protected]> 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 <[email protected]> Cc: Lucas De Marchi <[email protected]> Cc: Dibin Moolakadan Subrahmanian <[email protected]> Cc: Imre Deak <[email protected]> Cc: David Laight <[email protected]> Cc: Geert Uytterhoeven <[email protected]> Cc: Matt Wagantall <[email protected]> Cc: Dejin Zheng <[email protected]> Cc: [email protected] Cc: [email protected] Cc: [email protected] Signed-off-by: Ville Syrjälä <[email protected]> --- 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) { \ -- 2.49.0
