<div dir="ltr"><div dir="ltr"><br clear="all"><div><div dir="ltr" class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div dir="ltr"><div dir="ltr"><div>+Chris<br></div></div></div></div></div></div></div></div><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Aug 24, 2020 at 9:56 AM enh <<a href="mailto:enh@google.com">enh@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, Aug 23, 2020 at 11:59 PM Rob Landley <<a href="mailto:rob@landley.net" target="_blank">rob@landley.net</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 8/22/20 3:05 PM, enh via Toybox wrote:<br>
> On Sat, Aug 22, 2020 at 11:22 AM Chris Sarra via Toybox<br>
> <<a href="mailto:toybox@lists.landley.net" target="_blank">toybox@lists.landley.net</a> <mailto:<a href="mailto:toybox@lists.landley.net" target="_blank">toybox@lists.landley.net</a>>> wrote:<br>
> <br>
> This patch introduces a simple watchdog implementation for toybox. We<br>
> send the appropriate ioctls to set the relevant timeouts, and intercept<br>
> signals to safely shut down if required.<br>
> ---<br>
> toys/pending/watchdog.c | 128 ++++++++++++++++++++++++++++++++++++++++<br>
> 1 file changed, 128 insertions(+)<br>
> create mode 100644 toys/pending/watchdog.c<br>
> <br>
> diff --git a/toys/pending/watchdog.c b/toys/pending/watchdog.c<br>
> new file mode 100644<br>
> index 00000000..0813fe69<br>
> --- /dev/null<br>
> +++ b/toys/pending/watchdog.c<br>
> @@ -0,0 +1,128 @@<br>
> +/* watchdog - start a watchdog timer with configurable kick frequencies<br>
> +<br>
> + Author: Chris Sarra, <a href="mailto:chrissarra@google.com" target="_blank">chrissarra@google.com</a> <mailto:<a href="mailto:chrissarra@google.com" target="_blank">chrissarra@google.com</a>><br>
> <br>
> not sure why we're both working on a Saturday, but...<br>
<br>
Only reason I didn't get to it yesterday is I took a day off like you suggested. :)<br>
<br>
> + Date: 25 July 2019<br>
> + Ref: <a href="http://kernel.org/doc/Documentation/watchdog/watchdog-api.txt" rel="noreferrer" target="_blank">kernel.org/doc/Documentation/watchdog/watchdog-api.txt</a><br>
> <<a href="http://kernel.org/doc/Documentation/watchdog/watchdog-api.txt" rel="noreferrer" target="_blank">http://kernel.org/doc/Documentation/watchdog/watchdog-api.txt</a>><br>
> <br>
> use the " * " indent from the other source files?<br>
<br>
I already cleaned this up before spotting this email. :)<br>
<br>
> also mention that there's precedent in a busybox "watchdog", which this is<br>
> command-line compatible with?<br>
<br>
Not a clue. Last system I dealt with using watchdog stuff had a 5 line function<br>
in the main loop of the app poking the watchdog so if the app went down the<br>
system rebooted. I've always been mildly confused by standalone watchdog<br>
binaries: what do they prove exactly?<br>
<br>
> +USE_WATCHDOG(NEWTOY(watchdog, "Ft#T#", TOYFLAG_NEEDROOT|TOYFLAG_BIN))<br>
> <br>
> huh. i didn't realize that USE_ worked _inside_ the comment! probably best to<br>
> move it below the comment as is the convention though. <br>
<br>
It's a grep pulling NEWTOY()/OLDTOY() lines with no leading whitespace out of<br>
toys/*/*.c and running them through sort to make generated/newtoys.h.<br>
<br>
> +config WATCHDOG<br>
> + bool "watchdog"<br>
> + default y<br>
> + help<br>
> + usage: watchdog [-F] [-t SW_TIMER_S] [-T HW_TIMER_S] DEV<br>
> +<br>
> + Start the watchdog timer at DEV with optional timeout parameters.<br>
> <br>
> (blank line here.)<br>
> <br>
> + -F run in the foreground (do not daemonize)<br>
> + -t software timer (in seconds)<br>
> + -T hardware timer (in seconds)<br>
> <br>
> <br>
> say what the defaults are?<br>
<br>
Ooh, good suggestion.<br>
<br>
> the busybox help implies that it's possible to use more precision than seconds?<br>
> afaik from the kernel source and kernel docs, though, that's a lie?<br>
<br>
You can for the sleep, not for this watchdog ioctl. Might be a newer ioctl with<br>
more precision though... Not spotting an obvious candidate in the current kernel<br>
source though. (What's a PRETIMEOUT?)<br></blockquote><div><br></div><div>strace says they're using the same ioctl, so maybe just over-enthusiastic copy & paste in the help text...</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> i think it would help to explain that -T is what you set the hardware watchdog<br>
> to, and -t is how often you kick it. i didn't get that from either your --help<br>
> or the busybox --help.<br>
> <br>
> so something like:<br>
> <br>
> -T N Set hardware watchdog to N seconds (default 60).<br>
> -t N Kick watchdog every N seconds (default 30).<br>
<br>
Is kick the official term for this?<br></blockquote><div><br></div><div>it's the one i'm familiar with, and wikipedia says it's not just me, but `busybox watchdog --help` says "reset" and wikipedia calls the section "restart" but then _don't_ use the verb "restart" but do use both "kick" (the most) and "reset": <a href="https://en.wikipedia.org/wiki/Watchdog_timer#Watchdog_restart" target="_blank">https://en.wikipedia.org/wiki/Watchdog_timer#Watchdog_restart</a></div><div><br></div><div>(they reserve "restart" for what the device does if the watchdog _isn't_ kicked, despite naming the section "Watchdog restart". i think that was also why it wasn't obvious to me what "reset" meant in the busybox help. Maybe "Kick/reset/restart the timer every N seconds (default 30)"? actually, if busybox had just included "the timer", any of the three verbs would have been fine.)</div><div> </div></div></div></blockquote><div><br></div><div> </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> ? (note that it should be a tab between the "-T N" part and the "Set ..." part.)<br>
> <br>
> <br>
> +*/<br>
> +#define FOR_watchdog<br>
> +#include "toys.h"<br>
> +#include "linux/watchdog.h"<br>
> +<br>
> +// Make sure no DEBUG variable is set; change this if you need debug prints!<br>
> +#undef WATCHDOG_DEBUG<br>
> <br>
> <br>
> delete this or turn it into a -v flag, depending on whether it's still<br>
> useful now the toy has been written, or was just useful during development?<br>
> (rtcwake, for example, has this kind of thing in -v, but sleep doesn't.)<br>
<br>
rtcwake -v is in the man page. :)<br>
<br>
> + }<br>
> + }<br>
> +<br>
> + // Intercept terminating signals so we can call our shutdown routine first.<br>
> + if ( intercept_signals(safe_shutdown) ) {<br>
> + perror_exit("failed to intercept desired signals: %d", rc);<br>
> + }<br>
> +#if defined(WATCHDOG_DEBUG)<br>
> + printf("Successfully intercepted signals.\n");<br>
> +#endif<br>
> <br>
> <br>
> sigatexit()<br>
<br>
six of one...<br>
<br>
> + TT.fd = open(watchdog_dev, O_WRONLY);<br>
> + if ( TT.fd == -1 ) {<br>
> + perror_exit("failed to open '%s'", watchdog_dev);<br>
> + }<br>
> <br>
> <br>
> xopen()<br>
> <br>
> <br>
> +#if defined(WDIOC_SETTIMEOUT)<br>
> <br>
> <br>
> as far as i can tell, this has existed since 2.6 kernels, so you don't need the #if?<br>
<br>
I was wondering if maybe it was something like the way uclibc used to be able to<br>
config out support for stuff, but in that case the header would go away...?<br>
<br>
> <br>
> + // SETTIMEOUT takes time value in seconds: s = ms / (1000 ms/s)<br>
> + hw_timer_sec = TT.hw_timer_s;<br>
> + xioctl(TT.fd, WDIOC_SETTIMEOUT, (void *)&hw_timer_sec);<br>
> <br>
> <br>
> (no need for this local if you let USE_TOY do the default for you. even when you<br>
> can't, you can just assign to TT.whatever.)<br>
> <br>
> <br>
> +#endif // WDIOC_SETTIMEOUT<br>
> +<br>
> + // Now that we've got the watchdog device open, kick it periodically.<br>
> + while (1) {<br>
> + write(TT.fd, "\0", 1);<br>
> <br>
> <br>
> xwrite?<br>
<br>
Exiting on failure to write here seems... unhelpful. :)<br></blockquote><div><br></div><div>yeah, good point :-)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
You could do writeall() which retries short writes, but you're only going to get<br>
a 0 length EAGAIN write from a signal without SA_RESTART (which I believe all<br>
SIGDFL does in modern Linux) so the only culprit would be signals we set, which<br>
in this case is SIGTERM which doesn't return.<br>
<br>
That said, SIG_IGN on SIGSTOP might be useful. (Unless somebody WANTS to crash<br>
the system by stopping the watchdog?)<br>
<br>
But the main defense here is having more than one "wake up and poke" period<br>
before the hardware reboot triggers. If something did go wrong (which it<br>
shouldn't), you'd get another bite at the apple before getting thrown out of the<br>
garden.<br>
<br>
I do note that this doesn't seem to have a SOFTWARE reboot mechanism. But<br>
there's no app sending you periodic proof-of-life photos of the kidnapee with<br>
today's newspaper, so how would it know when to? (Again, I'm used to the<br>
embedded system's main app doing this. All _this_ process poking the watchdog<br>
proves is that the process scheduler is still running, not that the system is<br>
capable doing anything useful. The logical process to be doing this in android<br>
would probably be... I dunno, the zygote spawner? What responds to UI touch<br>
screen events, and what launches new apps? I watched several hours of tutorial<br>
on this years ago and then you rewrote it all like 8 months later.)<br>
<br>
Also, I thought you guys quiesce the HELL out of the system to keep battery<br>
usage low when the screen's off, and you're going to wake up a process to poke<br>
the watchdog? (Baseband checks for incoming cell data, that's not even the same<br>
chip? Maybe it's different now...)<br></blockquote><div><br></div><div>(this isn't for Android.)</div><div> </div></div></div></blockquote><div>Ya, sorry- I should probably add that I work in a different group and we're using toybox for a non-Android embedded Linux application.</div><div>Thanks for your comments and for taking the time to clean this up- I will follow up later today after I get a chance to test the updated version!</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Rob<br>
</blockquote></div></div>
</blockquote></div></div>