On Fri, 27 Jun 2025 19:26:22 +0300
Jani Nikula <jani.nik...@intel.com> wrote:
> On Fri, 27 Jun 2025, Ville Syrjälä <ville.syrj...@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
