[patch 05/14] char/hvsi: use wait_event_timeout()

[patch 05/14] char/hvsi: use wait_event_timeout()

[domen]

Please consider applying. This is my first wait-queue related patch, so comments
are very welcome.

Use wait_event_timeout() in place of custom wait-queue code. The
code is not changed in any way (I don't think), but is cleaned up quite a bit
(will get expanded to almost identical code).

Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>
Signed-off-by: Domen Puncer <domen@coderock.org>
---


 kj-domen/drivers/char/hvsi.c |   41 ++++-------------------------------------
 1 files changed, 4 insertions(+), 37 deletions(-)

diff -puN drivers/char/hvsi.c~wait_event_timeout-drivers_char_hvsi drivers/char/hvsi.c
--- kj/drivers/char/hvsi.c~wait_event_timeout-drivers_char_hvsi	2005-03-05 16:11:27.000000000 +0100
+++ kj-domen/drivers/char/hvsi.c	2005-03-05 16:11:27.000000000 +0100
@@ -44,6 +44,7 @@
 #include <linux/sysrq.h>
 #include <linux/tty.h>
 #include <linux/tty_flip.h>
+#include <linux/wait.h>
 #include <asm/hvcall.h>
 #include <asm/hvconsole.h>
 #include <asm/prom.h>
@@ -631,27 +632,10 @@ static int __init poll_for_state(struct 
 /* wait for irq handler to change our state */
 static int wait_for_state(struct hvsi_struct *hp, int state)
 {
-	unsigned long end_jiffies = jiffies + HVSI_TIMEOUT;
-	unsigned long timeout;
 	int ret = 0;
 
-	DECLARE_WAITQUEUE(myself, current);
-	set_current_state(TASK_INTERRUPTIBLE);
-	add_wait_queue(&hp->stateq, &myself);
-
-	for (;;) {
-		set_current_state(TASK_INTERRUPTIBLE);
-		if (hp->state == state)
-			break;
-		timeout = end_jiffies - jiffies;
-		if (time_after(jiffies, end_jiffies)) {
-			ret = -EIO;
-			break;
-		}
-		schedule_timeout(timeout);
-	}
-	remove_wait_queue(&hp->stateq, &myself);
-	set_current_state(TASK_RUNNING);
+	if(!wait_event_timeout(hp->stateq, (hp->state == state), jiffies + HVSI_TIMEOUT))
+		ret = -EIO;
 
 	return ret;
 }
@@ -868,24 +852,7 @@ static int hvsi_open(struct tty_struct *
 /* wait for hvsi_write_worker to empty hp->outbuf */
 static void hvsi_flush_output(struct hvsi_struct *hp)
 {
-	unsigned long end_jiffies = jiffies + HVSI_TIMEOUT;
-	unsigned long timeout;
-
-	DECLARE_WAITQUEUE(myself, current);
-	set_current_state(TASK_UNINTERRUPTIBLE);
-	add_wait_queue(&hp->emptyq, &myself);
-
-	for (;;) {
-		set_current_state(TASK_UNINTERRUPTIBLE);
-		if (hp->n_outbuf <= 0)
-			break;
-		timeout = end_jiffies - jiffies;
-		if (time_after(jiffies, end_jiffies))
-			break;
-		schedule_timeout(timeout);
-	}
-	remove_wait_queue(&hp->emptyq, &myself);
-	set_current_state(TASK_RUNNING);
+	wait_event_timeout(hp->emptyq, (hp->n_outbuf <= 0), jiffies + HVSI_TIMEOUT);
 
 	/* 'writer' could still be pending if it didn't see n_outbuf = 0 yet */
 	cancel_delayed_work(&hp->writer);
_

Re: [patch 05/14] char/hvsi: use wait_event_timeout()

[Andrew Morton]

domen@coderock.org wrote:
>
> Please consider applying. This is my first wait-queue related patch, so comments
>  are very welcome.
> 
>  Use wait_event_timeout() in place of custom wait-queue code. The
>  code is not changed in any way (I don't think), but is cleaned up quite a bit
>  (will get expanded to almost identical code).
> 
> ...
>  diff -puN drivers/char/hvsi.c~wait_event_timeout-drivers_char_hvsi drivers/char/hvsi.c
>  --- kj/drivers/char/hvsi.c~wait_event_timeout-drivers_char_hvsi	2005-03-05 16:11:27.000000000 +0100
>  +++ kj-domen/drivers/char/hvsi.c	2005-03-05 16:11:27.000000000 +0100
>  @@ -44,6 +44,7 @@
>   #include <linux/sysrq.h>
>   #include <linux/tty.h>
>   #include <linux/tty_flip.h>
>  +#include <linux/wait.h>
>   #include <asm/hvcall.h>
>   #include <asm/hvconsole.h>
>   #include <asm/prom.h>
>  @@ -631,27 +632,10 @@ static int __init poll_for_state(struct 
>   /* wait for irq handler to change our state */
>   static int wait_for_state(struct hvsi_struct *hp, int state)
>   {
>  -	unsigned long end_jiffies = jiffies + HVSI_TIMEOUT;
>  -	unsigned long timeout;
>   	int ret = 0;
>   
>  -	DECLARE_WAITQUEUE(myself, current);
>  -	set_current_state(TASK_INTERRUPTIBLE);
>  -	add_wait_queue(&hp->stateq, &myself);
>  -
>  -	for (;;) {
>  -		set_current_state(TASK_INTERRUPTIBLE);
>  -		if (hp->state == state)
>  -			break;
>  -		timeout = end_jiffies - jiffies;
>  -		if (time_after(jiffies, end_jiffies)) {
>  -			ret = -EIO;
>  -			break;
>  -		}
>  -		schedule_timeout(timeout);
>  -	}
>  -	remove_wait_queue(&hp->stateq, &myself);
>  -	set_current_state(TASK_RUNNING);
>  +	if(!wait_event_timeout(hp->stateq, (hp->state == state), jiffies + HVSI_TIMEOUT))
>  +		ret = -EIO;

wait_event_timeout()'s `timeout' arg is number-of-milliseconds-to-wait,
not an absolute time, yes?

Also, it's conventional to put a space between the `if' and the `('.

It's nice to squeeze the code into an 80-column xterm, too.

Re: [patch 05/14] char/hvsi: use wait_event_timeout()

[Nish Aravamudan]

On Sun, 6 Mar 2005 19:32:36 -0800, Andrew Morton <akpm@osdl.org> wrote:
> domen@coderock.org wrote:
> >
> > Please consider applying. This is my first wait-queue related patch, so comments
> >  are very welcome.
> >
> >  Use wait_event_timeout() in place of custom wait-queue code. The
> >  code is not changed in any way (I don't think), but is cleaned up quite a bit
> >  (will get expanded to almost identical code).
> >
> > ...

<snip>

> >  +    if(!wait_event_timeout(hp->stateq, (hp->state == state), jiffies + HVSI_TIMEOUT))
> >  +            ret = -EIO;
> 
> wait_event_timeout()'s `timeout' arg is number-of-milliseconds-to-wait,
> not an absolute time, yes?

D'oh. I will fix this and any other timeout version that are being
pushed to you tomorrow. Sadly, wait_event*() do not take milliseconds,
but jiffies. I am hoping to push some patches to change that, but it
may have to wait a while.
 
> Also, it's conventional to put a space between the `if' and the `('.
>
> It's nice to squeeze the code into an 80-column xterm, too.

Will fix these both as well.

Thanks,
Nish

[UPDATE PATCH] char/hvsi: use wait_event_timeout()

[Nishanth Aravamudan]

On Sun, Mar 06, 2005 at 07:32:36PM -0800, Andrew Morton wrote:
> domen@coderock.org wrote:
> >
> > Please consider applying. This is my first wait-queue related patch, so comments
> >  are very welcome.
> > 
> >  Use wait_event_timeout() in place of custom wait-queue code. The
> >  code is not changed in any way (I don't think), but is cleaned up quite a bit
> >  (will get expanded to almost identical code).
> > 
> > ...
> >  diff -puN drivers/char/hvsi.c~wait_event_timeout-drivers_char_hvsi drivers/char/hvsi.c
> >  --- kj/drivers/char/hvsi.c~wait_event_timeout-drivers_char_hvsi	2005-03-05 16:11:27.000000000 +0100
> >  +++ kj-domen/drivers/char/hvsi.c	2005-03-05 16:11:27.000000000 +0100

<snip>

> >  +	if(!wait_event_timeout(hp->stateq, (hp->state == state), jiffies + HVSI_TIMEOUT))
> >  +		ret = -EIO;
> 
> wait_event_timeout()'s `timeout' arg is number-of-milliseconds-to-wait,
> not an absolute time, yes?
> 
> Also, it's conventional to put a space between the `if' and the `('.
> 
> It's nice to squeeze the code into an 80-column xterm, too.

All fixed in the below patch. Would you prefer an incremental version?

Thanks,
Nish

Description: Use wait_event_timeout() in place of custom wait-queue
code. The code is not changed in any way (I don't think), but is cleaned
up quite a bit (will get expanded to almost identical code).

Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>

--- 2.6.11-v/drivers/char/hvsi.c	2005-03-01 23:38:13.000000000 -0800
+++ 2.6.11/drivers/char/hvsi.c	2005-03-07 16:14:51.000000000 -0800
@@ -631,27 +631,10 @@ static int __init poll_for_state(struct 
 /* wait for irq handler to change our state */
 static int wait_for_state(struct hvsi_struct *hp, int state)
 {
-	unsigned long end_jiffies = jiffies + HVSI_TIMEOUT;
-	unsigned long timeout;
 	int ret = 0;
 
-	DECLARE_WAITQUEUE(myself, current);
-	set_current_state(TASK_INTERRUPTIBLE);
-	add_wait_queue(&hp->stateq, &myself);
-
-	for (;;) {
-		set_current_state(TASK_INTERRUPTIBLE);
-		if (hp->state == state)
-			break;
-		timeout = end_jiffies - jiffies;
-		if (time_after(jiffies, end_jiffies)) {
-			ret = -EIO;
-			break;
-		}
-		schedule_timeout(timeout);
-	}
-	remove_wait_queue(&hp->stateq, &myself);
-	set_current_state(TASK_RUNNING);
+	if (!wait_event_timeout(hp->stateq, (hp->state == state), HVSI_TIMEOUT))
+		ret = -EIO;
 
 	return ret;
 }
@@ -868,24 +851,7 @@ static int hvsi_open(struct tty_struct *
 /* wait for hvsi_write_worker to empty hp->outbuf */
 static void hvsi_flush_output(struct hvsi_struct *hp)
 {
-	unsigned long end_jiffies = jiffies + HVSI_TIMEOUT;
-	unsigned long timeout;
-
-	DECLARE_WAITQUEUE(myself, current);
-	set_current_state(TASK_UNINTERRUPTIBLE);
-	add_wait_queue(&hp->emptyq, &myself);
-
-	for (;;) {
-		set_current_state(TASK_UNINTERRUPTIBLE);
-		if (hp->n_outbuf <= 0)
-			break;
-		timeout = end_jiffies - jiffies;
-		if (time_after(jiffies, end_jiffies))
-			break;
-		schedule_timeout(timeout);
-	}
-	remove_wait_queue(&hp->emptyq, &myself);
-	set_current_state(TASK_RUNNING);
+	wait_event_timeout(hp->emptyq, (hp->n_outbuf <= 0), HVSI_TIMEOUT);
 
 	/* 'writer' could still be pending if it didn't see n_outbuf = 0 yet */
 	cancel_delayed_work(&hp->writer);