[KJ] sc423x's usage of schedule_timeout()

[KJ] sc423x's usage of schedule_timeout()

[Nishanth Aravamudan]

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

Hi,

I am going through the kernel, as part of the Kernel-Janitors TODO and
replacing schedule_timeout() with msleep() and its variants. In this
process, I came across

sound/isa/cs423x/cs4231_lib.c

which contains the following code:

	timeout = HZ / 4 / 2;
	time = 2;
	while (snd_cs4231_in(chip, CS4231_TEST_INIT) & CS4231_CALIB_IN_PROGRESS) {
		set_current_state(TASK_INTERRUPTIBLE);
		time = schedule_timeout(time);
		if (time > 0)
			continue;
		time = 2;
		if (--timeout < 0) {
			snd_printk("mce_down - auto calibration time out (2)\n");
			return;
		}
	}

This code seems pretty strange, as the delay is in terms of time, which
seems to mean the loop (as long as the condition is true, of course)
sleeps for 2 jiffies, then sleeps for another 2, about HZ / 4 / 2 (HZ /
8) times. This results in a maximal delay of HZ / 4 jiffies...

This just seemed like a very strange coding style. Perhaps that is just
me, though.

My first concern, though, is whether perhaps here the time and timeout
values have been switched, as it's unusual to treat any variable whose
value is expressed in terms of HZ as a counter (or really anything
except a time variable).

Please let me know if this code should be left as is, or should be fixed
(and how?).

Thanks,
Nish

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

Re: sc423x's usage of schedule_timeout()

[Clemens Ladisch]

Nishanth Aravamudan wrote:
> sound/isa/cs423x/cs4231_lib.c
>
> which contains the following code:
>
> 	timeout = HZ / 4 / 2;
> 	time = 2;
> 	while (snd_cs4231_in(chip, CS4231_TEST_INIT) & CS4231_CALIB_IN_PROGRESS) {
> 		set_current_state(TASK_INTERRUPTIBLE);
> 		time = schedule_timeout(time);
> 		if (time > 0)
> 			continue;
> 		time = 2;
> 		if (--timeout < 0) {
> 			snd_printk("mce_down - auto calibration time out (2)\n");
> 			return;
> 		}
> 	}
>
> This code seems pretty strange, as the delay is in terms of time, which
> seems to mean the loop (as long as the condition is true, of course)
> sleeps for 2 jiffies, then sleeps for another 2, about HZ / 4 / 2 (HZ /
> 8) times. This results in a maximal delay of HZ / 4 jiffies...

This code was introduced in revision 1.29:
http://cvs.sourceforge.net/viewcvs.py/alsa/alsa-kernel/isa/cs423x/cs4231_lib.c?rev=HEAD&view=log#rev1.29

Originally, there was a single timeout of 250 ms, but it had to be
split up into several waits to be able to check the chip status more
frequently.

There is no reason to use 2 jiffies -- the code was written assuming
HZ=100 (for 20ms waits), but now this isn't the case anyway.

BTW: Using TASK_INTERRUPTIBLE isn't quite correct because we don't
check for signals here.


Regards,
Clemens



-------------------------------------------------------
This SF.Net email is sponsored by:
Sybase ASE Linux Express Edition - download now for FREE
LinuxWorld Reader's Choice Award Winner for best database on Linux.
http://ads.osdn.com/?ad_id=5588&alloc_id=12065&op=click

[KJ] Re: [Alsa-devel] sc423x's usage of schedule_timeout()

[Clemens Ladisch]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1280 bytes --]

Nishanth Aravamudan wrote:
> sound/isa/cs423x/cs4231_lib.c
>
> which contains the following code:
>
> 	timeout = HZ / 4 / 2;
> 	time = 2;
> 	while (snd_cs4231_in(chip, CS4231_TEST_INIT) & CS4231_CALIB_IN_PROGRESS) {
> 		set_current_state(TASK_INTERRUPTIBLE);
> 		time = schedule_timeout(time);
> 		if (time > 0)
> 			continue;
> 		time = 2;
> 		if (--timeout < 0) {
> 			snd_printk("mce_down - auto calibration time out (2)\n");
> 			return;
> 		}
> 	}
>
> This code seems pretty strange, as the delay is in terms of time, which
> seems to mean the loop (as long as the condition is true, of course)
> sleeps for 2 jiffies, then sleeps for another 2, about HZ / 4 / 2 (HZ /
> 8) times. This results in a maximal delay of HZ / 4 jiffies...

This code was introduced in revision 1.29:
http://cvs.sourceforge.net/viewcvs.py/alsa/alsa-kernel/isa/cs423x/cs4231_lib.c?rev=HEAD&view=log#rev1.29

Originally, there was a single timeout of 250 ms, but it had to be
split up into several waits to be able to check the chip status more
frequently.

There is no reason to use 2 jiffies -- the code was written assuming
HZ=100 (for 20ms waits), but now this isn't the case anyway.

BTW: Using TASK_INTERRUPTIBLE isn't quite correct because we don't
check for signals here.


Regards,
Clemens


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

[PATCH] sound/cs4231_lib: replace schedule_timeout() with msleep()

[Nishanth Aravamudan]

On Tue, Nov 02, 2004 at 01:20:04PM +0100, Clemens Ladisch wrote:
> Nishanth Aravamudan wrote:
> > sound/isa/cs423x/cs4231_lib.c
> >
> > which contains the following code:
> >
> > 	timeout = HZ / 4 / 2;
> > 	time = 2;
> > 	while (snd_cs4231_in(chip, CS4231_TEST_INIT) & CS4231_CALIB_IN_PROGRESS) {
> > 		set_current_state(TASK_INTERRUPTIBLE);
> > 		time = schedule_timeout(time);
> > 		if (time > 0)
> > 			continue;
> > 		time = 2;
> > 		if (--timeout < 0) {
> > 			snd_printk("mce_down - auto calibration time out (2)\n");
> > 			return;
> > 		}
> > 	}
> >
> > This code seems pretty strange, as the delay is in terms of time, which
> > seems to mean the loop (as long as the condition is true, of course)
> > sleeps for 2 jiffies, then sleeps for another 2, about HZ / 4 / 2 (HZ /
> > 8) times. This results in a maximal delay of HZ / 4 jiffies...
> 
> This code was introduced in revision 1.29:
> http://cvs.sourceforge.net/viewcvs.py/alsa/alsa-kernel/isa/cs423x/cs4231_lib.c?rev=HEAD&view=log#rev1.29
> 
> Originally, there was a single timeout of 250 ms, but it had to be
> split up into several waits to be able to check the chip status more
> frequently.
> 
> There is no reason to use 2 jiffies -- the code was written assuming
> HZ=100 (for 20ms waits), but now this isn't the case anyway.
> 
> BTW: Using TASK_INTERRUPTIBLE isn't quite correct because we don't
> check for signals here.

Ok, great! Thanks for the information! How about the following patch? I
tried to follow the information you gave me as well as what I saw as the
original intent of the code. Of course, I am open to changes... Also,
there is similar code in sound/sparc/cs4231.c. Can similar changes be
made there? It seems like it should be possible, as long as I keep the
irq_* functions in place.

Finally, I used TASK_UNINTERRUPTIBLE (i.e. msleep()). This, though, will
guarantee that the task sleeps *at least* the requested time (20 ms per
iteration). This may lead to a longer delay than desired, for instance,
if perhaps the condition should be checked after each signal. I can make
this change, of course, and use msleep_interruptible() instead, which to
me makes a little more sense...

-Nish

Description: Uses msleep() instead of schedule_timeout() to guarantee
the task delays as expected. This lead to several related changes, as
the current code assumes the value of HZ is 100. Use timeout as an
iteration variable to count out how many 20ms delays should be used.

Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>


--- 2.6.10-rc1-vanilla/sound/isa/cs423x/cs4231_lib.c	2004-10-30 15:34:19.000000000 -0700
+++ 2.6.10-rc1/sound/isa/cs423x/cs4231_lib.c	2004-11-02 11:27:40.000000000 -0800
@@ -390,34 +390,26 @@ void snd_cs4231_mce_down(cs4231_t *chip)
 #if 0
 	printk("(2) timeout = %i, jiffies = %li\n", timeout, jiffies);
 #endif
-	timeout = HZ / 4 / 2;
-	time = 2;
+	/* in 20 ms increments, check condition, up to 260 ms */
+	timeout = 8;
 	while (snd_cs4231_in(chip, CS4231_TEST_INIT) & CS4231_CALIB_IN_PROGRESS) {
-		set_current_state(TASK_INTERRUPTIBLE);
-		time = schedule_timeout(time);
-		if (time > 0)
-			continue;
-		time = 2;
 		if (--timeout < 0) {
 			snd_printk("mce_down - auto calibration time out (2)\n");
 			return;
 		}
+		msleep(20);
 	}
 #if 0
 	printk("(3) jiffies = %li\n", jiffies);
 #endif
-	timeout = HZ / 10 / 2;
-	time = 2;
+	/* in 20 ms increments, check condition, up to 100 ms */
+	timeout = 5;
 	while (cs4231_inb(chip, CS4231P(REGSEL)) & CS4231_INIT) {
-		set_current_state(TASK_INTERRUPTIBLE);		
-		time = schedule_timeout(time);
-		if (time > 0)
-			continue;
-		time = 2;
 		if (--timeout < 0) {
 			snd_printk(KERN_ERR "mce_down - auto calibration time out (3)\n");
 			return;
 		}
+		msleep(20);
 	}
 #if 0
 	printk("(4) jiffies = %li\n", jiffies);


-------------------------------------------------------
This SF.Net email is sponsored by:
Sybase ASE Linux Express Edition - download now for FREE
LinuxWorld Reader's Choice Award Winner for best database on Linux.
http://ads.osdn.com/?ad_id=5588&alloc_id=12065&op=click