Patchwork [1/2,v8] powerpc: introduce macro spin_event_timeout()

login
register
mail settings
Submitter Timur Tabi
Date May 19, 2009, 7:26 p.m.
Message ID <1242761199-17875-2-git-send-email-timur@freescale.com>
Download mbox | patch
Permalink /patch/27414/
State Awaiting Upstream, archived
Headers show

Comments

Timur Tabi - May 19, 2009, 7:26 p.m.
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(-)
Jon Smirl - May 25, 2009, 5:46 p.m.
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
>
Timur Tabi - May 26, 2009, 3:27 a.m.
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.
Geoff Thorpe - May 26, 2009, 4:20 p.m.
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
Jon Smirl - May 26, 2009, 4:27 p.m.
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
>
>
>
>
Timur Tabi - May 26, 2009, 5:03 p.m.
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.
Jon Smirl - May 26, 2009, 5:56 p.m.
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
>
Timur Tabi - May 26, 2009, 6:01 p.m.
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?
Geoff Thorpe - May 26, 2009, 6:09 p.m.
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
Timur Tabi - May 26, 2009, 6:17 p.m.
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.
Jon Smirl - May 26, 2009, 7:04 p.m.
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
>

Patch

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 */