diff --git a/TODO b/TODO index 3c187e017e1..62d2e562ce7 100644 --- a/TODO +++ b/TODO @@ -1,4 +1,4 @@ -NuttX TODO List (Last updated October 9, 2016) +NuttX TODO List (Last updated November 2, 2016) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This file summarizes known NuttX bugs, limitations, inconsistencies with @@ -9,7 +9,7 @@ issues related to each board port. nuttx/: - (13) Task/Scheduler (sched/) + (14) Task/Scheduler (sched/) (1) Memory Management (mm/) (1) Power Management (drivers/pm) (3) Signals (sched/signal, arch/) @@ -216,6 +216,60 @@ o Task/Scheduler (sched/) Status: Open Priority: Medium-ish + Title: ISSUES WITH PRIORITY INHERITANCE WHEN SEMAPHORE USED AS IPC + Description: Semaphores have multiple uses. The typical usage is where + the semaphore is used as lock on one or more resources. In + this typical case, priority inheritance works perfectly: The + holder of a semaphore count must be remembered so that its + priority can be boosted if a higher priority task requires a + count from the semaphore. It remains the holder until the + same task calls sem_post() to release the count on the + semaphore. + + But a different usage model for semaphores is for signalling + events. In this case, the semaphore count is initialized to + zero and the receiving task calls sem_wait() to wait for the + next event of interest. When an event of interest is + detected by another task (or even an interrupt handler), + sem_post() is called which increments the count to 1 and + wakes up the receiving task. + + For example, in the following TASK A waits for events and + TASK B (or perhaps an interrupt handler) signals task A of + the occurence of the events by posting the semaphore: + + ---------------------- --------------- + TASK A TASK B + ---------------------- --------------- + sem_init(sem, 0, 0); + sem_wait(sem); + sem_post(sem); + Awakens as holder + ---------------------- --------------- + + These two usage models are really very different and priority + inheritance simply does not apply when the semaphore is used for + signalling rather than locking. In this signalling case + priority inheritance can interfere with the operation of the + semaphore. The problem is that when TASK A is awakened it is + a holder of the semaphore. Normally, a task is removed from + the holder list when it finally releases the semaphore via + sem_post(). + + However, TASK A never calls sem_post(sem) so it becomes + *permanently* a holder of the semaphore and may have its + priority boosted at any time when any other task tries to + acquire the semaphore. + + The fix is to call sem_setprotocol(SEM_PRIO_NONE) immediately + after the sem_init() call so that there will be no priority + inheritance operations on this semaphore used for signalling. + Status: Open + Priority: High. If you have priority inheritance enabled and you use + semaphores for signalling events, then you *must* call + sem_setprotocol(SEM_PRIO_NONE) immediately after initializing + the semaphore. + Title: SCALABILITY Description: Task control information is retained in simple lists. This is completely appropriate for small embedded systems where diff --git a/arch/arm/src/stm32/stm32_adc.c b/arch/arm/src/stm32/stm32_adc.c index acbb707d345..83939499c86 100644 --- a/arch/arm/src/stm32/stm32_adc.c +++ b/arch/arm/src/stm32/stm32_adc.c @@ -2203,6 +2203,7 @@ static void adc_shutdown(FAR struct adc_dev_s *dev) static void adc_rxint(FAR struct adc_dev_s *dev, bool enable) { FAR struct stm32_dev_s *priv = (FAR struct stm32_dev_s *)dev->ad_priv; + uint32_t regval; ainfo("intf: %d enable: %d\n", priv->intf, enable ? 1 : 0); @@ -2212,8 +2213,15 @@ static void adc_rxint(FAR struct adc_dev_s *dev, bool enable) * end-of-conversion ADC. */ - adc_modifyreg(priv, STM32_ADC_IER_OFFSET, 0, - priv->hasdma ? ADC_IER_AWD | ADC_ISR_OVR : ADC_IER_ALLINTS); + regval = ADC_IER_ALLINTS; +#ifdef ADC_HAVE_DMA + if (priv->hasdma) + { + regval &= ~(ADC_IER_EOC | ADC_IER_JEOC); + } +#endif + + adc_modifyreg(priv, STM32_ADC_IER_OFFSET, 0, regval); } else { @@ -2803,7 +2811,7 @@ static int adc_interrupt(FAR struct adc_dev_s *dev) /* by MR regval &= ~pending; */ /* by MR adc_putreg(priv, STM32_ADC_ISR_OFFSET, regval); - + adc_putreg(priv, STM32_ADC_ISR_OFFSET, pending); */ return OK; } diff --git a/arch/xtensa/src/esp32/esp32_cpuhead.S b/arch/xtensa/src/esp32/esp32_cpuhead.S index 9fb05f66ea8..40cbf2e85d6 100644 --- a/arch/xtensa/src/esp32/esp32_cpuhead.S +++ b/arch/xtensa/src/esp32/esp32_cpuhead.S @@ -110,9 +110,7 @@ __cpu1_start: l32r sp, .Lcpu1_bottomofstack - /* Does it make since to have co-processors enabled on the IDLE thread? */ - -//#warning REVISIT: Must set aside co-processor save ares + /* REVIST: Does it make since to have co-processors enabled on the IDLE thread? */ #ifdef CONFIG_STACK_COLORATION /* Write a known value to the IDLE thread stack to support stack diff --git a/arch/xtensa/src/esp32/esp32_cpuidlestack.c b/arch/xtensa/src/esp32/esp32_cpuidlestack.c index e1f0941ce92..7b2e90364e9 100644 --- a/arch/xtensa/src/esp32/esp32_cpuidlestack.c +++ b/arch/xtensa/src/esp32/esp32_cpuidlestack.c @@ -102,9 +102,7 @@ int up_cpu_idlestack(int cpu, FAR struct tcb_s *tcb, size_t stack_size) tcb->adj_stack_ptr = (uint32_t *)topofstack; #if XCHAL_CP_NUM > 0 - /* Does it make since to have co-processors enabled on the IDLE thread? */ - -//#warning REVISIT: Need to set co-processor save are in TCB + /* REVISIT: Does it make since to have co-processors enabled on the IDLE thread? */ #endif return OK; diff --git a/drivers/Kconfig b/drivers/Kconfig index 9b508fdd7fa..4ac8a9d0c69 100644 --- a/drivers/Kconfig +++ b/drivers/Kconfig @@ -345,9 +345,7 @@ menuconfig SPI should be enabled by all platforms that support SPI interfaces. See include/nuttx/spi/spi.h for further SPI driver information. -if SPI source drivers/spi/Kconfig -endif menuconfig I2S bool "I2S Driver Support" diff --git a/drivers/serial/serial.c b/drivers/serial/serial.c index 2b6d7ec8e53..64354e2a769 100644 --- a/drivers/serial/serial.c +++ b/drivers/serial/serial.c @@ -43,7 +43,6 @@ #include #include #include -#include #include #include #include @@ -52,6 +51,7 @@ #include #include +#include #include #include #include @@ -112,16 +112,6 @@ static const struct file_operations g_serialops = * Private Functions ************************************************************************************/ -/************************************************************************************ - * Name: sem_reinit - ************************************************************************************/ - -static int sem_reinit(FAR sem_t *sem, int pshared, unsigned int value) -{ - sem_destroy(sem); - return sem_init(sem, pshared, value); -} - /************************************************************************************ * Name: uart_takesem ************************************************************************************/ @@ -1253,19 +1243,14 @@ static int uart_close(FAR struct file *filep) /* We need to re-initialize the semaphores if this is the last close * of the device, as the close might be caused by pthread_cancel() of * a thread currently blocking on any of them. - * - * REVISIT: This logic *only* works in the case where the cancelled - * thread had the only reference to the serial driver. If there other - * references, then the this logic will not be executed and the - * semaphore count will still be incorrect. */ - sem_reinit(&dev->xmitsem, 0, 0); - sem_reinit(&dev->recvsem, 0, 0); - sem_reinit(&dev->xmit.sem, 0, 1); - sem_reinit(&dev->recv.sem, 0, 1); + sem_reset(&dev->xmitsem, 0); + sem_reset(&dev->recvsem, 0); + sem_reset(&dev->xmit.sem, 1); + sem_reset(&dev->recv.sem, 1); #ifndef CONFIG_DISABLE_POLL - sem_reinit(&dev->pollsem, 0, 1); + sem_reset(&dev->pollsem, 1); #endif uart_givesem(&dev->closesem); @@ -1416,6 +1401,8 @@ errout_with_sem: int uart_register(FAR const char *path, FAR uart_dev_t *dev) { + /* Initialize semaphores */ + sem_init(&dev->xmit.sem, 0, 1); sem_init(&dev->recv.sem, 0, 1); sem_init(&dev->closesem, 0, 1); @@ -1425,6 +1412,15 @@ int uart_register(FAR const char *path, FAR uart_dev_t *dev) sem_init(&dev->pollsem, 0, 1); #endif + /* The recvsem and xmitsem are used for signaling and, hence, should not have + * priroity inheritance enabled. + */ + + sem_setprotocol(&dev->xmitsem, SEM_PRIO_NONE); + sem_setprotocol(&dev->recvsem, SEM_PRIO_NONE); + + /* Register the serial driver */ + _info("Registering %s\n", path); return register_driver(path, &g_serialops, 0666, dev); } diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig index ea8b8ac9abc..177d863ee2c 100644 --- a/drivers/spi/Kconfig +++ b/drivers/spi/Kconfig @@ -3,6 +3,18 @@ # see the file kconfig-language.txt in the NuttX tools repository. # +config ARCH_HAVE_SPI_CRCGENERATION + bool + default n + +config ARCH_HAVE_SPI_CS_CONTROL + bool + default n + +config ARCH_HAVE_SPI_BITORDER + bool + default n + if SPI config SPI_SLAVE @@ -55,10 +67,6 @@ config SPI_HWFEATURES basically the OR of any specific hardware feature and eanbles the SPI hwfeatures() interface method. -config ARCH_HAVE_SPI_CRCGENERATION - bool - default n - config SPI_CRCGENERATION bool default n @@ -69,10 +77,6 @@ config SPI_CRCGENERATION generation of SPI CRCs. Enables the HWFEAT_CRCGENERATION option as well as the hwfeartures() interface method. -config ARCH_HAVE_SPI_CS_CONTROL - bool - default n - config SPI_CS_CONTROL bool "SPI CS Behavior Control" default n @@ -82,10 +86,6 @@ config SPI_CS_CONTROL Enables possibilities to define the behavior of CS. Also enables the hwfeatures() interface method. -config ARCH_HAVE_SPI_BITORDER - bool - default n - config SPI_BITORDER bool "SPI Bit Order Control" default n diff --git a/include/nuttx/semaphore.h b/include/nuttx/semaphore.h index 1d0afca38eb..04fde1bacbd 100644 --- a/include/nuttx/semaphore.h +++ b/include/nuttx/semaphore.h @@ -51,6 +51,12 @@ * Pre-processor Definitions ****************************************************************************/ +/* Values for protocol attribute */ + +#define SEM_PRIO_NONE 0 +#define SEM_PRIO_INHERIT 1 +#define SEM_PRIO_PROTECT 2 + /**************************************************************************** * Public Type Definitions ****************************************************************************/ @@ -61,11 +67,13 @@ struct inode; struct nsem_inode_s { + /* This must be the first element of the structure. In sem_close() this + * structure must be cast compatible with sem_t. + */ + sem_t ns_sem; /* The contained semaphore */ - /* Inode payload unique to named semaphores. ns_inode must appear first - * in this structure in order to support casting between type sem_t and - * types of struct nsem_inode_s. */ + /* Inode payload unique to named semaphores. */ FAR struct inode *ns_inode; /* Containing inode */ }; @@ -134,6 +142,72 @@ int sem_tickwait(FAR sem_t *sem, systime_t start, uint32_t delay); int sem_reset(FAR sem_t *sem, int16_t count); +/**************************************************************************** + * Function: sem_getprotocol + * + * Description: + * Return the value of the semaphore protocol attribute. + * + * Parameters: + * sem - A pointer to the semaphore whose attributes are to be + * queried. + * protocol - The user provided location in which to store the protocol + * value. + * + * Return Value: + * 0 if successful. Otherwise, -1 is returned and the errno value is set + * appropriately. + * + ****************************************************************************/ + +#ifdef CONFIG_PRIORITY_INHERITANCE +int sem_getprotocol(FAR sem_t *sem, FAR int *protocol); +#else +# define sem_getprotocol(s,p) do { *(p) == SEM_PRIO_NONE); } while (0) +#endif + +/**************************************************************************** + * Function: sem_setprotocol + * + * Description: + * Set semaphore protocol attribute. + * + * One particularly important use of this function is when a semaphore + * is used for inter-task communication like: + * + * TASK A TASK B + * sem_init(sem, 0, 0); + * sem_wait(sem); + * sem_post(sem); + * Awakens as holder + * + * In this case priority inheritance can interfere with the operation of + * the semaphore. The problem is that when TASK A is restarted it is a + * holder of the semaphore. However, it never calls sem_post(sem) so it + * becomes *permanently* a holder of the semaphore and may have its + * priority boosted when any other task tries to acquire the semaphore. + * + * The fix is to call sem_setprotocol(SEM_PRIO_NONE) immediately after + * the sem_init() call so that there will be no priority inheritance + * operations on this semaphore. + * + * Parameters: + * sem - A pointer to the semaphore whose attributes are to be + * modified + * protocol - The new protocol to use + * + * Return Value: + * 0 if successful. Otherwise, -1 is returned and the errno value is set + * appropriately. + * + ****************************************************************************/ + +#ifdef CONFIG_PRIORITY_INHERITANCE +int sem_setprotocol(FAR sem_t *sem, int protocol); +#else +# define sem_setprotocol(s,p) DEBUGASSERT((p) == SEM_PRIO_NONE); +#endif + #undef EXTERN #ifdef __cplusplus } diff --git a/include/pthread.h b/include/pthread.h index e25e63d41fe..1c368d737b7 100644 --- a/include/pthread.h +++ b/include/pthread.h @@ -40,18 +40,19 @@ * Included Files ********************************************************************************/ -#include /* Default settings */ -#include /* Compiler settings, noreturn_function */ +#include /* Default settings */ +#include /* Compiler settings, noreturn_function */ -#include /* Needed for general types */ -#include /* Needed by pthread_[set|get]name_np */ +#include /* Needed for general types */ +#include /* Needed by pthread_[set|get]name_np */ -#include /* C99 fixed width integer types */ -#include /* C99 boolean types */ -#include /* For getpid */ -#include /* Needed for sem_t */ -#include /* Needed for sigset_t, includes this file */ -#include /* Needed for struct timespec */ +#include /* C99 fixed width integer types */ +#include /* C99 boolean types */ +#include /* For getpid */ +#include /* Needed for sigset_t, includes this file */ +#include /* Needed for struct timespec */ + +#include /* For sem_t and SEM_PRIO_* defines */ /******************************************************************************** * Pre-processor Definitions @@ -112,9 +113,7 @@ #define PTHREAD_INHERIT_SCHED 0 #define PTHREAD_EXPLICIT_SCHED 1 -#define PTHREAD_PRIO_NONE 0 -#define PTHREAD_PRIO_INHERIT 1 -#define PTHREAD_PRIO_PROTECT 2 +/* Default priority */ #define PTHREAD_DEFAULT_PRIORITY 100 @@ -135,7 +134,7 @@ #define PTHREAD_BARRIER_SERIAL_THREAD 0x1000 -/* Values for protocol attribute */ +/* Values for protocol mutex attribute */ #define PTHREAD_PRIO_NONE SEM_PRIO_NONE #define PTHREAD_PRIO_INHERIT SEM_PRIO_INHERIT diff --git a/include/semaphore.h b/include/semaphore.h index 41db9caa8bf..8821b129d09 100644 --- a/include/semaphore.h +++ b/include/semaphore.h @@ -45,24 +45,10 @@ #include #include -#ifdef __cplusplus -#define EXTERN extern "C" -extern "C" -{ -#else -#define EXTERN extern -#endif - /**************************************************************************** * Pre-processor Definitions ****************************************************************************/ -/* Values for protocol attribute */ - -#define SEM_PRIO_NONE 0 -#define SEM_PRIO_INHERIT 1 -#define SEM_PRIO_PROTECT 2 - /* Bit definitions for the struct sem_s flags field */ #define PRIOINHERIT_FLAGS_DISABLE (1 << 0) /* Bit 0: Priority inheritance @@ -130,6 +116,14 @@ typedef struct sem_s sem_t; * Public Data ****************************************************************************/ +#ifdef __cplusplus +#define EXTERN extern "C" +extern "C" +{ +#else +#define EXTERN extern +#endif + /**************************************************************************** * Public Function Prototypes ****************************************************************************/ @@ -153,13 +147,6 @@ int sem_close(FAR sem_t *sem); int sem_unlink(FAR const char *name); #endif -#ifdef CONFIG_PRIORITY_INHERITANCE -/* Non-standard interfaces to manage priority inheritance */ - -int sem_getprotocol(FAR sem_t *sem, FAR int *protocol); -int sem_setprotocol(FAR sem_t *sem, int protocol); -#endif - #undef EXTERN #ifdef __cplusplus } diff --git a/libc/semaphore/sem_getprotocol.c b/libc/semaphore/sem_getprotocol.c index abbc4c02dbf..4ae1606ff7b 100644 --- a/libc/semaphore/sem_getprotocol.c +++ b/libc/semaphore/sem_getprotocol.c @@ -39,9 +39,10 @@ #include -#include #include +#include + #ifdef CONFIG_PRIORITY_INHERITANCE /**************************************************************************** diff --git a/sched/pthread/pthread_mutexinit.c b/sched/pthread/pthread_mutexinit.c index 0c65fbc7ed2..fe9620da0b4 100644 --- a/sched/pthread/pthread_mutexinit.c +++ b/sched/pthread/pthread_mutexinit.c @@ -44,6 +44,8 @@ #include #include +#include + #include "pthread/pthread.h" /**************************************************************************** diff --git a/sched/semaphore/sem_setprotocol.c b/sched/semaphore/sem_setprotocol.c index c5609fdcd5f..1350206941b 100644 --- a/sched/semaphore/sem_setprotocol.c +++ b/sched/semaphore/sem_setprotocol.c @@ -39,10 +39,11 @@ #include -#include #include #include +#include + #include "semaphore/semaphore.h" #ifdef CONFIG_PRIORITY_INHERITANCE @@ -57,6 +58,25 @@ * Description: * Set semaphore protocol attribute. * + * One particularly important use of this furnction is when a semaphore + * is used for inter-task communication like: + * + * TASK A TASK B + * sem_init(sem, 0, 0); + * sem_wait(sem); + * sem_post(sem); + * Awakens as holder + * + * In this case priority inheritance can interfere with the operation of + * the semaphore. The problem is that when TASK A is restarted it is a + * holder of the semaphore. However, it never calls sem_post(sem) so it + * becomes *permanently* a holder of the semaphore and may have its + * priority boosted when any other task tries to acquire the semaphore. + * + * The fix is to call sem_setprotocol(SEM_PRIO_NONE) immediately after + * the sem_init() call so that there will be no priority inheritance + * operations on this semaphore. + * * Parameters: * sem - A pointer to the semaphore whose attributes are to be * modified diff --git a/syscall/syscall.csv b/syscall/syscall.csv index 8d8307f42a9..8ecf52854fc 100644 --- a/syscall/syscall.csv +++ b/syscall/syscall.csv @@ -118,7 +118,7 @@ "sem_destroy","semaphore.h","","int","FAR sem_t*" "sem_open","semaphore.h","defined(CONFIG_FS_NAMED_SEMAPHORES)","FAR sem_t*","FAR const char*","int","..." "sem_post","semaphore.h","","int","FAR sem_t*" -"sem_setprotocol","semaphore.h","defined(CONFIG_PRIORITY_INHERITANCE)","int","FAR sem_t*","int" +"sem_setprotocol","nuttx/semaphore.h","defined(CONFIG_PRIORITY_INHERITANCE)","int","FAR sem_t*","int" "sem_timedwait","semaphore.h","","int","FAR sem_t*","FAR const struct timespec *" "sem_trywait","semaphore.h","","int","FAR sem_t*" "sem_unlink","semaphore.h","defined(CONFIG_FS_NAMED_SEMAPHORES)","int","FAR const char*"