Fix syslog mutual exclusion and interrupt level logic

git-svn-id: https://nuttx.svn.sourceforge.net/svnroot/nuttx/trunk@4999 7fd9a85b-ad96-42d3-883c-3090e2eb8679

nuttx/Documentation/NuttxPortingGuide.html

@@ -4101,6 +4101,7 @@ build
   <li>
     <code>CONFIG_SYSLOG_CHAR</code>:
       Enable the generic character device for the SYSLOG.
+      A disadvantage of using the generic character device for the SYSLOG is that it cannot handle debug output generated from interrupt level handlers.
       NOTE:  No more than one SYSLOG device should be configured.
   <p>
     Alternatively, a circular buffer in RAM can be used as the SYSLOGing device.
@@ -4118,11 +4119,11 @@ build
     circular buffer and can be viewed using the NSH 'dmesg' command.
   </li>
   <li>
-    <code>CONFIG_RAMLOG_SYSLOG</code>: Use the RAM logging device for the syslogging
+    <code>CONFIG_RAMLOG_SYSLOG</code>:
-    interface.  If this feature is enabled (along with <code>CONFIG_SYSLOG</code>),
+    Use the RAM logging device for the syslogging interface.
-    then all debug output (only) will be re-directed to the circular
+    If this feature is enabled (along with <code>CONFIG_SYSLOG</code>), then all debug output (only) will be re-directed to the circular buffer in RAM.
-    buffer in RAM.  This RAM log can be view from NSH using the 'dmesg'
+    This RAM log can be view from NSH using the <code>dmesg</code> command.
-    command.
+    NOTE: Unlike the limited, generic character driver SYSLOG device, the RAMLOG <i>can</i> be used to generate debug output from interrupt level handlers.
   </li>
   <li>
     <code>CONFIG_RAMLOG_NPOLLWAITERS</code>: The number of threads than can be waiting

nuttx/configs/README.txt

@@ -381,6 +381,8 @@ defconfig -- This is a configuration file similar to the Linux
     is selected, then these options are also available.
 
     CONFIG_SYSLOG_CHAR - Enable the generic character device for the SYSLOG.
+      A disadvantage of using the generic character device for the SYSLOG is that
+      it cannot handle debug output generated from interrupt level handlers.
       NOTE:  No more than one SYSLOG device should be configured.
 
     CONFIG_RAMLOG - Enables the RAM logging feature. The RAM log is a circular
@@ -395,7 +397,9 @@ defconfig -- This is a configuration file similar to the Linux
       interface.  If this feature is enabled (along with CONFIG_SYSLOG),
       then all debug output (only) will be re-directed to the circular
       buffer in RAM.  This RAM log can be view from NSH using the 'dmesg'
-      command.
+      command.  NOTE:  Unlike the limited, generic character driver SYSLOG
+      device, the RAMLOG *can* be used to generate debug output from interrupt
+      level handlers.
     CONFIG_RAMLOG_NPOLLWAITERS - The number of threads than can be waiting
      for this driver on poll().  Default: 4
 

nuttx/drivers/syslog/README.txt

@@ -14,10 +14,16 @@ syslog_putc().
 One version of syslog_putc() is defined in fs/fs_syslog.c; that version is
 used when CONFIG_SYSLOG_CHAR is defined.  That version of syslog_putc()
 just integrates with the file system to re-direct debug output to a
-character device or to a file.
+character device or to a file. A disadvantage of using the generic character
+device for the SYSLOG is that it cannot handle debug output generated from
+interrupt level handles.
 
 If CONFIG_SYSLOG_CHAR is not defined, then other custom SYSLOG drivers
-can be used.  Those custom SYSLOG drivers reside in this directory.
+can be used.  These custom SYSLOG drivers can do things like handle
+unusual logging media and since they can avoid the general file system
+interfaces, can be designed to support debug output from interrupt handlers.
+
+Those custom SYSLOG drivers reside in this directory.
 
 ramlog.c
 --------
@@ -46,7 +52,9 @@ ramlog.c
       interface.  If this feature is enabled (along with CONFIG_SYSLOG),
       then all debug output (only) will be re-directed to the circular
       buffer in RAM.  This RAM log can be view from NSH using the 'dmesg'
-      command.
+      command.  NOTE:  Unlike the limited, generic character driver SYSLOG
+      device, the RAMLOG *can* be used to generate debug output from interrupt
+      level handlers.
     CONFIG_RAMLOG_NPOLLWAITERS - The number of threads than can be waiting
       for this driver on poll().  Default: 4
 

nuttx/fs/fs_syslog.c

@@ -49,6 +49,7 @@
 #include <assert.h>
 
 #include <nuttx/fs/fs.h>
+#include <nuttx/arch.h>
 #include <nuttx/syslog.h>
 
 #include "fs_internal.h"
@@ -66,6 +67,10 @@
 
 #define SYSLOG_OFLAGS (O_WRONLY | O_CREAT | O_APPEND)
 
+/* An invalid thread ID */
+
+#define NO_HOLDER     ((pid_t)-1)
+
 /****************************************************************************
  * Private Types
  ****************************************************************************/
@@ -75,7 +80,7 @@
 enum syslog_state_e
 {
   SYSLOG_UNINITIALIZED = 0, /* SYSLOG has not been initialized */
-  SYSLOG_BUSY,              /* SYSLOG is not available for this thread */
+  SYSLOG_INITIALIZING,      /* SYSLOG is being initialized */
   SYSLOG_REOPEN,            /* SYSLOG open failed... try again later */
   SYSLOG_FAILURE,           /* SYSLOG open failed... don't try again */
   SYSLOG_OPENED,            /* SYSLOG device is open and ready to use */
@@ -87,6 +92,7 @@ struct syslog_dev_s
 {
   uint8_t     sl_state;     /* See enum syslog_state_e */
   sem_t       sl_sem;       /* Enforces mutually exclusive access */
+  pid_t       sl_holder;    /* PID of the thread that holds the semaphore */
   struct file sl_file;      /* The syslog file structure */
 };
 
@@ -107,6 +113,69 @@ static const uint8_t       g_syscrlf[2]  = { '\r', '\n' };
  * Private Functions
  ****************************************************************************/
 
+/****************************************************************************
+ * Name: syslog_takesem
+ *
+ * Description:
+ *   Write to the syslog device
+ *
+ ****************************************************************************/
+
+static inline int syslog_takesem(void)
+{
+  pid_t me = getpid();
+  int ret;
+
+  /* Does this thread already hold the semaphore?  That could happen if
+   * we wer called recursively, i.e., if the logic kicked off by
+   * syslog_write() where to generate more debug output.  Return an error
+   * in that case.
+   */
+
+  if (g_sysdev.sl_holder == me)
+    {
+      /* Return an error (instead of deadlocking) */
+
+      return -EWOULDBLOCK;
+    }
+
+  /* Either the semaphore is available or is currently held by another
+   * thread.  Wait for it to become available.
+   */
+
+  ret = sem_wait(&g_sysdev.sl_sem);
+  if (ret < 0)
+    {
+      return -get_errno();
+    }
+
+  /* We hold the semaphore.  We can safely mark ourself as the holder
+   * of the semaphore.
+   */
+
+  g_sysdev.sl_holder = me;
+  return OK;
+}
+
+/****************************************************************************
+ * Name: syslog_givesem
+ *
+ * Description:
+ *   Write to the syslog device
+ *
+ ****************************************************************************/
+
+static inline void syslog_givesem(void)
+{
+  pid_t me = getpid();
+  DEBUGASSERT(g_sysdev.sl_holder == me);
+
+  /* Relinquish the semaphore */
+
+  g_sysdev.sl_holder = NO_HOLDER;
+  sem_post(&g_sysdev.sl_sem);
+}
+  
 /****************************************************************************
  * Name: syslog_write
  *
@@ -172,13 +241,13 @@ int syslog_initialize(void)
   int                  ret;
 
   /* At this point, the only expected states are SYSLOG_UNINITIALIZED or
-   * SYSLOG_REOPEN..  Not SYSLOG_BUSY, SYSLOG_FAILURE, SYSLOG_OPENED.
+   * SYSLOG_REOPEN..  Not SYSLOG_INITIALIZING, SYSLOG_FAILURE, SYSLOG_OPENED.
    */
 
   DEBUGASSERT(g_sysdev.sl_state == SYSLOG_UNINITIALIZED ||
               g_sysdev.sl_state == SYSLOG_REOPEN);
 
-  g_sysdev.sl_state = SYSLOG_BUSY;
+  g_sysdev.sl_state = SYSLOG_INITIALIZING;
 
   /* Try to open the device.
    *
@@ -272,7 +341,8 @@ int syslog_initialize(void)
   /* The SYSLOG device is open and ready for writing. */
 
   sem_init(&g_sysdev.sl_sem, 0, 1);
-  g_sysdev.sl_state = SYSLOG_OPENED;
+  g_sysdev.sl_holder = NO_HOLDER;
+  g_sysdev.sl_state  = SYSLOG_OPENED;
   return OK;
 
  errout_with_inode:
@@ -300,27 +370,33 @@ int syslog_putc(int ch)
 
   /* Ignore any output:
    *
-   *   (1) Before the SYSLOG device has been initialized.  This could happen
+   * (1) Before the SYSLOG device has been initialized.  This could happen
-   *       from debug output that occurs early in the boot sequence before
+   *     from debug output that occurs early in the boot sequence before
-   *       syslog_initialize() is called (SYSLOG_UNINITIALIZED_.
+   *     syslog_initialize() is called (SYSLOG_UNINITIALIZED).
-   *   (2) While the device is being initialized.  The case could happen if
+   * (2) While the device is being initialized.  The case could happen if
-   *       debug output is generated while syslog_initialize() executes
+   *     debug output is generated while syslog_initialize() executes
-   *       (SYSLOG_BUSY).
+   *     (SYSLOG_INITIALIZING).
-   *   (2) While we are generating SYSLOG output.  The case could happen if
+   * (3) While we are generating SYSLOG output.  The case could happen if
-   *       debug output is generated while syslog_putc() executes (also
+   *     debug output is generated while syslog_putc() executes
-   *       SYSLOG_BUSY).
+   *     (This case is actually handled inside of syslog_semtake()).
+   * (4) Any debug output generated from interrupt handlers.  A disadvantage
+   *     of using the generic character device for the SYSLOG is that it
+   *     cannot handle debug output generated from interrupt level handlers.
+   * (5) If an irrecoverable failure occurred during initialization.  In
+   *     this case, we won't ever bother to try again (ever).
+   *
+   * NOTE: That the third case is different.  It applies only to the thread
+   * that currently holds the sl_sem sempaphore.  Other threads should wait.
+   * that is why that case is handled in syslog_semtake().
    */
 
   if (g_sysdev.sl_state == SYSLOG_UNINITIALIZED ||
-      g_sysdev.sl_state == SYSLOG_BUSY)
+      g_sysdev.sl_state == SYSLOG_INITIALIZING ||
+      up_interrupt_context())
     {
       return -EAGAIN;
     }
 
-  /* If an irrecoverable failure occurred, then don't bother to try again
-   * (ever)
-   */
-
   if (g_sysdev.sl_state == SYSLOG_FAILURE)
     {
       return -ENXIO;
@@ -369,18 +445,20 @@ int syslog_putc(int ch)
    * value to write.
    */
 
-  ret = sem_wait(&g_sysdev.sl_sem);
+  ret = syslog_takesem();
   if (ret < 0)
     {
-      return -get_errno();
+      /* We probably already hold the semaphore and were probably
+       * re-entered by the logic kicked off by syslog_write().
+       * We might also have been interrupted by a signal.  Either
+       * way, we are outta here.
+       */
+
+      return ret;
     }
 
-  /* Pre-pend a newline with a carriage return.  Setting sl_state to
+  /* Pre-pend a newline with a carriage return. */
-   * SYSLOG_BUSY will suppress any recursive debug logic generated by
-   * the syslog_write call.
-   */
 
-  g_sysdev.sl_state == SYSLOG_BUSY;
   if (ch == '\n')
     {
       /* Write the CR-LF sequence */
@@ -405,8 +483,7 @@ int syslog_putc(int ch)
       nbytes = syslog_write(&ch, 1);
     }
 
-  g_sysdev.sl_state == SYSLOG_OPENED;
+  syslog_givesem();
-  sem_post(&g_sysdev.sl_sem);
 
   /* Check if the write was successful */
 

nuttx/include/nuttx/ramlog.h

@@ -73,7 +73,9 @@
  *   interface.  If this feature is enabled (along with CONFIG_SYSLOG),
  *   then all debug output (only) will be re-directed to the circular
  *   buffer in RAM.  This RAM log can be view from NSH using the 'dmesg'
- *   command.
+ *   command.  NOTE:  Unlike the limited, generic character driver SYSLOG
+ *   device, the RAMLOG *can* be used to generate debug output from interrupt
+ *   level handlers.
  * CONFIG_RAMLOG_NPOLLWAITERS - The number of threads than can be waiting
  *   for this driver on poll().  Default: 4
  *

nuttx/include/nuttx/syslog.h

@@ -59,7 +59,9 @@
  *      described in the include/nuttx/ramlog.h header file.
  *
  *   2. And a generic character device that may be used as the SYSLOG.  The
- *      generic device interfaces are described in this file.
+ *      generic device interfaces are described in this file.  A disadvantage
+ *      of using the generic character device for the SYSLOG is that it
+ *      cannot handle debug output generated from interrupt level handlers.
  *
  * CONFIG_SYSLOG_CHAR - Enable the generic character device for the SYSLOG.
  *   The full path to the SYSLOG device is provided by CONFIG_SYSLOG_DEVPATH.