ENC28J60: All work is now performed on worker thread, not the in interrupt handler

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

nuttx/configs/fire-stm32v2/nsh/defconfig

@@ -201,7 +201,7 @@ CONFIG_ARCH_STACKDUMP=y
 CONFIG_DRAM_START=0x20000000
 CONFIG_DRAM_SIZE=65536
 CONFIG_ARCH_HAVE_INTERRUPTSTACK=y
-# CONFIG_ARCH_INTERRUPTSTACK is not set
+CONFIG_ARCH_INTERRUPTSTACK=0
 
 #
 # Boot options
@@ -430,7 +430,7 @@ CONFIG_USBMSC_REMOVABLE=y
 # Networking Support
 #
 CONFIG_NET=y
-# CONFIG_NET_NOINTS is not set
+CONFIG_NET_NOINTS=y
 # CONFIG_NET_MULTIBUFFER is not set
 # CONFIG_NET_IPv6 is not set
 CONFIG_NSOCKET_DESCRIPTORS=16

nuttx/drivers/net/enc28j60.c

@@ -110,7 +110,7 @@
 
 /* We need to have the work queue to handle SPI interrupts */
 
-#if !defined(CONFIG_SCHED_WORKQUEUE) && !defined(CONFIG_SPI_OWNBUS)
+#ifndef CONFIG_SCHED_WORKQUEUE
 #  error "Worker thread support is required (CONFIG_SCHED_WORKQUEUE)"
 #endif
 
@@ -122,6 +122,12 @@
 #  define enc_dumppacket(m,a,n)
 #endif
 
+/* The ENC28J60 will not do interrupt level processing */
+
+#ifndef CONFIG_NET_NOINTS
+#  warrning "CONFIG_NET_NOINTS should be set"
+#endif
+
 /* Timing *******************************************************************/
 
 /* TX poll deley = 1 seconds. CLK_TCK is the number of clock ticks per second */
@@ -200,10 +206,10 @@ struct enc_driver_s
   /* If we don't own the SPI bus, then we cannot do SPI accesses from the
    * interrupt handler.
    */
- 
-#ifndef CONFIG_SPI_OWNBUS
-  struct work_s         work;          /* Work queue support */
-#endif
+
+  struct work_s         irqwork;       /* Interrupt continuation work queue support */
+  struct work_s         towork;        /* Tx timeout work queue support */
+  struct work_s         pollwork;      /* Poll timeout work queue support */
 
   /* This is the contained SPI driver intstance */
 
@@ -279,15 +285,17 @@ static void enc_txif(FAR struct enc_driver_s *priv);
 static void enc_txerif(FAR struct enc_driver_s *priv);
 static void enc_txerif(FAR struct enc_driver_s *priv);
 static void enc_rxerif(FAR struct enc_driver_s *priv);
-static void enc_rxdispath(FAR struct enc_driver_s *priv);
+static void enc_rxdispatch(FAR struct enc_driver_s *priv);
 static void enc_pktif(FAR struct enc_driver_s *priv);
-static void enc_worker(FAR void *arg);
+static void enc_irqworker(FAR void *arg);
 static int  enc_interrupt(int irq, FAR void *context);
 
 /* Watchdog timer expirations */
 
-static void enc_polltimer(int argc, uint32_t arg, ...);
+static void enc_toworker(FAR void *arg);
 static void enc_txtimeout(int argc, uint32_t arg, ...);
+static void enc_pollworker(FAR void *arg);
+static void enc_polltimer(int argc, uint32_t arg, ...);
 
 /* NuttX callback functions */
 
@@ -1026,6 +1034,7 @@ static int enc_transmit(FAR struct enc_driver_s *priv)
  *   OK on success; a negated errno on failure
  *
  * Assumptions:
+ *   Interrupts are enabled but the caller holds the uIP lock.
  *
  ****************************************************************************/
 
@@ -1095,6 +1104,7 @@ static void enc_linkstatus(FAR struct enc_driver_s *priv)
  *   None
  *
  * Assumptions:
+ *   Interrupts are enabled but the caller holds the uIP lock.
  *
  ****************************************************************************/
 
@@ -1195,7 +1205,7 @@ static void enc_rxerif(FAR struct enc_driver_s *priv)
 }
 
 /****************************************************************************
- * Function: enc_rxdispath
+ * Function: enc_rxdispatch
  *
  * Description:
  *   Give the newly received packet to uIP.
@@ -1207,10 +1217,11 @@ static void enc_rxerif(FAR struct enc_driver_s *priv)
  *   None
  *
  * Assumptions:
+ *   Interrupts are enabled but the caller holds the uIP lock.
  *
  ****************************************************************************/
 
-static void enc_rxdispath(FAR struct enc_driver_s *priv)
+static void enc_rxdispatch(FAR struct enc_driver_s *priv)
 {
  /* We only accept IP packets of the configured type and ARP packets */
 
@@ -1267,6 +1278,7 @@ static void enc_rxdispath(FAR struct enc_driver_s *priv)
  *   None
  *
  * Assumptions:
+ *   Interrupts are enabled but the caller holds the uIP lock.
  *
  ****************************************************************************/
 
@@ -1326,7 +1338,7 @@ static void enc_pktif(FAR struct enc_driver_s *priv)
       nlldbg("Bad packet size dropped (%d)\n", pktlen);
 #ifdef CONFIG_ENC28J60_STATS
       priv->stats.rxpktlen++;
-#endif	    
+#endif
     }
 
   /* Otherwise, read and process the packet */
@@ -1344,7 +1356,7 @@ static void enc_pktif(FAR struct enc_driver_s *priv)
 
       /* Dispatch the packet to uIP */
  
-      enc_rxdispath(priv);
+      enc_rxdispatch(priv);
     }
 
   /* Move the RX read pointer to the start of the next received packet.
@@ -1360,7 +1372,7 @@ static void enc_pktif(FAR struct enc_driver_s *priv)
 }
 
 /****************************************************************************
- * Function: enc_worker
+ * Function: enc_irqworker
  *
  * Description:
  *   Perform interrupt handling logic outside of the interrupt handler (on
@@ -1376,13 +1388,18 @@ static void enc_pktif(FAR struct enc_driver_s *priv)
  *
  ****************************************************************************/
 
-static void enc_worker(FAR void *arg)
+static void enc_irqworker(FAR void *arg)
 {
   FAR struct enc_driver_s *priv = (FAR struct enc_driver_s *)arg;
+  uip_lock_t lock;
   uint8_t eir;
 
   DEBUGASSERT(priv);
 
+  /* Get exclusive access to uIP. */
+
+  lock = uip_lock();
+
   /* Disable further interrupts by clearing the global interrupt enable bit.
    * "After an interrupt occurs, the host controller should clear the global
    * enable bit for the interrupt pin before servicing the interrupt. Clearing
@@ -1560,15 +1577,17 @@ static void enc_worker(FAR void *arg)
         }
     }
 
+  /* Release lock on uIP */
+
+  uip_unlock(lock);
+
   /* Enable Ethernet interrupts */
 
   enc_bfsgreg(priv, ENC_EIE, EIE_INTIE);
 
-  /* Enable GPIO interrupts if they were disbled in enc_interrupt */
+  /* Enable GPIO interrupts */
 
-#ifndef CONFIG_SPI_OWNBUS
   priv->lower->enable(priv->lower);
-#endif
 }
 
 /****************************************************************************
@@ -1592,15 +1611,6 @@ static int enc_interrupt(int irq, FAR void *context)
 {
   register FAR struct enc_driver_s *priv = &g_enc28j60[0];
 
-#ifdef CONFIG_SPI_OWNBUS
-  /* In very simple environments, we own the SPI and can do data transfers
-   * from the interrupt handler.  That is actually a very bad idea in any
-   * case because it keeps interrupts disabled for a long time.
-   */
-
-  enc_worker((FAR void*)priv);
-  return OK;
-#else
   /* In complex environments, we cannot do SPI transfers from the interrupt
    * handler because semaphores are probably used to lock the SPI bus.  In
    * this case, we will defer processing to the worker thread.  This is also
@@ -1608,28 +1618,26 @@ static int enc_interrupt(int irq, FAR void *context)
    * a good thing to do in any event.
    */
 
-  DEBUGASSERT(work_available(&priv->work));
+  DEBUGASSERT(work_available(&priv->irqwork));
 
   /* Notice that further GPIO interrupts are disabled until the work is
    * actually performed.  This is to prevent overrun of the worker thread.
-   * Interrupts are re-enabled in enc_worker() when the work is completed.
+   * Interrupts are re-enabled in enc_irqworker() when the work is completed.
    */
 
   priv->lower->disable(priv->lower);
-  return work_queue(HPWORK, &priv->work, enc_worker, (FAR void *)priv, 0);
-#endif
+  return work_queue(HPWORK, &priv->irqwork, enc_irqworker, (FAR void *)priv, 0);
 }
 
 /****************************************************************************
- * Function: enc_txtimeout
+ * Function: enc_toworker
  *
  * Description:
- *   Our TX watchdog timed out.  Called from the timer interrupt handler.
- *   The last TX never completed.  Reset the hardware and start again.
+ *   Our TX watchdog timed out.  This is the worker thread continuation of
+ *   the watchdog timer interrupt.  Reset the hardware and start again.
  *
  * Parameters:
- *   argc - The number of available arguments
- *   arg  - The first argument
+ *   arg     - The reference to the driver structure (case to void*)
  *
  * Returned Value:
  *   None
@@ -1638,14 +1646,21 @@ static int enc_interrupt(int irq, FAR void *context)
  *
  ****************************************************************************/
 
-static void enc_txtimeout(int argc, uint32_t arg, ...)
+static void enc_toworker(FAR void *arg)
 {
   FAR struct enc_driver_s *priv = (FAR struct enc_driver_s *)arg;
+  uip_lock_t lock;
   int ret;
 
+  nlldbg("Tx timeout\n");
+  DEBUGASSERT(priv);
+
+  /* Get exclusive access to uIP. */
+
+  lock = uip_lock();
+
   /* Increment statistics and dump debug info */
 
-  nlldbg("Tx timeout\n");
 #ifdef CONFIG_ENC28J60_STATS
   priv->stats.txtimeouts++;
 #endif
@@ -1662,13 +1677,18 @@ static void enc_txtimeout(int argc, uint32_t arg, ...)
   /* Then poll uIP for new XMIT data */
 
   (void)uip_poll(&priv->dev, enc_uiptxpoll);
+
+  /* Release lock on uIP */
+
+  uip_unlock(lock);
 }
 
 /****************************************************************************
- * Function: enc_polltimer
+ * Function: enc_txtimeout
  *
  * Description:
- *   Periodic timer handler.  Called from the timer interrupt handler.
+ *   Our TX watchdog timed out.  Called from the timer interrupt handler.
+ *   The last TX never completed.  Perform work on the worker thread.
  *
  * Parameters:
  *   argc - The number of available arguments
@@ -1681,9 +1701,55 @@ static void enc_txtimeout(int argc, uint32_t arg, ...)
  *
  ****************************************************************************/
 
-static void enc_polltimer(int argc, uint32_t arg, ...)
+static void enc_txtimeout(int argc, uint32_t arg, ...)
 {
   FAR struct enc_driver_s *priv = (FAR struct enc_driver_s *)arg;
+  int ret;
+
+  /* In complex environments, we cannot do SPI transfers from the timout
+   * handler because semaphores are probably used to lock the SPI bus.  In
+   * this case, we will defer processing to the worker thread.  This is also
+   * much kinder in the use of system resources and is, therefore, probably
+   * a good thing to do in any event.
+   */
+
+  DEBUGASSERT(priv && work_available(&priv->towork));
+
+  /* Notice that Tx timeout watchdog is not active so further Tx timeouts
+   * can occur until we restart the Tx timeout watchdog.
+   */
+
+  ret = work_queue(HPWORK, &priv->towork, enc_toworker, (FAR void *)priv, 0);
+  DEBUGASSERT(ret == OK);
+}
+
+/****************************************************************************
+ * Function: enc_pollworker
+ *
+ * Description:
+ *   Periodic timer handler continuation.
+ *
+ * Parameters:
+ *   argc - The number of available arguments
+ *   arg  - The first argument
+ *
+ * Returned Value:
+ *   None
+ *
+ * Assumptions:
+ *
+ ****************************************************************************/
+
+static void enc_pollworker(FAR void *arg)
+{
+  FAR struct enc_driver_s *priv = (FAR struct enc_driver_s *)arg;
+  uip_lock_t lock;
+
+  DEBUGASSERT(priv);
+
+  /* Get exclusive access to uIP. */
+
+  lock = uip_lock();
 
   /* Verify that the hardware is ready to send another packet.  The driver
    * start a transmission process by setting ECON1.TXRTS. When the packet is
@@ -1701,11 +1767,54 @@ static void enc_polltimer(int argc, uint32_t arg, ...)
       (void)uip_timer(&priv->dev, enc_uiptxpoll, ENC_POLLHSEC);
     }
 
+  /* Release lock on uIP */
+
+  uip_unlock(lock);
+
   /* Setup the watchdog poll timer again */
 
   (void)wd_start(priv->txpoll, ENC_WDDELAY, enc_polltimer, 1, arg);
 }
 
+/****************************************************************************
+ * Function: enc_polltimer
+ *
+ * Description:
+ *   Periodic timer handler.  Called from the timer interrupt handler.
+ *
+ * Parameters:
+ *   argc - The number of available arguments
+ *   arg  - The first argument
+ *
+ * Returned Value:
+ *   None
+ *
+ * Assumptions:
+ *
+ ****************************************************************************/
+
+static void enc_polltimer(int argc, uint32_t arg, ...)
+{
+  FAR struct enc_driver_s *priv = (FAR struct enc_driver_s *)arg;
+  int ret;
+
+  /* In complex environments, we cannot do SPI transfers from the timout
+   * handler because semaphores are probably used to lock the SPI bus.  In
+   * this case, we will defer processing to the worker thread.  This is also
+   * much kinder in the use of system resources and is, therefore, probably
+   * a good thing to do in any event.
+   */
+
+  DEBUGASSERT(priv && work_available(&priv->pollwork));
+
+  /* Notice that poll watchdog is not active so further poll timeouts can
+   * occur until we restart the poll timeout watchdog.
+   */
+
+  ret = work_queue(HPWORK, &priv->pollwork, enc_pollworker, (FAR void *)priv, 0);
+  DEBUGASSERT(ret == OK);
+}
+
 /****************************************************************************
  * Function: enc_ifup
  *

nuttx/net/Kconfig

@@ -34,7 +34,7 @@ config NET_NOINTS
 	bool "Not interrupt driven"
 	default n
 	---help---
-		NET_NOINT indicates that uIP not called from the interrupt level. 
+		NET_NOINT indicates that uIP is not called from the interrupt level. 
 		If NET_NOINTS is defined, critical sections will be managed with semaphores;
 		Otherwise, it assumed that uIP will be called from interrupt level handling
 		and critical sections will be managed by enabling and disabling interrupts.