[PATCH v2] add function spin_event_timeout()

[PATCH v2] add function spin_event_timeout()

[Timur Tabi]

The function spin_event_timeout() takes a condition and timeout value
(in jiffies) as parameters.  It spins until either the condition is true
or the timeout expires.  It returns non-zero if the condition is true,
zero otherwise.

Signed-off-by: Timur Tabi <timur@freescale.com>
---

v2: changes based on feedback

 include/linux/delay.h |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/include/linux/delay.h b/include/linux/delay.h
index fd832c6..235ca25 100644
--- a/include/linux/delay.h
+++ b/include/linux/delay.h
@@ -51,4 +51,26 @@ static inline void ssleep(unsigned int seconds)
 	msleep(seconds * 1000);
 }
 
+/**
+ * spin_event_timeout - spin until a condition gets true or a timeout elapses
+ * @condition: a C expression for the event to wait for
+ * @timeout: timeout, in jiffies
+ *
+ * The process spins until the @condition evaluates to true or the @timeout
+ * elapses.
+ *
+ * The function returns non-zero if the @condition evaluated to true, or
+ * zero if the @timeout elapsed.  If both occurs (e.g. the loop was
+ * pre-empted and the @condition became true in the meantime, but when the
+ * loop resumed the @timeout had already elapsed), then non-zero will be
+ * returned.
+ */
+#define spin_event_timeout(condition, timeout)			\
+({								\
+	unsigned long __timeout = jiffies + (timeout);		\
+	while (!(condition) && time_before(jiffies, __timeout))	\
+		cpu_relax();					\
+	(condition);						\
+})
+
 #endif /* defined(_LINUX_DELAY_H) */
-- 
1.6.1.3

Re: [PATCH v2] add function spin_event_timeout()

[Will Newton]

On Mon, Mar 9, 2009 at 3:18 PM, Timur Tabi <timur@freescale.com> wrote:
> The function spin_event_timeout() takes a condition and timeout value
> (in jiffies) as parameters.  It spins until either the condition is true
> or the timeout expires.  It returns non-zero if the condition is true,
> zero otherwise.
>
> Signed-off-by: Timur Tabi <timur@freescale.com>
> ---
>
> v2: changes based on feedback
>
>  include/linux/delay.h |   22 ++++++++++++++++++++++
>  1 files changed, 22 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/delay.h b/include/linux/delay.h
> index fd832c6..235ca25 100644
> --- a/include/linux/delay.h
> +++ b/include/linux/delay.h
> @@ -51,4 +51,26 @@ static inline void ssleep(unsigned int seconds)
>        msleep(seconds * 1000);
>  }
>
> +/**
> + * spin_event_timeout - spin until a condition gets true or a timeout elapses
> + * @condition: a C expression for the event to wait for
> + * @timeout: timeout, in jiffies
> + *
> + * The process spins until the @condition evaluates to true or the @timeout
> + * elapses.
> + *
> + * The function returns non-zero if the @condition evaluated to true, or
> + * zero if the @timeout elapsed.  If both occurs (e.g. the loop was
> + * pre-empted and the @condition became true in the meantime, but when the
> + * loop resumed the @timeout had already elapsed), then non-zero will be
> + * returned.
> + */
> +#define spin_event_timeout(condition, timeout)                 \
> +({                                                             \
> +       unsigned long __timeout = jiffies + (timeout);          \
> +       while (!(condition) && time_before(jiffies, __timeout)) \
> +               cpu_relax();                                    \
> +       (condition);                                            \
> +})
> +
>  #endif /* defined(_LINUX_DELAY_H) */

Are you sure you want to evaluate condition a second time when
returning? Some memory mapped registers don't have a stable value so
e.g. the first test could succeed but the return value could still be
zero.

> --
> 1.6.1.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [PATCH v2] add function spin_event_timeout()

[Timur Tabi]

Will Newton wrote:

> Are you sure you want to evaluate condition a second time when
> returning? Some memory mapped registers don't have a stable value so
> e.g. the first test could succeed but the return value could still be
> zero.

Ah, good point.  I was hoping to keep the loop simple by combining the
two test conditions, but I guess that's not going to work.

v3 coming up.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH v2] add function spin_event_timeout()

[Peter Zijlstra]

On Mon, 2009-03-09 at 10:18 -0500, Timur Tabi wrote:
> The function spin_event_timeout() takes a condition and timeout value
> (in jiffies) as parameters.  It spins until either the condition is true
> or the timeout expires.  It returns non-zero if the condition is true,
> zero otherwise.

This changelog utterly fails to justify this interface. And to me it
seems a rather bad one. Why would we ever be wanting to spin in order of
jiffies?

> Signed-off-by: Timur Tabi <timur@freescale.com>
> ---
> 
> v2: changes based on feedback
> 
>  include/linux/delay.h |   22 ++++++++++++++++++++++
>  1 files changed, 22 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/delay.h b/include/linux/delay.h
> index fd832c6..235ca25 100644
> --- a/include/linux/delay.h
> +++ b/include/linux/delay.h
> @@ -51,4 +51,26 @@ static inline void ssleep(unsigned int seconds)
>  	msleep(seconds * 1000);
>  }
>  
> +/**
> + * spin_event_timeout - spin until a condition gets true or a timeout elapses
> + * @condition: a C expression for the event to wait for
> + * @timeout: timeout, in jiffies
> + *
> + * The process spins until the @condition evaluates to true or the @timeout
> + * elapses.
> + *
> + * The function returns non-zero if the @condition evaluated to true, or
> + * zero if the @timeout elapsed.  If both occurs (e.g. the loop was
> + * pre-empted and the @condition became true in the meantime, but when the
> + * loop resumed the @timeout had already elapsed), then non-zero will be
> + * returned.
> + */
> +#define spin_event_timeout(condition, timeout)			\
> +({								\
> +	unsigned long __timeout = jiffies + (timeout);		\
> +	while (!(condition) && time_before(jiffies, __timeout))	\
> +		cpu_relax();					\
> +	(condition);						\
> +})
> +
>  #endif /* defined(_LINUX_DELAY_H) */

Re: [PATCH v2] add function spin_event_timeout()

[Timur Tabi]

Peter Zijlstra wrote:

> This changelog utterly fails to justify this interface. And to me it
> seems a rather bad one. 

I'll update the commit to include a justification, but in the meantime,
let me answer your question.

The primary purpose of this macro is to provide a way for drivers to
poll a status register waiting for a bit to change.  In most cases, this
 bit should change very quickly, even within a few cycles.  The timeout
is used as a way to easily deal with situations where the hardware is
misprogrammed such that the bit never changes.

Too often, developers do something like this:

	while (!(in_be32(&ssi->sisr) & CCSR_SSI_SISR_RFF0));

If something goes wrong and RFF0 never gets set, then this loop will
never exit.  So to encourage developers to do it the right way, and to
make it simpler to identify code which does this sort of thing, I'm
introducing spin_event_timeout().

> Why would we ever be wanting to spin in order of
> jiffies?

I picked jiffies because it seems straightforward, but I think I see
your point.  It has two drawbacks:

1) It's too coarse of a time measurement.  Most people will probably use
a value of 1 or 2.

2) It can't be used if interrupts are disabled, because then jiffies
won't be updated.

So I can switch that to using ndelay().

-- 
Timur Tabi
Linux kernel developer at Freescale