Re: [PATCH v2 1/4] iopoll: Generalize read_poll_timeout() into poll_timeout_us()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Jul 08, 2025 at 04:16:34PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> 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.
> 
> 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().
> 
> v2: Fix typos (Jani)
>     Fix delay_us docs for poll_timeout_us_atomic() (Jani)
> 
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: Dibin Moolakadan Subrahmanian <dibin.moolakadan.subrahmanian@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: David Laight <david.laight.linux@gmail.com>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Matt Wagantall <mattw@codeaurora.org>
> Cc: Dejin Zheng <zhengdejin5@gmail.com>
> Cc: intel-gfx@lists.freedesktop.org
> Cc: intel-xe@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  include/linux/iopoll.h | 110 +++++++++++++++++++++++++++++------------
>  1 file changed, 78 insertions(+), 32 deletions(-)

Any thoughs how we should get this stuff in? Jani will need it for
some i915 stuff once he returns from vacation, so I could just push
it into drm-intel-next...

Are people OK with that, or is there a better tree that could pick 
this up?

> 
> diff --git a/include/linux/iopoll.h b/include/linux/iopoll.h
> index 91324c331a4b..440aca5b4b59 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 operation 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
> - * @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 operation until
> + *                          a condition is met or a timeout occurs
> + *
> + * @op: Operation
> + * @cond: Break condition
> + * @delay_us: Time to udelay between operations 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
> + * @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) \
>  ({ \
>  	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)
> +
>  /**
>   * 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)
> -- 
> 2.49.0

-- 
Ville Syrjälä
Intel