Re: [PATCH 18/18] drm/i915/ddi: prefer read_poll_timeout() over readx_poll_timeout()

[Jani Nikula <jani.nikula@intel.com>]

On Fri, 27 Jun 2025, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Fri, Jun 27, 2025 at 04:34:23PM +0300, Jani Nikula wrote:
>> Internally the macro has:
>> 
>> #define read_poll_timeout(op, val, cond, sleep_us, timeout_us, \
>> 				sleep_before_read, args...) \
>> 
>> ...
>> 
>> 		(val) = op(args); \
>> 
>> So you do need to provide an lvalue val, and you need to be able to add
>> () after op. I think GCC allows not passing varargs. IOW you'd need to
>> implement another macro (which could be used to implement the existing
>> one, but not the other way round).
>
> Just get rid of the 'args' and 'val' and it'll work just fine.
> Then it'll be almost identical to wait_for() (basically just missing the
> increasing backoff stuff).
>
> AFAICS this thing was originally added just for reading a single
> register and checking some bit/etc, so the name made some sense.
> But now we're abusing it for all kinds of random things, so even
> the name no longer makes that much sense.

Yeah, evolution not intelligent design.

> So I think just something like this would work fine for us:

LGTM, with the _atomic version for completeness.

Want to send it to lkml?


BR,
Jani.


>
> diff --git a/include/linux/iopoll.h b/include/linux/iopoll.h
> index 91324c331a4b..9c38fd732028 100644
> --- a/include/linux/iopoll.h
> +++ b/include/linux/iopoll.h
> @@ -14,27 +14,24 @@
>  #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 - 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
> - * @sleep_before_read: if it is true, sleep @sleep_us before read.
> - * @args: arguments for @op poll
> + * @sleep_before_read: 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(op, cond, sleep_us, timeout_us, sleep_before_read) \
>  ({ \
>  	u64 __timeout_us = (timeout_us); \
>  	unsigned long __sleep_us = (sleep_us); \
> @@ -43,12 +40,12 @@
>  	if (sleep_before_read && __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) \
> @@ -58,6 +55,30 @@
>  	(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((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
> -- 
> 2.49.0

-- 
Jani Nikula, Intel