> I have been looking for sources of jiffy inconsistencies (missed timer
> interrupts, mostly), and I think I just found a nasty one: the beep code
> in /drivers/char/vt.c.
>
> First, it has the horrible cli()-sti() pair in function _kd_mksound. I
> checked 2.1.92 and 2.0.33, and both have cli()-sti(). I don't know about
> 2.1.10x kernels. Could you check that for me, since I know you keep your
> kernel at the cutting edge?
>
> Second, interrupts shouldn't be disabled the way they are. There is
> nothing critical at all around some parts of the code.
>
The following patch (against 2.1.103) attempts to fix that. I believe it
also makes it SMP safe, but I only have a UP, so can't test.
For those about to yell that a race exists for port 0x43, between the vt
driver and the timer interrupt, don't. Timer accesses may be interleaved
(as long as it is to different timers!).
Spinlocks seemed to be a monstruous overkill, so I used test_and_set
instead. Interrupts are no longer disabled.
The changes are few, but the identation makes the patch long :-(
Any Comments?
Cheers,
-- Rafael--------------------- begin patch ---------------------------------- --- /usr/src/linux-2.1.103/drivers/char/vt.c Thu May 28 13:11:21 1998 +++ /usr/src/linux/drivers/char/vt.c Thu May 28 17:51:05 1998 @@ -25,6 +25,7 @@ #include <asm/io.h> #include <asm/uaccess.h> +#include <asm/bitops.h> #include <linux/kbd_kern.h> #include <linux/vt_kern.h> @@ -152,11 +153,16 @@ * We also return immediately, which is what was implied within the X * comments - KDMKTONE doesn't put the process to sleep. */ + +static volatile unsigned mksound_lock = 0; + static void kd_nosound(unsigned long ignored) { - /* disable counter 2 */ - outb(inb_p(0x61)&0xFC, 0x61); + /* if sound is being set up, don't turn it off */ + if (!mksound_lock) + /* disable counter 2 */ + outb(inb_p(0x61)&0xFC, 0x61); return; } @@ -171,24 +177,27 @@ if (hz > 20 && hz < 32767) count = 1193180 / hz; - cli(); - del_timer(&sound_timer); - if (count) { - /* enable counter 2 */ - outb_p(inb_p(0x61)|3, 0x61); - /* set command for counter 2, 2 byte write */ - outb_p(0xB6, 0x43); - /* select desired HZ */ - outb_p(count & 0xff, 0x42); - outb((count >> 8) & 0xff, 0x42); - - if (ticks) { - sound_timer.expires = jiffies+ticks; - add_timer(&sound_timer); - } - } else - kd_nosound(0); - sti(); + /* ignore multiple simultaneous requests for sound */ + if (!test_and_set_bit(0, &mksound_lock)) { + del_timer(&sound_timer); + if (count) { + /* enable counter 2 */ + outb_p(inb_p(0x61)|3, 0x61); + /* set command for counter 2, 2 byte write */ + outb_p(0xB6, 0x43); + /* select desired HZ */ + outb_p(count & 0xff, 0x42); + outb((count >> 8) & 0xff, 0x42); + + if (ticks) { + sound_timer.expires = jiffies+ticks; + add_timer(&sound_timer); + } + } else + kd_nosound(0); + + mksound_lock = 0; + } return; }
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.rutgers.edu