Patchwork [2/2] Convert PowerPC macro spin_event_timeout() to architecture independent macro

login
register
mail settings
Submitter Arpit Goel
Date July 30, 2013, 12:38 p.m.
Message ID <1375187900-17582-3-git-send-email-B44344@freescale.com>
Download mbox | patch
Permalink /patch/263371/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Arpit Goel - July 30, 2013, 12:38 p.m.
This patch ports PowerPC implementation of spin_event_timeout() for generic
use. Architecture specific implementation can be added to asm/delay.h, which
will override the generic linux implementation.

Signed-off-by: Arpit Goel <B44344@freescale.com>
---
 include/linux/delay.h | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)
Stephen Boyd - July 31, 2013, 7:16 a.m.
On 07/30, Arpit Goel wrote:
> This patch ports PowerPC implementation of spin_event_timeout() for generic
> use. Architecture specific implementation can be added to asm/delay.h, which
> will override the generic linux implementation.
> 
> Signed-off-by: Arpit Goel <B44344@freescale.com>
> ---

We use something similar internally but it's tied specifically to
readl.

>  
> +#ifndef spin_event_timeout
> +/**
> + * spin_event_timeout - spin until a condition gets true or a timeout elapses
> + * @condition: a C expression to evalate
> + * @timeout: timeout, in microseconds
> + * @delay: the number of microseconds to delay between each evaluation of
> + *         @condition
> + *
> + * The process spins until the condition evaluates to true (non-zero) or the
> + * timeout elapses.  The return value of this macro is the value of
> + * @condition when the loop terminates. This allows you to determine the cause
> + * of the loop terminates.  If the return value is zero, then you know a
> + * timeout has occurred.
> + *
> + * This primary purpose of this macro is to poll on a hardware register
> + * until a status bit changes.  The timeout ensures that the loop still
> + * terminates even if the bit never changes.  The delay is for devices that
> + * need a delay in between successive reads.
> + *
> + * gcc will optimize out the if-statement if @delay is a constant.
> + *
> + * This is copied from PowerPC based spin_event_timeout() implementation
> + * and modified for generic usecase.
> + */
> +#define spin_event_timeout(condition, timeout, delay)		\
> +({								\
> +	typeof(condition) __ret;				\
> +	unsigned long __loops = timeout/USECS_PER_JIFFY;	\
> +	unsigned long __start = jiffies;			\
> +	while (!(__ret = (condition)) &&			\
> +		time_before(jiffies, __start + __loops + 1))	\
> +		if (delay)					\
> +			udelay(delay);				\
> +		else						\
> +			schedule();				\
> +	if (!__ret)						\
> +		__ret = (condition);				\
> +	__ret;							\
> +})

What do you do here if jiffies aren't incrementing (i.e
interrupts are disabled). The time_before() check won't work
there and it would be nice if we were able to use this in such
situations. I think powerpc gets around this by reading the
hardware timer directly?
Timur Tabi - July 31, 2013, 11:44 p.m.
On Wed, Jul 31, 2013 at 2:16 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>
> What do you do here if jiffies aren't incrementing (i.e
> interrupts are disabled). The time_before() check won't work
> there and it would be nice if we were able to use this in such
> situations. I think powerpc gets around this by reading the
> hardware timer directly?

I believe that jiffies is always a global variable.  It should behave
the same on PowerPC as on other architectures.

The answer to your question is that you should not use
spin_event_timeout() in interrupt context, because it yields.

(FYI, I'm the author of spin_event_timeout(), so please CC: me on all
changes to it)
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Timur Tabi - Aug. 1, 2013, 12:02 a.m.
On Tue, Jul 30, 2013 at 7:38 AM, Arpit Goel <B44344@freescale.com> wrote:
>
> +#ifndef spin_event_timeout
> +/**


Why don't you make PowerPC also use this generic version?  It seems
silly to have two virtually identical implementations of this macro?
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Boyd - Aug. 1, 2013, 12:04 a.m.
On 07/31/13 16:44, Timur Tabi wrote:
> On Wed, Jul 31, 2013 at 2:16 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>> What do you do here if jiffies aren't incrementing (i.e
>> interrupts are disabled). The time_before() check won't work
>> there and it would be nice if we were able to use this in such
>> situations. I think powerpc gets around this by reading the
>> hardware timer directly?
> I believe that jiffies is always a global variable.  It should behave
> the same on PowerPC as on other architectures.

Yes it's global but it doesn't increment while interrupts are off.

>
> The answer to your question is that you should not use
> spin_event_timeout() in interrupt context, because it yields.
>

If it yields why are we using udelay? Why not usleep_range()? It would
be useful to have a variant that worked in interrupt context and it
looked like that was almost possible.

BTW, couldn't we skip the first patch and just use usecs_to_jiffies()?
Timur Tabi - Aug. 1, 2013, 12:13 a.m.
On 07/31/2013 07:04 PM, Stephen Boyd wrote:
> If it yields why are we using udelay? Why not usleep_range()? It would
> be useful to have a variant that worked in interrupt context and it
> looked like that was almost possible.

I've never heard of usleep_range() before, so I don't know if it
applies.  Apparently, udelay() includes its own call to cpu_relax().  Is
it possible that cpu_relax() is a "lightweight" yield, compared to sleeping?

FYI, you might want to look at the code reviews for spin_event_timeout()
on the linuxppc-dev mailing list, back in March 2009.
Stephen Boyd - Aug. 1, 2013, 12:16 a.m.
On 07/31/13 17:13, Timur Tabi wrote:
> On 07/31/2013 07:04 PM, Stephen Boyd wrote:
>> If it yields why are we using udelay? Why not usleep_range()? It would
>> be useful to have a variant that worked in interrupt context and it
>> looked like that was almost possible.
> I've never heard of usleep_range() before, so I don't know if it
> applies.  Apparently, udelay() includes its own call to cpu_relax().  Is
> it possible that cpu_relax() is a "lightweight" yield, compared to sleeping?

cpu_relax() is usually just a compiler barrier or an instruction hint to
the cpu that it should cool down because we're spinning in a tight loop.
It certainly shouldn't be calling into the  scheduler.

>
> FYI, you might want to look at the code reviews for spin_event_timeout()
> on the linuxppc-dev mailing list, back in March 2009.
>

Sure. Any pointers? Otherwise I'll go digging around the archives.
Timur Tabi - Aug. 1, 2013, 12:20 a.m.
On 07/31/2013 07:16 PM, Stephen Boyd wrote:
> cpu_relax() is usually just a compiler barrier or an instruction hint to
> the cpu that it should cool down because we're spinning in a tight loop.
> It certainly shouldn't be calling into the  scheduler.

Ah yes, I remember now.  So it does seem that if we can fix the problem
of non-incrementing 'jiffies', then this macro can be used in interrupts.

Of course, that assumes that spinning in interrupt context is a good
idea to begin with.  Maybe we shouldn't be encouraging it?

>> > FYI, you might want to look at the code reviews for spin_event_timeout()
>> > on the linuxppc-dev mailing list, back in March 2009.
>> >
> Sure. Any pointers? Otherwise I'll go digging around the archives.

https://lists.ozlabs.org/pipermail/linuxppc-dev/2009-March/thread.html
Stephen Boyd - Aug. 1, 2013, 1:36 a.m.
On 07/31/13 17:20, Timur Tabi wrote:
> On 07/31/2013 07:16 PM, Stephen Boyd wrote:
>> cpu_relax() is usually just a compiler barrier or an instruction hint to
>> the cpu that it should cool down because we're spinning in a tight loop.
>> It certainly shouldn't be calling into the  scheduler.
> Ah yes, I remember now.  So it does seem that if we can fix the problem
> of non-incrementing 'jiffies', then this macro can be used in interrupts.

That's encouraging. It looks like you introduced it to use in interrupt
context but then it got shot down[1]? I lost track in all the versions.

>
> Of course, that assumes that spinning in interrupt context is a good
> idea to begin with.  Maybe we shouldn't be encouraging it?

I read through the v5 discussion and it seems I'm about to walk through
some tall grass on the way to Cerulean City.

Andrew Morton, I choose you! Use your mind-power move to convince
everyone that having a macro for spinning on a register in interrupt
context is a good thing. At least it will be more obvious.

>
>>>> FYI, you might want to look at the code reviews for spin_event_timeout()
>>>> on the linuxppc-dev mailing list, back in March 2009.
>>>>
>> Sure. Any pointers? Otherwise I'll go digging around the archives.
> https://lists.ozlabs.org/pipermail/linuxppc-dev/2009-March/thread.html
>

[1] https://lists.ozlabs.org/pipermail/linuxppc-dev/2009-May/072521.html

Patch

diff --git a/include/linux/delay.h b/include/linux/delay.h
index a6ecb34..e975994 100644
--- a/include/linux/delay.h
+++ b/include/linux/delay.h
@@ -52,4 +52,44 @@  static inline void ssleep(unsigned int seconds)
 	msleep(seconds * 1000);
 }
 
+#ifndef spin_event_timeout
+/**
+ * spin_event_timeout - spin until a condition gets true or a timeout elapses
+ * @condition: a C expression to evalate
+ * @timeout: timeout, in microseconds
+ * @delay: the number of microseconds to delay between each evaluation of
+ *         @condition
+ *
+ * The process spins until the condition evaluates to true (non-zero) or the
+ * timeout elapses.  The return value of this macro is the value of
+ * @condition when the loop terminates. This allows you to determine the cause
+ * of the loop terminates.  If the return value is zero, then you know a
+ * timeout has occurred.
+ *
+ * This primary purpose of this macro is to poll on a hardware register
+ * until a status bit changes.  The timeout ensures that the loop still
+ * terminates even if the bit never changes.  The delay is for devices that
+ * need a delay in between successive reads.
+ *
+ * gcc will optimize out the if-statement if @delay is a constant.
+ *
+ * This is copied from PowerPC based spin_event_timeout() implementation
+ * and modified for generic usecase.
+ */
+#define spin_event_timeout(condition, timeout, delay)		\
+({								\
+	typeof(condition) __ret;				\
+	unsigned long __loops = timeout/USECS_PER_JIFFY;	\
+	unsigned long __start = jiffies;			\
+	while (!(__ret = (condition)) &&			\
+		time_before(jiffies, __start + __loops + 1))	\
+		if (delay)					\
+			udelay(delay);				\
+		else						\
+			schedule();				\
+	if (!__ret)						\
+		__ret = (condition);				\
+	__ret;							\
+})
+#endif
 #endif /* defined(_LINUX_DELAY_H) */