From: Ben Pfaff <blp@cs.stanford.edu>
Date: Sat, 11 Sep 2004 22:43:06 +0000 (+0000)
Subject: Really make it safe to call printf() from any context.
X-Git-Url: https://pintos-os.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=eb718e3b5a5470b11e58dcc652f79e115272257a;p=pintos-anon

Really make it safe to call printf() from any context.
---

diff --git a/src/devices/intq.c b/src/devices/intq.c
index 7f73991..8754c85 100644
--- a/src/devices/intq.c
+++ b/src/devices/intq.c
@@ -3,7 +3,6 @@
 #include "threads/thread.h"
 
 static int next (int pos);
-static bool owned_by_current_thread (const struct intq *);
 static void wait (struct intq *q, struct thread **waiter);
 static void signal (struct intq *q, struct thread **waiter);
 
@@ -17,29 +16,6 @@ intq_init (struct intq *q, const char *name)
   q->head = q->tail = 0;
 }
 
-/* Locks out other threads from Q (with Q's lock) and interrupt
-   handlers (by disabling interrupts). */
-void
-intq_lock (struct intq *q) 
-{
-  ASSERT (!intr_context ());
-  ASSERT (!owned_by_current_thread (q));
-
-  lock_acquire (&q->lock);
-  q->old_level = intr_disable ();
-}
-
-/* Unlocks Q. */
-void
-intq_unlock (struct intq *q) 
-{
-  ASSERT (!intr_context ());
-  ASSERT (owned_by_current_thread (q));
-
-  lock_release (&q->lock);
-  intr_set_level (q->old_level);
-}
-
 /* Returns true if Q is empty, false otherwise. */
 bool
 intq_empty (const struct intq *q) 
@@ -58,18 +34,22 @@ intq_full (const struct intq *q)
 
 /* Removes a byte from Q and returns it.
    Q must not be empty if called from an interrupt handler.
-   Otherwise, if Q is empty, first waits until a byte is added.
-   Either Q must be locked or we must be in an interrupt
-   handler. */   
+   Otherwise, if Q is empty, first sleeps until a byte is
+   added. */
 uint8_t
 intq_getc (struct intq *q) 
 {
   uint8_t byte;
   
-  ASSERT (owned_by_current_thread (q));
-  while (intq_empty (q))
-    wait (q, &q->not_empty);
-
+  ASSERT (intr_get_level () == INTR_OFF);
+  while (intq_empty (q)) 
+    {
+      ASSERT (!intr_context ());
+      lock_acquire (&q->lock);
+      wait (q, &q->not_empty);
+      lock_release (&q->lock);
+    }
+  
   byte = q->buf[q->tail];
   q->tail = next (q->tail);
   signal (q, &q->not_full);
@@ -78,15 +58,19 @@ intq_getc (struct intq *q)
 
 /* Adds BYTE to the end of Q.
    Q must not be full if called from an interrupt handler.
-   Otherwise, if Q is full, first waits until a byte is removed.
-   Either Q must be locked or we must be in an interrupt
-   handler. */   
+   Otherwise, if Q is full, first sleeps until a byte is
+   removed. */
 void
 intq_putc (struct intq *q, uint8_t byte) 
 {
-  ASSERT (owned_by_current_thread (q));
+  ASSERT (intr_get_level () == INTR_OFF);
   while (intq_full (q))
-    wait (q, &q->not_full);
+    {
+      ASSERT (!intr_context ());
+      lock_acquire (&q->lock);
+      wait (q, &q->not_full);
+      lock_release (&q->lock);
+    }
 
   q->buf[q->head] = byte;
   q->head = next (q->head);
@@ -100,24 +84,13 @@ next (int pos)
   return (pos + 1) % INTQ_BUFSIZE;
 }
 
-/* Returns true if Q is "owned by" the current thread; that is,
-   if Q is locked by the current thread or if we're in an
-   external interrupt handler. */
-static bool
-owned_by_current_thread (const struct intq *q) 
-{
-  return (intr_context ()
-          || (lock_held_by_current_thread (&q->lock)
-              && intr_get_level () == INTR_OFF));
-}
-
 /* WAITER must be the address of Q's not_empty or not_full
    member.  Waits until the given condition is true. */
 static void
 wait (struct intq *q, struct thread **waiter) 
 {
   ASSERT (!intr_context ());
-  ASSERT (owned_by_current_thread (q));
+  ASSERT (intr_get_level () == INTR_OFF);
   ASSERT ((waiter == &q->not_empty && intq_empty (q))
           || (waiter == &q->not_full && intq_full (q)));
 
@@ -132,7 +105,7 @@ wait (struct intq *q, struct thread **waiter)
 static void
 signal (struct intq *q, struct thread **waiter) 
 {
-  ASSERT (owned_by_current_thread (q));
+  ASSERT (intr_get_level () == INTR_OFF);
   ASSERT ((waiter == &q->not_empty && !intq_empty (q))
           || (waiter == &q->not_full && !intq_full (q)));
 
diff --git a/src/devices/intq.h b/src/devices/intq.h
index 161d7eb..2b3edea 100644
--- a/src/devices/intq.h
+++ b/src/devices/intq.h
@@ -7,23 +7,14 @@
 /* An "interrupt queue", a circular buffer shared between
    kernel threads and external interrupt handlers.
 
-   A kernel thread that touches an interrupt queue must bracket
-   its accesses with calls to intq_lock() and intq_unlock().
-   These functions take a lock associated with the queue (which
-   locks out other kernel threads) and disable interrupts (which
-   locks out interrupt handlers).
-
-   An external interrupt handler that touches an interrupt queue
-   need not take any special precautions.  Interrupts are
-   disabled in an external interrupt handler, so other code will
-   not interfere.  The interrupt cannot occur during an update to
-   the interrupt queue by a kernel thread because kernel threads
-   disable interrupts while touching interrupt queues.
+   These functions can be called from kernel threads or from
+   external interrupt handlers.  Except for intq_init(),
+   interrupts must be off in either case.
 
    Incidentally, this has the structure of a "monitor".  Normally
    we'd use locks and condition variables from threads/synch.h to
-   implement a monitor.  Unfortunately, those are intended only
-   to protect kernel threads from one another, not from interrupt
+   implement a monitor, but those are intended only to protect
+   kernel threads from one another, not from interrupt
    handlers. */
 
 /* Queue buffer size, in bytes. */
@@ -32,11 +23,8 @@
 /* A circular queue of bytes. */
 struct intq
   {
-    /* Mutual exclusion. */
-    enum intr_level old_level;  /* Excludes interrupt handlers. */
-    struct lock lock;           /* Excludes kernel threads. */
-
     /* Waiting threads. */
+    struct lock lock;           /* Only one thread may wait at once. */
     struct thread *not_full;    /* Thread waiting for not-full condition. */
     struct thread *not_empty;   /* Thread waiting for not-empty condition. */
 
@@ -47,8 +35,6 @@ struct intq
   };
 
 void intq_init (struct intq *, const char *);
-void intq_lock (struct intq *);
-void intq_unlock (struct intq *);
 bool intq_empty (const struct intq *);
 bool intq_full (const struct intq *);
 uint8_t intq_getc (struct intq *);
diff --git a/src/devices/kbd.c b/src/devices/kbd.c
index ea98358..9cd2840 100644
--- a/src/devices/kbd.c
+++ b/src/devices/kbd.c
@@ -40,11 +40,12 @@ kbd_init (void)
 uint8_t
 kbd_getc (void) 
 {
+  enum intr_level old_level;
   uint8_t key;
-  
-  intq_lock (&buffer);
+
+  old_level = intr_disable ();
   key = intq_getc (&buffer);
-  intq_unlock (&buffer);
+  intr_set_level (old_level);
   
   return key;
 }
diff --git a/src/devices/serial.c b/src/devices/serial.c
index 64ce90f..230a320 100644
--- a/src/devices/serial.c
+++ b/src/devices/serial.c
@@ -15,6 +15,7 @@ static enum { UNINIT, POLL, QUEUE } mode;
 static struct intq txq;
 
 static void set_serial (int bps, int bits, enum parity_type parity, int stop);
+static void putc_poll (uint8_t);
 static void write_ier (void);
 static intr_handler_func serial_interrupt;
 
@@ -30,6 +31,7 @@ serial_init_poll (void)
   outb (FCR_REG, 0);                    /* Disable FIFO. */
   set_serial (9600, 8, NONE, 1);        /* 9600 bps, N-8-1. */
   outb (MCR_REG, 8);                    /* Turn on OUT2 output line. */
+  intq_init (&txq, "serial xmit");
   mode = POLL;
 } 
 
@@ -40,7 +42,6 @@ void
 serial_init_queue (void) 
 {
   ASSERT (mode == POLL);
-  intq_init (&txq, "serial xmit");
   intr_register (0x20 + 4, 0, INTR_OFF, serial_interrupt, "serial");
   mode = QUEUE;
 }
@@ -49,23 +50,36 @@ serial_init_queue (void)
 void
 serial_putc (uint8_t byte) 
 {
-  if (mode == POLL || intr_context ())
+  enum intr_level old_level = intr_disable ();
+
+  if (mode == POLL || old_level == INTR_OFF)
     {
-      /* Poll the serial port until it's ready for a byte, and
-         then transmit. */
-      while ((inb (LSR_REG) & LSR_THRE) == 0)
-        continue;
-      outb (THR_REG, byte);
+      /* If we're not set up for interrupt-driven I/O yet,
+         or if interrupts are off,
+         use dumb polling for serial I/O. */
+      serial_flush ();
+      putc_poll (byte); 
     }
   else
     {
-      /* Lock the queue, add a byte, and update the interrupt
-         enable register. */
-      intq_lock (&txq);
+      /* Otherwise, queue a byte and update the interrupt enable
+         register. */
       intq_putc (&txq, byte); 
       write_ier ();
-      intq_unlock (&txq);
     }
+  
+  intr_set_level (old_level);
+}
+
+/* Flushes anything in the serial buffer out the port in polling
+   mode. */
+void
+serial_flush (void) 
+{
+  enum intr_level old_level = intr_disable ();
+  while (!intq_empty (&txq))
+    putc_poll (intq_getc (&txq));
+  intr_set_level (old_level);
 }
 
 /* Configures the serial port for BPS bits per second, BITS bits
@@ -95,6 +109,19 @@ write_ier (void)
   outb (IER_REG, intq_empty (&txq) ? 0 : IER_XMIT);
 }
 
+
+/* Polls the serial port until it's ready,
+   and then transmits BYTE. */
+static void
+putc_poll (uint8_t byte) 
+{
+  ASSERT (intr_get_level () == INTR_OFF);
+
+  while ((inb (LSR_REG) & LSR_THRE) == 0)
+    continue;
+  outb (THR_REG, byte);
+}
+
 /* Serial interrupt handler.
    As long as we have a byte to transmit,
    and the hardware is ready to accept a byte for transmission,
diff --git a/src/devices/serial.h b/src/devices/serial.h
index 3500ffa..6d49c11 100644
--- a/src/devices/serial.h
+++ b/src/devices/serial.h
@@ -6,5 +6,6 @@
 void serial_init_poll (void);
 void serial_init_queue (void);
 void serial_putc (uint8_t);
+void serial_flush (void);
 
 #endif /* devices/serial.h */