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

[David Laight <david.laight.linux@gmail.com>]

On Fri, 27 Jun 2025 19:26:22 +0300
Jani Nikula <jani.nikula@intel.com> wrote:

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

I might name it poll_timeout_us(...) so that the units are obvious
at the call site.
There are so many different units for timeouts its is worth always
appending _sec, _ms, _us (etc) just to avoid all the silly bugs.

	David