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.

This commit is contained in:
Gregory Nutt 2017-03-04 11:33:36 -06:00
parent e2eb5f1ae0
commit c976a66f8d
3 changed files with 48 additions and 88 deletions

8
TODO
View File

@ -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.

View File

@ -43,7 +43,6 @@
#include <stdint.h>
#include <stdbool.h>
#include <time.h>
#include <semaphore.h>
#include <string.h>
#include <errno.h>
#include <assert.h>
@ -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().

View File

@ -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"