cdrom/aztcd: remove sleep_on() usage

PATCH In review [email protected] 4 participants 5 messages Jun 20, 2005 – Jun 21, 2005

lore

Share & tools

lore

[email protected]

author

2005-06-20 23:01 UTC #0 lore raw

PATCH 2/4 cdrom/aztcd: remove sleep_on() usage

From: Nishanth Aravamudan <[email protected]>

Directly use wait-queues instead of the deprecated sleep_on(). This required adding a local waitqueue. Patch is compile-tested.

Signed-off-by Nishanth Aravamudan <[email protected]>

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

---
 aztcd.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletion(-)

drivers/cdrom/aztcd.c +5 -1

Index: quilt/drivers/cdrom/aztcd.c
===================================================================
@@ -179,6 +179,7 @@
 #include <linux/ioport.h>
 #include <linux/string.h>
 #include <linux/major.h>
+#include <linux/wait.h>
 
 #include <linux/init.h>

@@ -429,9 +430,12 @@ static void dten_low(void)
 #define STEN_LOW_WAIT   statusAzt()
 static void statusAzt(void)
 {
+	DEFINE_WAIT(wait);
 	AztTimeout = AZT_STATUS_DELAY;
 	SET_TIMER(aztStatTimer, HZ / 100);
-	sleep_on(&azt_waitq);
+	prepare_to_wait(&azt_waitq, &wait, TASK_UNINTERRUPTIBLE);
+	schedule();
+	finish_wait(&azt_waitq, &wait);
 	if (AztTimeout <= 0)
 		printk("aztcd: Error Wait STEN_LOW_WAIT command:%x\n",
 		       aztCmd);

Jesper Juhl

2005-06-20 23:16 UTC #1 lore raw

Quoted 30 lines · On 6/20/05, [email protected] <[email protected]> wrote:

From: Nishanth Aravamudan <[email protected]>
Directly use wait-queues instead of the deprecated sleep_on(). This required adding a local waitqueue. Patch is compile-tested.
Signed-off-by Nishanth Aravamudan <[email protected]>
Signed-off-by Domen Puncer <[email protected]>
---
 aztcd.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletion(-)
Index: quilt/drivers/cdrom/aztcd.c
===================================================================
drivers/cdrom/aztcd.c +1 -0
@@ -179,6 +179,7 @@
 #include <linux/ioport.h>
 #include <linux/string.h>
 #include <linux/major.h>
+#include <linux/wait.h>
 
 #include <linux/init.h>
 
@@ -429,9 +430,12 @@ static void dten_low(void)
 #define STEN_LOW_WAIT   statusAzt()
 static void statusAzt(void)
 {

drivers/cdrom/aztcd.c static void dten_low(void) · line 441

Quoted 10 lines

drivers/cdrom/aztcd.c +4 -1

+       DEFINE_WAIT(wait);
        AztTimeout = AZT_STATUS_DELAY;
        SET_TIMER(aztStatTimer, HZ / 100);
-       sleep_on(&azt_waitq);
+       prepare_to_wait(&azt_waitq, &wait, TASK_UNINTERRUPTIBLE);
+       schedule();
+       finish_wait(&azt_waitq, &wait);
        if (AztTimeout <= 0)
                printk("aztcd: Error Wait STEN_LOW_WAIT command:%x\n",
                       aztCmd);

Hmm, now that noone's sleeping on azt_waitq the two wake_up(&azt_waitq); calls in aztStatTimer() don't seem to make much sense any more... Can they just go away or? If they can go away then
axt_waitq itself would seem to be a goner as well... It might just
be me missing something, but this patch looks incomplete and not completely thought through to me.

Signature · 3 lines

Jesper Juhl <[email protected]>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Nishanth Aravamudan

2005-06-21 01:50 UTC #2 lore raw

Quoted 40 lines · Jesper Juhl

Quoted 39 lines · On 6/20/05, [email protected] <[email protected]> wrote:
From: Nishanth Aravamudan <[email protected]>
Directly use wait-queues instead of the deprecated sleep_on(). This required adding a local waitqueue. Patch is compile-tested.
Signed-off-by Nishanth Aravamudan <[email protected]>
Signed-off-by Domen Puncer <[email protected]>
---
 aztcd.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletion(-)
Index: quilt/drivers/cdrom/aztcd.c
===================================================================
drivers/cdrom/aztcd.c +5 -1
@@ -179,6 +179,7 @@
 #include <linux/ioport.h>
 #include <linux/string.h>
 #include <linux/major.h>
+#include <linux/wait.h>
 
 #include <linux/init.h>
 
@@ -429,9 +430,12 @@ static void dten_low(void)
 #define STEN_LOW_WAIT   statusAzt()
 static void statusAzt(void)
 {
+       DEFINE_WAIT(wait);
        AztTimeout = AZT_STATUS_DELAY;
        SET_TIMER(aztStatTimer, HZ / 100);
-       sleep_on(&azt_waitq);
+       prepare_to_wait(&azt_waitq, &wait, TASK_UNINTERRUPTIBLE);
+       schedule();
+       finish_wait(&azt_waitq, &wait);
        if (AztTimeout <= 0)
                printk("aztcd: Error Wait STEN_LOW_WAIT command:%x\n",
                       aztCmd);

Quoted 8 lines

Quoted 1 line
Hmm, now that noone's sleeping on azt_waitq the two wake_up(&azt_waitq); calls in aztStatTimer() don't seem to make much sense any more... Can they just go away or? If they can go away then
axt_waitq itself would seem to be a goner as well... It might just
be me missing something, but this patch looks incomplete and not completely thought through to me.

Huh? prepare_to_wait() means the 'wait' here is sleeping on azt_waitq...

Thanks,
Nish

Jesper Juhl

2005-06-21 00:02 UTC #3 lore raw

Quoted 50 lines · Nishanth Aravamudan

Quoted 40 lines · Jesper Juhl
Quoted 39 lines · On 6/20/05, [email protected] <[email protected]> wrote:
From: Nishanth Aravamudan <[email protected]>
Directly use wait-queues instead of the deprecated sleep_on(). This required adding a local waitqueue. Patch is compile-tested.
Signed-off-by Nishanth Aravamudan <[email protected]>
Signed-off-by Domen Puncer <[email protected]>
---
 aztcd.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletion(-)
Index: quilt/drivers/cdrom/aztcd.c
===================================================================
drivers/cdrom/aztcd.c +5 -1
@@ -179,6 +179,7 @@
 #include <linux/ioport.h>
 #include <linux/string.h>
 #include <linux/major.h>
+#include <linux/wait.h>
 
 #include <linux/init.h>
 
@@ -429,9 +430,12 @@ static void dten_low(void)
 #define STEN_LOW_WAIT   statusAzt()
 static void statusAzt(void)
 {
+       DEFINE_WAIT(wait);
        AztTimeout = AZT_STATUS_DELAY;
        SET_TIMER(aztStatTimer, HZ / 100);
-       sleep_on(&azt_waitq);
+       prepare_to_wait(&azt_waitq, &wait, TASK_UNINTERRUPTIBLE);
+       schedule();
+       finish_wait(&azt_waitq, &wait);
        if (AztTimeout <= 0)
                printk("aztcd: Error Wait STEN_LOW_WAIT command:%x\n",
                       aztCmd);
Quoted 8 lines
Quoted 1 line
Hmm, now that noone's sleeping on azt_waitq the two wake_up(&azt_waitq); calls in aztStatTimer() don't seem to make much sense any more... Can they just go away or? If they can go away then
axt_waitq itself would seem to be a goner as well... It might just
be me missing something, but this patch looks incomplete and not completely thought through to me.

Quoted 2 lines

Huh? prepare_to_wait() means the 'wait' here is sleeping on azt_waitq...

Ahh damn, sorry. Somehow I misread the first argument of prepare_to_wait() and completely missed the fact that it was sleeping on azt_waitq. It must be past my bedtime...
My bad - it looks good.

Signature · 3 lines

Jesper Juhl <[email protected]>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Johannes Stezenbach

2005-06-21 09:59 UTC #4 lore raw

In reply to [email protected]

Quoted 6 lines · [email protected] wrote:

From: Nishanth Aravamudan <[email protected]>
Directly use wait-queues instead of the deprecated sleep_on(). This required adding a local waitqueue. Patch is compile-tested.

IMHO this patch doesn't improve anything, it just makes the code harder to read. sleep_on() is deprecated because there's an inherent race between sleep_on() and wake_up(). To address this you need to add a condition variable (e.g. AztStenLow) and convert the code to use wait_event().
The timer trick used here makes it pretty hard to hit the race, but with a preemptible kernel one could be preempted between the SET_TIMER() and the sleep_on().

Johannes

Quoted 33 lines

Signed-off-by Nishanth Aravamudan <[email protected]>

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

---
 aztcd.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletion(-)

Index: quilt/drivers/cdrom/aztcd.c
===================================================================

drivers/cdrom/aztcd.c +5 -1

@@ -179,6 +179,7 @@
 #include <linux/ioport.h>
 #include <linux/string.h>
 #include <linux/major.h>
+#include <linux/wait.h>
 
 #include <linux/init.h>

@@ -429,9 +430,12 @@ static void dten_low(void)
 #define STEN_LOW_WAIT   statusAzt()
 static void statusAzt(void)
 {
+	DEFINE_WAIT(wait);
 	AztTimeout = AZT_STATUS_DELAY;
 	SET_TIMER(aztStatTimer, HZ / 100);
-	sleep_on(&azt_waitq);
+	prepare_to_wait(&azt_waitq, &wait, TASK_UNINTERRUPTIBLE);
+	schedule();
+	finish_wait(&azt_waitq, &wait);
 	if (AztTimeout <= 0)
 		printk("aztcd: Error Wait STEN_LOW_WAIT command:%x\n",
 		       aztCmd);