[Kernel-janitors] Re: sis-agp: replace schedule_timeout() with

[Kernel-janitors] Re: sis-agp: replace schedule_timeout() with

[Domen Puncer]

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

On 20/07/04 21:01 +0000, Nishanth Aravamudan wrote:
...
> --- linux-vanilla/drivers/char/agp/sis-agp.c	2004-06-16 05:18:56.000000000 +0000
...
> -			set_current_state(TASK_UNINTERRUPTIBLE);
> -			schedule_timeout (1+(HZ*10)/1000);
> +			msleep(HZ);		/* sleep 1s */

Umm... what?
I mean... HZ jiffies is one second, but HZ miliseconds is not necessary 1 s.
And that delay looks wierd, what was the point of HZ*10/1000 in the first
place? Why not HZ/100?

Anyway, IMO msleep(10) seems right.


	Domen

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

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

[Kernel-janitors] Re: sis-agp: replace schedule_timeout() with

[Nishanth Aravamudan]

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

On Wed, Jul 21, 2004 at 12:39:19AM +0200, Domen Puncer wrote:
> On 20/07/04 21:01 +0000, Nishanth Aravamudan wrote:
> ...
> > --- linux-vanilla/drivers/char/agp/sis-agp.c	2004-06-16 05:18:56.000000000 +0000
> ...
> > -			set_current_state(TASK_UNINTERRUPTIBLE);
> > -			schedule_timeout (1+(HZ*10)/1000);
> > +			msleep(HZ);		/* sleep 1s */
> 
> Umm... what?
> I mean... HZ jiffies is one second, but HZ miliseconds is not necessary 1 s.
> And that delay looks wierd, what was the point of HZ*10/1000 in the first
> place? Why not HZ/100?

Thanks again Domen! I totally missed this one - I'm trying to check my
patches as I submit them to the list, but I must not have been paying
close enough attention. You are right, it should be msleep(10). Please
find the corrected patch below. I am not positive as to why the conversion
was originally done the way it was, but I think it prevents rounding
erros. If HZ is very small, then the 1+ guarantees *some* timeout. As to
why they didn't finish their math, well the compiler should take care of
the immediate values (I think) but also, it is a readability thing.
Again, working with msecs directly is clearer. msleep(10) it is.

-Nish


--- linux-vanilla/drivers/char/agp/sis-agp.c	2004-06-16 05:18:56.000000000 +0000
+++ linux-dev/drivers/char/agp/sis-agp.c	2004-07-15 23:11:23.000000000 +0000
@@ -5,6 +5,7 @@
 #include <linux/module.h>
 #include <linux/pci.h>
 #include <linux/init.h>
+#include <linux/delay.h>
 #include <linux/agp_backend.h>
 #include "agp.h"
 
@@ -102,8 +103,7 @@ static void sis_delayed_enable(u32 mode)
 		 */
 		if (device->device == agp_bridge->dev->device) {
 			printk(KERN_INFO PFX "SiS delay workaround: giving bridge time to recover.\n");
-			set_current_state(TASK_UNINTERRUPTIBLE);
-			schedule_timeout (1+(HZ*10)/1000);
+			msleep(10);		/* sleep 10ms */
 		}
 	}
 }

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

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