Re: I made a patch and would like feedback/testers (drivers/cdrom/aztcd.c)

Re: I made a patch and would like feedback/testers (drivers/cdrom/aztcd.c)

[Bodo Eggert]

Daniel Marjamäki <daniel.marjamaki@comhem.se> wrote:

> -     aztTimeOutCount = 0;
> +     aztTimeOut = jiffies + 2;

Different timeout based on HZ seems wrong.
-- 
Ich danke GMX dafür, die Verwendung meiner Adressen mittels per SPF
verbreiteten Lügen zu sabotieren.

Re: I made a patch and would like feedback/testers (drivers/cdrom/aztcd.c)

[Nish Aravamudan]

On 11/20/05, Bodo Eggert <harvested.in.lkml@posting.7eggert.dyndns.org> wrote:
> Daniel Marjamäki <daniel.marjamaki@comhem.se> wrote:
>
> > -     aztTimeOutCount = 0;
> > +     aztTimeOut = jiffies + 2;
>
> Different timeout based on HZ seems wrong.

True; I'm trying to think of a good way to emulate 8000000 iterations
of loop, though. Really, this is not terrible to use 2 jiffies of
offset, as we try to sleep 1 jiffy each time. As long as we don't get
a signal *right* away, we'll sleep probably for 2 loops. Not sure,
though, may be useful to see what happens in practice and then debug
further for the right value.

May also want to use time_after_eq() not time_after().

Thanks,
Nish

Re: I made a patch and would like feedback/testers (drivers/cdrom/aztcd.c)

[Daniel Marjamäki]

Bodo Eggert wrote:
> Daniel Marjamäki <daniel.marjamaki@comhem.se> wrote:
> 
> 
>>-     aztTimeOutCount = 0;
>>+     aztTimeOut = jiffies + 2;
> 
> 
> Different timeout based on HZ seems wrong.

Yes, but..

If I'd say "HZ/100", then all systems that uses my driver must have HZ>=200.

The way I do it:
All systems will give me a delay for at least a few ms.
I get the shortest timeout possible on each computer.

Re: I made a patch and would like feedback/testers (drivers/cdrom/aztcd.c)

[Con Kolivas]

On Mon, 21 Nov 2005 19:09, Daniel Marjamäki wrote:
> Bodo Eggert wrote:
> > Daniel Marjamäki <daniel.marjamaki@comhem.se> wrote:
> >>-     aztTimeOutCount = 0;
> >>+     aztTimeOut = jiffies + 2;
> >
> > Different timeout based on HZ seems wrong.
>
> Yes, but..
>
> If I'd say "HZ/100", then all systems that uses my driver must have
> HZ>=200.
>
> The way I do it:
> All systems will give me a delay for at least a few ms.
> I get the shortest timeout possible on each computer.

Convention in the kernel would be 
	aztTimeOut =  HZ / 100 ? : 1;
to be at least one tick (works for HZ even below 100) and is at least 10ms. If 
you wanted 2 ms then use
	aztTimeOut =  HZ / 500 ? : 1;
which would give you at least 2ms

Cheers,
Con

Re: I made a patch and would like feedback/testers (drivers/cdrom/aztcd.c)

[Daniel Marjamäki]

Con Kolivas wrote:
> 
> Convention in the kernel would be 
> 	aztTimeOut =  HZ / 100 ? : 1;
> to be at least one tick (works for HZ even below 100) and is at least 10ms. If 
> you wanted 2 ms then use
> 	aztTimeOut =  HZ / 500 ? : 1;
> which would give you at least 2ms
> 

Thank you Con for the feedback.

Hmm.. The minimum value should be 2, right?
Otherwise the loop could time out after only a few nanoseconds.. since the loop will then timeout immediately on a clock tick.
Or am I wrong?

Best regards,
Daniel Marjamäki

Re: I made a patch and would like feedback/testers (drivers/cdrom/aztcd.c)

[Con Kolivas]

On Mon, 21 Nov 2005 19:42, Daniel Marjamäki wrote:
> Con Kolivas wrote:
> > Convention in the kernel would be
> > 	aztTimeOut =  HZ / 100 ? : 1;
> > to be at least one tick (works for HZ even below 100) and is at least
> > 10ms. If you wanted 2 ms then use
> > 	aztTimeOut =  HZ / 500 ? : 1;
> > which would give you at least 2ms
>
> Thank you Con for the feedback.
>
> Hmm.. The minimum value should be 2, right?
> Otherwise the loop could time out after only a few nanoseconds.. since the
> loop will then timeout immediately on a clock tick. Or am I wrong?

 	aztTimeOut =  HZ / 500 ? : 1;
Would give you a 2ms timeout on 1000Hz and 500Hz
It would give you 5ms on 250Hz and 10ms on 100Hz

ie the absolute minimum it would be would be 2ms, but it would always be at 
least one timer tick which is longer than 2ms at low HZ values.

Cheers,
Con

Re: I made a patch and would like feedback/testers (drivers/cdrom/aztcd.c)

[Nick Piggin]

Con Kolivas wrote:
> On Mon, 21 Nov 2005 19:42, Daniel Marjamäki wrote:

>>Hmm.. The minimum value should be 2, right?
>>Otherwise the loop could time out after only a few nanoseconds.. since the
>>loop will then timeout immediately on a clock tick. Or am I wrong?
> 
> 
>  	aztTimeOut =  HZ / 500 ? : 1;
> Would give you a 2ms timeout on 1000Hz and 500Hz
> It would give you 5ms on 250Hz and 10ms on 100Hz
> 
> ie the absolute minimum it would be would be 2ms, but it would always be at 
> least one timer tick which is longer than 2ms at low HZ values.
> 

Not true. From 'now', the next timer interrupt is somewhere
between epsilon and 1/HZ seconds away.

Luckily, time_after is < rather than <=, so your aztTimeOut would
actually make Daniel's code wait until a minimum of *two* timer
ticks have elapsed since reading jiffies. So it would manage to
scrape by the values of HZ you quoted.

OTOH, if HZ were between 500 and 1000, it would again be too short
due to truncation.

Better I think would be to use the proper interfaces for converting
msecs to jiffies:

   aztTimeOut = jiffies + msecs_to_jiffies(2);

Nick

-- 
SUSE Labs, Novell Inc.

Send instant messages to your online friends http://au.messenger.yahoo.com 

Re: I made a patch and would like feedback/testers (drivers/cdrom/aztcd.c)

[Nish Aravamudan]

On 11/21/05, Nick Piggin <nickpiggin@yahoo.com.au> wrote:
> Con Kolivas wrote:
> > On Mon, 21 Nov 2005 19:42, Daniel Marjamäki wrote:
>
> >>Hmm.. The minimum value should be 2, right?
> >>Otherwise the loop could time out after only a few nanoseconds.. since the
> >>loop will then timeout immediately on a clock tick. Or am I wrong?
> >
> >
> >       aztTimeOut =  HZ / 500 ? : 1;
> > Would give you a 2ms timeout on 1000Hz and 500Hz
> > It would give you 5ms on 250Hz and 10ms on 100Hz
> >
> > ie the absolute minimum it would be would be 2ms, but it would always be at
> > least one timer tick which is longer than 2ms at low HZ values.
> >
>
> Not true. From 'now', the next timer interrupt is somewhere
> between epsilon and 1/HZ seconds away.
>
> Luckily, time_after is < rather than <=, so your aztTimeOut would
> actually make Daniel's code wait until a minimum of *two* timer
> ticks have elapsed since reading jiffies. So it would manage to
> scrape by the values of HZ you quoted.
>
> OTOH, if HZ were between 500 and 1000, it would again be too short
> due to truncation.
>
> Better I think would be to use the proper interfaces for converting
> msecs to jiffies:
>
>    aztTimeOut = jiffies + msecs_to_jiffies(2);

That's what I get for sleeping. Yes, Nick is right, don't do this
conversion yourself, please, use the interfaces designed exactly to
avoid any confusion / HZ-issues (and if those interfaces have
limitations, as Willy discovered earlier, please fix them).

Thanks,
Nish

Re: I made a patch and would like feedback/testers (drivers/cdrom/aztcd.c)

[Bodo Eggert]

On Mon, 21 Nov 2005, Con Kolivas wrote:
> On Mon, 21 Nov 2005 19:09, Daniel Marjamäki wrote:

> > The way I do it:
> > All systems will give me a delay for at least a few ms.
> > I get the shortest timeout possible on each computer.
> 
> Convention in the kernel would be 
> 	aztTimeOut =  HZ / 100 ? : 1;
> to be at least one tick (works for HZ even below 100) and is at least 10ms.

You have to add one to the timeout, since you might be just before the end 
of the timeslice when the delay starts. aztTimeOut = (HZ / 100 ? : 1) + 1;

-- 
Top 100 things you don't want the sysadmin to say:
11. Can you get VMS for this Sparc thingy?

Re: I made a patch and would like feedback/testers (drivers/cdrom/aztcd.c)

[Nish Aravamudan]

On 11/20/05, Arjan van de Ven <arjan@infradead.org> wrote:
> >   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);
>
> this I think is not quite right; schedule_timeout_*() doesn't do
> anything unless you set current->state to something. And at that point
> you might as well start using msleep()!

Not true, as Thomas points out. You are right for schedule_timeout(),
but that's why we introduced the _interruptible() and
_uninterruptible(). And there are reasons to use schedule_timeout_*()
instead of msleep() [not necessarily in this case, but in general],
specifically the presence of wait-queues.

> but what you're doing is generally a good idea; busy waits as the
> original code did is quite wrong...

I agree, and I recommend Daniel post to LKML to get some testing / see
if anyone actually uses this driver :)

Thanks,
Nish

Re: I made a patch and would like feedback/testers (drivers/cdrom/aztcd.c)

[Thomas Gleixner]

On Sun, 2005-11-20 at 16:38 +0100, Arjan van de Ven wrote:	>   		}
> > +		schedule_timeout_interruptible(1);
> 
> this I think is not quite right; schedule_timeout_*() doesn't do
> anything unless you set current->state to something.

Err, schedule_timeout_(un)interruptible sets current->state.

	tglx

Re: I made a patch and would like feedback/testers (drivers/cdrom/aztcd.c)

[Arjan van de Ven]

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

this I think is not quite right; schedule_timeout_*() doesn't do
anything unless you set current->state to something. And at that point
you might as well start using msleep()!


but what you're doing is generally a good idea; busy waits as the
original code did is quite wrong...

I made a patch and would like feedback/testers (drivers/cdrom/aztcd.c)

[Daniel Marjamäki]

Hello!

I made a patch and I would like feedback.
If you can test the patch.. please let me know.

Background:
There are busy loops that time out after 8 million repetitions.
I made these improvements:
* Sleep during the busy loops
* Time out is based on elapsed time

If this patch is accepted, AZT_TIMEOUT can be removed.

Best regards,
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 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);
  }