Messages in this thread Patch in this message |  | | | From | Julie Brandon <> | | Subject | Bug in 2.0.35; vt.c (bad problem with console beep semaphore code) | | Date | Fri, 24 Jul 1998 00:51:42 +0100 (BST) |
| |
Hiyas!
[This e-mail includes full details of a bug + a suggested patch, I'm not on the list myself so if you want to contact me please feel free to e-mail me. Note: the patch is at the very end of this text.... search for START CUT to jump there now.] Sorry if you folks already know this, but I've otherwise not managed to find out any reference to this bug on the net anywhere, so I though I better tell you folks.... In the latest stable kernel release, 2.0.35, there seems to be a new bug in the console beeping code -- introduced by the new fix in the console beep code! Ooops!!!!! The problem is, the new semaphore mechanism has a small mistake which prevents the console beep sound being turned off by KIOCSOUND if it was initiated with KIOCSOUND (ticks of 0). This does not effect the normal 'ASCII 7 bell' console beep (which calls the functions designed to beep only for a set period -- using a timer to call the routine to stop the sound), only sound initiated and stoped with KIOCSOUND.
For an example, if you have "minicom", try firing that up and connecting to something. The final "I'm connected" 'beep' just stays on indefinitely! Oooooow!!!! (You can shut it up by making a console beep.) (If you don't have minicom and are desperate for a simple example, e-mail me and I'll forward some example 'C' code you can try.)
Whoooops. *8-)
The bug is a terribly simple one, and a simple one to correct as well.
The following is the routine, _kd_mksound in vt.c that KIOCSOUND calls (with a ticks of 0?).
It begins by a test & set of a semaphore to ensure that there's only ever one invocation of this routine running at any one time.
Consider the case where it is called with a frequency of 0, which is used to turn a currently sounding beep off (if previously turned on with KIOCSOUND). It attempts to turn the sound off by calling another routine, kd_nosound. *BUT* in this particular circumstance the call to kd_nosound will *not* turn the sound off...
...cos it itself checks to see if that 'kd_mksound is running' semaphore is set. Which of course it will be, always, during the course of kd_mksound!
void _kd_mksound(unsigned int hz, unsigned int ticks) { static struct timer_list sound_timer = { NULL, NULL, 0, 0, kd_nosound }; unsigned int count = 0;
if (hz > 20 && hz < 32767) count = 1193180 / hz; /* ignore multiple simultaneous requests for sound */ if (!set_bit(0, &mksound_lock)) { ***** ^^^^^^^^^^^^^^^^^^^^^^^^^^ ***** Semaphore test and test /* set_bit in 2.0.x is same as test-and-set in 2.1.x */ 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); ***** ^^^^^^^^^^^^^ ***** If frequency is 0, call kd_nosound in ***** an attempt to stop the current beep ***** ***** *PROBLEM* kd_nosound checks the same semaphore ***** which is set above and not cleared until later in ***** this routine - kd_nosound won't stop the sound if ***** it is set ... which of course it will always be ***** at this point! Ooops. mksound_lock = 0;
***** ^^^^^^^^^^^^^^^^ ***** Clear semaphore } return; }
This is the routine used to stop the beep. Note that before stoping the sound, it checks the semaphore -- the semaphore used to make sure the above routine is not presently running.
static void kd_nosound(unsigned long ignored) { /* if sound is being set up, don't turn it off */ if (!mksound_lock) /* disable counter 2 */ outb(inb_p(0x61)&0xFC, 0x61); return; }
The fix??!?!?!?!
Well, in kd_mksound, you can't just clear the semaphore before calling kd_nosound, as that risks a situation where one task starts a sound right after the semaphore is cleared and before kd_nosound is executed, and then kd_nosound will stop the sound immediately.
I don't think we can remove the semaphore check in kd_nosound either, as that routine can/will be called when the sound timer expires after setting up a timed beep. As such, we need to be sure that the sound timer expiring won't prematurely end the sound just as a new sound is being started up.
What I suggest is to change kd_mksound so that instead of calling kd_nosound(0) when when frequency = 0 (i.e. when count = 0, as per the else statement in kd_mksound) to instead just use the instruction from kd_nosound that stops the sound. We don't have to worry about that semaphore check at this point, as that is already done at the start of kd_mksound, and the semaphore is not cleared until the end of kd_mksound, so we're already guaranteed exclusive access at this point anyway.
[Sorry if I've not made much sense.... the suggested patch probably speaks louder and clearer than my words... *8-) ] Here is my suggested patch-
####################### START CUT ############################ --- linux/drivers/char/vt.c.orig Mon Jul 13 21:47:30 1998 +++ linux/drivers/char/vt.c Fri Jul 24 00:25:19 1998 @@ -190,7 +190,8 @@ add_timer(&sound_timer); } } else - kd_nosound(0); + /* disable counter 2 */ + outb(inb_p(0x61)&0xFC, 0x61); mksound_lock = 0; } ####################### END CUT ############################ Caveat: It does seem that there's another potential problem where the speaker can perhaps be left on indefinitely... but only on the rare occasion where the system was running incredibly sluggish and the length of a beep set very very short...
...what happens if a set-length beep is requested, and the sound timer runs out before the code reaches the part that clears the semaphore - the beep will never be turned off. I've no idea whether this situation is realistic or not though? If so, and assuming I'm guessing what cli() & sti() do correctly (unlikely) - perhaps we could still do with an invocation of cli() just before 'add_timer' & sti() just before return in kd_mksound (noting that cli() & sti() were used in 2.0.34 in this routine but dropped in 2.0.35 - presumably as the author now assumed that blocking interrupts was now a no longer necessary hack)?
Thanks for reading this... and I really hope I've reported this bug correctly and not bothered anyone. *8-X My *sincere* appologies if I've messed up (I'm a linux-kernel-bug-report virgin...)!!!! Take care y'all!!!
Ta-ra, Julie (julie@merp.demon.co.uk)
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.rutgers.edu Please read the FAQ at http://www.altern.org/andrebalsa/doc/lkml-faq.html
|  |