char/hvsi: use wait_event_timeout()

PATCH In review [email protected] 4 participants 4 messages Mar 6, 2005 – Mar 8, 2005 Char and misc drivers

lore

Share & tools

lore

[email protected]

author

2005-03-06 22:41 UTC #0 lore raw

PATCH 5/14 char/hvsi: use wait_event_timeout()

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 <[email protected]>

Signed-off-by Domen Puncer <[email protected]>

---

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

drivers/char/hvsi.c renamed from drivers/char/hvsi.c~wait_event_timeout-drivers_char_hvsi +4 ~~-37~~

diff -puN drivers/char/hvsi.c~wait_event_timeout-drivers_char_hvsi drivers/char/hvsi.c
@@ -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);

Andrew Morton

2005-03-07 03:33 UTC #1 lore raw

Quoted 39 lines · [email protected] 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;

Quoted 8 lines

- 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.

Nish Aravamudan

2005-03-07 04:56 UTC #2 lore raw

Quoted 10 lines · Andrew Morton

Quoted 9 lines · [email protected] 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>

Quoted 5 lines

Quoted 2 lines
+ 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.

Quoted 3 lines

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

Nishanth Aravamudan

2005-03-08 00:33 UTC #3 lore raw

PATCH 1/1 char/hvsi: use wait_event_timeout()

In reply to Andrew Morton

Quoted 13 lines · Andrew Morton

Quoted 12 lines · [email protected] 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>

Quoted 9 lines

Quoted 2 lines
+ 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 <[email protected]>

drivers/char/hvsi.c +3 ~~-37~~

@@ -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);