lkml.org 
[lkml]   [1998]   [May]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: cli/sti in char/vt.c [revised patch]
Hello,

On Fri, 29 May 1998, Andrew Derrick Balsa wrote:

> Hi Pavel,
>
> > Hi!
> >
> > > 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.
> >
> > Don't think so. Well, it is safe on i386, but on other architectures,
> > test_and_set_bit is not guaranteed to be atomic. [And I'm afraid that
> > this code is used on non-i386 but similar hw around machines?]
>
> a) As you wrote above, the code is SMP safe on i386 architectures.
>
> b) The code wraps a PC-specific feature (the speaker port, driven by a
> 8253/82C54 counter/timer chip or equivalent).
>
> > You should better use atomic_t mksound_lock, and
> > atomic_inc/atomic_dec.
> >
> > Correct and post to linus ;-)
>
> No. The patch by Rafael is correct as it stands now.

It is fuctionally correct, yes, but Pavel (kind of) has a point here. We
want readable code that doesn't make unecessary assumptions about the
architecture. I looked at the asm output, using atomic_t cost only one
more instruction.

> First come up with a real-life example of an *SMP* motherboard which
> has:
>
> 1) a PC-style speaker port with an 8253/82C54 chip, and
> 2) two or more non-i386 processors with a non-SMP-friendly
> test_and_set_bit(),

> _then_ the patch will eventually have to be changed.

IMHO, we should have an atomic_test_and_set in atomic.h, so that
architectures that support it can take direct advantage of it. In the x86
it'll just be a wrapper around test_and_set_bit; on others, it can be
simulated with atomic equivalents. (Sounds like an easy project)

> Otherwise your entire argument above is entirely hypothetical.

True, but valid to an extent. If (1) we have an atomic type, and (2) we
need an atomic operation, why not do the right thing? It's not like the
PC speaker is a speed critical piece of hardware ;-)

The revised patch follows [now, how do I get this to Linus?]

Smile :)

--
Rafael

--- linux-2.1.103/drivers/char/vt.c Thu May 28 13:11:21 1998
+++ linux/drivers/char/vt.c Fri May 29 18:49:28 1998
@@ -152,11 +152,16 @@
* We also return immediately, which is what was implied within the X
* comments - KDMKTONE doesn't put the process to sleep.
*/
+
+static atomic_t mksound_lock = ATOMIC_INIT(1);
+
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 (atomic_read(&mksound_lock) == 1)
+ /* disable counter 2 */
+ outb(inb_p(0x61)&0xFC, 0x61);
return;
}

@@ -171,24 +176,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 concurrent requests for sound */
+ if (atomic_dec_and_test(&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);
+
+ atomic_set(&mksound_lock, 1);
+ }
return;
}

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu

\
 
 \ /
  Last update: 2005-03-22 13:42    [W:0.331 / U:0.816 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site