sc423x's usage of schedule_timeout()

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

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

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

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

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

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.

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