| Message ID | 1242761199-17875-2-git-send-email-timur@freescale.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream, archived |
| Headers | show |
On Tue, May 19, 2009 at 3:26 PM, Timur Tabi <timur@freescale.com> wrote: > The macro spin_event_timeout() takes a condition and timeout value > (in microseconds) as parameters. It spins until either the condition is true > or the timeout expires. It returns the result of the condition when the loop > was terminated. > > 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 if the > bit doesn't change as expected. This macro makes it easier for driver > developers to perform this kind of operation properly. I just tried using this. The !rc has the effect of making the error return be zero instead the normal not zero. /* Wait for command send status zero = ready */ spin_event_timeout(!(in_be16(&psc_dma->psc_regs->sr_csr.status) & MPC52xx_PSC_SR_CMDSEND), 100, 0, rc); if (rc == 0) { pr_err("timeout on ac97 bus (rdy)\n"); return -ENODEV; } I want the register to be zero, would this be more obvious? /* Wait for command send status zero = ready */ spin_event_timeout((in_be16(&psc_dma->psc_regs->sr_csr.status) & MPC52xx_PSC_SR_CMDSEND), 100, 0, rc); if (rc != 0) { pr_err("timeout on ac97 bus (rdy)\n"); return -ENODEV; } > > Signed-off-by: Timur Tabi <timur@freescale.com> > --- > > v8: added a copyright notice > > arch/powerpc/include/asm/delay.h | 33 +++++++++++++++++++++++++++++++++ > 1 files changed, 33 insertions(+), 0 deletions(-) > > diff --git a/arch/powerpc/include/asm/delay.h b/arch/powerpc/include/asm/delay.h > index f9200a6..af4a270 100644 > --- a/arch/powerpc/include/asm/delay.h > +++ b/arch/powerpc/include/asm/delay.h > @@ -2,8 +2,11 @@ > #define _ASM_POWERPC_DELAY_H > #ifdef __KERNEL__ > > +#include <asm/time.h> > + > /* > * Copyright 1996, Paul Mackerras. > + * Copyright (C) 2009 Freescale Semiconductor, Inc. All rights reserved. > * > * This program is free software; you can redistribute it and/or > * modify it under the terms of the GNU General Public License > @@ -30,5 +33,35 @@ extern void udelay(unsigned long usecs); > #define mdelay(n) udelay((n) * 1000) > #endif > > +/** > + * 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 eache evaluation of > + * @condition > + * @rc: the last value of the condition > + * > + * The process spins until the condition evaluates to true (non-zero) or the > + * timeout elapses. Upon exit, @rc contains the value of the condition. This > + * allows you to test the condition without incurring any side effects. > + * > + * 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. > + */ > +#define spin_event_timeout(condition, timeout, delay, rc) \ > +{ \ > + unsigned long __loops = tb_ticks_per_usec * timeout; \ > + unsigned long __start = get_tbl(); \ > + while (!(rc = (condition)) && (tb_ticks_since(__start) <= __loops)) \ > + if (delay) \ > + udelay(delay); \ > + else \ > + cpu_relax(); \ > +} > + > #endif /* __KERNEL__ */ > #endif /* _ASM_POWERPC_DELAY_H */ > -- > 1.6.0.6 > > _______________________________________________ > Linuxppc-dev mailing list > Linuxppc-dev@ozlabs.org > https://ozlabs.org/mailman/listinfo/linuxppc-dev >
On Mon, May 25, 2009 at 12:46 PM, Jon Smirl <jonsmirl@gmail.com> wrote: > I just tried using this. The !rc has the effect of making the error > return be zero instead the normal not zero. You're confused. It's not a "return code", it's a return value. I guess I should have called the parameter "ret" instead of "rc", but I didn't expect people to get confused. 'rc' is the value of the expression when the loop terminates. That's what makes the most sense, because the developer will want to know what that value is. If you're expression happens to rely on negative logic (e.g. wait until a bit is cleared), then of course it's going to appear "backwards" when you test it.
Timur Tabi wrote: > On Mon, May 25, 2009 at 12:46 PM, Jon Smirl <jonsmirl@gmail.com> wrote: > >> I just tried using this. The !rc has the effect of making the error >> return be zero instead the normal not zero. > > You're confused. It's not a "return code", it's a return value. I > guess I should have called the parameter "ret" instead of "rc", but I > didn't expect people to get confused. > > 'rc' is the value of the expression when the loop terminates. That's > what makes the most sense, because the developer will want to know > what that value is. If you're expression happens to rely on negative > logic (e.g. wait until a bit is cleared), then of course it's going to > appear "backwards" when you test it. I've just been going through some hassles associated with wait_event variants and their return codes. Eg. wait_event_interruptible()'s return value is documented as; "The function will return -ERESTARTSYS if it was interrupted by a signal and 0 if @condition evaluated to true." And wait_event_timeout()'s return value is; "The function returns 0 if the @timeout elapsed, and the remaining jiffies if the condition evaluated to true before the timeout elapsed." In all cases it seems, they don't return the what the expression evaluates to, but instead return an indication about the wait outcome. This is why I've taken to doing things like; wait_event_***(queue, !(ret = try_something())); So from this user's perspective (FWIW), it would come as a surprise if the return value reflected the evaluated expression rather than what happened w.r.t. the spin/timeout. Cheers, Geoff
On Tue, May 26, 2009 at 12:20 PM, Geoff Thorpe <Geoff.Thorpe@freescale.com> wrote: > Timur Tabi wrote: >> On Mon, May 25, 2009 at 12:46 PM, Jon Smirl <jonsmirl@gmail.com> wrote: >> >>> I just tried using this. The !rc has the effect of making the error >>> return be zero instead the normal not zero. >> >> You're confused. It's not a "return code", it's a return value. I >> guess I should have called the parameter "ret" instead of "rc", but I >> didn't expect people to get confused. >> >> 'rc' is the value of the expression when the loop terminates. That's >> what makes the most sense, because the developer will want to know >> what that value is. If you're expression happens to rely on negative >> logic (e.g. wait until a bit is cleared), then of course it's going to >> appear "backwards" when you test it. > > I've just been going through some hassles associated with wait_event > variants and their return codes. > > Eg. wait_event_interruptible()'s return value is documented as; "The > function will return -ERESTARTSYS if it was interrupted by a signal and > 0 if @condition evaluated to true." And wait_event_timeout()'s return > value is; "The function returns 0 if the @timeout elapsed, and the > remaining jiffies if the condition evaluated to true before the timeout > elapsed." > > In all cases it seems, they don't return the what the expression > evaluates to, but instead return an indication about the wait outcome. > This is why I've taken to doing things like; > wait_event_***(queue, !(ret = try_something())); That's a good trick, if you want the result of the condition pass the assignment in. That frees up the return value to indicate the error of the time out expiring. I'd really like to keep a consistent if(rc!=0)error; model. > So from this user's perspective (FWIW), it would come as a surprise if > the return value reflected the evaluated expression rather than what > happened w.r.t. the spin/timeout. > > Cheers, > Geoff > > > >
Geoff Thorpe wrote: > So from this user's perspective (FWIW), it would come as a surprise if > the return value reflected the evaluated expression rather than what > happened w.r.t. the spin/timeout. It shouldn't come as a surprise because I've thoroughly documented the behavior. I also think returning the actual value of the expression is better than a return code. Remember, the primary purpose of this macro is to wait for a hardware register to change. Contrast this to wait_event_xxx, which usually queries a variable. Therefore, the hardware register may set multiple bits. For instance, you could do this: ret = spin_event_timeout(in_be32(x) & 0x14, ...); if (ret & 0x10) do something here if (ret & 0x04) do something else here I think the ability to do this is more important than making the code as similar as possible to wait_event_xxx.
On Tue, May 26, 2009 at 1:03 PM, Timur Tabi <timur@freescale.com> wrote: > Geoff Thorpe wrote: > >> So from this user's perspective (FWIW), it would come as a surprise if >> the return value reflected the evaluated expression rather than what >> happened w.r.t. the spin/timeout. > > It shouldn't come as a surprise because I've thoroughly documented the behavior. I also think returning the actual value of the expression is better than a return code. Remember, the primary purpose of this macro is to wait for a hardware register to change. Contrast this to wait_event_xxx, which usually queries a variable. Therefore, the hardware register may set multiple bits. For instance, you could do this: > > ret = spin_event_timeout(in_be32(x) & 0x14, ...); > > if (ret & 0x10) > do something here > > if (ret & 0x04) > do something else here If (ret == 0) timeout_happened; That's the part that looks wrong. > > I think the ability to do this is more important than making the code as similar as possible to wait_event_xxx. Why not this? rc = spin_event_timeout(result = (in_be32(x) & 0x14), ...); if (rc) timeout_happened; if (result & 0x10) do something here if (result & 0x04) do something else here Then if I don't care about the result (which I think is the common case)... rc = spin_event_timeout(in_be32(x) & 0x14, ...); if (rc) timeout_happened; > > -- > Timur Tabi > Linux kernel developer at Freescale >
Jon Smirl wrote: > Then if I don't care about the result (which I think is the common case)... > > rc = spin_event_timeout(in_be32(x) & 0x14, ...); > if (rc) > timeout_happened; That's another way of doing it, but I'm already at version 9 of my patch, and I'm not inclined to make any changes that don't add any real functionality. Every time I post a new version of this patch, someone new comes out of the woodwork to tell me that I should do it a different way. Where were you two months ago?
Timur Tabi wrote: > Geoff Thorpe wrote: > >> So from this user's perspective (FWIW), it would come as a surprise if >> the return value reflected the evaluated expression rather than what >> happened w.r.t. the spin/timeout. > > It shouldn't come as a surprise because I've thoroughly documented the behavior. I also think returning the actual value of the expression is better than a return code. Remember, the primary purpose of this macro is to wait for a hardware register to change. Contrast this to wait_event_xxx, which usually queries a variable. Therefore, the hardware register may set multiple bits. For instance, you could do this: > > ret = spin_event_timeout(in_be32(x) & 0x14, ...); > > if (ret & 0x10) > do something here > > if (ret & 0x04) > do something else here > > I think the ability to do this is more important than making the code as similar as possible to wait_event_xxx. I was just providing feedback after (recently) coming to grips with the subtleties of similar assists in the kernel. Documentation helps when you don't know what to expect - but if you're expecting a certain behaviour (consistency of success==0), it's easy to misread the doc, or just not read it at all. BTW, your example above does not illustrate the use of the timeout, and you'd have to infer it because the return value does not provide any direct indication. As for access to the evaluated expression, I believe the following is functionally equivalent; rc = spin_event_timeout((ret = in_be32(x) & 0x14), ...); You can ignore the return value if you don't care, which gives the same thing as your original example, but with the 'ret' assignment inside the spin call instead of outside it. Anyway, just my $0.02 FWIW. I have no strongly-held opinion here, especially as the closest precedent I know of is wait_event_timeout(), which also does something unexpected, but it doesn't do the same unexpected thing as your function does. :-) It's based on jiffies rather than microseconds, and it returns zero if the timeout elapses otherwise it returns the number of remaining jiffies when the condition evaluated true. Cheers, Geoff
Geoff Thorpe wrote:
> rc = spin_event_timeout((ret = in_be32(x) & 0x14), ...);
It's an interesting idea, but I have two problems with it:
1) This approach is that it depends on the internals of the macro. That is, you're sneaking in an assignment in the hopes that the code will behave properly. You see that the current code evaluates the condition only once, so it works. The code will look like this:
2) Unlike the wait_event_xxx macros, I believe that the actual evaluated expression is important to the caller. So 90% of the time, the caller is going to pass in "ret = (condition)", which means it makes more sense to have this as a built-in feature of the macro.
So if you're okay with v9 of the patch, please ACK it.
On Tue, May 26, 2009 at 2:17 PM, Timur Tabi <timur@freescale.com> wrote: > Geoff Thorpe wrote: > >> rc = spin_event_timeout((ret = in_be32(x) & 0x14), ...); > > It's an interesting idea, but I have two problems with it: > > 1) This approach is that it depends on the internals of the macro. That is, you're sneaking in an assignment in the hopes that the code will behave properly. You see that the current code evaluates the condition only once, so it works. The code will look like this: > > 2) Unlike the wait_event_xxx macros, I believe that the actual evaluated expression is important to the caller. So 90% of the time, the caller is going to pass in "ret = (condition)", which means it makes more sense to have this as a built-in feature of the macro. The only time you need the result is when you are waiting on multiple bits. I don't think looping while waiting on multiple bits is the common case. > So if you're okay with v9 of the patch, please ACK it. I'll ACK ths so that I can get my driver in. But every time I used this function I got it wrong on the first try. This is going to be a source of bugs. > > -- > Timur Tabi > Linux kernel developer at Freescale > _______________________________________________ > Linuxppc-dev mailing list > Linuxppc-dev@ozlabs.org > https://ozlabs.org/mailman/listinfo/linuxppc-dev >
diff --git a/arch/powerpc/include/asm/delay.h b/arch/powerpc/include/asm/delay.h index f9200a6..af4a270 100644 --- a/arch/powerpc/include/asm/delay.h +++ b/arch/powerpc/include/asm/delay.h @@ -2,8 +2,11 @@ #define _ASM_POWERPC_DELAY_H #ifdef __KERNEL__ +#include <asm/time.h> + /* * Copyright 1996, Paul Mackerras. + * Copyright (C) 2009 Freescale Semiconductor, Inc. All rights reserved. * * This program is free software; you can redistribute it and/or * modify it under the terms of the GNU General Public License @@ -30,5 +33,35 @@ extern void udelay(unsigned long usecs); #define mdelay(n) udelay((n) * 1000) #endif +/** + * 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 eache evaluation of + * @condition + * @rc: the last value of the condition + * + * The process spins until the condition evaluates to true (non-zero) or the + * timeout elapses. Upon exit, @rc contains the value of the condition. This + * allows you to test the condition without incurring any side effects. + * + * 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. + */ +#define spin_event_timeout(condition, timeout, delay, rc) \ +{ \ + unsigned long __loops = tb_ticks_per_usec * timeout; \ + unsigned long __start = get_tbl(); \ + while (!(rc = (condition)) && (tb_ticks_since(__start) <= __loops)) \ + if (delay) \ + udelay(delay); \ + else \ + cpu_relax(); \ +} + #endif /* __KERNEL__ */ #endif /* _ASM_POWERPC_DELAY_H */
The macro spin_event_timeout() takes a condition and timeout value (in microseconds) as parameters. It spins until either the condition is true or the timeout expires. It returns the result of the condition when the loop was terminated. 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 if the bit doesn't change as expected. This macro makes it easier for driver developers to perform this kind of operation properly. Signed-off-by: Timur Tabi <timur@freescale.com> --- v8: added a copyright notice arch/powerpc/include/asm/delay.h | 33 +++++++++++++++++++++++++++++++++ 1 files changed, 33 insertions(+), 0 deletions(-)