[KJ] [PATCH] drivers/cdrom/aztcd.c: busy loop updates + error

[KJ] [PATCH] drivers/cdrom/aztcd.c: busy loop updates + error

[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

--- a/drivers/cdrom/aztcd.c	2005-11-17 11:39:53.000000000 +0100
+++ b/drivers/cdrom/aztcd.c	2005-11-19 08:06:18.000000000 +0100
@@ -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
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] drivers/cdrom/aztcd.c: busy loop updates + error

[Nish Aravamudan]

On 11/18/05, Daniel Marjamäki <daniel.marjamaki@comhem.se> wrote:
>
> 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
>
> --- a/drivers/cdrom/aztcd.c     2005-11-17 11:39:53.000000000 +0100
> +++ b/drivers/cdrom/aztcd.c     2005-11-19 08:06:18.000000000 +0100
> @@ -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);

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>

> @@ -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
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] drivers/cdrom/aztcd.c: busy loop updates + error

[Daniel Marjamäki]

[-- Attachment #1: Type: text/plain, Size: 3293 bytes --]



Thanks for the feedback Nish!


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


 > 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


--- a/drivers/cdrom/aztcd.c	2005-11-17 11:39:53.000000000 +0100
+++ b/drivers/cdrom/aztcd.c	2005-11-19 17:14:58.000000000 +0100
@@ -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);
  }










[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] drivers/cdrom/aztcd.c: busy loop updates + error

[Nish Aravamudan]

On 11/19/05, Daniel Marjamäki <daniel.marjamaki@comhem.se> wrote:
>
>
> Thanks for the feedback Nish!
>
>
>  > 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.

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

> 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
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors