Skip to contentSkip to conversation

drivers/cdrom/aztcd.c: busy loop updates + error

PATCH In review Daniel Marjamäki 2 participants 4 messages Nov 19, 2005 Kernel janitors

lore
Share & tools
lore

Daniel Marjamäki

author
#0 lore raw

PATCH 1/1 drivers/cdrom/aztcd.c: busy loop updates + error

Summary of changes:
* long busy loops now sleeps between each repetition.
* using jiffies for timeout rather than repetitions.
* The return code of aztSendCmd is now checked.

Signed-off-by Daniel Marjamäki

diff -up a/drivers/cdrom/aztcd.c b/drivers/cdrom/aztcd.c

drivers/cdrom/aztcd.c +20 -16
@@ -179,6 +179,7 @@
  #include <linux/ioport.h>
  #include <linux/string.h>
  #include <linux/major.h>
+#include <linux/jiffies.h>
 
  #include <linux/init.h>
 

@@ -308,7 +309,7 @@ static char aztDiskChanged = 1;
  static char aztTocUpToDate = 0;
 
  static unsigned char aztIndatum;
-static unsigned long aztTimeOutCount;
+static unsigned long aztTimeOutCount, aztTimeOut;
  static int aztCmd = 0;
 
  static DEFINE_SPINLOCK(aztSpin);

@@ -361,14 +362,14 @@ static int azt_bcd2bin(unsigned char bcd
  # define OP_OK op_ok()
  static void op_ok(void)
  {
-	aztTimeOutCount = 0;
+	aztTimeOut = jiffies + 2;
  	do {
  		aztIndatum = inb(DATA_PORT);
-		aztTimeOutCount++;
-		if (aztTimeOutCount >= AZT_TIMEOUT) {
+		if (time_after(jiffies, aztTimeOut)) {
  			printk("aztcd: Error Wait OP_OK\n");
  			break;
  		}
+		schedule_timeout(1);
  	} while (aztIndatum != AFL_OP_OK);
  }
 

@@ -377,14 +378,14 @@ static void op_ok(void)
  # define PA_OK pa_ok()
  static void pa_ok(void)
  {
-	aztTimeOutCount = 0;
+	aztTimeOut = jiffies + 2;
  	do {
  		aztIndatum = inb(DATA_PORT);
-		aztTimeOutCount++;
-		if (aztTimeOutCount >= AZT_TIMEOUT) {
+		if (time_after(jiffies, aztTimeOut)) {
  			printk("aztcd: Error Wait PA_OK\n");
  			break;
  		}
+		schedule_timeout(1);
  	} while (aztIndatum != AFL_PA_OK);
  }
  #endif

@@ -393,17 +394,17 @@ static void pa_ok(void)
  # define STEN_LOW  sten_low()
  static void sten_low(void)
  {
-	aztTimeOutCount = 0;
+	aztTimeOut = jiffies + 2;
  	do {
  		aztIndatum = inb(STATUS_PORT);
-		aztTimeOutCount++;
-		if (aztTimeOutCount >= AZT_TIMEOUT) {
+		if (time_after(jiffies, aztTimeOut)) {
  			if (azt_init_end)
  				printk
  				    ("aztcd: Error Wait STEN_LOW commands:%x\n",
  				     aztCmd);
  			break;
  		}
+		schedule_timeout(1);
  	} while (aztIndatum & AFL_STATUS);
  }
 

@@ -411,14 +412,14 @@ static void sten_low(void)
  # define DTEN_LOW dten_low()
  static void dten_low(void)
  {
-	aztTimeOutCount = 0;
+	aztTimeOut = jiffies + 2;
  	do {
  		aztIndatum = inb(STATUS_PORT);
-		aztTimeOutCount++;
-		if (aztTimeOutCount >= AZT_TIMEOUT) {
+		if (time_after(jiffies, aztTimeOut)) {
  			printk("aztcd: Error Wait DTEN_OK\n");
  			break;
  		}
+		schedule_timeout(1);
  	} while (aztIndatum & AFL_DATA);
  }
 

@@ -520,7 +521,8 @@ static int sendAztCmd(int cmd, struct az
  	       params->end.min, params->end.sec, params->end.frame);
  #endif
  	for (retry = 0; retry < AZT_RETRY_ATTEMPTS; retry++) {
-		aztSendCmd(cmd);
+		if (aztSendCmd(cmd))
+			continue;
  		outb(params->start.min, CMD_PORT);
  		outb(params->start.sec, CMD_PORT);
  		outb(params->start.frame, CMD_PORT);

@@ -560,7 +562,8 @@ static int aztSeek(struct azt_Play_msf *
  	       params->start.min, params->start.sec, params->start.frame);
  #endif
  	for (retry = 0; retry < AZT_RETRY_ATTEMPTS; retry++) {
-		aztSendCmd(ACMD_SEEK);
+		if (aztSendCmd(ACMD_SEEK))
+			continue;
  		outb(params->start.min, CMD_PORT);
  		outb(params->start.sec, CMD_PORT);
  		outb(params->start.frame, CMD_PORT);

@@ -595,7 +598,8 @@ static int aztSetDiskType(int type)
  	printk("aztcd: set disk type command: type= %i\n", type);
  #endif
  	for (retry = 0; retry < AZT_RETRY_ATTEMPTS; retry++) {
-		aztSendCmd(ACMD_SET_DISK_TYPE);
+		if (aztSendCmd(ACMD_SET_DISK_TYPE))
+			continue;
  		outb(type, CMD_PORT);
  		STEN_LOW;
  		data = inb(DATA_PORT);

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

#1 lore raw
Quoted 34 lines · Daniel Marjamäki

Summary of changes:
* long busy loops now sleeps between each repetition.
* using jiffies for timeout rather than repetitions.
* The return code of aztSendCmd is now checked.

Signed-off-by Daniel Marjamäki

diff -up a/drivers/cdrom/aztcd.c b/drivers/cdrom/aztcd.c

drivers/cdrom/aztcd.c 2005-11-19 08:06:18.000000000 +0100 renamed from drivers/cdrom/aztcd.c 2005-11-17 11:39:53.000000000 +0100 +2 -2
@@ -179,6 +179,7 @@
  #include <linux/ioport.h>
  #include <linux/string.h>
  #include <linux/major.h>
+#include <linux/jiffies.h>
 
  #include <linux/init.h>
 

@@ -308,7 +309,7 @@ static char aztDiskChanged = 1;
  static char aztTocUpToDate = 0;
 
  static unsigned char aztIndatum;
-static unsigned long aztTimeOutCount;
+static unsigned long aztTimeOutCount, aztTimeOut;
  static int aztCmd = 0;
 
  static DEFINE_SPINLOCK(aztSpin);

@@ -361,14 +362,14 @@ static int azt_bcd2bin(unsigned char bcd
  # define OP_OK op_ok()
  static void op_ok(void)
  {
-       aztTimeOutCount = 0;

drivers/cdrom/aztcd.c 2005-11-19 08:06:18.000000000 +0100 static int azt_bcd2bin(unsigned char bcd · line 372

Quoted 10 lines
drivers/cdrom/aztcd.c 2005-11-19 08:06:18.000000000 +0100 renamed from drivers/cdrom/aztcd.c 2005-11-17 11:39:53.000000000 +0100 +3 -2
+       aztTimeOut = jiffies + 2;
        do {
                aztIndatum = inb(DATA_PORT);
-               aztTimeOutCount++;
-               if (aztTimeOutCount >= AZT_TIMEOUT) {
+               if (time_after(jiffies, aztTimeOut)) {
                        printk("aztcd: Error Wait OP_OK\n");
                        break;
                }
+               schedule_timeout(1);

Nope, you *must* set the state before calling schedule_timeout. We have even simplified this further by adding schedule_timeout_interruptible() and schedule_timeout_uninterruptible(). Same for all the others.

<snip>

drivers/cdrom/aztcd.c 2005-11-19 08:06:18.000000000 +0100 static int sendAztCmd(int cmd, struct az · line 525

Quoted 7 lines
drivers/cdrom/aztcd.c 2005-11-19 08:06:18.000000000 +0100 renamed from drivers/cdrom/aztcd.c 2005-11-17 11:39:53.000000000 +0100 +2 -1
@@ -520,7 +521,8 @@ static int sendAztCmd(int cmd, struct az
               params->end.min, params->end.sec, params->end.frame);
  #endif
        for (retry = 0; retry < AZT_RETRY_ATTEMPTS; retry++) {
-               aztSendCmd(cmd);
+               if (aztSendCmd(cmd))
+                       continue;

Is this fixing a bug you've actually seen? Otherwise you're changing functionality without a real reason.

Thanks,
Nish

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

author
#2 lore raw
In reply to Daniel Marjamäki

Thanks for the feedback Nish!

Quoted 4 lines

Nope, you *must* set the state before calling schedule_timeout. We have even simplified this further by adding schedule_timeout_interruptible() and schedule_timeout_uninterruptible(). Same for all the others.

I wasn't aware of that. I've changed to schedule_timeout_interruptible.. I cannot find any attempts to disable interrupts during the busy loops in the original code. Therefore I think it should be safe to say "interruptible".

Quoted 2 lines

Is this fixing a bug you've actually seen? Otherwise you're changing functionality without a real reason.

I thought it was a bad idea not to handle errors.
But I can really see your point and therefore I removed these changes from my patch.

The new suggested patch looks like this:
Changes:
* Sleep during busy loops
* Time out is based on elapsed time

drivers/cdrom/aztcd.c +14 -13
@@ -179,6 +179,7 @@
 #include <linux/ioport.h>
 #include <linux/string.h>
 #include <linux/major.h>
+#include <linux/jiffies.h>
 
 #include <linux/init.h>
 

@@ -308,7 +309,7 @@ static char aztDiskChanged = 1;
 static char aztTocUpToDate = 0;
 
 static unsigned char aztIndatum;
-static unsigned long aztTimeOutCount;
+static unsigned long aztTimeOutCount, aztTimeOut;
 static int aztCmd = 0;
 
 static DEFINE_SPINLOCK(aztSpin);

@@ -361,14 +362,14 @@ static int azt_bcd2bin(unsigned char bcd
 # define OP_OK op_ok()
 static void op_ok(void)
 {
-	aztTimeOutCount = 0;
+	aztTimeOut = jiffies + 2;
 	do {
 		aztIndatum = inb(DATA_PORT);
-		aztTimeOutCount++;
-		if (aztTimeOutCount >= AZT_TIMEOUT) {
+		if (time_after(jiffies, aztTimeOut)) {
 			printk("aztcd: Error Wait OP_OK\n");
 			break;
 		}
+		schedule_timeout_interruptible(1);
 	} while (aztIndatum != AFL_OP_OK);
 }
 

@@ -377,14 +378,14 @@ static void op_ok(void)
 # define PA_OK pa_ok()
 static void pa_ok(void)
 {
-	aztTimeOutCount = 0;
+	aztTimeOut = jiffies + 2;
 	do {
 		aztIndatum = inb(DATA_PORT);
-		aztTimeOutCount++;
-		if (aztTimeOutCount >= AZT_TIMEOUT) {
+		if (time_after(jiffies, aztTimeOut)) {
 			printk("aztcd: Error Wait PA_OK\n");
 			break;
 		}
+		schedule_timeout_interruptible(1);
 	} while (aztIndatum != AFL_PA_OK);
 }
 #endif

@@ -393,17 +394,17 @@ static void pa_ok(void)
 # define STEN_LOW  sten_low()
 static void sten_low(void)
 {
-	aztTimeOutCount = 0;
+	aztTimeOut = jiffies + 2;
 	do {
 		aztIndatum = inb(STATUS_PORT);
-		aztTimeOutCount++;
-		if (aztTimeOutCount >= AZT_TIMEOUT) {
+		if (time_after(jiffies, aztTimeOut)) {
 			if (azt_init_end)
 				printk
 				    ("aztcd: Error Wait STEN_LOW commands:%x\n",
 				     aztCmd);
 			break;
 		}
+		schedule_timeout_interruptible(1);
 	} while (aztIndatum & AFL_STATUS);
 }
 

@@ -411,14 +412,14 @@ static void sten_low(void)
 # define DTEN_LOW dten_low()
 static void dten_low(void)
 {
-	aztTimeOutCount = 0;
+	aztTimeOut = jiffies + 2;
 	do {
 		aztIndatum = inb(STATUS_PORT);
-		aztTimeOutCount++;
-		if (aztTimeOutCount >= AZT_TIMEOUT) {
+		if (time_after(jiffies, aztTimeOut)) {
 			printk("aztcd: Error Wait DTEN_OK\n");
 			break;
 		}
+		schedule_timeout_interruptible(1);
 	} while (aztIndatum & AFL_DATA);
 }

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

#3 lore raw
In reply to Daniel Marjamäki
Quoted 5 lines · Daniel Marjamäki

Thanks for the feedback Nish!

Quoted 8 lines
Quoted 4 lines

Nope, you *must* set the state before calling schedule_timeout. We have even simplified this further by adding schedule_timeout_interruptible() and schedule_timeout_uninterruptible(). Same for all the others.

I wasn't aware of that. I've changed to schedule_timeout_interruptible.. I cannot find any attempts to disable interrupts during the busy loops in the original code. Therefore I think it should be safe to say "interruptible".

Looks good.

Quoted 6 lines
Quoted 2 lines

Is this fixing a bug you've actually seen? Otherwise you're changing functionality without a real reason.

I thought it was a bad idea not to handle errors.
But I can really see your point and therefore I removed these changes from my patch.

Certainly generally you are correct. But the kernel is a sensitive beast. Does this driver have a maintainer? May be worth posting to LKML if not asking if anyone uses the driver (perhaps referring to the appropriate CONFIG_ option(s) that enable it), and asking if they would be willing to test your patch.

Quoted 4 lines

The new suggested patch looks like this:
Changes:
* Sleep during busy loops
* Time out is based on elapsed time

Patch looks good, but is AZT_TIMEOUT used anywhere now (can we get rid of it completely)?

Thanks,
Nish

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors