Message-ID: <6509cf62cc5c28e1626a6ee82c9f9caf62a7ef4b@intel.com>
Date: Thu, 31 Jul 2025 11:51:19 +0300
From: Jani Nikula <jani.nikula@...el.com>
To: Ville Syrjälä <ville.syrjala@...ux.intel.com>,
 linux-kernel@...r.kernel.org
Cc: Lucas De Marchi <lucas.demarchi@...el.com>, Dibin Moolakadan
 Subrahmanian <dibin.moolakadan.subrahmanian@...el.com>, Imre Deak
 <imre.deak@...el.com>, David Laight <david.laight.linux@...il.com>, Geert
 Uytterhoeven <geert+renesas@...der.be>, Matt Wagantall
 <mattw@...eaurora.org>, Dejin Zheng <zhengdejin5@...il.com>,
 intel-gfx@...ts.freedesktop.org, intel-xe@...ts.freedesktop.org, Andrew
 Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH v2 1/4] iopoll: Generalize read_poll_timeout() into
 poll_timeout_us()

On Tue, 15 Jul 2025, Ville Syrjälä <ville.syrjala@...ux.intel.com> wrote:
> On Tue, Jul 08, 2025 at 04:16:34PM +0300, Ville Syrjala wrote:
>> From: Ville Syrjälä <ville.syrjala@...ux.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@...el.com>
>> Cc: Dibin Moolakadan Subrahmanian <dibin.moolakadan.subrahmanian@...el.com>
>> Cc: Imre Deak <imre.deak@...el.com>
>> Cc: David Laight <david.laight.linux@...il.com>
>> Cc: Geert Uytterhoeven <geert+renesas@...der.be>
>> Cc: Matt Wagantall <mattw@...eaurora.org>
>> Cc: Dejin Zheng <zhengdejin5@...il.com>
>> Cc: intel-gfx@...ts.freedesktop.org
>> Cc: intel-xe@...ts.freedesktop.org
>> Cc: linux-kernel@...r.kernel.org
>> Reviewed-by: Jani Nikula <jani.nikula@...el.com>
>> Signed-off-by: Ville Syrjälä <ville.syrjala@...ux.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?

Cc: Andrew

The iopoll.h file is not in MAINTAINERS, and previous changes to it
appear to have gone through various trees. I'd like to base follow-up
work in i915 on this, but who could ack merging the patches via
drm-intel-next? Though doesn't look like anyone's acked the earlier
changes either...


BR,
Jani.


>
>> 
>> 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

-- 
Jani Nikula, Intel

Powered by blists - more mailing lists