| Message ID | 1375187900-17582-3-git-send-email-B44344@freescale.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
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?
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
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
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()?
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.
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.
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
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
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) */
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(+)