From c976a66f8dbee1c408dda3312f548ad581a9c4f8 Mon Sep 17 00:00:00 2001 From: Gregory Nutt Date: Sat, 4 Mar 2017 11:33:36 -0600 Subject: [PATCH] net/drivers/skeleton.c: Back out serialization changes of the last commit. They are not necessary in the skeleton.c example because the calls to net_lock() at the beginning of each worker function will enforce serialization. --- TODO | 8 ++- drivers/net/skeleton.c | 112 +++++++++++++---------------------------- sched/Kconfig | 16 +++--- 3 files changed, 48 insertions(+), 88 deletions(-) diff --git a/TODO b/TODO index daecdedbed..e3457de5d8 100644 --- a/TODO +++ b/TODO @@ -1055,8 +1055,12 @@ o Network (net/, drivers/net) additional mechanism would have to be added to enforce that serialization. - See nuttx/drivers/net/skeleton.c for an example of how - serialization may be added to an Ethernet driver. + NOTE: Most drivers will call net_lock() and net_unlock() around + the critical portions of the driver work. In that case, all work + will be properly serialized. This issue only applies to drivers + that may perform operations that require protection outside of + the net_lock'ed region. Sometimes, this may require extending + the netlock() to be beginning of the driver work function. Status: Open Priority: High if you happen to be using Ethernet in this configuration. diff --git a/drivers/net/skeleton.c b/drivers/net/skeleton.c index aa9d2fe3d5..b33c46e6e0 100644 --- a/drivers/net/skeleton.c +++ b/drivers/net/skeleton.c @@ -43,7 +43,6 @@ #include #include #include -#include #include #include #include @@ -65,34 +64,24 @@ /**************************************************************************** * Pre-processor Definitions ****************************************************************************/ + /* If processing is not done at the interrupt level, then work queue support * is required. */ -#undef ETH_SERIALIZATION - #if !defined(CONFIG_SCHED_WORKQUEUE) # error Work queue support is required in this configuration (CONFIG_SCHED_WORKQUEUE) #else - /* Use the low priority work queue if possible */ + /* Use the selected work queue */ # if defined(CONFIG_skeleton_HPWORK) # define ETHWORK HPWORK # elif defined(CONFIG_skeleton_LPWORK) # define ETHWORK LPWORK - - /* Serialization may be required in the special case where there are - * multiple low priority work queues. - */ - -# if CONFIG_SCHED_LPNTHREADS > 1 -# define ETH_SERIALIZATION 1 -# endif # else # error Neither CONFIG_skeleton_HPWORK nor CONFIG_skeleton_LPWORK defined # endif - #endif /* CONFIG_skeleton_NINTERFACES determines the number of physical interfaces @@ -130,9 +119,6 @@ struct skel_driver_s WDOG_ID sk_txtimeout; /* TX timeout timer */ struct work_s sk_irqwork; /* For deferring interupt work to the work queue */ struct work_s sk_pollwork; /* For deferring poll work to the work queue */ -#ifdef ETH_SERIALIZATION - sem_t sk_lock; /* Serialization semaphore */ -#endif /* This holds the information visible to the NuttX network */ @@ -162,17 +148,6 @@ static struct skel_driver_s g_skel[CONFIG_skeleton_NINTERFACES]; * Private Function Prototypes ****************************************************************************/ -#ifdef ETH_SERIALIZATION -/* Serialization support */ - -static void skel_lock(FAR struct skel_driver_s *priv); -# define skel_unlock(p) (void)sem_post(&(p)->sk_lock) - -#else -# define skel_lock(p) -# define skel_unlock(p) -#endif - /* Common TX logic */ static int skel_transmit(FAR struct skel_driver_s *priv); @@ -216,36 +191,6 @@ static void skel_ipv6multicast(FAR struct skel_driver_s *priv); * Private Functions ****************************************************************************/ -/**************************************************************************** - * Function: skel_lock - * - * Description: - * When there are multiple LP work queues, we must force serialization of - * work. - * - * Parameters: - * priv - Reference to the driver state structure - * - * Returned Value: - * None - * - * Assumptions: - * Called in the context of thread of the LP work queue. - * - ****************************************************************************/ - -#ifdef ETH_SERIALIZATION -static void skel_lock(FAR struct skel_driver_s *priv) -{ - while (sem_wait(&priv->sk_lock) < 0) - { - /* EINTR is the only expected error value */ - - DEBUGASSERT(errno == EINTR); - } -} -#endif - /**************************************************************************** * Function: skel_transmit * @@ -562,7 +507,13 @@ static void skel_interrupt_work(FAR void *arg) { FAR struct skel_driver_s *priv = (FAR struct skel_driver_s *)arg; - skel_lock(priv); + /* Lock the network and serialize driver operations if necessary. + * NOTE: Serialization is only required in the case where the driver work + * is performed on an LP worker thread and where more than one LP worker + * thread has been configured. + */ + + net_lock(); /* Process pending Ethernet interrupts */ @@ -572,7 +523,6 @@ static void skel_interrupt_work(FAR void *arg) /* Check if we received an incoming packet, if so, call skel_receive() */ - net_lock(); skel_receive(priv); /* Check if a packet transmission just completed. If so, call skel_txdone. @@ -586,7 +536,6 @@ static void skel_interrupt_work(FAR void *arg) /* Re-enable Ethernet interrupts */ up_enable_irq(CONFIG_skeleton_IRQ); - skel_unlock(priv); } /**************************************************************************** @@ -655,7 +604,13 @@ static void skel_txtimeout_work(FAR void *arg) { FAR struct skel_driver_s *priv = (FAR struct skel_driver_s *)arg; - skel_lock(priv); + /* Lock the network and serialize driver operations if necessary. + * NOTE: Serialization is only required in the case where the driver work + * is performed on an LP worker thread and where more than one LP worker + * thread has been configured. + */ + + net_lock(); /* Increment statistics and dump debug info */ @@ -665,11 +620,8 @@ static void skel_txtimeout_work(FAR void *arg) /* Then poll the network for new XMIT data */ - net_lock(); (void)devif_poll(&priv->sk_dev, skel_txpoll); net_unlock(); - - skel_unlock(priv); } /**************************************************************************** @@ -749,7 +701,13 @@ static void skel_poll_work(FAR void *arg) { FAR struct skel_driver_s *priv = (FAR struct skel_driver_s *)arg; - skel_lock(priv); + /* Lock the network and serialize driver operations if necessary. + * NOTE: Serialization is only required in the case where the driver work + * is performed on an LP worker thread and where more than one LP worker + * thread has been configured. + */ + + net_lock(); /* Perform the poll */ @@ -762,7 +720,6 @@ static void skel_poll_work(FAR void *arg) * progress, we will missing TCP time state updates? */ - net_lock(); (void)devif_timer(&priv->sk_dev, skel_txpoll); /* Setup the watchdog poll timer again */ @@ -770,8 +727,6 @@ static void skel_poll_work(FAR void *arg) (void)wd_start(priv->sk_txpoll, skeleton_WDDELAY, skel_poll_expiry, 1, (wdparm_t)priv); net_unlock(); - - skel_unlock(priv); } /**************************************************************************** @@ -920,11 +875,16 @@ static void skel_txavail_work(FAR void *arg) { FAR struct skel_driver_s *priv = (FAR struct skel_driver_s *)arg; - skel_lock(priv); + /* Lock the network and serialize driver operations if necessary. + * NOTE: Serialization is only required in the case where the driver work + * is performed on an LP worker thread and where more than one LP worker + * thread has been configured. + */ + + net_lock(); /* Ignore the notification if the interface is not yet up */ - net_lock(); if (priv->sk_bifup) { /* Check if there is room in the hardware to hold another outgoing packet. */ @@ -935,7 +895,6 @@ static void skel_txavail_work(FAR void *arg) } net_unlock(); - skel_unlock(priv); } /**************************************************************************** @@ -1095,6 +1054,7 @@ static void skel_ipv6multicast(FAR struct skel_driver_s *priv) (void)skel_addmac(dev, g_ipv6_ethallnodes.ether_addr_octet); #endif /* CONFIG_NET_ICMPv6_AUTOCONF */ + #ifdef CONFIG_NET_ICMPv6_ROUTER /* Add the IPv6 all link-local routers Ethernet address. This is the * address that we expect to receive ICMPv6 Router Solicitation @@ -1163,14 +1123,10 @@ int skel_initialize(int intf) /* Create a watchdog for timing polling for and timing of transmisstions */ - priv->sk_txpoll = wd_create(); /* Create periodic poll timer */ - priv->sk_txtimeout = wd_create(); /* Create TX timeout timer */ + priv->sk_txpoll = wd_create(); /* Create periodic poll timer */ + priv->sk_txtimeout = wd_create(); /* Create TX timeout timer */ -#ifdef ETH_SERIALIZATION - /* Initialize the serialization semaphore */ - - sem_init(&priv->sk_lock, 0, 1); -#endif + DEBUGASSERT(priv->sk_txpoll != NULL && priv->sk_txtimeout != NULL); /* Put the interface in the down state. This usually amounts to resetting * the device and/or calling skel_ifdown(). diff --git a/sched/Kconfig b/sched/Kconfig index b5f7727ca9..3dc0e91eba 100644 --- a/sched/Kconfig +++ b/sched/Kconfig @@ -1223,14 +1223,14 @@ config SCHED_LPNTHREADS Such behavior is necessary to support asynchronous I/O, AIO (for example). - CAUTION: Some drivers, such as most Ethernet drivers, use the work - queue to serialize network operations. The will also use the low- - priority work queue if it is available. If there are multiple low- - priority worker thread, then this can result in the loss of that - serialization. There may be concurrent Ethernet operations running - on different LP threads and this could lead to a failure. You may - need to visit the use of the LP work queue on your configuration - is you select CONFIG_SCHED_LPNTHREADS > 1 + CAUTION: Some drivers may use the work queue to serialize + operations. The may also use the low-priority work queue if it is + available. If there are multiple low-priority worker thread, then + this can result in the loss of that serialization. There may be + concurrent driver operations running on different LP threads and + this could lead to a failure. You may need to visit the use of the + LP work queue on your configuration is you select + CONFIG_SCHED_LPNTHREADS > 1 config SCHED_LPWORKPRIORITY int "Low priority worker thread priority"