daniel-p-carvalho commented on code in PR #20782:
URL: https://github.com/apache/nuttx/pull/20782#discussion_r4371843792
##########
drivers/timers/watchdog.c:
##########
@@ -569,6 +924,7 @@ static ssize_t wdog_read(FAR struct file *filep, FAR char
*buffer,
return ret;
}
+#ifndef CONFIG_WATCHDOG_TASKMONITOR
Review Comment:
Done.
##########
drivers/timers/watchdog.c:
##########
@@ -502,10 +831,36 @@ static int wdog_open(FAR struct file *filep)
static int wdog_close(FAR struct file *filep)
{
- kmm_free(filep->f_priv);
+ FAR struct inode *inode = filep->f_inode;
+ FAR struct watchdog_upperhalf_s *upper = inode->i_private;
+ int ret;
- return OK;
+ wdinfo("crefs: %d\n", upper->crefs);
+
+ /* Get exclusive access to the device structures */
+
+ ret = nxmutex_lock(&upper->lock);
+ if (ret < 0)
+ {
+ goto errout;
+ }
+
+ /* Decrement the references to the driver. If the reference count will
+ * decrement to 0, then uninitialize the driver.
+ */
+
+ if (upper->crefs > 0)
+ {
+ upper->crefs--;
+ }
+
+ nxmutex_unlock(&upper->lock);
+ ret = OK;
+
+errout:
+ return ret;
}
+#endif
Review Comment:
Done.
##########
drivers/timers/watchdog.c:
##########
@@ -502,10 +831,36 @@ static int wdog_open(FAR struct file *filep)
static int wdog_close(FAR struct file *filep)
{
- kmm_free(filep->f_priv);
+ FAR struct inode *inode = filep->f_inode;
+ FAR struct watchdog_upperhalf_s *upper = inode->i_private;
+ int ret;
- return OK;
+ wdinfo("crefs: %d\n", upper->crefs);
+
+ /* Get exclusive access to the device structures */
+
+ ret = nxmutex_lock(&upper->lock);
+ if (ret < 0)
+ {
+ goto errout;
+ }
+
+ /* Decrement the references to the driver. If the reference count will
+ * decrement to 0, then uninitialize the driver.
+ */
+
+ if (upper->crefs > 0)
Review Comment:
Done. crefs is gone, and wdog_open()/wdog_close() were removed, their
entries in the file_operations are NULL.
##########
drivers/timers/watchdog.c:
##########
@@ -466,12 +510,266 @@ static int wdog_notifier(FAR struct notifier_block *nb,
unsigned long action,
}
#endif
-static void wdog_callback(wdparm_t arg)
+#ifdef CONFIG_WATCHDOG_TASKMONITOR
+/* Restart the timeout of a channel, if it is armed */
+
+static void watchdog_taskmonitor_feed(FAR struct watchdog_user_s *user)
+{
+ if (user->armed)
+ {
+ user->deadline = clock_systime_ticks() + user->timeout;
+ }
+}
+
+/* Feed the lower half while no armed channel timed out. Running from the
+ * low priority work queue also makes the hardware reset the system if that
+ * queue stops.
+ */
+
+static void watchdog_taskmonitor_worker(FAR void *arg)
+{
+ FAR struct watchdog_upperhalf_s *upper = arg;
+ FAR struct watchdog_lowerhalf_s *lower = upper->lower;
+ FAR struct watchdog_user_s *user;
+ bool expired = false;
+ clock_t now;
+
+ nxmutex_lock(&upper->lock);
+ now = clock_systime_ticks();
+
+ list_for_every_entry(&upper->users, user, struct watchdog_user_s, node)
+ {
+ if (user->armed && clock_compare(user->deadline, now))
+ {
+ _alert("%s: timeout of task %d (%s), %ju ms\n", upper->path,
+ user->pid, user->name, (uintmax_t)TICK2MSEC(user->timeout));
+ expired = true;
+ }
+ }
+
+ nxmutex_unlock(&upper->lock);
+
+ if (expired)
+ {
+#ifdef CONFIG_WATCHDOG_TASKMONITOR_PANIC
+ PANIC();
+#endif
+
+ /* Stop feeding, the hardware watchdog resets the system */
+
+ return;
+ }
+
+ lower->ops->keepalive(lower);
+ work_queue(LPWORK, &upper->work, watchdog_taskmonitor_worker, upper,
+ WATCHDOG_TASKMONITOR_INTERVAL_TICK);
+}
+
+/* Arm the lower half and start feeding it. The lock must be held. */
+
+static int watchdog_taskmonitor_start(FAR struct watchdog_upperhalf_s *upper)
+{
+ FAR struct watchdog_lowerhalf_s *lower = upper->lower;
+ int ret;
+
+ if (upper->started)
+ {
+ return OK;
+ }
+
+ if (lower->ops->settimeout != NULL) /* Optional */
+ {
+ ret = lower->ops->settimeout(lower,
+ CONFIG_WATCHDOG_TASKMONITOR_HWTIMEOUT);
+ if (ret < 0)
+ {
+ return ret;
+ }
+ }
+
+ DEBUGASSERT(lower->ops->start); /* Required */
+ ret = lower->ops->start(lower);
+ if (ret < 0)
+ {
+ return ret;
+ }
+
+ upper->started = true;
+ return work_queue(LPWORK, &upper->work, watchdog_taskmonitor_worker, upper,
+ WATCHDOG_TASKMONITOR_INTERVAL_TICK);
+}
+
+/* Each open() creates a channel */
+
+static int watchdog_taskmonitor_open(FAR struct file *filep)
+{
+ FAR struct watchdog_upperhalf_s *upper = filep->f_inode->i_private;
+ FAR struct tcb_s *tcb = nxsched_self();
+ FAR struct watchdog_user_s *user;
+ int ret;
+
+ user = kmm_zalloc(sizeof(struct watchdog_user_s));
+ if (user == NULL)
+ {
+ return -ENOMEM;
+ }
+
+ user->pid = tcb->pid;
+ user->timeout = MSEC2TICK(CONFIG_WATCHDOG_TASKMONITOR_DEFAULT_TIMEOUT);
+ user->armed = true;
+ strlcpy(user->name, get_task_name(tcb), sizeof(user->name));
+ watchdog_taskmonitor_feed(user);
+
+ ret = nxmutex_lock(&upper->lock);
+ if (ret >= 0)
+ {
+ ret = watchdog_taskmonitor_start(upper);
+ if (ret >= 0)
+ {
+ list_add_tail(&upper->users, &user->node);
+ filep->f_priv = user;
+ }
+
+ nxmutex_unlock(&upper->lock);
+ }
+
+ if (ret < 0)
+ {
+ kmm_free(user);
+ }
+
+ return ret;
+}
+
+/* An armed channel stays in the list and times out, see the structure */
+
+static int watchdog_taskmonitor_close(FAR struct file *filep)
{
- wdinfo("Watchdog timeout,the system will PANIC ... \n");
- PANIC();
+ FAR struct watchdog_upperhalf_s *upper = filep->f_inode->i_private;
+ FAR struct watchdog_user_s *user = filep->f_priv;
+ int ret;
+
+ ret = nxmutex_lock(&upper->lock);
+ if (ret < 0)
+ {
+ return ret;
+ }
+
+ if (!user->armed)
+ {
+ list_delete(&user->node);
+ kmm_free(user);
+ }
+
+ nxmutex_unlock(&upper->lock);
+ return OK;
}
+/* Feed the channel, or disarm it with a 'V' */
+
+static ssize_t watchdog_taskmonitor_write(FAR struct file *filep,
+ FAR const char *buffer,
+ size_t buflen)
+{
+ FAR struct watchdog_upperhalf_s *upper = filep->f_inode->i_private;
+ FAR struct watchdog_user_s *user = filep->f_priv;
+ int ret;
+
+ ret = nxmutex_lock(&upper->lock);
+ if (ret < 0)
+ {
+ return ret;
+ }
+
+ if (memchr(buffer, 'V', buflen) != NULL)
+ {
+ user->armed = false;
+ }
+ else
+ {
+ watchdog_taskmonitor_feed(user);
+ }
+
+ nxmutex_unlock(&upper->lock);
+ return buflen;
+}
+
+/* The commands act on the channel, never on the lower half, so that no task
+ * can stop the hardware watchdog, change its timeout or replace its reset
+ * with a capture handler.
+ */
+
+static int watchdog_taskmonitor_ioctl(FAR struct file *filep, int cmd,
+ unsigned long arg)
+{
+ FAR struct watchdog_upperhalf_s *upper = filep->f_inode->i_private;
+ FAR struct watchdog_user_s *user = filep->f_priv;
+ FAR struct watchdog_status_s *status;
+ clock_t now;
+ int ret;
+
+ ret = nxmutex_lock(&upper->lock);
+ if (ret < 0)
+ {
+ return ret;
+ }
+
+ switch (cmd)
+ {
+ case WDIOC_START:
+ user->armed = true;
+ watchdog_taskmonitor_feed(user);
+ break;
+
+ case WDIOC_STOP:
+ user->armed = false;
+ break;
+
+ case WDIOC_KEEPALIVE:
+ watchdog_taskmonitor_feed(user);
+ break;
+
+ case WDIOC_SETTIMEOUT:
+ if (arg == 0)
+ {
+ ret = -EINVAL;
+ break;
+ }
+
+ user->timeout = MSEC2TICK(arg);
+ watchdog_taskmonitor_feed(user);
+ break;
+
+ case WDIOC_GETSTATUS:
+ status = (FAR struct watchdog_status_s *)((uintptr_t)arg);
+ if (status == NULL)
+ {
+ ret = -EINVAL;
+ break;
+ }
+
+ now = clock_systime_ticks();
+ status->flags = user->armed ? WDFLAGS_ACTIVE | WDFLAGS_RESET : 0;
+ status->timeout = TICK2MSEC(user->timeout);
+ status->timeleft = user->armed && !clock_compare(user->deadline, now)
+ ? TICK2MSEC(user->deadline - now) : 0;
+ break;
+
+ case WDIOC_CAPTURE:
+ ret = -ENOSYS;
+ break;
+
+ default:
+ ret = -ENOTTY;
+ break;
+ }
+
+ nxmutex_unlock(&upper->lock);
+ return ret;
+}
+#endif
+
+#ifndef CONFIG_WATCHDOG_TASKMONITOR
Review Comment:
Done, the monitor functions and wdog_write()/wdog_ioctl() are now in one
#ifdef / #else.
##########
drivers/timers/watchdog.c:
##########
@@ -466,12 +510,266 @@ static int wdog_notifier(FAR struct notifier_block *nb,
unsigned long action,
}
#endif
-static void wdog_callback(wdparm_t arg)
+#ifdef CONFIG_WATCHDOG_TASKMONITOR
+/* Restart the timeout of a channel, if it is armed */
+
+static void watchdog_taskmonitor_feed(FAR struct watchdog_user_s *user)
+{
+ if (user->armed)
+ {
+ user->deadline = clock_systime_ticks() + user->timeout;
+ }
+}
+
+/* Feed the lower half while no armed channel timed out. Running from the
+ * low priority work queue also makes the hardware reset the system if that
+ * queue stops.
+ */
+
+static void watchdog_taskmonitor_worker(FAR void *arg)
+{
+ FAR struct watchdog_upperhalf_s *upper = arg;
+ FAR struct watchdog_lowerhalf_s *lower = upper->lower;
+ FAR struct watchdog_user_s *user;
+ bool expired = false;
+ clock_t now;
+
+ nxmutex_lock(&upper->lock);
+ now = clock_systime_ticks();
+
+ list_for_every_entry(&upper->users, user, struct watchdog_user_s, node)
+ {
+ if (user->armed && clock_compare(user->deadline, now))
+ {
+ _alert("%s: timeout of task %d (%s), %ju ms\n", upper->path,
+ user->pid, user->name, (uintmax_t)TICK2MSEC(user->timeout));
+ expired = true;
+ }
+ }
+
+ nxmutex_unlock(&upper->lock);
+
+ if (expired)
+ {
+#ifdef CONFIG_WATCHDOG_TASKMONITOR_PANIC
+ PANIC();
+#endif
+
+ /* Stop feeding, the hardware watchdog resets the system */
+
+ return;
+ }
+
+ lower->ops->keepalive(lower);
+ work_queue(LPWORK, &upper->work, watchdog_taskmonitor_worker, upper,
+ WATCHDOG_TASKMONITOR_INTERVAL_TICK);
+}
+
+/* Arm the lower half and start feeding it. The lock must be held. */
+
+static int watchdog_taskmonitor_start(FAR struct watchdog_upperhalf_s *upper)
+{
+ FAR struct watchdog_lowerhalf_s *lower = upper->lower;
+ int ret;
+
+ if (upper->started)
+ {
+ return OK;
+ }
+
+ if (lower->ops->settimeout != NULL) /* Optional */
+ {
+ ret = lower->ops->settimeout(lower,
+ CONFIG_WATCHDOG_TASKMONITOR_HWTIMEOUT);
+ if (ret < 0)
+ {
+ return ret;
+ }
+ }
+
+ DEBUGASSERT(lower->ops->start); /* Required */
+ ret = lower->ops->start(lower);
+ if (ret < 0)
+ {
+ return ret;
+ }
+
+ upper->started = true;
+ return work_queue(LPWORK, &upper->work, watchdog_taskmonitor_worker, upper,
+ WATCHDOG_TASKMONITOR_INTERVAL_TICK);
+}
+
+/* Each open() creates a channel */
+
+static int watchdog_taskmonitor_open(FAR struct file *filep)
+{
+ FAR struct watchdog_upperhalf_s *upper = filep->f_inode->i_private;
+ FAR struct tcb_s *tcb = nxsched_self();
+ FAR struct watchdog_user_s *user;
+ int ret;
+
+ user = kmm_zalloc(sizeof(struct watchdog_user_s));
+ if (user == NULL)
+ {
+ return -ENOMEM;
+ }
+
+ user->pid = tcb->pid;
+ user->timeout = MSEC2TICK(CONFIG_WATCHDOG_TASKMONITOR_DEFAULT_TIMEOUT);
+ user->armed = true;
+ strlcpy(user->name, get_task_name(tcb), sizeof(user->name));
+ watchdog_taskmonitor_feed(user);
Review Comment:
Done, the deadline is now set after the channel is added to the list, with
the lock held, so the time spent waiting for the lock is not counted.
##########
drivers/timers/watchdog.c:
##########
@@ -466,12 +510,266 @@ static int wdog_notifier(FAR struct notifier_block *nb,
unsigned long action,
}
#endif
-static void wdog_callback(wdparm_t arg)
+#ifdef CONFIG_WATCHDOG_TASKMONITOR
+/* Restart the timeout of a channel, if it is armed */
+
+static void watchdog_taskmonitor_feed(FAR struct watchdog_user_s *user)
+{
+ if (user->armed)
+ {
+ user->deadline = clock_systime_ticks() + user->timeout;
+ }
+}
+
+/* Feed the lower half while no armed channel timed out. Running from the
+ * low priority work queue also makes the hardware reset the system if that
+ * queue stops.
+ */
+
+static void watchdog_taskmonitor_worker(FAR void *arg)
+{
+ FAR struct watchdog_upperhalf_s *upper = arg;
+ FAR struct watchdog_lowerhalf_s *lower = upper->lower;
+ FAR struct watchdog_user_s *user;
+ bool expired = false;
+ clock_t now;
+
+ nxmutex_lock(&upper->lock);
+ now = clock_systime_ticks();
+
+ list_for_every_entry(&upper->users, user, struct watchdog_user_s, node)
+ {
+ if (user->armed && clock_compare(user->deadline, now))
+ {
+ _alert("%s: timeout of task %d (%s), %ju ms\n", upper->path,
+ user->pid, user->name, (uintmax_t)TICK2MSEC(user->timeout));
+ expired = true;
+ }
+ }
+
+ nxmutex_unlock(&upper->lock);
+
+ if (expired)
+ {
+#ifdef CONFIG_WATCHDOG_TASKMONITOR_PANIC
+ PANIC();
+#endif
+
+ /* Stop feeding, the hardware watchdog resets the system */
+
+ return;
+ }
+
+ lower->ops->keepalive(lower);
+ work_queue(LPWORK, &upper->work, watchdog_taskmonitor_worker, upper,
+ WATCHDOG_TASKMONITOR_INTERVAL_TICK);
+}
+
+/* Arm the lower half and start feeding it. The lock must be held. */
+
+static int watchdog_taskmonitor_start(FAR struct watchdog_upperhalf_s *upper)
+{
+ FAR struct watchdog_lowerhalf_s *lower = upper->lower;
+ int ret;
+
+ if (upper->started)
+ {
+ return OK;
+ }
+
+ if (lower->ops->settimeout != NULL) /* Optional */
+ {
+ ret = lower->ops->settimeout(lower,
+ CONFIG_WATCHDOG_TASKMONITOR_HWTIMEOUT);
Review Comment:
A channel timeout can be longer than WATCHDOG_TASKMONITOR_HWTIMEOUT. The
channel timeout is only compared by the worker, which feeds the hardware every
interval whatever the channels are, so the hardware timeout only limits how
long the low priority work queue can stay stopped. I tested a channel of 9000
ms with a hardware timeout of 5000 ms on the simulator: the system stayed alive
and reset after the 9 s. The documentation says this now.
##########
drivers/timers/watchdog.c:
##########
@@ -466,12 +510,266 @@ static int wdog_notifier(FAR struct notifier_block *nb,
unsigned long action,
}
#endif
-static void wdog_callback(wdparm_t arg)
+#ifdef CONFIG_WATCHDOG_TASKMONITOR
+/* Restart the timeout of a channel, if it is armed */
+
+static void watchdog_taskmonitor_feed(FAR struct watchdog_user_s *user)
+{
+ if (user->armed)
+ {
+ user->deadline = clock_systime_ticks() + user->timeout;
+ }
+}
+
+/* Feed the lower half while no armed channel timed out. Running from the
+ * low priority work queue also makes the hardware reset the system if that
+ * queue stops.
Review Comment:
It already does. With WATCHDOG_TASKMONITOR_AUTOSTART the worker feeds the
hardware even with no channel, so the system resets if the low priority work
queue stops (global monitor), and the channels add the per task monitor on top
of it: the hardware is fed only while no armed channel expired. Both are tested
on the simulator and on the ev49n51a (idle case and the starve/hog cases).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
