drivers/serial.c: Fix some race conditions. Some bad things code happen if we lost a USB connection at certain times.

git-svn-id: svn://svn.code.sf.net/p/nuttx/code/trunk@5596 42af7a65-404d-4744-a932-0658087f49c3

2 files changed, 78 insertions, 29 deletions

diff --git a/nuttx/ChangeLog b/nuttx/ChangeLog
index e2e7ae29b..5bdd1aad8 100644
--- a/nuttx/ChangeLog
+++ b/nuttx/ChangeLog
@@ -4097,3 +4097,6 @@
 	  well.  From Petteri Aimonen.
 
 6.26 2013-xx-xx Gregory Nutt <gnutt@nuttx.org>
+
+	* drivers/serial/serial.c:  Correct some race conditions when checking
+	  for disconnection of a removable serial device.
diff --git a/nuttx/drivers/serial/serial.c b/nuttx/drivers/serial/serial.c
index cc1c697c0..68f0f43a2 100644
--- a/nuttx/drivers/serial/serial.c
+++ b/nuttx/drivers/serial/serial.c
@@ -211,27 +211,40 @@ static int uart_putxmitchar(FAR uart_dev_t *dev, int ch)
            */
 
           flags = irqsave();
-          dev->xmitwaiting = true;
 
+#ifdef CONFIG_SERIAL_REMOVABLE
+          /* Check if the removable device is no longer connected while we
+           * have interrupts off.  We do not want the transition to occur
+           * as a race condition before we begin the wait.
+           */
+
+          if (dev->disconnected)
+            {
+              irqrestore(flags);
+              return -ENOTCONN;
+            }
+#endif
           /* Wait for some characters to be sent from the buffer with the TX
            * interrupt enabled.  When the TX interrupt is enabled, uart_xmitchars
            * should execute and remove some of the data from the TX buffer.
            */
 
+          dev->xmitwaiting = true;
           uart_enabletxint(dev);
           ret = uart_takesem(&dev->xmitsem, true);
           uart_disabletxint(dev);
           irqrestore(flags);
 
 #ifdef CONFIG_SERIAL_REMOVABLE
-          /* Check if the removable device is no longer connected */
+          /* Check if the removable device was disconnected while we were
+           * waiting.
+           */
 
           if (dev->disconnected)
             {
               return -ENOTCONN;
             }
 #endif
-
           /* Check if we were awakened by signal. */
 
           if (ret < 0)
@@ -342,7 +355,9 @@ static ssize_t uart_write(FAR struct file *filep, FAR const char *buffer, size_t
 
 #ifdef CONFIG_SERIAL_REMOVABLE
   /* If the removable device is no longer connected, refuse to write to the
-   * device.
+   * device.  This check occurs after taking the xmit.sem because the
+   * disconnection event might have occurred while we were waiting for
+   * access to the transmit buffers.
    */
 
   if (dev->disconnected)
@@ -377,8 +392,12 @@ static ssize_t uart_write(FAR struct file *filep, FAR const char *buffer, size_t
           ret = uart_putxmitchar(dev, ch);
         }
 
-      /* Were we awakened by a signal?  That should be the only condition that
-       * uart_putxmitchar() should return an error.
+      /* uart_putxmitchar() might return an error under one of two
+       * conditions:  (1) The wait for buffer space might have been
+       * interrupted by a signal (ret should be -EINTR), or (2) if
+       * CONFIG_SERIAL_REMOVABLE is defined, then uart_putxmitchar()
+       * might also return if the serial device was disconnected
+       * (wtih -ENOTCONN).
        */
 
       if (ret < 0)
@@ -402,7 +421,7 @@ static ssize_t uart_write(FAR struct file *filep, FAR const char *buffer, size_t
                * set the errno value appropriately).
                */
  
-              nread = -EINTR;
+              nread = ret;
             }
 
           break;
@@ -444,16 +463,6 @@ static ssize_t uart_read(FAR struct file *filep, FAR char *buffer, size_t buflen
       return ret;
     }
 
-#ifdef CONFIG_SERIAL_REMOVABLE
-  /* If the removable device is no longer connected, refuse to read from the device */
-
-  if (dev->disconnected)
-    {
-      uart_givesem(&dev->recv.sem);
-      return -ENOTCONN;
-    }
-#endif
-
   /* Loop while we still have data to copy to the receive buffer.
    * we add data to the head of the buffer; uart_xmitchars takes the
    * data from the end of the buffer.
@@ -461,6 +470,22 @@ static ssize_t uart_read(FAR struct file *filep, FAR char *buffer, size_t buflen
 
   while (recvd < buflen)
     {
+#ifdef CONFIG_SERIAL_REMOVABLE
+      /* If the removable device is no longer connected, refuse to read any
+       * further from the device.
+       */
+
+      if (dev->disconnected)
+        {
+          if (recvd == 0)
+            {
+              recvd = -ENOTCONN;
+            }
+
+          break;
+        }
+#endif
+
       /* Check if there is more data to return in the circular buffer.
        * NOTE: Rx interrupt handling logic may aynchronously increment
        * the head index but must not modify the tail index.  The tail
@@ -509,6 +534,7 @@ static ssize_t uart_read(FAR struct file *filep, FAR char *buffer, size_t buflen
             {
               recvd = -EAGAIN;
             }
+
           break;
        }
 #else
@@ -560,19 +586,34 @@ static ssize_t uart_read(FAR struct file *filep, FAR char *buffer, size_t buflen
                */
 
               flags = irqsave();
-              dev->recvwaiting = true;
-              uart_enablerxint(dev);
 
+#ifdef CONFIG_SERIAL_REMOVABLE
+              /* Check if the removable device is no longer connected while
+               * we have interrupts off.  We do not want the transition to
+               * occur as a race condition before we begin the wait.
+               */
+
+              if (dev->disconnected)
+               {
+                  uart_enablerxint(dev);
+                  irqrestore(flags);
+                  ret = -ENOTCONN;
+                  break;
+                }
+#endif
               /* Now wait with the Rx interrupt re-enabled.  NuttX will
                * automatically re-enable global interrupts when this thread
                * goes to sleep.
                */
 
+              dev->recvwaiting = true;
+              uart_enablerxint(dev);
               ret = uart_takesem(&dev->recvsem, true);
               irqrestore(flags);
 
               /* Was a signal received while waiting for data to be
-               * received?  Was a removable device disconnected?
+               * received?  Was a removable device disconnected while
+               * we were waiting?
                */
 
 #ifdef CONFIG_SERIAL_REMOVABLE
@@ -840,6 +881,7 @@ static int uart_close(FAR struct file *filep)
     {
       uart_shutdown(dev);  /* Disable the UART */
     }
+
   irqrestore(flags);
 
   uart_givesem(&dev->closesem);
@@ -861,15 +903,6 @@ static int uart_open(FAR struct file *filep)
   uint8_t       tmp;
   int           ret;
 
-#ifdef CONFIG_SERIAL_REMOVABLE
-  /* If the removable device is no longer connected, refuse to open the device */
-
-  if (dev->disconnected)
-    {
-      return -ENOTCONN;
-    }
-#endif
-
   /* If the port is the middle of closing, wait until the close is finished.
    * If a signal is received while we are waiting, then return EINTR.
    */
@@ -882,6 +915,19 @@ static int uart_open(FAR struct file *filep)
       return ret;
     }
 
+#ifdef CONFIG_SERIAL_REMOVABLE
+  /* If the removable device is no longer connected, refuse to open the
+   * device.  We check this after obtaining the close semaphore because
+   * we might have been waiting when the device was disconnected.
+   */
+
+  if (dev->disconnected)
+    {
+      ret = -ENOTCONN;
+      goto errout_with_sem;
+    }
+#endif
+
   /* Start up serial port */
   /* Increment the count of references to the device. */