xiaoxiang781216 commented on code in PR #20782: URL: https://github.com/apache/nuttx/pull/20782#discussion_r4355804001
########## drivers/timers/taskwdt.c: ########## @@ -0,0 +1,436 @@ +/**************************************************************************** + * drivers/timers/taskwdt.c + * + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. The + * ASF licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the + * License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + * + ****************************************************************************/ + +/**************************************************************************** + * Included Files + ****************************************************************************/ + +#include <nuttx/config.h> + +#include <sys/types.h> +#include <stdint.h> +#include <stdbool.h> +#include <string.h> +#include <fcntl.h> +#include <assert.h> +#include <errno.h> +#include <nuttx/debug.h> + +#include <nuttx/clock.h> +#include <nuttx/fs/fs.h> +#include <nuttx/kmalloc.h> +#include <nuttx/list.h> +#include <nuttx/mutex.h> +#include <nuttx/sched.h> +#include <nuttx/wqueue.h> +#include <nuttx/timers/taskwdt.h> + +/**************************************************************************** + * Pre-processor Definitions + ****************************************************************************/ + +#define TASKWDT_INTERVAL_TICK MSEC2TICK(CONFIG_TASKWDT_INTERVAL) + +/**************************************************************************** + * Private Types + ****************************************************************************/ + +/* A channel is the state behind one open() of the device */ + +struct taskwdt_chan_s +{ + struct list_node node; /* Entry in taskwdt_s.chans */ + pid_t pid; /* Task that opened it */ + char name[CONFIG_TASK_NAME_SIZE + 1]; /* Its name */ + clock_t timeout; /* Timeout in ticks */ + clock_t deadline; /* Tick the channel expires */ + bool armed; /* Is the channel supervised */ +}; + +struct taskwdt_s +{ + struct file wdt; /* The hardware watchdog */ + struct work_s work; /* Checks the channels, feeds the hardware */ + struct list_node chans; /* All the channels (struct taskwdt_chan_s) */ + mutex_t lock; /* Protects everything above and below */ + bool started; /* The hardware watchdog is armed */ +}; + +/**************************************************************************** + * Private Function Prototypes + ****************************************************************************/ + +static int taskwdt_open(FAR struct file *filep); Review Comment: > Thanks for the time you put into the review. I read [a2c0156](https://github.com/apache/nuttx/commit/a2c01567a8332b638ef5116d6a040915e2642329), and the two approaches start from different philosophies: the patch makes each open() an independent software watchdog (a timer per descriptor that calls PANIC, hardware untouched), while taskwdt multiplexes ONE hardware watchdog and feeds it only while every armed channel is in time. So the chip resets if a task hangs or if the system itself stops (a software timer does not fire if the scheduler stops), and the pid and name of the task are reported. The patch also adds WDIOC_SETSOFT, which the Linux watchdog API does not have. > yes, that's why I mention incorporate both implementations. > About integrating into watchdog.c: I tried before answering. I built a prototype where the supervisor is a virtual lower half (watchdog_register()), leaving open/close/write/ioctl to watchdog.c. It works in the simulator, but the design gets worse, for a fundamental reason: the watchdog_ops_s methods do not receive a handle, and the state in watchdog.c is per device. The supervisor needs per-descriptor state (f_priv), as in Linux. Without the handle the prototype had to infer the channel from the pid of the calling thread, which causes: > > * a channel per thread instead of per descriptor: if one thread opens and > another one feeds, the channel of the first expires and resets the system > wrongly (tested in the simulator: the PR version passes, the prototype > resets after 2 s); > * a reused pid inheriting a channel, and a dependency on the upper half > calling the lower half in the caller's context, which the interface does > not guarantee; > * the need for an optional open() in watchdog_ops_s (and later a close()), > coupling the supervisor to watchdog.c with every fix. > yes, it's hard to support multiple instances by implement the supervisor as a virtual lower half, but it's possible and easy to add the per task watchdog in the upper half layer like https://github.com/xiaoxiang781216/nuttx/commit/a2c01567a8332b638ef5116d6a040915e2642329. > Integrating for real means giving the upper half per-open() state and passing that handle to the lower halves, which is basically the structure of your patch (watchdog_user_s): an API rework that affects every watchdog driver, quite different from this PR. If you prefer that path I can do it, as a separate PR ([a2c0156](https://github.com/apache/nuttx/commit/a2c01567a8332b638ef5116d6a040915e2642329) is not in master yet). the simple way is: 1. cherry-pick that patch 1. remove WDIOC_SETSOFT and software watchdog 3. add implement your hardware watchdog on top of it > Today taskwdt already reuses watchdog.c at the right layer: it is a client of it (opens the hardware device and only uses the WDIOC_* commands), and it only repeats about a hundred lines of open/close/write/ioctl for its own node. > the code dup is one reason, another reason include: 1. we don't need change board code to enable per task watchdog 2. we can support multiple watchdog hardware BTW, it would be great if we can support per task and global monitor at the same time, which mean that system will reboot when either task or system stop response. > Question: does your use case need the chip reset on a hung task (with the task report), or is a PANIC per channel enough? If only PANIC, your patch is enough and taskwdt is not needed; if you need the reset, the supervisor still makes sense. > yes, the reset is better than PANIC. that's why I think both implementations should merge into one: 1. reuse the framework from https://github.com/xiaoxiang781216/nuttx/commit/a2c01567a8332b638ef5116d6a040915e2642329 2. but replace the implementation with your code. > (I will answer the timer_wdog.c comment and the TASKWDT_ -> WATCHDOG_TASKMONITOR_ rename separately.) -- 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]
