Check return from nxsem_wait_initialize()

Resolution of Issue 619 will require multiple steps, this part of the first step in that resolution:  Every call to nxsem_wait_uninterruptible() must handle the return value from nxsem_wait_uninterruptible properly.  This commit is only for those files under drivers/pipes and drivers/wireless.

drivers/pipes/pipe_common.c

@@ -1,36 +1,20 @@
 /****************************************************************************
  * drivers/pipes/pipe_common.c
  *
- *   Copyright (C) 2008-2009, 2011, 2015-2016, 2018 Gregory Nutt. All
- *     rights reserved.
- *   Author: Gregory Nutt <gnutt@nuttx.org>
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.  The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License.  You may obtain a copy of the License at
  *
- * Redistribution and use in source and binary forms, with or without
- * modification, are permitted provided that the following conditions
- * are met:
+ *   http://www.apache.org/licenses/LICENSE-2.0
  *
- * 1. Redistributions of source code must retain the above copyright
- *    notice, this list of conditions and the following disclaimer.
- * 2. Redistributions in binary form must reproduce the above copyright
- *    notice, this list of conditions and the following disclaimer in
- *    the documentation and/or other materials provided with the
- *    distribution.
- * 3. Neither the name NuttX nor the names of its contributors may be
- *    used to endorse or promote products derived from this software
- *    without specific prior written permission.
- *
- * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
- * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
- * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
- * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
- * COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
- * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING,
- * BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS
- * OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED
- * AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
- * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN
- * ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
- * POSSIBILITY OF SUCH DAMAGE.
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
  *
  ****************************************************************************/
 
@@ -79,12 +63,6 @@
 #  define pipe_dumpbuffer(m,a,n)
 #endif
 
-/****************************************************************************
- * Private Function Prototypes
- ****************************************************************************/
-
-static void pipecommon_semtake(sem_t *sem);
-
 /****************************************************************************
  * Private Functions
  ****************************************************************************/
@@ -93,9 +71,9 @@ static void pipecommon_semtake(sem_t *sem);
  * Name: pipecommon_semtake
  ****************************************************************************/
 
-static void pipecommon_semtake(FAR sem_t *sem)
+static int pipecommon_semtake(FAR sem_t *sem)
 {
-  nxsem_wait_uninterruptible(sem);
+  return nxsem_wait_uninterruptible(sem);
 }
 
 /****************************************************************************
@@ -201,7 +179,8 @@ int pipecommon_open(FAR struct file *filep)
   DEBUGASSERT(dev != NULL);
 
   /* Make sure that we have exclusive access to the device structure.  The
-   * nxsem_wait() call should fail only if we are awakened by a signal.
+   * nxsem_wait() call should fail if we are awakened by a signal or if the
+   * thread was canceled.
    */
 
   ret = nxsem_wait(&dev->d_bfsem);
@@ -232,8 +211,9 @@ int pipecommon_open(FAR struct file *filep)
     {
       dev->d_nwriters++;
 
-      /* If this this is the first writer, then the read semaphore indicates the
-       * number of readers waiting for the first writer.  Wake them all up.
+      /* If this this is the first writer, then the read semaphore indicates
+       * the number of readers waiting for the first writer.  Wake them all
+       * up.
        */
 
       if (dev->d_nwriters == 1)
@@ -276,8 +256,8 @@ int pipecommon_open(FAR struct file *filep)
       ret = nxsem_wait(&dev->d_rdsem);
       if (ret < 0)
         {
-          /* The nxsem_wait() call should fail only if we are awakened by
-           * a signal.
+          /* The nxsem_wait() call should fail if we are awakened by a
+           * signal or if the task is canceled.
            */
 
           ferr("ERROR: nxsem_wait failed: %d\n", ret);
@@ -301,6 +281,7 @@ int pipecommon_close(FAR struct file *filep)
   FAR struct inode      *inode = filep->f_inode;
   FAR struct pipe_dev_s *dev   = inode->i_private;
   int                    sval;
+  int                    ret;
 
   DEBUGASSERT(dev && filep->f_inode->i_crefs > 0);
 
@@ -309,7 +290,13 @@ int pipecommon_close(FAR struct file *filep)
    * I've never seen anyone check that.
    */
 
-  pipecommon_semtake(&dev->d_bfsem);
+  ret = pipecommon_semtake(&dev->d_bfsem);
+  if (ret < 0)
+    {
+      /* The close will not be performed if the task was canceled */
+
+      return ret;
+    }
 
   /* Decrement the number of references on the pipe.  Check if there are
    * still outstanding references to the pipe.
@@ -325,8 +312,8 @@ int pipecommon_close(FAR struct file *filep)
 
       if ((filep->f_oflags & O_WROK) != 0)
         {
-          /* If there are no longer any writers on the pipe, then notify all of the
-           * waiting readers that they must return end-of-file.
+          /* If there are no longer any writers on the pipe, then notify all
+           * of the waiting readers that they must return end-of-file.
            */
 
           if (--dev->d_nwriters <= 0)
@@ -424,6 +411,10 @@ ssize_t pipecommon_read(FAR struct file *filep, FAR char *buffer, size_t len)
   ret = nxsem_wait(&dev->d_bfsem);
   if (ret < 0)
     {
+      /* May fail because a signal was received or if the task was
+       * canceled.
+       */
+
       return ret;
     }
 
@@ -456,6 +447,10 @@ ssize_t pipecommon_read(FAR struct file *filep, FAR char *buffer, size_t len)
 
       if (ret < 0 || (ret = nxsem_wait(&dev->d_bfsem)) < 0)
         {
+          /* May fail because a signal was received or if the task was
+           * canceled.
+           */
+
           return ret;
         }
     }
@@ -527,16 +522,16 @@ ssize_t pipecommon_write(FAR struct file *filep, FAR const char *buffer,
     }
 
   /* At present, this method cannot be called from interrupt handlers.  That
-   * is because it calls nxsem_wait (via pipecommon_semtake below) and
-   * nxsem_wait cannot be called from interrupt level.  This actually
-   * happens fairly commonly IF [a-z]err() is called from interrupt handlers
-   * and stdout is being redirected via a pipe.  In that case, the debug
-   * output will try to go out the pipe (interrupt handlers should use the
-   * _err() APIs).
+   * is because it calls nxsem_wait() and nxsem_wait() cannot be called from
+   * interrupt level.  This actually happens fairly commonly IF [a-z]err()
+   * is called from interrupt handlers and stdout is being redirected via a
+   * pipe.  In that case, the debug output will try to go out the pipe
+   * (interrupt handlers should use the _err() APIs).
    *
    * On the other hand, it would be very valuable to be able to feed the pipe
    * from an interrupt handler!  TODO:  Consider disabling interrupts instead
-   * of taking semaphores so that pipes can be written from interrupt handlers
+   * of taking semaphores so that pipes can be written from interrupt
+   * handlers.
    */
 
   DEBUGASSERT(up_interrupt_context() == false);
@@ -546,6 +541,10 @@ ssize_t pipecommon_write(FAR struct file *filep, FAR const char *buffer,
   ret = nxsem_wait(&dev->d_bfsem);
   if (ret < 0)
     {
+      /* May fail because a signal was received or if the task was
+       * canceled.
+       */
+
       return ret;
     }
 
@@ -626,13 +625,23 @@ ssize_t pipecommon_write(FAR struct file *filep, FAR const char *buffer,
               return nwritten;
             }
 
-          /* There is more to be written.. wait for data to be removed from the pipe */
+          /* There is more to be written.. wait for data to be removed fro
+           * the pipe
+           */
 
           sched_lock();
           nxsem_post(&dev->d_bfsem);
-          pipecommon_semtake(&dev->d_wrsem);
+          ret = nxsem_wait(&dev->d_wrsem);
           sched_unlock();
-          pipecommon_semtake(&dev->d_bfsem);
+
+          if (ret < 0 || (ret = nxsem_wait(&dev->d_bfsem)) < 0)
+            {
+              /* Either call nxsem_wait may fail because a signal was
+               * received or if the task was canceled.
+               */
+
+              return nwritten == 0 ? (ssize_t)ret : nwritten;
+            }
         }
     }
 }
@@ -648,14 +657,19 @@ int pipecommon_poll(FAR struct file *filep, FAR struct pollfd *fds,
   FAR struct pipe_dev_s *dev      = inode->i_private;
   pollevent_t            eventset;
   pipe_ndx_t             nbytes;
-  int                    ret      = OK;
+  int                    ret;
   int                    i;
 
   DEBUGASSERT(dev && fds);
 
   /* Are we setting up the poll?  Or tearing it down? */
 
-  pipecommon_semtake(&dev->d_bfsem);
+  ret = pipecommon_semtake(&dev->d_bfsem);
+  if (ret < 0)
+    {
+      return ret;
+    }
+
   if (setup)
     {
       /* This is a request to set up the poll.  Find an available
@@ -778,7 +792,11 @@ int pipecommon_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
     }
 #endif
 
-  pipecommon_semtake(&dev->d_bfsem);
+  ret = pipecommon_semtake(&dev->d_bfsem);
+  if (ret < 0)
+    {
+      return ret;
+    }
 
   switch (cmd)
     {
@@ -851,6 +869,7 @@ int pipecommon_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
         break;
 
       default:
+        ret = -ENOTTY;
         break;
     }
 

drivers/wireless/gs2200m.c

@@ -39,6 +39,7 @@
  * See "GS2200MS2W Adapter Command Reference Guide" for the explanation
  * of AT commands. You can find the document at:
  * https://www.telit.com/m2m-iot-products/wifi-bluetooth-modules/wi-fi-gs2200m/
+ *
  ****************************************************************************/
 
 /****************************************************************************
@@ -672,9 +673,9 @@ static bool _copy_data_from_pkt(FAR struct gs2200m_dev_s *dev,
  * Name: gs2200m_lock
  ****************************************************************************/
 
-static void gs2200m_lock(FAR struct gs2200m_dev_s *dev)
+static int gs2200m_lock(FAR struct gs2200m_dev_s *dev)
 {
-  nxsem_wait_uninterruptible(&dev->dev_sem);
+  return nxsem_wait_uninterruptible(&dev->dev_sem);
 }
 
 /****************************************************************************
@@ -713,6 +714,7 @@ static ssize_t gs2200m_read(FAR struct file *filep, FAR char *buffer,
 {
   FAR struct inode *inode;
   FAR struct gs2200m_dev_s *dev;
+  int ret;
 
   DEBUGASSERT(filep);
   inode = filep->f_inode;
@@ -722,7 +724,15 @@ static ssize_t gs2200m_read(FAR struct file *filep, FAR char *buffer,
 
   ASSERT(1 == len);
 
-  gs2200m_lock(dev);
+  ret = nxsem_wait(dev);
+  if (ret < 0)
+    {
+      /* Return if a signal is received or if the the task was canceled
+       * while we were waiting.
+       */
+
+      return ret;
+    }
 
   ASSERT(0 < _notif_q_count(dev));
   char cid = _notif_q_pop(dev);
@@ -744,7 +754,7 @@ static ssize_t gs2200m_read(FAR struct file *filep, FAR char *buffer,
 static ssize_t gs2200m_write(FAR struct file *filep, FAR const char *buffer,
                              size_t len)
 {
-  return 0;
+  return 0;  /* REVISIT:  Zero is not a legal return value from write() */
 }
 
 /****************************************************************************
@@ -2528,7 +2538,13 @@ static int gs2200m_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
 
   /* Lock the device */
 
-  gs2200m_lock(dev);
+  ret = gs2200m_lock(dev);
+  if (ret < 0)
+    {
+      /* Return only if the task was canceled */
+
+      return ret;
+    }
 
   /* Disable gs2200m irq to poll dready */
 
@@ -2649,7 +2665,13 @@ static int gs2200m_poll(FAR struct file *filep, FAR struct pollfd *fds,
   DEBUGASSERT(inode && inode->i_private);
   dev = (FAR struct gs2200m_dev_s *)inode->i_private;
 
-  gs2200m_lock(dev);
+  ret = gs2200m_lock(dev);
+  if (ret < 0)
+    {
+      /* Return if the task was canceled */
+
+      return ret;
+    }
 
   /* Are we setting up the poll?  Or tearing it down? */
 
@@ -2708,11 +2730,22 @@ static void gs2200m_irq_worker(FAR void *arg)
   char c_cid;
   int n;
   int ec;
+  int ret;
 
   DEBUGASSERT(arg != NULL);
   dev = (FAR struct gs2200m_dev_s *)arg;
 
-  gs2200m_lock(dev);
+  do
+    {
+      ret = gs2200m_lock(dev);
+
+      /* The only failure would be if the worker thread were canceled.  That
+       * is very unlikely, however.
+       */
+
+      DEBUGASSERT(ret == OK || ret == -ECANCELED);
+    }
+  while (ret < 0);
 
   n = dev->lower->dready(&ec);
   wlinfo("== start (dready=%d, ec=%d) \n", n, ec);
@@ -2893,6 +2926,7 @@ static int gs2200m_start(FAR struct gs2200m_dev_s *dev)
 
 #if CONFIG_WL_GS2200M_LOGLEVEL > 0
   /* Set log level */
+
   t = gs2200m_set_loglevel(dev, CONFIG_WL_GS2200M_LOGLEVEL);
   ASSERT(TYPE_OK == t);
 #endif