diff --git a/TODO b/TODO index aa048fde334..3d54ecf7f75 100644 --- a/TODO +++ b/TODO @@ -1,4 +1,4 @@ -NuttX TODO List (Last updated November 2, 2016) +NuttX TODO List (Last updated November 17, 2016) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This file summarizes known NuttX bugs, limitations, inconsistencies with @@ -15,7 +15,7 @@ nuttx/: (3) Signals (sched/signal, arch/) (2) pthreads (sched/pthread) (0) Message Queues (sched/mqueue) - (9) Kernel/Protected Build + (8) Kernel/Protected Build (3) C++ Support (6) Binary loaders (binfmt/) (12) Network (net/, drivers/net) @@ -683,14 +683,6 @@ o Kernel/Protected Build improvement. However, there is no strong motivation now do do that partitioning work. - Title: TIMER INTERRUPT CALLBACK - Description: The timer upper half driver at drivers/timers/timer.c performs - interrupt level callbacks into applications. This, of course, - will never work in anything but a non-secure, flat build. - Status: Open - Priority: Medium. The driver is only usable with all of its features - in a FLAT build. - Title: USER MODE TASKS CAN MODIFY PRIVILEGED TASKS Description: Certain interfaces, such as sched_setparam(), sched_setscheduler(), etc. can be used by user mode tasks to diff --git a/configs/sabre-6quad/README.txt b/configs/sabre-6quad/README.txt index e784bae7356..4284c6de2aa 100644 --- a/configs/sabre-6quad/README.txt +++ b/configs/sabre-6quad/README.txt @@ -498,10 +498,11 @@ Open Issues: CPU (which may not be CPU0). Perhaps that should be a spinlock to prohibit execution of interrupts on CPU0 when other CPUs are in a critical section? - 2016-11-15: When the critical section is used to lock a resource that is also - used by interupt handling, I believe that what is required is that the interrupt - handling logic must also take the spinlock. This will cause the interrupt handlers - on other CPUs to spin until leave_critical_section() is called. + 2016-11-17: A fix was added to sched/irq/irq_csection that should correct this + problem. When the critical section is used to lock a resource that is also used by + interupt handling, the interrupt handling logic must also take the spinlock. This + will cause the interrupt handlers on other CPUs to spin until leave_critical_section() + is called. More verification is needed, however. 2. Cache Concurency. This is a complex problem. There is logic in place now to clean CPU0 D-cache before starting a new CPU and for invalidating the D-Cache @@ -537,6 +538,19 @@ Open Issues: PMD_SECT_DOM(0) | PMD_SECT_XN) #define MMU_STRONGLY_ORDERED (PMD_TYPE_SECT | PMD_SECT_AP_RW1 | \ + Another alternative would be to place all spinlocks in a non-cachable memory + region. That is problem what will have to be done. + + This is a VERIFIED PROBLEM: I have seen cases where CPU0 sets a spinlock=1 then + tries to lock the spinlock. CPU0 will wait in this case until CPU1 unlocks the + spinlock. Most of this happens correctly; I can see that CPU1 does set the + spinlock=0, but CPU0 never sees the change and spins forever. That is surely + a consequence of cache issues. + + This was observed between up_cpu_pause() and arm_pause_handler() with the + spinlock "g_cpu_paused[cpu]". CPU1 correctly sets g_cpu_paused[cpu] to zero + but CPU0 never sees the change. + 3. Assertions. On a fatal assertions, other CPUs need to be stopped. The SCR, however, only supports disabling CPUs 1 through 3. Perhaps if the assertion occurs on CPUn, n > 0, then it should use and SGI to perform the assertion diff --git a/drivers/timers/timer.c b/drivers/timers/timer.c index 0186e9977f1..16c85fa5d73 100644 --- a/drivers/timers/timer.c +++ b/drivers/timers/timer.c @@ -65,8 +65,13 @@ struct timer_upperhalf_s { - uint8_t crefs; /* The number of times the device has been opened */ - FAR char *path; /* Registration path */ + uint8_t crefs; /* The number of times the device has been opened */ +#ifdef HAVE_NOTIFICATION + uint8_t signal; /* The signal number to use in the notification */ + pid_t pid; /* The ID of the task/thread to receive the signal */ + FAR void *arg; /* An argument to pass with the signal */ +#endif + FAR char *path; /* Registration path */ /* The contained lower-half driver */ @@ -77,6 +82,12 @@ struct timer_upperhalf_s * Private Function Prototypes ****************************************************************************/ +#ifdef HAVE_NOTIFICATION + /* REVISIT: This function prototype is insufficient to support signaling */ + +static bool timer_notifier(FAR uint32_t *next_interval_us); +#endif + static int timer_open(FAR struct file *filep); static int timer_close(FAR struct file *filep); static ssize_t timer_read(FAR struct file *filep, FAR char *buffer, @@ -110,6 +121,36 @@ static const struct file_operations g_timerops = * Private Functions ****************************************************************************/ +/************************************************************************************ + * Name: timer_notifier + * + * Description: + * Notify the application via a signal when the timer interrupt occurs + * + * REVISIT: This function prototype is insufficient to support signaling + * + ************************************************************************************/ + +#ifdef HAVE_NOTIFICATION +static bool timer_notifier(FAR uint32_t *next_interval_us) +{ + FAR struct timer_upperhalf_s *upper = HOW?; +#ifdef CONFIG_CAN_PASS_STRUCTS + union sigval value; +#endif + int ret; + +#ifdef CONFIG_CAN_PASS_STRUCTS + value.sival_ptr = upper->arg; + ret = sigqueue(upper->pid, upper->signo, value); +#else + ret = sigqueue(upper->pid, upper->signo, upper->arg); +#endif + + return ret == OK; +} +#endif + /************************************************************************************ * Name: timer_open * @@ -120,10 +161,10 @@ static const struct file_operations g_timerops = static int timer_open(FAR struct file *filep) { - FAR struct inode *inode = filep->f_inode; + FAR struct inode *inode = filep->f_inode; FAR struct timer_upperhalf_s *upper = inode->i_private; - uint8_t tmp; - int ret; + uint8_t tmp; + int ret; tmrinfo("crefs: %d\n", upper->crefs); @@ -322,41 +363,31 @@ static int timer_ioctl(FAR struct file *filep, int cmd, unsigned long arg) } break; - /* cmd: TCIOC_SETHANDLER - * Description: Call this handler on timeout - * Argument: A pointer to struct timer_sethandler_s. +#ifdef HAVE_NOTIFICATION + /* cmd: TCIOC_NOTIFICATION + * Description: Notify application via a signal when the timer expires. + * Argument: signal number * * NOTE: This ioctl cannot be support in the kernel build mode. In that * case direct callbacks from kernel space into user space is forbidden. */ -#if !defined(CONFIG_BUILD_PROTECTED) && !defined(CONFIG_BUILD_KERNEL) - case TCIOC_SETHANDLER: + case TCIOC_NOTIFICATION: { - FAR struct timer_sethandler_s *sethandler; + FAR struct timer_notify_s *notify = + (FAR struct timer_notify_s *)((uintptr_t)arg) - /* Don't reset on timer timeout; instead, call this user - * provider timeout handler. NOTE: Providing handler==NULL will - * restore the reset behavior. - */ - - if (lower->ops->sethandler) /* Optional */ + if (notify != NULL) { - sethandler = (FAR struct timer_sethandler_s *)((uintptr_t)arg); - if (sethandler) - { - sethandler->oldhandler = - lower->ops->sethandler(lower, sethandler->newhandler); - ret = OK; - } - else - { - ret = -EINVAL; - } + upper->signo = notify->signal; + upper->get = notify->signal; + upper->arg = noify->arg; + + ret = timer_sethandler((FAR void *handle)upper, timer_notifier, NULL); } else { - ret = -ENOSYS; + ret = -EINVAL; } } break; @@ -496,8 +527,8 @@ void timer_unregister(FAR void *handle) /* Recover the pointer to the upper-half driver state */ upper = (FAR struct timer_upperhalf_s *)handle; + DEBUGASSERT(upper != NULL && upper->lower != NULL); lower = upper->lower; - DEBUGASSERT(upper && lower); tmrinfo("Unregistering: %s\n", upper->path); @@ -516,4 +547,57 @@ void timer_unregister(FAR void *handle) kmm_free(upper); } +/**************************************************************************** + * Name: timer_sethandler + * + * Description: + * This function can be called to add a callback into driver-related code + * to handle timer expirations. This is a strictly OS internal interface + * and may NOT be used by appliction code. + * + * Input parameters: + * handle - This is the handle that was returned by timer_register() + * newhandler - The new timer interrupt handler + * oldhandler - The previous timer interrupt handler (if any) + * + * Returned Value: + * None + * + ****************************************************************************/ + +int timer_sethandler(FAR void *handle, tccb_t newhandler, + FAR tccb_t *oldhandler) +{ + FAR struct timer_upperhalf_s *upper; + FAR struct timer_lowerhalf_s *lower; + tccb_t tmphandler; + + /* Recover the pointer to the upper-half driver state */ + + upper = (FAR struct timer_upperhalf_s *)handle; + DEBUGASSERT(upper != NULL && upper->lower != NULL); + lower = upper->lower; + DEBUGASSERT(lower->ops != NULL); + + /* Check if the lower half driver supports the sethandler method */ + + if (lower->ops->sethandler != NULL) /* Optional */ + { + /* Yes.. Defer the hander attachment to the lower half driver */ + + tmphandler = lower->ops->sethandler(lower, newhandler); + + /* Return the oldhandler if a location to return it was provided */ + + if (oldhandler != NULL) + { + *oldhandler = tmphandler; + } + + return OK; + } + + return -ENOSYS; +} + #endif /* CONFIG_TIMER */ diff --git a/include/nuttx/timers/timer.h b/include/nuttx/timers/timer.h index 8df658376ae..eb2830bd560 100644 --- a/include/nuttx/timers/timer.h +++ b/include/nuttx/timers/timer.h @@ -52,6 +52,12 @@ /**************************************************************************** * Pre-processor Definitions ****************************************************************************/ +/* It would require some interface modifcations in order to support + * notifications. + */ + +#undef HAVE_NOTIFICATION + /* IOCTL Commands ***********************************************************/ /* The timer driver uses a standard character driver framework. However, * since the timer driver is a device control interface and not a data @@ -60,16 +66,18 @@ * * These are detected and handled by the "upper half" timer driver. * - * TCIOC_START - Start the timer - * Argument: Ignored - * TCIOC_STOP - Stop the timer - * Argument: Ignored - * TCIOC_GETSTATUS - Get the status of the timer. - * Argument: A writeable pointer to struct timer_status_s. - * TCIOC_SETTIMEOUT - Reset the timer timeout to this value - * Argument: A 32-bit timeout value in microseconds. - * TCIOC_SETHANDLER - Call this handler on timer expiration - * Argument: A pointer to struct timer_sethandler_s. + * TCIOC_START - Start the timer + * Argument: Ignored + * TCIOC_STOP - Stop the timer + * Argument: Ignored + * TCIOC_GETSTATUS - Get the status of the timer. + * Argument: A writeable pointer to struct timer_status_s. + * TCIOC_SETTIMEOUT - Reset the timer timeout to this value + * Argument: A 32-bit timeout value in microseconds. + * TCIOC_NOTIFICATION - Set up to notify an application via a signal when + * the timer expires. + * Argument: A read-only pointer to an instance of + * stuct timer_notify_s. * * WARNING: May change TCIOC_SETTIMEOUT to pass pointer to 64bit nanoseconds * or timespec structure. @@ -84,18 +92,20 @@ * range. */ -#define TCIOC_START _TCIOC(0x0001) -#define TCIOC_STOP _TCIOC(0x0002) -#define TCIOC_GETSTATUS _TCIOC(0x0003) -#define TCIOC_SETTIMEOUT _TCIOC(0x0004) -#define TCIOC_SETHANDLER _TCIOC(0x0005) +#define TCIOC_START _TCIOC(0x0001) +#define TCIOC_STOP _TCIOC(0x0002) +#define TCIOC_GETSTATUS _TCIOC(0x0003) +#define TCIOC_SETTIMEOUT _TCIOC(0x0004) +#ifdef HAVE_NOTIFICATION +#define TCIOC_NOTIFICATION _TCIOC(0x0005) +#endif /* Bit Settings *************************************************************/ /* Bit settings for the struct timer_status_s flags field */ -#define TCFLAGS_ACTIVE (1 << 0) /* 1=The timer is running */ -#define TCFLAGS_HANDLER (1 << 1) /* 1=Call the user function when the - * timer expires */ +#define TCFLAGS_ACTIVE (1 << 0) /* 1=The timer is running */ +#define TCFLAGS_HANDLER (1 << 1) /* 1=Call the user function when the + * timer expires */ /**************************************************************************** * Public Types @@ -105,15 +115,7 @@ * function can modify the next interval if desired. */ -typedef bool (*tccb_t)(FAR uint32_t *next_interval_us); - -/* This is the type of the argument passed to the TCIOC_SETHANDLER ioctl */ - -struct timer_sethandler_s -{ - CODE tccb_t newhandler; /* The new timer interrupt handler */ - CODE tccb_t oldhandler; /* The previous timer interrupt handler (if any) */ -}; +typedef CODE bool (*tccb_t)(FAR uint32_t *next_interval_us); /* This is the type of the argument passed to the TCIOC_GETSTATUS ioctl and * and returned by the "lower half" getstatus() method. @@ -127,6 +129,17 @@ struct timer_status_s * (in microseconds) */ }; +#ifdef HAVE_NOTIFICATION +/* This is the type of the argument passed to the TCIOC_NOTIFICATION ioctl */ + +struct timer_notify_s +{ + FAR void *arg; /* An argument to pass with the signal */ + pid_t pid; /* The ID of the task/thread to receive the signal */ + uint8_t signal; /* The signal number to use in the notification */ +}; +#endif + /* This structure provides the "lower-half" driver operations available to * the "upper-half" driver. */ @@ -254,6 +267,33 @@ FAR void *timer_register(FAR const char *path, void timer_unregister(FAR void *handle); +/**************************************************************************** + * Kernal internal interfaces. Thse may not be used by application logic + ****************************************************************************/ + +/**************************************************************************** + * Name: timer_sethandler + * + * Description: + * This function can be called to add a callback into driver-related code + * to handle timer expirations. This is a strictly OS internal interface + * and may NOT be used by appliction code. + * + * Input parameters: + * handle - This is the handle that was returned by timer_register() + * newhandler - The new timer interrupt handler + * oldhandler - The previous timer interrupt handler (if any) + * + * Returned Value: + * None + * + ****************************************************************************/ + +#ifdef __KERNEL__ +int timer_sethandler(FAR void *handle, tccb_t newhandler, + FAR tccb_t *oldhandler); +#endif + /**************************************************************************** * Platform-Independent "Lower-Half" Timer Driver Interfaces ****************************************************************************/