On Thu, 03 Jul 2025, Ville Syrjala <[email protected]> wrote: > 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]>
Reviewed-by: Jani Nikula <[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) { \ -- Jani Nikula, Intel
