I would appreciate any comments from the janitor@sternweltens list. This is one (of
many) cases where I made a decision about replacing
set_current_state(TASK_INTERRUPTIBLE);
schedule_timeout(some_time);
with
msleep(jiffies_to_msecs(some_time));
msleep() is not exactly the same as the previous code, but I only did
this replacement where I thought long delays were *desired*. If this is
not the case here, then just disregard this patch.
Note: I looked for the appropriate maintainer of this driver, but I did
not find anyone. If someone could tell me who that would be, I would
appreciate it.
Thanks,
Nish
Description: Uses msleep() instead of schedule_timeout() to guarantee
the task delays at least the desired time amount.
Signed-off-by: Nishanth Aravamudan <[email protected]>
Signed-off-by: Maximilian Attems <[email protected]>
---
linux-2.6.9-rc1-bk7-max/drivers/char/ds1620.c | 3 +--
1 files changed, 1 insertion(+), 2 deletions(-)
diff -puN drivers/char/ds1620.c~msleep-drivers_char_ds1620 drivers/char/ds1620.c
--- linux-2.6.9-rc1-bk7/drivers/char/ds1620.c~msleep-drivers_char_ds1620 2004-09-01 19:34:43.000000000 +0200
+++ linux-2.6.9-rc1-bk7-max/drivers/char/ds1620.c 2004-09-01 19:34:43.000000000 +0200
@@ -373,8 +373,7 @@ static int __init ds1620_init(void)
th_start.hi = 1;
ds1620_write_state(&th_start);
- set_current_state(TASK_INTERRUPTIBLE);
- schedule_timeout(2*HZ);
+ msleep(2000);
ds1620_write_state(&th);
_
On Wed, 1 Sep 2004 [email protected] wrote:
>
> I would appreciate any comments from the janitor@sternweltens list. This is one (of
> many) cases where I made a decision about replacing
>
> set_current_state(TASK_INTERRUPTIBLE);
> schedule_timeout(some_time);
>
> with
>
> msleep(jiffies_to_msecs(some_time));
>
> msleep() is not exactly the same as the previous code, but I only did
> this replacement where I thought long delays were *desired*. If this is
> not the case here, then just disregard this patch.
>
> Note: I looked for the appropriate maintainer of this driver, but I did
> not find anyone. If someone could tell me who that would be, I would
> appreciate it.
>
> Thanks,
> Nish
>
>
>
> Description: Uses msleep() instead of schedule_timeout() to guarantee
> the task delays at least the desired time amount.
>
> Signed-off-by: Nishanth Aravamudan <[email protected]>
> Signed-off-by: Maximilian Attems <[email protected]>
>
>
>
> ---
>
> linux-2.6.9-rc1-bk7-max/drivers/char/ds1620.c | 3 +--
> 1 files changed, 1 insertion(+), 2 deletions(-)
>
> diff -puN drivers/char/ds1620.c~msleep-drivers_char_ds1620 drivers/char/ds1620.c
> --- linux-2.6.9-rc1-bk7/drivers/char/ds1620.c~msleep-drivers_char_ds1620 2004-09-01 19:34:43.000000000 +0200
> +++ linux-2.6.9-rc1-bk7-max/drivers/char/ds1620.c 2004-09-01 19:34:43.000000000 +0200
> @@ -373,8 +373,7 @@ static int __init ds1620_init(void)
> th_start.hi = 1;
> ds1620_write_state(&th_start);
>
> - set_current_state(TASK_INTERRUPTIBLE);
> - schedule_timeout(2*HZ);
> + msleep(2000);
>
> ds1620_write_state(&th);
>
I'm wondering if 2000 is really the value we want here. As far as I can
see, the schedule_timeout(2*HZ); line has been there as long back as
since HZ was 100, so back then the delay would have been 200. if 200 is
all it needs, then we are now sleeping 10 times as long as really needed.
What is the argument behind the value used?
--
Jesper Juhl <[email protected]>
On 28/11/04 18:39 +0100, Jesper Juhl wrote:
> > +++ linux-2.6.9-rc1-bk7-max/drivers/char/ds1620.c 2004-09-01 19:34:43.000000000 +0200
> > @@ -373,8 +373,7 @@ static int __init ds1620_init(void)
> > th_start.hi = 1;
> > ds1620_write_state(&th_start);
> >
> > - set_current_state(TASK_INTERRUPTIBLE);
> > - schedule_timeout(2*HZ);
> > + msleep(2000);
> >
> > ds1620_write_state(&th);
> >
> I'm wondering if 2000 is really the value we want here. As far as I can
> see, the schedule_timeout(2*HZ); line has been there as long back as
> since HZ was 100, so back then the delay would have been 200. if 200 is
> all it needs, then we are now sleeping 10 times as long as really needed.
> What is the argument behind the value used?
It's right:
schedule_timeout(2*HZ) sleeps for 2 seconds;
msleep(2000) sleeps for 2000 miliseconds, and does not depend on what
HZ is.
Domen
On Mon, 29 Nov 2004, Domen Puncer wrote:
> On 28/11/04 18:39 +0100, Jesper Juhl wrote:
> > > +++ linux-2.6.9-rc1-bk7-max/drivers/char/ds1620.c 2004-09-01 19:34:43.000000000 +0200
> > > @@ -373,8 +373,7 @@ static int __init ds1620_init(void)
> > > th_start.hi = 1;
> > > ds1620_write_state(&th_start);
> > >
> > > - set_current_state(TASK_INTERRUPTIBLE);
> > > - schedule_timeout(2*HZ);
> > > + msleep(2000);
> > >
> > > ds1620_write_state(&th);
> > >
> > I'm wondering if 2000 is really the value we want here. As far as I can
> > see, the schedule_timeout(2*HZ); line has been there as long back as
> > since HZ was 100, so back then the delay would have been 200. if 200 is
> > all it needs, then we are now sleeping 10 times as long as really needed.
> > What is the argument behind the value used?
>
> It's right:
> schedule_timeout(2*HZ) sleeps for 2 seconds;
> msleep(2000) sleeps for 2000 miliseconds, and does not depend on what
> HZ is.
>
It seems I didn't understand schedule_timeout() properly, thank you for
the clarification.
--
Jesper
On Mon, Nov 29, 2004 at 11:37:48PM +0100, Jesper Juhl wrote:
> On Mon, 29 Nov 2004, Domen Puncer wrote:
> > It's right:
> > schedule_timeout(2*HZ) sleeps for 2 seconds;
> > msleep(2000) sleeps for 2000 miliseconds, and does not depend on what
> > HZ is.
>
> It seems I didn't understand schedule_timeout() properly, thank you for
> the clarification.
As part-author of this driver, and actually of this particular bit
of code, a 2 second delay is intented here. The fan needs to be run
at full power in order to start running, so the idea here is to give
it full power for 2 seconds and then to restore the temperature trip
points to the configured values.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 PCMCIA - http://pcmcia.arm.linux.org.uk/
2.6 Serial core
On Mon, 29 Nov 2004, Russell King wrote:
> On Mon, Nov 29, 2004 at 11:37:48PM +0100, Jesper Juhl wrote:
> > On Mon, 29 Nov 2004, Domen Puncer wrote:
> > > It's right:
> > > schedule_timeout(2*HZ) sleeps for 2 seconds;
> > > msleep(2000) sleeps for 2000 miliseconds, and does not depend on what
> > > HZ is.
> >
> > It seems I didn't understand schedule_timeout() properly, thank you for
> > the clarification.
>
> As part-author of this driver, and actually of this particular bit
> of code, a 2 second delay is intented here. The fan needs to be run
> at full power in order to start running, so the idea here is to give
> it full power for 2 seconds and then to restore the temperature trip
> points to the configured values.
>
That makes sense - thanks.
--
Jesper