Avoid calling pthread_join() to wait for USB MSC thread to terminate: This does not work if the caller of usb_mscuninitialize() is in a different task group than the MSC thread. From David Sidrane

4 files changed, 73 insertions, 28 deletions

diff --git a/nuttx/ChangeLog b/nuttx/ChangeLog
index 7b6d5a05d..c129d79e8 100644
--- a/nuttx/ChangeLog
+++ b/nuttx/ChangeLog
@@ -5938,3 +5938,9 @@
 	* fs/fat/fs_fat32util.c:  In one error return case, the error return
 	  value was not being set, making the failure look like success. From
 	  David Sidrane (2011-10-1).
+	* drivers/usbdev/usbmsc.c and usbmsc_scsi.c:  pthread_join() does not
+	  work if called from a different task group than the pthread.  This
+	  is correct behavior, but a problem.  The correct solution would be
+	  configure the USB MSC thread to a task, however, this workaround
+	  from David Sidrane plugs the hole for now (2013-11-1).
+
diff --git a/nuttx/drivers/usbdev/usbmsc.c b/nuttx/drivers/usbdev/usbmsc.c
index 5f1b86802..710b3ba73 100644
--- a/nuttx/drivers/usbdev/usbmsc.c
+++ b/nuttx/drivers/usbdev/usbmsc.c
@@ -1628,6 +1628,17 @@ int usbmsc_exportluns(FAR void *handle)
       goto errout_with_mutex;
     }
 
+  /* Detach the pthread so that we do not create a memory leak.
+   *
+   * REVISIT:  See related comments in usbmsc_uninitialize()
+   */
+
+  ret = pthread_detach(priv->thread);
+  if (ret != OK)
+    {
+      usbtrace(TRACE_CLSERROR(USBMSC_TRACEERR_DETACH), (uint16_t)-ret);
+    }
+
   /* Register the USB storage class driver (unless we are part of a composite device) */
 
 #ifndef CONFIG_USBMSC_COMPOSITE
@@ -1752,7 +1763,33 @@ void usbmsc_uninitialize(FAR void *handle)
        * garbage
        */
 
+#if 0
+      /* REVISIT:  pthread_join does not work in all contexts.  In
+       * particular, if usbmsc_uninitialize() executes in a different
+       * task group than the group that includes the SCSI thread, then
+       * pthread_join will fail.
+       *
+       * NOTE: If, for some reason, you wanted to restore this code,
+       * there is now a matching pthread_detach() elsewhere to prevent
+       * memory leaks.
+       */
+
       (void)pthread_join(priv->thread, &value);
+
+#else
+      /* REVISIT:  Calling pthread_mutex_lock and pthread_cond_wait
+       * from outside of the task group is equally non-standard.
+       * However, this actually works.
+       */
+
+      pthread_mutex_lock(&priv->mutex);
+      while ((priv->theventset & USBMSC_EVENT_TERMINATEREQUEST) != 0)
+        {
+          pthread_cond_wait(&priv->cond, &priv->mutex);
+        }
+
+      pthread_mutex_unlock(&priv->mutex);
+#endif
     }
 
   priv->thread = 0;
diff --git a/nuttx/drivers/usbdev/usbmsc_scsi.c b/nuttx/drivers/usbdev/usbmsc_scsi.c
index 7791141d8..44475311b 100644
--- a/nuttx/drivers/usbdev/usbmsc_scsi.c
+++ b/nuttx/drivers/usbdev/usbmsc_scsi.c
@@ -85,7 +85,7 @@
  * "seems to relate to stalling the endpoint when a short response is
  * generated which causes a residue to exist when the CSW would be returned.
  * I think there's two issues here.  The first being if the transfer which
- * had just been queued before the stall had not completed then it wouldn’t
+ * had just been queued before the stall had not completed then it wouldn't
  * then complete once the endpoint was stalled?  The second is that the
  * subsequent transfer for the CSW would be dropped on the floor (by the
  * epsubmit() function) if the end point was still stalled as the control
@@ -2678,5 +2678,6 @@ void *usbmsc_workerthread(void *arg)
   /* Transition to the TERMINATED state and exit */
 
   priv->thstate = USBMSC_STATE_TERMINATED;
+  pthread_cond_signal(&priv->cond);             /* See comments in usbmsc_uninitialize() */
   return NULL;
 }
diff --git a/nuttx/include/nuttx/usb/usbdev_trace.h b/nuttx/include/nuttx/usb/usbdev_trace.h
index 5a5d40670..2e210ac07 100644
--- a/nuttx/include/nuttx/usb/usbdev_trace.h
+++ b/nuttx/include/nuttx/usb/usbdev_trace.h
@@ -365,33 +365,34 @@
 #define USBMSC_TRACEERR_SNDSTATUSSUBMIT              0x00da
 #define USBMSC_TRACEERR_SYNCCACHEMEDIANOTPRESENT     0x00db
 #define USBMSC_TRACEERR_THREADCREATE                 0x00dc
-#define USBMSC_TRACEERR_TOOMANYLUNS                  0x00dd
-#define USBMSC_TRACEERR_UNBINDINVALIDARGS            0x00de
-#define USBMSC_TRACEERR_UNBINDLUNINVALIDARGS1        0x00df
-#define USBMSC_TRACEERR_UNBINDLUNINVALIDARGS2        0x00e0
-#define USBMSC_TRACEERR_UNINITIALIZEINVALIDARGS      0x00e1
-#define USBMSC_TRACEERR_UNSUPPORTEDSTDREQ            0x00e2
-#define USBMSC_TRACEERR_VERIFY10FLAGS                0x00e3
-#define USBMSC_TRACEERR_VERIFY10LBARANGE             0x00e4
-#define USBMSC_TRACEERR_VERIFY10MEDIANOTPRESENT      0x00e5
-#define USBMSC_TRACEERR_VERIFY10NOBLOCKS             0x00e6
-#define USBMSC_TRACEERR_VERIFY10READFAIL             0x00e7
-#define USBMSC_TRACEERR_WRALLOCREQ                   0x00e8
-#define USBMSC_TRACEERR_WRCOMPLETEINVALIDARGS        0x00e9
-#define USBMSC_TRACEERR_WRITE10FLAGS                 0x00ea
-#define USBMSC_TRACEERR_WRITE10LBARANGE              0x00eb
-#define USBMSC_TRACEERR_WRITE10MEDIANOTPRESENT       0x00ec
-#define USBMSC_TRACEERR_WRITE10READONLY              0x00ed
-#define USBMSC_TRACEERR_WRITE12FLAGS                 0x00ee
-#define USBMSC_TRACEERR_WRITE12LBARANGE              0x00ef
-#define USBMSC_TRACEERR_WRITE12MEDIANOTPRESENT       0x00f0
-#define USBMSC_TRACEERR_WRITE12READONLY              0x00f1
-#define USBMSC_TRACEERR_WRITE6LBARANGE               0x00f2
-#define USBMSC_TRACEERR_WRITE6MEDIANOTPRESENT        0x00f3
-#define USBMSC_TRACEERR_WRITE6READONLY               0x00f4
-#define USBMSC_TRACEERR_WRSHUTDOWN                   0x00f5
-#define USBMSC_TRACEERR_WRUNEXPECTED                 0x00f6
-#define USBMSC_TRACEERR_UNSUPPORTEDTYPE              0x00f7
+#define USBMSC_TRACEERR_DETACH                       0x00dd
+#define USBMSC_TRACEERR_TOOMANYLUNS                  0x00de
+#define USBMSC_TRACEERR_UNBINDINVALIDARGS            0x00df
+#define USBMSC_TRACEERR_UNBINDLUNINVALIDARGS1        0x00e0
+#define USBMSC_TRACEERR_UNBINDLUNINVALIDARGS2        0x00e1
+#define USBMSC_TRACEERR_UNINITIALIZEINVALIDARGS      0x00ee
+#define USBMSC_TRACEERR_UNSUPPORTEDSTDREQ            0x00e3
+#define USBMSC_TRACEERR_VERIFY10FLAGS                0x00e4
+#define USBMSC_TRACEERR_VERIFY10LBARANGE             0x00e5
+#define USBMSC_TRACEERR_VERIFY10MEDIANOTPRESENT      0x00e6
+#define USBMSC_TRACEERR_VERIFY10NOBLOCKS             0x00e7
+#define USBMSC_TRACEERR_VERIFY10READFAIL             0x00e8
+#define USBMSC_TRACEERR_WRALLOCREQ                   0x00e9
+#define USBMSC_TRACEERR_WRCOMPLETEINVALIDARGS        0x00ea
+#define USBMSC_TRACEERR_WRITE10FLAGS                 0x00eb
+#define USBMSC_TRACEERR_WRITE10LBARANGE              0x00ec
+#define USBMSC_TRACEERR_WRITE10MEDIANOTPRESENT       0x00ed
+#define USBMSC_TRACEERR_WRITE10READONLY              0x00ee
+#define USBMSC_TRACEERR_WRITE12FLAGS                 0x00ef
+#define USBMSC_TRACEERR_WRITE12LBARANGE              0x00f0
+#define USBMSC_TRACEERR_WRITE12MEDIANOTPRESENT       0x00f1
+#define USBMSC_TRACEERR_WRITE12READONLY              0x00f2
+#define USBMSC_TRACEERR_WRITE6LBARANGE               0x00f3
+#define USBMSC_TRACEERR_WRITE6MEDIANOTPRESENT        0x00f4
+#define USBMSC_TRACEERR_WRITE6READONLY               0x00f5
+#define USBMSC_TRACEERR_WRSHUTDOWN                   0x00f6
+#define USBMSC_TRACEERR_WRUNEXPECTED                 0x00f7
+#define USBMSC_TRACEERR_UNSUPPORTEDTYPE              0x00f8
 
 /****************************************************************************
  * Public Types