linux-kernel - Re: [PATCH 18/18] drm/i915/ddi: prefer read_poll_timeout() over readx_poll

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250630215218.1aa32a1f@pumpkin>
Date: Mon, 30 Jun 2025 21:52:18 +0100
From: David Laight <david.laight.linux@...il.com>
To: Jani Nikula <jani.nikula@...el.com>
Cc: Ville Syrjälä <ville.syrjala@...ux.intel.com>,
 intel-gfx@...ts.freedesktop.org, intel-xe@...ts.freedesktop.org, Imre Deak
 <imre.deak@...el.com>, Geert Uytterhoeven <geert+renesas@...der.be>, Matt
 Wagantall <mattw@...eaurora.org>, Dejin Zheng <zhengdejin5@...il.com>,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 18/18] drm/i915/ddi: prefer read_poll_timeout() over
 readx_poll_timeout()

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

> On Fri, 27 Jun 2025, Ville Syrjälä <ville.syrjala@...ux.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