[PATCH 0/2] introduce macro spin_event_timeout()

[PATCH 0/2] introduce macro spin_event_timeout()

[Timur Tabi]

Introduce the spin_event_timeout() macro, and update the QE library to use it.

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

[Timur Tabi]

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(-)

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

[PATCH 2/2] qe: add polling timeout to qe_issue_cmd()

[Timur Tabi]

The qe_issue_cmd() function (Freescale PowerPC QUICC Engine library) polls on
a register until a status bit changes, but does not include a timeout to
handle the situation if the bit never changes.  Change the code to use the new
spin_event_timeout() macro, which simplifies polling on a register without
a timeout.

Signed-off-by: Timur Tabi <timur@freescale.com>
---

This patch depends on my previous patch, "powerpc: introduce macro
spin_event_timeout()".

 arch/powerpc/sysdev/qe_lib/qe.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/sysdev/qe_lib/qe.c b/arch/powerpc/sysdev/qe_lib/qe.c
index 01bce37..810e1df 100644
--- a/arch/powerpc/sysdev/qe_lib/qe.c
+++ b/arch/powerpc/sysdev/qe_lib/qe.c
@@ -111,6 +111,7 @@ int qe_issue_cmd(u32 cmd, u32 device, u8 mcn_protocol, u32 cmd_input)
 {
 	unsigned long flags;
 	u8 mcn_shift = 0, dev_shift = 0;
+	int ret;
 
 	spin_lock_irqsave(&qe_lock, flags);
 	if (cmd == QE_RESET) {
@@ -138,11 +139,13 @@ int qe_issue_cmd(u32 cmd, u32 device, u8 mcn_protocol, u32 cmd_input)
 	}
 
 	/* wait for the QE_CR_FLG to clear */
-	while(in_be32(&qe_immr->cp.cecr) & QE_CR_FLG)
-		cpu_relax();
+	spin_event_timeout((in_be32(&qe_immr->cp.cecr) & QE_CR_FLG) == 0,
+			   100, 0, ret);
+	/* On timeout (e.g. failure), the expression will be false (ret == 0),
+	   otherwise it will be true (ret == 1). */
 	spin_unlock_irqrestore(&qe_lock, flags);
 
-	return 0;
+	return ret == 1;
 }
 EXPORT_SYMBOL(qe_issue_cmd);
 
-- 
1.6.0.6

Re: [PATCH 2/2] qe: add polling timeout to qe_issue_cmd()

[Grant Likely]

On Tue, May 19, 2009 at 1:26 PM, Timur Tabi <timur@freescale.com> wrote:
> The qe_issue_cmd() function (Freescale PowerPC QUICC Engine library) poll=
s on
> a register until a status bit changes, but does not include a timeout to
> handle the situation if the bit never changes. =A0Change the code to use =
the new
> spin_event_timeout() macro, which simplifies polling on a register withou=
t
> a timeout.
>
> Signed-off-by: Timur Tabi <timur@freescale.com>
> ---
>
> This patch depends on my previous patch, "powerpc: introduce macro
> spin_event_timeout()".
>
> =A0arch/powerpc/sysdev/qe_lib/qe.c | =A0 =A09 ++++++---
> =A01 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/sysdev/qe_lib/qe.c b/arch/powerpc/sysdev/qe_lib=
/qe.c
> index 01bce37..810e1df 100644
> --- a/arch/powerpc/sysdev/qe_lib/qe.c
> +++ b/arch/powerpc/sysdev/qe_lib/qe.c
> @@ -111,6 +111,7 @@ int qe_issue_cmd(u32 cmd, u32 device, u8 mcn_protocol=
, u32 cmd_input)
> =A0{
> =A0 =A0 =A0 =A0unsigned long flags;
> =A0 =A0 =A0 =A0u8 mcn_shift =3D 0, dev_shift =3D 0;
> + =A0 =A0 =A0 int ret;
>
> =A0 =A0 =A0 =A0spin_lock_irqsave(&qe_lock, flags);
> =A0 =A0 =A0 =A0if (cmd =3D=3D QE_RESET) {
> @@ -138,11 +139,13 @@ int qe_issue_cmd(u32 cmd, u32 device, u8 mcn_protoc=
ol, u32 cmd_input)
> =A0 =A0 =A0 =A0}
>
> =A0 =A0 =A0 =A0/* wait for the QE_CR_FLG to clear */
> - =A0 =A0 =A0 while(in_be32(&qe_immr->cp.cecr) & QE_CR_FLG)
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 cpu_relax();
> + =A0 =A0 =A0 spin_event_timeout((in_be32(&qe_immr->cp.cecr) & QE_CR_FLG)=
 =3D=3D 0,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0100, 0, ret);
> + =A0 =A0 =A0 /* On timeout (e.g. failure), the expression will be false =
(ret =3D=3D 0),
> + =A0 =A0 =A0 =A0 =A0otherwise it will be true (ret =3D=3D 1). */
> =A0 =A0 =A0 =A0spin_unlock_irqrestore(&qe_lock, flags);
>
> - =A0 =A0 =A0 return 0;
> + =A0 =A0 =A0 return ret =3D=3D 1;

Hmmm, the ret value is backwards from what most coders would expect
(zero on success, non-zero on failure).  I'd personally recommend
reversing the polarity in the macro.

Otherwise, feel free to add my acked-by line to both patches.

g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 2/2] qe: add polling timeout to qe_issue_cmd()

[Timur Tabi]

Grant Likely wrote:

> Hmmm, the ret value is backwards from what most coders would expect
> (zero on success, non-zero on failure).  I'd personally recommend
> reversing the polarity in the macro.

The ret value is documented as being the value of the expression when the loop terminates.  The reason it appears backwards is because the expression is

	(in_be32(&qe_immr->cp.cecr) & QE_CR_FLG) == 0

That is, the loop should spin until the QE_CR_FLG is zero.  So when the loop terminates normally, the expression (x) == 0 is true, which is equal to one.

I would expect that in most cases, the loop spins until some bit is *set*.  Let's pretend that QE_CR_FLG operates this way.  In that case, the call would look like this:

spin_event_timeout(in_be32(&qe_immr->cp.cecr) & QE_CR_FLG, 100, 0, ret);

At loop termination, the result of the expression will be either QE_CR_FLG or zero.

> Otherwise, feel free to add my acked-by line to both patches.

Thanks.

Ben, would you please apply this to your 'next' branch?

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH 1/2 v8] powerpc: introduce macro spin_event_timeout()

[Jon Smirl]

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. =A0It spins until either the condition i=
s true
> or the timeout expires. =A0It returns the result of the condition when th=
e loop
> was terminated.
>
> This primary purpose of this macro is to poll on a hardware register unti=
l a
> status bit changes. =A0The timeout ensures that the loop still terminates=
 if the
> bit doesn't change as expected. =A0This 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 =3D ready */
	spin_event_timeout(!(in_be16(&psc_dma->psc_regs->sr_csr.status) &
				MPC52xx_PSC_SR_CMDSEND), 100, 0, rc);
	if (rc =3D=3D 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 =3D ready */
	spin_event_timeout((in_be16(&psc_dma->psc_regs->sr_csr.status) &
				MPC52xx_PSC_SR_CMDSEND), 100, 0, rc);
	if (rc !=3D 0) {
		pr_err("timeout on ac97 bus (rdy)\n");
		return -ENODEV;
	}


>
> Signed-off-by: Timur Tabi <timur@freescale.com>
> ---
>
> v8: added a copyright notice
>
> =A0arch/powerpc/include/asm/delay.h | =A0 33 ++++++++++++++++++++++++++++=
+++++
> =A01 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 @@
> =A0#define _ASM_POWERPC_DELAY_H
> =A0#ifdef __KERNEL__
>
> +#include <asm/time.h>
> +
> =A0/*
> =A0* Copyright 1996, Paul Mackerras.
> + * Copyright (C) 2009 Freescale Semiconductor, Inc. All rights reserved.
> =A0*
> =A0* This program is free software; you can redistribute it and/or
> =A0* modify it under the terms of the GNU General Public License
> @@ -30,5 +33,35 @@ extern void udelay(unsigned long usecs);
> =A0#define mdelay(n) =A0 =A0 =A0udelay((n) * 1000)
> =A0#endif
>
> +/**
> + * spin_event_timeout - spin until a condition gets true or a timeout el=
apses
> + * @condition: a C expression to evalate
> + * @timeout: timeout, in microseconds
> + * @delay: the number of microseconds to delay between eache evaluation =
of
> + * =A0 =A0 =A0 =A0 @condition
> + * @rc: the last value of the condition
> + *
> + * The process spins until the condition evaluates to true (non-zero) or=
 the
> + * timeout elapses. =A0Upon exit, @rc contains the value of the conditio=
n. 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. =A0The timeout ensures that the loop stil=
l
> + * terminates even if the bit never changes. =A0The 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) =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 \
> +{ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 \
> + =A0 =A0 =A0 unsigned long __loops =3D tb_ticks_per_usec * timeout; =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0\
> + =A0 =A0 =A0 unsigned long __start =3D get_tbl(); =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0\
> + =A0 =A0 =A0 while (!(rc =3D (condition)) && (tb_ticks_since(__start) <=
=3D __loops)) \
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (delay) =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0\
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 udelay(delay); =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0\
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 else =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
\
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 cpu_relax(); =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0\
> +}
> +
> =A0#endif /* __KERNEL__ */
> =A0#endif /* _ASM_POWERPC_DELAY_H */
> --
> 1.6.0.6
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev
>



--=20
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH 1/2 v8] powerpc: introduce macro spin_event_timeout()

[Timur Tabi]

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
Linux kernel developer at Freescale

Re: [PATCH 1/2 v8] powerpc: introduce macro spin_event_timeout()

[Geoff Thorpe]

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

Re: [PATCH 1/2 v8] powerpc: introduce macro spin_event_timeout()

[Jon Smirl]

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. =A0It's not a "return code", it's a return value. =A0I
>> 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. =A0That's
>> what makes the most sense, because the developer will want to know
>> what that value is. =A0If 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;
> =A0 wait_event_***(queue, !(ret =3D 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 expiri=
ng.

I'd really like to keep a consistent if(rc!=3D0)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
>
>
>
>



--=20
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH 1/2 v8] powerpc: introduce macro spin_event_timeout()

[Timur Tabi]

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.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH 1/2 v8] powerpc: introduce macro spin_event_timeout()

[Jon Smirl]

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 be=
havior. =A0I also think returning the actual value of the expression is bet=
ter than a return code. =A0Remember, the primary purpose of this macro is t=
o wait for a hardware register to change. =A0Contrast this to wait_event_xx=
x, which usually queries a variable. =A0Therefore, the hardware register ma=
y set multiple bits. =A0For instance, you could do this:
>
> ret =3D spin_event_timeout(in_be32(x) & 0x14, ...);
>
> if (ret & 0x10)
> =A0 =A0 =A0 =A0do something here
>
> if (ret & 0x04)
> =A0 =A0 =A0 =A0do something else here

If (ret =3D=3D 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 =3D spin_event_timeout(result =3D (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 =3D spin_event_timeout(in_be32(x) & 0x14, ...);
if (rc)
   timeout_happened;



>
> --
> Timur Tabi
> Linux kernel developer at Freescale
>



--=20
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH 1/2 v8] powerpc: introduce macro spin_event_timeout()

[Timur Tabi]

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
Linux kernel developer at Freescale

Re: [PATCH 1/2 v8] powerpc: introduce macro spin_event_timeout()

[Geoff Thorpe]

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

Re: [PATCH 1/2 v8] powerpc: introduce macro spin_event_timeout()

[Timur Tabi]

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.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH 1/2 v8] powerpc: introduce macro spin_event_timeout()

[Jon Smirl]

On Tue, May 26, 2009 at 2:17 PM, Timur Tabi <timur@freescale.com> wrote:
> Geoff Thorpe wrote:
>
>> rc =3D spin_event_timeout((ret =3D 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. =A0Tha=
t is, you're sneaking in an assignment in the hopes that the code will beha=
ve properly. =A0You see that the current code evaluates the condition only =
once, so it works. =A0The code will look like this:
>
> 2) Unlike the wait_event_xxx macros, I believe that the actual evaluated =
expression is important to the caller. =A0So 90% of the time, the caller is=
 going to pass in "ret =3D (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
>



--=20
Jon Smirl
jonsmirl@gmail.com