On Thu, 03 Jul 2025, Ville Syrjala <[email protected]> wrote: > From: Ville Syrjälä <[email protected]> > > While read_poll_timeout() & co. were originally introduced just > for simple I/O usage scenarios they have since been generalized to > be useful in more cases. > > However the interface is very cumbersome to use in the general case. > Attempt to make it more flexible by combining the 'op', 'var' and > 'args' parameter into just a single 'op' that the caller can fully > specify. > > For example i915 has one case where one might currently > have to write something like: > ret = read_poll_timeout(drm_dp_dpcd_read_byte, err, > err || (status & mask), > 0 * 1000, 200 * 1000, false, > aux, DP_FEC_STATUS, &status); > which is practically illegible, but with the adjusted macro > we do: > ret = poll_timeout_us(err = drm_dp_dpcd_read_byte(aux, DP_FEC_STATUS, > &status), > err || (status & mask), > 0 * 1000, 200 * 1000, false); > which much easier to understand. > > One could even combine the 'op' and 'cond' parameters into > one, but that might make the caller a bit too unwieldly with > assignments and checks being done on the same statement.
I think it's better like this, with separate op and cond. It might be worth mentioning this patch should have no functional changes to the existing read_poll_timeout*() users. > This makes poll_timeout_us() closer to the i915 __wait_for() > macro, with the main difference being that __wait_for() uses > expenential backoff as opposed to the fixed polling interval > used by poll_timeout_us(). Eventually we might be able to switch > (at least most of) i915 to use poll_timeout_us(). > > 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 | 110 +++++++++++++++++++++++++++++------------ > 1 file changed, 78 insertions(+), 32 deletions(-) > > diff --git a/include/linux/iopoll.h b/include/linux/iopoll.h > index 91324c331a4b..0d8186d3df03 100644 > --- a/include/linux/iopoll.h > +++ b/include/linux/iopoll.h > @@ -14,41 +14,38 @@ > #include <linux/io.h> > > /** > - * read_poll_timeout - Periodically poll an address until a condition is > - * met or a timeout occurs > - * @op: accessor function (takes @args as its arguments) > - * @val: Variable to read the value into > - * @cond: Break condition (usually involving @val) > - * @sleep_us: Maximum time to sleep between reads in us (0 tight-loops). > Please > - * read usleep_range() function description for details and > + * poll_timeout_us - Periodically poll and perform an operaion until *operation > + * a condition is met or a timeout occurs > + * > + * @op: Operation > + * @cond: Break condition > + * @sleep_us: Maximum time to sleep between operations in us (0 tight-loops). > + * Please read usleep_range() function description for details and > * limitations. Side note, I've sent a patch [1] to switch from usleep_range() to fsleep(), which is perhaps better for the longer waits. But the patch at hand is much more important to us than that. [1] https://lore.kernel.org/r/[email protected] > * @timeout_us: Timeout in us, 0 means never timeout > - * @sleep_before_read: if it is true, sleep @sleep_us before read. > - * @args: arguments for @op poll > + * @sleep_before_op: if it is true, sleep @sleep_us before operation. > * > * When available, you'll probably want to use one of the specialized > * macros defined below rather than this macro directly. > * > - * Returns: 0 on success and -ETIMEDOUT upon a timeout. In either > - * case, the last read value at @args is stored in @val. Must not > + * Returns: 0 on success and -ETIMEDOUT upon a timeout. Must not > * be called from atomic context if sleep_us or timeout_us are used. > */ > -#define read_poll_timeout(op, val, cond, sleep_us, timeout_us, \ > - sleep_before_read, args...) \ > +#define poll_timeout_us(op, cond, sleep_us, timeout_us, sleep_before_op) \ > ({ \ > u64 __timeout_us = (timeout_us); \ > unsigned long __sleep_us = (sleep_us); \ > ktime_t __timeout = ktime_add_us(ktime_get(), __timeout_us); \ > might_sleep_if((__sleep_us) != 0); \ > - if (sleep_before_read && __sleep_us) \ > + if ((sleep_before_op) && __sleep_us) \ > usleep_range((__sleep_us >> 2) + 1, __sleep_us); \ > for (;;) { \ > - (val) = op(args); \ > + op; \ > if (cond) \ > break; \ > if (__timeout_us && \ > ktime_compare(ktime_get(), __timeout) > 0) { \ > - (val) = op(args); \ > + op; \ > break; \ > } \ > if (__sleep_us) \ > @@ -59,17 +56,16 @@ > }) > > /** > - * read_poll_timeout_atomic - Periodically poll an address until a condition > is > - * met or a timeout occurs > - * @op: accessor function (takes @args as its arguments) > - * @val: Variable to read the value into > - * @cond: Break condition (usually involving @val) > - * @delay_us: Time to udelay between reads in us (0 tight-loops). Please > - * read udelay() function description for details and > + * poll_timeout_us_atomic - Periodically poll and perform an operaion until > + * a condition is met or a timeout occurs > + * > + * @op: Operation > + * @cond: Break condition > + * @sleep_us: Maximum time to sleep between operations in us (0 tight-loops). > + * Please read usleep_range() function description for details and > * limitations. > * @timeout_us: Timeout in us, 0 means never timeout > - * @delay_before_read: if it is true, delay @delay_us before read. > - * @args: arguments for @op poll > + * @delay_before_op: if it is true, delay @delay_us before operation. > * > * This macro does not rely on timekeeping. Hence it is safe to call even > when > * timekeeping is suspended, at the expense of an underestimation of wall > clock > @@ -78,27 +74,26 @@ > * When available, you'll probably want to use one of the specialized > * macros defined below rather than this macro directly. > * > - * Returns: 0 on success and -ETIMEDOUT upon a timeout. In either > - * case, the last read value at @args is stored in @val. > + * Returns: 0 on success and -ETIMEDOUT upon a timeout. > */ > -#define read_poll_timeout_atomic(op, val, cond, delay_us, timeout_us, \ > - delay_before_read, args...) \ > +#define poll_timeout_us_atomic(op, cond, delay_us, timeout_us, \ > + delay_before_op) \ There's a sleep_us/delay_us mismatch in kernel-doc and macro args. Arguably it should remain delay for the _atomic() variants. > ({ \ > u64 __timeout_us = (timeout_us); \ > s64 __left_ns = __timeout_us * NSEC_PER_USEC; \ > unsigned long __delay_us = (delay_us); \ > u64 __delay_ns = __delay_us * NSEC_PER_USEC; \ > - if (delay_before_read && __delay_us) { \ > + if ((delay_before_op) && __delay_us) { \ > udelay(__delay_us); \ > if (__timeout_us) \ > __left_ns -= __delay_ns; \ > } \ > for (;;) { \ > - (val) = op(args); \ > + op; \ > if (cond) \ > break; \ > if (__timeout_us && __left_ns < 0) { \ > - (val) = op(args); \ > + op; \ > break; \ > } \ > if (__delay_us) { \ > @@ -113,6 +108,57 @@ > (cond) ? 0 : -ETIMEDOUT; \ > }) > > +/** > + * read_poll_timeout - Periodically poll an address until a condition is > + * met or a timeout occurs > + * @op: accessor function (takes @args as its arguments) > + * @val: Variable to read the value into > + * @cond: Break condition (usually involving @val) > + * @sleep_us: Maximum time to sleep between reads in us (0 tight-loops). > Please > + * read usleep_range() function description for details and > + * limitations. > + * @timeout_us: Timeout in us, 0 means never timeout > + * @sleep_before_read: if it is true, sleep @sleep_us before read. > + * @args: arguments for @op poll > + * > + * When available, you'll probably want to use one of the specialized > + * macros defined below rather than this macro directly. > + * > + * Returns: 0 on success and -ETIMEDOUT upon a timeout. In either > + * case, the last read value at @args is stored in @val. Must not > + * be called from atomic context if sleep_us or timeout_us are used. > + */ > +#define read_poll_timeout(op, val, cond, sleep_us, timeout_us, \ > + sleep_before_read, args...) \ > + poll_timeout_us((val) = op(args), cond, sleep_us, timeout_us, > sleep_before_read) > + > +/** > + * read_poll_timeout_atomic - Periodically poll an address until a condition > is > + * met or a timeout occurs > + * @op: accessor function (takes @args as its arguments) > + * @val: Variable to read the value into > + * @cond: Break condition (usually involving @val) > + * @delay_us: Time to udelay between reads in us (0 tight-loops). Please > + * read udelay() function description for details and > + * limitations. > + * @timeout_us: Timeout in us, 0 means never timeout > + * @delay_before_read: if it is true, delay @delay_us before read. > + * @args: arguments for @op poll > + * > + * This macro does not rely on timekeeping. Hence it is safe to call even > when > + * timekeeping is suspended, at the expense of an underestimation of wall > clock > + * time, which is rather minimal with a non-zero delay_us. > + * > + * When available, you'll probably want to use one of the specialized > + * macros defined below rather than this macro directly. > + * > + * Returns: 0 on success and -ETIMEDOUT upon a timeout. In either > + * case, the last read value at @args is stored in @val. > + */ > +#define read_poll_timeout_atomic(op, val, cond, sleep_us, timeout_us, \ > + sleep_before_read, args...) \ > + poll_timeout_us_atomic((val) = op(args), cond, sleep_us, timeout_us, > sleep_before_read) Ditto about delay_us/sleep_us. With the nitpicks fixed, Reviewed-by: Jani Nikula <[email protected]> > + > /** > * readx_poll_timeout - Periodically poll an address until a condition is > met or a timeout occurs > * @op: accessor function (takes @addr as its only argument) -- Jani Nikula, Intel
