Reimagine the USB MSC worker thread as a kernel thread (instead of a pthread)

ChangeLog

@@ -7031,3 +7031,17 @@
 	  Kconfig; update naming to include SAM34_ (2014-3-24).
 	* configs/sam4e-ek/include/board.h:  Update HSMCI timing to use the
 	  CLKODD bit (2014-3-24).
+	* drivers/include/mtd/Kconfig, sector512.c, and include/nuttx/mtd/mtd.h:
+	  Add a new MTD driver that can be used to contain another driver and
+	  force its apparent sector size to be 512 bytes (2014-3-24).
+	* arch/arm/src/sam34/sam_lowputc.c sam_serial.c:  Fix a mysterious
+	  multithreading bug that can log up the serial port (2014-3-14).
+	* drivers/usbdev/Kconfig, usbmsc.c, usbmsc.h, and usbmsc_scsi.c:
+	  Redesign threading module used with the USB MSC driver.  It was using
+	  pthreads before and these were changed to a kernel thread.  The reason
+	  for this has to do with task grouping:  A pthread is a memory of the
+	  group of the task that started it.  A kernel thread is independent of
+	  the task that started in (other than knowing it as the parent).  This
+	  allows me to remove so kludge logic to "deparent" the pthread on
+	  startup (2014-3-25).
+

drivers/usbdev/Kconfig

@@ -584,10 +584,25 @@ config USBMSC_VERSIONNO
 	default "0x399"
 
 config USBMSC_REMOVABLE
-	bool "Mass storage remove able"
+	bool "Mass storage removable"
 	default n
 	---help---
 		Select if the media is removable
 		USB Composite Device Configuration
 
+config USBMSC_SCSI_PRIO
+	int "USBMSC SCSI daemon priority"
+	default 128
+	---help---
+		Priority of the SCSI kernel thread.  This must be a relatively high
+		priority so that the SCSI daemon can be response to USB block driver
+		accesses.
+
+config USBMSC_SCSI_STACKSIZE
+	int "USBMSC SCSI daemon stack size"
+	default 2048
+	---help---
+		Stack size used with the SCSI kernel thread.  The default value
+		is not tuned.
+
 endif

drivers/usbdev/usbmsc.c

@@ -66,13 +66,13 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <unistd.h>
-#include <pthread.h>
 #include <string.h>
 #include <errno.h>
 #include <queue.h>
 #include <debug.h>
 
 #include <nuttx/kmalloc.h>
+#include <nuttx/kthread.h>
 #include <nuttx/arch.h>
 #include <nuttx/fs/fs.h>
 #include <nuttx/usb/usb.h>
@@ -160,6 +160,10 @@ static struct usbdevclass_driverops_s g_driverops =
   NULL               /* resume */
 };
 
+/* Used to hand-off the state structure when the SCSI worker thread is started */
+
+FAR struct usbmsc_dev_s *g_usbmsc_handoff;
+
 /****************************************************************************
  * Public Data
  ****************************************************************************/
@@ -642,7 +646,7 @@ static int usbmsc_setup(FAR struct usbdevclass_driver_s *driver,
 
                 priv->theventset |= USBMSC_EVENT_CFGCHANGE;
                 priv->thvalue     = value;
-                pthread_cond_signal(&priv->cond);
+                usbmsc_scsi_signal(priv);
 
                 /* Return here... the response will be provided later by the
                  * worker thread.
@@ -681,7 +685,7 @@ static int usbmsc_setup(FAR struct usbdevclass_driver_s *driver,
                     /* Signal to instantiate the interface change */
 
                     priv->theventset |= USBMSC_EVENT_IFCHANGE;
-                    pthread_cond_signal(&priv->cond);
+                    usbmsc_scsi_signal(priv);
 
                     /* Return here... the response will be provided later by the
                      * worker thread.
@@ -748,7 +752,7 @@ static int usbmsc_setup(FAR struct usbdevclass_driver_s *driver,
                     /* Signal to stop the current operation and reinitialize state */
 
                      priv->theventset |= USBMSC_EVENT_RESET;
-                     pthread_cond_signal(&priv->cond);
+                     usbmsc_scsi_signal(priv);
 
                     /* Return here... the response will be provided later by the
                      * worker thread.
@@ -870,7 +874,7 @@ static void usbmsc_disconnect(FAR struct usbdevclass_driver_s *driver,
   /* Signal the worker thread */
 
   priv->theventset |= USBMSC_EVENT_DISCONNECT;
-  pthread_cond_signal(&priv->cond);
+  usbmsc_scsi_signal(priv);
   irqrestore(flags);
 
   /* Perform the soft connect function so that we will we can be
@@ -1109,7 +1113,7 @@ void usbmsc_wrcomplete(FAR struct usbdev_ep_s *ep, FAR struct usbdev_req_s *req)
   /* Inform the worker thread that a write request has been returned */
 
   priv->theventset |= USBMSC_EVENT_WRCOMPLETE;
-  pthread_cond_signal(&priv->cond);
+  usbmsc_scsi_signal(priv);
 }
 
 /****************************************************************************
@@ -1160,7 +1164,7 @@ void usbmsc_rdcomplete(FAR struct usbdev_ep_s *ep, FAR struct usbdev_req_s *req)
         /* Signal the worker thread that there is received data to be processed */
 
         priv->theventset |= USBMSC_EVENT_RDCOMPLETE;
-        pthread_cond_signal(&priv->cond);
+        usbmsc_scsi_signal(priv);
       }
       break;
 
@@ -1261,6 +1265,26 @@ void usbmsc_deferredresponse(FAR struct usbmsc_dev_s *priv, bool failed)
     }
 }
 
+/****************************************************************************
+ * Name: usbmsc_sync_wait
+ *
+ * Description:
+ *   Wait for the worker thread to obtain the USB MSC state data
+ *
+ ****************************************************************************/
+
+static inline void usbmsc_sync_wait(FAR struct usbmsc_dev_s *priv)
+{
+  int ret;
+
+  do
+    {
+      ret = sem_wait(&priv->thsynch);
+      DEBUGASSERT(ret == OK || errno == EINTR);
+    }
+  while (ret < 0);
+}
+
 /****************************************************************************
  * User Interfaces
  ****************************************************************************/
@@ -1314,8 +1338,9 @@ int usbmsc_configure(unsigned int nluns, void **handle)
   priv = &alloc->dev;
   memset(priv, 0, sizeof(struct usbmsc_dev_s));
 
-  pthread_mutex_init(&priv->mutex, NULL);
+  sem_init(&priv->thsynch, 0, 0);
-  pthread_cond_init(&priv->cond, NULL);
+  sem_init(&priv->thlock, 0, 1);
+  sem_init(&priv->thwaitsem, 0, 0);
   sq_init(&priv->wrreqlist);
 
   priv->nluns = nluns;
@@ -1549,7 +1574,7 @@ int usbmsc_unbindlun(FAR void *handle, unsigned int lunno)
 #endif
 
   lun = &priv->luntab[lunno];
-  pthread_mutex_lock(&priv->mutex);
+  usbmsc_scsi_lock(priv);
 
 #ifdef CONFIG_DEBUG
   if (lun->inode == NULL)
@@ -1566,7 +1591,7 @@ int usbmsc_unbindlun(FAR void *handle, unsigned int lunno)
      ret = OK;
    }
 
-  pthread_mutex_unlock(&priv->mutex);
+  usbmsc_scsi_unlock(priv);
   return ret;
 }
 
@@ -1594,10 +1619,7 @@ int usbmsc_exportluns(FAR void *handle)
   FAR struct usbmsc_dev_s *priv;
   FAR struct usbmsc_driver_s *drvr;
   irqstate_t flags;
-#ifdef SDCC
+  int ret = OK;
-  pthread_attr_t attr;
-#endif
-  int ret;
 
 #ifdef CONFIG_DEBUG
   if (!alloc)
@@ -1610,35 +1632,35 @@ int usbmsc_exportluns(FAR void *handle)
   priv = &alloc->dev;
   drvr = &alloc->drvr;
 
-  /* Start the worker thread */
+  /* Start the worker thread
-
-  pthread_mutex_lock(&priv->mutex);
-  priv->thstate    = USBMSC_STATE_NOTSTARTED;
-  priv->theventset = USBMSC_EVENT_NOEVENTS;
-
-#ifdef SDCC
-  (void)pthread_attr_init(&attr);
-  ret = pthread_create(&priv->thread, &attr, usbmsc_workerthread, (pthread_addr_t)priv);
-#else
-  ret = pthread_create(&priv->thread, NULL, usbmsc_workerthread, (pthread_addr_t)priv);
-#endif
-  if (ret != OK)
-    {
-      usbtrace(TRACE_CLSERROR(USBMSC_TRACEERR_THREADCREATE), (uint16_t)-ret);
-      goto errout_with_mutex;
-    }
-
-  /* Detach the pthread so that we do not create a memory leak.
    *
-   * REVISIT:  See related comments in usbmsc_uninitialize()
+   * REVISIT:  g_usbmsc_handoff is a global and, hence, really requires
+   * some protection against re-entrant usage.
    */
 
-  ret = pthread_detach(priv->thread);
+  usbmsc_scsi_lock(priv);
-  if (ret != OK)
+
+  priv->thstate = USBMSC_STATE_NOTSTARTED;
+  priv->theventset = USBMSC_EVENT_NOEVENTS;
+
+  g_usbmsc_handoff = priv;
+
+  uvdbg("Starting SCSI worker thread\n");
+  priv->thpid = KERNEL_THREAD("scsid", CONFIG_USBMSC_SCSI_PRIO,
+                              CONFIG_USBMSC_SCSI_STACKSIZE,
+                              usbmsc_scsi_main, NULL);
+  if (priv->thpid <= 0)
     {
-      usbtrace(TRACE_CLSERROR(USBMSC_TRACEERR_DETACH), (uint16_t)-ret);
+      usbtrace(TRACE_CLSERROR(USBMSC_TRACEERR_THREADCREATE), (uint16_t)errno);
+      goto errout_with_lock;
     }
 
+  /* Wait for the worker thread to run and initialize */
+
+  uvdbg("Waiting for the SCSI worker thread\n");
+  usbmsc_sync_wait(priv);
+  DEBUGASSERT(g_usbmsc_handoff == NULL);
+
   /* Register the USB storage class driver (unless we are part of a composite device) */
 
 #ifndef CONFIG_USBMSC_COMPOSITE
@@ -1646,19 +1668,20 @@ int usbmsc_exportluns(FAR void *handle)
   if (ret != OK)
     {
       usbtrace(TRACE_CLSERROR(USBMSC_TRACEERR_DEVREGISTER), (uint16_t)-ret);
-      goto errout_with_mutex;
+      goto errout_with_lock;
     }
 #endif
 
   /* Signal to start the thread */
 
+  uvdbg("Signalling for the SCSI worker thread\n");
   flags = irqsave();
   priv->theventset |= USBMSC_EVENT_READY;
-  pthread_cond_signal(&priv->cond);
+  usbmsc_scsi_signal(priv);
   irqrestore(flags);
 
-errout_with_mutex:
+errout_with_lock:
-  pthread_mutex_unlock(&priv->mutex);
+  usbmsc_scsi_unlock(priv);
   return ret;
 }
 
@@ -1742,7 +1765,7 @@ void usbmsc_uninitialize(FAR void *handle)
    * first pass uninitialization.
    */
 
-  if (priv->thread == 0)
+  if (priv->thpid == 0)
     {
       /* In this second and final pass, all that remains to be done is to
        * free the memory resources.
@@ -1759,54 +1782,28 @@ void usbmsc_uninitialize(FAR void *handle)
     {
        /* The thread was started.. Is it still running? */
 
-      pthread_mutex_lock(&priv->mutex);
+      usbmsc_scsi_lock(priv);
       if (priv->thstate != USBMSC_STATE_TERMINATED)
         {
           /* Yes.. Ask the thread to stop */
 
           flags = irqsave();
           priv->theventset |= USBMSC_EVENT_TERMINATEREQUEST;
-          pthread_cond_signal(&priv->cond);
+          usbmsc_scsi_signal(priv);
           irqrestore(flags);
         }
 
-      pthread_mutex_unlock(&priv->mutex);
+      usbmsc_scsi_unlock(priv);
 
-      /* Wait for the thread to exit.  This is necessary even if the
+      /* Wait for the thread to exit */
-       * thread has already exitted in order to collect the join
-       * 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);
+          usbmsc_sync_wait(priv);
         }
-
-      pthread_mutex_unlock(&priv->mutex);
-#endif
     }
 
-  priv->thread = 0;
+  priv->thpid = 0;
 
   /* Unregister the driver (unless we are a part of a composite device) */
 
@@ -1832,14 +1829,15 @@ void usbmsc_uninitialize(FAR void *handle)
 
   /* Uninitialize and release the driver structure */
 
-  pthread_mutex_destroy(&priv->mutex);
+  sem_destroy(&priv->thsynch);
-  pthread_cond_destroy(&priv->cond);
+  sem_destroy(&priv->thlock);
+  sem_destroy(&priv->thwaitsem);
 
 #ifndef CONFIG_USBMSC_COMPOSITE
   /* For the case of the composite driver, there is a two pass
    * uninitialization sequence.  We cannot yet free the driver structure.
    * We will do that on the second pass (and we will know that it is the
-   * second pass because of priv->thread == 0)
+   * second pass because of priv->thpid == 0)
    */
 
   kfree(priv);

drivers/usbdev/usbmsc.h

@@ -47,8 +47,8 @@
 #include <sys/types.h>
 #include <stdint.h>
 #include <stdbool.h>
-#include <pthread.h>
 #include <queue.h>
+#include <semaphore.h>
 
 #include <nuttx/fs/fs.h>
 #include <nuttx/usb/storage.h>
@@ -194,6 +194,16 @@
 #undef CONFIG_USBMSC_CONFIGSTR
 #define CONFIG_USBMSC_CONFIGSTR "Bulk"
 
+/* SCSI daemon */
+
+#ifndef CONFIG_USBMSC_SCSI_PRIO
+#  define CONFIG_USBMSC_SCSI_PRIO 128
+#endif
+
+#ifndef CONFIG_USBMSC_SCSI_STACKSIZE
+#  define CONFIG_USBMSC_SCSI_STACKSIZE 2048
+#endif
+
 /* Debug -- must be consistent with include/debug.h */
 
 #ifdef CONFIG_CPP_HAVE_VARARGS
@@ -443,17 +453,19 @@ struct usbmsc_lun_s
   size_t           nsectors;          /* Number of sectors in the partition */
 };
 
-/* Describes the overall state of the driver */
+/* Describes the overall state of one instance of the driver */
 
 struct usbmsc_dev_s
 {
   FAR struct usbdev_s *usbdev;        /* usbdev driver pointer (Non-null if registered) */
 
-  /* Worker thread interface */
+  /* SCSI worker kernel thread interface */
 
-  pthread_t         thread;           /* The worker thread */
+  pid_t             thpid;            /* The worker thread task ID */
-  pthread_mutex_t   mutex;            /* Mutually exclusive access to resources*/
+  sem_t             thsynch;          /* Used to synchronizer terminal events */
-  pthread_cond_t    cond;             /* Used to signal worker thread */
+  sem_t             thlock;           /* Used to get exclusive access to the state data */
+  sem_t             thwaitsem;        /* Used to signal worker thread */
+  volatile bool     thwaiting;        /* True: worker thread is waiting for an event */
   volatile uint8_t  thstate;          /* State of the worker thread */
   volatile uint16_t theventset;       /* Set of pending events signaled to worker thread */
   volatile uint8_t  thvalue;          /* Value passed with the event (must persist) */
@@ -542,10 +554,55 @@ EXTERN const char g_compserialstr[];
 #define g_mscproductstr g_compproductstr
 #define g_mscserialstr  g_compserialstr
 #endif
+
+/* Used to hand-off the state structure when the SCSI worker thread is started */
+
+EXTERN FAR struct usbmsc_dev_s *g_usbmsc_handoff;
+
 /************************************************************************************
  * Public Function Prototypes
  ************************************************************************************/
 
+/************************************************************************************
+ * Name: usbmsc_scsi_lock
+ *
+ * Description:
+ *   Get exclusive access to SCSI state data.
+ *
+ ****************************************************************************/
+
+void usbmsc_scsi_lock(FAR struct usbmsc_dev_s *priv);
+
+/************************************************************************************
+ * Name: usbmsc_scsi_unlock
+ *
+ * Description:
+ *   Relinquish exclusive access to SCSI state data.
+ *
+ ************************************************************************************/
+
+#define usbmsc_scsi_unlock(priv) sem_post(&priv->thlock)
+
+/************************************************************************************
+ * Name: usbmsc_scsi_signal
+ *
+ * Description:
+ *   Signal the SCSI worker thread that SCSI events need service.
+ *
+ ************************************************************************************/
+
+void usbmsc_scsi_signal(FAR struct usbmsc_dev_s *priv);
+
+/************************************************************************************
+ * Name: usbmsc_synch_signal
+ *
+ * Description:
+ *   ACK controlling tasks request for synchronization.
+ *
+ ************************************************************************************/
+
+#define usbmsc_synch_signal(priv) sem_post(&priv->thsynch)
+
 /************************************************************************************
  * Name: usbmsc_mkstrdesc
  *
@@ -607,7 +664,7 @@ FAR const struct usb_qualdesc_s *usbmsc_getqualdesc(void);
 #endif
 
 /****************************************************************************
- * Name: usbmsc_workerthread
+ * Name: usbmsc_scsi_main
  *
  * Description:
  *   This is the main function of the USB storage worker thread.  It loops
@@ -615,7 +672,7 @@ FAR const struct usb_qualdesc_s *usbmsc_getqualdesc(void);
  *
  ****************************************************************************/
 
-EXTERN void *usbmsc_workerthread(void *arg);
+int usbmsc_scsi_main(int argc, char *argv[]);
 
 /****************************************************************************
  * Name: usbmsc_setconfig
@@ -626,7 +683,7 @@ EXTERN void *usbmsc_workerthread(void *arg);
  *
  ****************************************************************************/
 
-EXTERN int usbmsc_setconfig(FAR struct usbmsc_dev_s *priv, uint8_t config);
+int usbmsc_setconfig(FAR struct usbmsc_dev_s *priv, uint8_t config);
 
 /****************************************************************************
  * Name: usbmsc_resetconfig
@@ -636,7 +693,7 @@ EXTERN int usbmsc_setconfig(FAR struct usbmsc_dev_s *priv, uint8_t config);
  *
  ****************************************************************************/
 
-EXTERN void usbmsc_resetconfig(FAR struct usbmsc_dev_s *priv);
+void usbmsc_resetconfig(FAR struct usbmsc_dev_s *priv);
 
 /****************************************************************************
  * Name: usbmsc_wrcomplete
@@ -647,8 +704,8 @@ EXTERN void usbmsc_resetconfig(FAR struct usbmsc_dev_s *priv);
  *
  ****************************************************************************/
 
-EXTERN void usbmsc_wrcomplete(FAR struct usbdev_ep_s *ep,
+void usbmsc_wrcomplete(FAR struct usbdev_ep_s *ep,
-                              FAR struct usbdev_req_s *req);
+                       FAR struct usbdev_req_s *req);
 
 /****************************************************************************
  * Name: usbmsc_rdcomplete
@@ -659,8 +716,8 @@ EXTERN void usbmsc_wrcomplete(FAR struct usbdev_ep_s *ep,
  *
  ****************************************************************************/
 
-EXTERN void usbmsc_rdcomplete(FAR struct usbdev_ep_s *ep,
+void usbmsc_rdcomplete(FAR struct usbdev_ep_s *ep,
-                              FAR struct usbdev_req_s *req);
+                       FAR struct usbdev_req_s *req);
 
 /****************************************************************************
  * Name: usbmsc_deferredresponse
@@ -684,7 +741,7 @@ EXTERN void usbmsc_rdcomplete(FAR struct usbdev_ep_s *ep,
  *
  ****************************************************************************/
 
-EXTERN void usbmsc_deferredresponse(FAR struct usbmsc_dev_s *priv, bool failed);
+void usbmsc_deferredresponse(FAR struct usbmsc_dev_s *priv, bool failed);
 
 #undef EXTERN
 #if defined(__cplusplus)