xiaoxiang781216 commented on code in PR #20782:
URL: https://github.com/apache/nuttx/pull/20782#discussion_r4371456463
##########
drivers/timers/Kconfig:
##########
@@ -622,6 +622,54 @@ endif # !WATCHDOG_AUTOMONITOR_BY_CAPTURE &&
!WATCHDOG_AUTOMONITOR_BY_IDLE
endif # WATCHDOG_AUTOMONITOR
+config WATCHDOG_TASKMONITOR
+ bool "Task monitor"
+ default n
+ depends on !WATCHDOG_AUTOMONITOR && WATCHDOG_MAGIC_V && SCHED_LPWORK
Review Comment:
remove WATCHDOG_MAGIC_V
##########
drivers/timers/Kconfig:
##########
@@ -622,6 +622,54 @@ endif # !WATCHDOG_AUTOMONITOR_BY_CAPTURE &&
!WATCHDOG_AUTOMONITOR_BY_IDLE
endif # WATCHDOG_AUTOMONITOR
+config WATCHDOG_TASKMONITOR
Review Comment:
how about make
WATCHDOG_AUTOMONITOR/WATCHDOG_TASKMONITOR/WATCHDOG_NONEMONITOR into a choice
##########
drivers/timers/Kconfig:
##########
@@ -622,6 +622,54 @@ endif # !WATCHDOG_AUTOMONITOR_BY_CAPTURE &&
!WATCHDOG_AUTOMONITOR_BY_IDLE
endif # WATCHDOG_AUTOMONITOR
+config WATCHDOG_TASKMONITOR
+ bool "Task monitor"
+ default n
+ depends on !WATCHDOG_AUTOMONITOR && WATCHDOG_MAGIC_V && SCHED_LPWORK
+ ---help---
+ Supervise the tasks that use the watchdog devices. Each open()
of a
+ device creates a channel with its own timeout, which the task
feeds
+ with write() or WDIOC_KEEPALIVE. The hardware watchdog is fed
by the
+ monitor only while all the armed channels are within their
timeouts,
+ so a task that hangs, exits or crashes makes the system reset.
This
+ replaces the Auto-monitor, which feeds the watchdog
unconditionally.
+
+if WATCHDOG_TASKMONITOR
+
+config WATCHDOG_TASKMONITOR_AUTOSTART
+ bool "Arm the hardware watchdog when it is registered"
+ default y
+ ---help---
+ Arm the hardware watchdog in watchdog_register() and feed it
from the
+ low priority work queue even if no task opened the device, so
that the
+ system resets if that queue stops. If not set, the first open()
arms it.
+
+config WATCHDOG_TASKMONITOR_HWTIMEOUT
+ int "Hardware watchdog timeout (millisecond)"
+ default 5000
+
+config WATCHDOG_TASKMONITOR_INTERVAL
+ int "Supervision interval (millisecond)"
+ default 500
Review Comment:
should we default to half of WATCHDOG_TASKMONITOR_HWTIMEOUT
##########
drivers/timers/watchdog.c:
##########
@@ -1087,6 +1406,15 @@ void watchdog_unregister(FAR void *handle)
watchdog_automonitor_stop(upper);
#endif
+#ifdef CONFIG_WATCHDOG_TASKMONITOR
+ work_cancel(LPWORK, &upper->work);
Review Comment:
work_cancel_sync
##########
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:
move after line 629
##########
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:
remove crefs and change wdog_close/wdog_open to empty
##########
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:
remove `#ifndef`
##########
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:
change `#endif/#ifndef` to `#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);
Review Comment:
how to handle all task timeout bigger than
CONFIG_WATCHDOG_TASKMONITOR_HWTIMEOUT
##########
drivers/timers/watchdog.c:
##########
@@ -127,8 +144,15 @@ struct watchdog_upperhalf_s
bool monitor;
#endif
- FAR char *path; /* Registration path */
+#ifdef CONFIG_WATCHDOG_TASKMONITOR
+ struct work_s work; /* Checks the channels, feeds the hardware */
+ struct list_node users; /* All the channels (struct watchdog_user_s) */
+ bool started; /* The hardware watchdog is armed */
+#endif
+
+ uint8_t crefs; /* The number of times the device has been opened */
Review Comment:
why not check whether users is empty instead?
##########
drivers/timers/watchdog.c:
##########
Review Comment:
move before before line 512
##########
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:
remove `#endif`
##########
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:
can we support the task monitor and global monitor at the same time?
--
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]
