| Message ID | 1375187900-17582-3-git-send-email-B44344@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| 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)
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?
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
On Tue, Jul 30, 2013 at 7:38 AM, Arpit Goel <B44344@freescale.com> wrote: > > +#ifndef spin_event_timeout > +/** On Thursday, August 01, 2013 5:32 AM Timur Tabi [mailto:timur@tabi.org] wrote: > Why don't you make PowerPC also use this generic version? It seems silly to have two virtually identical implementations of this macro? PowerPC uses hardware timer registers provided by PowerPC architecture to implement spin_event_timeout(). These registers make PowerPC implementation of spin_event_timeout() more granular in comparison to ones using generic USECS_PER_JIFFY framework. This would handle those cases more granularly in which evaluation of "condition" is taking time or fails.
Goel Arpit-B44344 wrote: > PowerPC uses hardware timer registers provided by PowerPC architecture to implement spin_event_timeout(). These registers make > PowerPC implementation of spin_event_timeout() more granular in comparison to ones using generic USECS_PER_JIFFY framework. This > would handle those cases more granularly in which evaluation of "condition" is taking time or fails. LOL, it seems I forgot how my own code works. Thanks for the reminder. For some reason, I thought the PowerPC version was also using 'jiffies'. The concern about using 'jiffies' makes a lot more sense now. Almost every architecture has a timebase counter. Wouldn't it make more sense to port tb_ticks_per_usec, get_tbl(), and tb_ticks_since() to other architectures instead? I don't think 'jiffies' is granular enough for spin_event_timeout().
-----Original Message----- From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-owner@vger.kernel.org] On Behalf Of Stephen Boyd Sent: Thursday, August 01, 2013 5:34 AM >> 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. Looks like the macro yields when 'delay', the third arg to the macro, is zero; otherwise the macro performs one or more udelays. If this is correct, can usleep_range() help?
Thursday, August 01, 2013 5:34 AM Stephen Boyd [mailto:sboyd@codeaurora.org] wrote:
> BTW, couldn't we skip the first patch and just use usecs_to_jiffies()?
[Arpit] It is a good idea to use usecs_to_jiffies() in spin_event_timeout() implementation. But wont moving USECS_PER_JIFFY from
C <arch/time.c> files to include <arch/timex.h> files also required as this would enable using USECS_PER_JIFFY.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
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(+)