wdt: make the watchdog reset the board when the thing it guards dies - #223
Merged
Merged
Conversation
A camera on the field reported rebooting in a loop, healthy the whole
time, and cured only by telling its streamer not to enable the watchdog
at all. That is what this device does to anyone who takes the Linux
watchdog API at its word.
## The margin meant half of itself
The SP805 runs its load value twice: reaching zero raises the interrupt
and reloads, and only reaching zero a *second* time with that interrupt
still pending drives the reset. `dog_set_timeout()` loaded a whole
margin, so every timeout was worth twice what it said — the vendor
readme even documented it, "a reset signal will be sended to the system
after 2 timer interrupt". Measured on a lab gk7205v200, `WDIOC_SETTIMEOUT(30)`
then no feeding: the board came back at 58 s.
Load half a margin, as `drivers/watchdog/sp805_wdt.c` does. Same board,
same test: 29 s.
## A ping did nothing for the first half of every cycle
`dog_feed()` read `WDT_RIS` first and returned without touching a
register when no interrupt was pending. So a ping only counted once the
counter had already run out — the deadline stayed wherever the previous
*accepted* ping had put it, and the textbook cadence of one ping per
half-margin was worth exactly one accepted ping per cycle. Miss it and a
process that had been feeding the device all along gets the board reset
under it.
Write the load and clear the interrupt unconditionally, which is what
WDIOC_KEEPALIVE means and what sp805_wdt.c does. Proven through the new
GETTIMELEFT on the lab board: margin 60, time left falls 59 -> 39 over
twenty seconds, and a single KEEPALIVE puts it back to 59. Before, that
ping moved nothing.
## And it could not be stopped
`ident.options` has advertised `WDIOF_MAGICCLOSE` all along with
`dog_write()` compiled out, so the documented way to say "this shutdown
is deliberate" — write "V", then close — returned EINVAL, and a process
that started the device had no way to stop it. `dog_write()` is enabled
and wired into `dog_fops`; `dog_release()` honours it and otherwise says
"Unexpected close, not stopping watchdog!" like every other driver, plus
a line naming `nodeamon=1` as the case where nothing is left feeding.
## The rest of the API surface
- `WDIOC_SETTIMEOUT` reports back the margin programmed, falling through
to GETTIMEOUT the way the watchdog core does, and leaves the margin
alone on a value out of range instead of half-applying it.
- `WDIOC_GETTIMELEFT` is implemented, and uncommented in `watchdog.h`.
It adds the outstanding load back when no interrupt is pending, so it
answers seconds-to-reset rather than seconds-to-first-expiry.
- Errors are `-EINVAL` / `-ENOTTY` rather than a bare `-1`, which
userspace was reading as EPERM.
- `dog_deamon` paces itself off the margin in force rather than off
`default_margin`. Handed a device whose margin userspace had made
smaller, it was feeding slower than the margin it was feeding, and
reset the board on a cycle nobody configured. Capped at 30 s so a long
margin cannot make module unload wait out a sleep.
- `expect_close` is cleared on open, so a "V" from a previous opener
cannot stop the timer for the next one.
`readme-eng.txt` and the `default_margin` parameter description say the
new contract; the old bound in the latter ("0<default_margin<80") was
never the real one.
Built for gk7205v200 against the OpenIPC 4.9.37 kernel and run on a lab
gk7205v200: the numbers above, then majestic on top of it feeding at its
own cadence, uptime past 900 s.
The driver's own kernel thread fed the watchdog whenever dog_state was DOG_SELFCLR, and dog_release() set DOG_SELFCLR on every close. So the one event the device exists for -- the process being guarded dies, and the kernel closes its descriptor for it -- handed the watchdog straight back to the driver, which then fed it for the rest of the board's life. Nothing ever reset. OpenIPC/firmware#1803 reports exactly this on hi3516ev300 and gk7205v300, and the only workaround available was nodeamon=1: no kernel feeder at all, and therefore no cover for the window between module load and the first open either. WDIOF_MAGICCLOSE has been advertised all along, and it means a close without a preceding "V" leaves the timer running. Honour that: stop the device only when the caller asked for it, and otherwise leave it armed and unfed. The feeder now covers the pre-open window and nothing else -- dog_open() latches DOG_EXTCLR and dog_release() no longer undoes it. Measured on a lab hi3516ev300, stock module: majestic SIGKILLed at uptime 40195, board still up and counting at 40400.
watchdog-api.txt is unambiguous -- the timer runs from the moment /dev/watchdog is opened -- and that held here only for as long as nothing had deliberately stopped the counter. A magic close, or WDIOC_SETOPTIONS with WDIOS_DISABLECARD, clears WDT_CTRL, and neither the ping in dog_open() nor WDIOC_SETTIMEOUT puts it back. From the first deliberate stop until someone thought to send WDIOS_ENABLECARD the device was inert while looking entirely healthy: the open succeeded, the margin was accepted and read back, the pings were accepted, and nothing would ever have reset the board. majestic did not notice because it sends WDIOS_ENABLECARD itself right after opening, which is what re-armed the counter. A client that only does what the API documents -- busybox watchdog, systemd -- got nothing. Measured on a lab hi3516ev300: with a prior magic close, open + margin 30 + die without "V" left the board up indefinitely before this, and resets it between 26 s and 35 s after.
hi3516cv500 does not build wdt/wdt.c -- hi3516cv500.kbuild points at
wdt/hi3516cv500/hi_wtdg.c, a second copy of the same vendor driver with
the same defects, and the boards it covers (hi3516cv500, hi3516av300)
are affected identically. Measured on a lab hi3516av300 before this:
majestic SIGKILLed at uptime 215043, board still up and counting at
215249.
Carries over, one for one:
- the margin is the time to the reset. hidog_set_timeout() loaded a
whole margin into a counter the SP805 runs twice, so every timeout
meant twice itself.
- a ping restarts the timer. hidog_feed() returned without touching
anything unless an interrupt was already pending, and even then only
cleared it without reloading -- so a ping did nothing until the
counter had already run out once.
- an unexpected close leaves the dog to bite. hidog_release() set
HIDOG_SELFCLR and re-fed, handing the device to the driver's own
kernel thread at exactly the moment userspace stopped feeding it.
- opening the device starts the watchdog. hidog_open() only pinged,
and nothing re-enabled HIWDT_CTRL after a stop.
Plus what the V4 copy also got: a write handler, so the WDIOF_MAGICCLOSE
this has always advertised is reachable at all; WDIOC_GETTIMELEFT;
-EINVAL for an out-of-range margin instead of clamping to one nobody
asked for; SETTIMEOUT reading back the margin in force; -ENOTTY for an
unknown ioctl; and a feeder paced on the margin in force rather than the
module default.
Measured after, on the same board: open, margin 30, die without "V"
resets between 27 s and 36 s, and GETTIMELEFT reports 299 of 300.
Seven more copies of this driver remain in wdt/, for chips with no
hardware here to measure on.
watchdog_exit() ran dog_exit() first and unregistered afterwards.
dog_exit() sets DOG_EXIT and then blocks in kthread_stop() waiting for
the feeder thread to notice -- and the feeder's loop condition is
dog_state, which dog_open() sets straight back to DOG_EXTCLR. With the
device still registered for that whole window, one open() racing the
unload left the thread looping for ever:
rmmod D kthread_stop
osal_kthread_destory [open_osal]
watchdog_exit [open_wdt]
wdg_remove [open_wdt]
platform_drv_remove
SyS_delete_module
module_mutex held, the task unkillable, and the board did not survive a
clean reboot afterwards -- it needed the power pulled. Hit on a lab
gk7205v200 by nothing more exotic than running a probe against
/dev/watchdog while an rmmod was in flight. Nothing can rescue it once
lost, either: dog_release() leaves dog_state at SELFCLR and dog_open()
at EXTCLR, and neither is DOG_EXIT.
Unregistering first closes the window: after osal_deregisterdevice()
there is no node to open, so nothing can put dog_state back while
dog_exit() waits.
Both copies of the driver have it; both are fixed. Measured on the same
gk7205v200: with a shell loop opening and closing /dev/watchdog as fast
as it can, rmmod now returns 0 in two seconds and the module is gone.
The SP805 watchdog was in the tree ten times: wdt/wdt.c for the V4 parts
and a near-identical copy under wdt/<chip>/ for everyone else, differing
by register base, a prefix on every identifier, and nothing else that
mattered. Six API defects had to be found and fixed twice already.
hi3516cv500 and hi3516cv300 now build wdt/wdt.c, which takes what
actually differs from the kbuild:
WDT_BASE the register map
WDT_OSAL_HI OSAL is hi_osal.h here, not osal.h
WDT_OSAL_NO_KTHREAD_STOP_FLAG V3's osal_kthread_destory() takes the
task alone and always stops it
WDT_NO_OF no watchdog node in the device tree
WDT_SCTL_BASE the 3 MHz clock needs ungating first
wdt_init.c grew the second binding style for WDT_NO_OF and now names
the device-tree compatibles outright instead of assembling one from
compat.h, which only knows the two V4 chips. That retires
init/hi3516cv500/wtdg_init.c as well.
hi3516cv300 gains a watchdog it never had. Its copy had no module entry
point at all -- hi3516cv300_wdt.ko loaded, did nothing, and the board
came up with no /dev/watchdog, so majestic logged "Watchdog device
(/dev/watchdog) not found" and every cv300 ran unguarded. Confirmed on
a lab hi3516cv300 before the change; after it the node is there and a
crash resets the board at the margin.
Measured after, all four boards and both binding styles: open, margin
30, die without "V" resets between 26 s and 36 s and the board comes
back; GETTIMELEFT reads 299 of 300; the magic-close write returns 1; a
clean majestic stop resets nothing.
hi3516cv300 WDT_NO_OF, 0x12080000 (was: no device at all)
hi3516cv500 platform, 0x12051000 (hi3516av300)
hi3516ev200 platform, 0x12030000 (hi3516ev300)
gk7205v200 platform, 0x12030000
Left with their own copy, for reasons that are not cosmetic:
hi3516cv6xx, hi3519dv500 a later vendor generation -- optional FIQ
feeding path, an hrtimer, ot_osal.h, command-list ioctl dispatch.
All six fixes are carried over to both in place instead. There is
no firmware tree here for either, so those two changes are
reviewed but neither compiled nor run.
hi3516cv100, hi3519v101 their kbuild links a vendor .o; the source
beside it is reference and reaches no board.
hi3516cv200, hi3516av100, hi3520dv200 V2 has no OSAL at all (the
osal_v2_shim is a kernel-API shim for blobs, not an OSAL), and the
driver there is written against plain kernel APIs. Sharing this
file would mean writing an OSAL for the generation first.
PR Summary by QodoFix watchdog reset semantics and consolidate SP805 drivers
AI Description
Diagram
High-Level Assessment
Files changed (10)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Three things review caught, all of them consequences of the open path arming the hardware where it previously did not. dog_stop() parks the load at the counter maximum, because it disarms by way of dog_set_timeout(0). Starting the device in dog_open() and then merely pinging it wrote that maximum straight back, so a client that stopped the watchdog and reopened it without setting the timeout again got roughly the counter's whole range -- 1431 s at 3 MHz -- while WDIOC_GETTIMEOUT went on reporting the margin it had asked for. Restore the heartbeat instead of pinging, which is what the WDIOS_ENABLECARD path already did for the same reason. Reproduced and fixed on all four boards: before GETTIMEOUT -> 30, GETTIMELEFT -> 1431 after GETTIMEOUT -> 30, GETTIMELEFT -> 29 dog_init() arms the hardware before creating the feeder, and returned failure on osal_kthread_create() without disarming. watchdog_init() then unmapped the registers and failed the load, leaving an armed watchdog with nothing in the system able to feed it: the board resets one margin later, every boot. It only mattered on paths that reach dog_init() at all, which hi3516cv300 now does for the first time. dog_write() set expect_close on the first "V" and could then return -EFAULT on a later byte with the flag still set, so a write that failed part way through a buffer crossing into an unmapped page armed a magic close the caller never completed -- and the next close stopped the watchdog instead of leaving it to bite. The watchdog core has the same shape, but here the cost of getting it wrong is a board that quietly stops being guarded, so clear it on the error path. All three in wdt.c and in both later-generation copies.
Member
Author
|
All five findings were real and are fixed in 88528b4. Two of them were mine and my testing had masked them — every probe set a timeout after opening, which is exactly the step that hid the problem. 2 and 3 — reopen ignores the configured timeout. Correct, and the more serious of the five.
1 — a failed load leaves the dog armed. Correct. 4 and 5 — All three changes are in On the alternative approaches in the summary: agreed that migrating to the Linux watchdog framework is the right long-term answer and the wrong thing to combine with a behavioural fix for these vendor kernels. Runtime platform data would also be a reasonable shape, but every target already builds a SoC-specific module, so the compile-time flags cost nothing here and keep the no-device-tree path simple. One correction to the PR description while I am here: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes OpenIPC/firmware#1803.
The watchdog driver did not honour the Linux watchdog API in six ways, and the
driver exists ten times in this tree, so each one had to be found more than
once. This fixes all six and retires the duplication where the copies were the
same driver.
The defects
Measured on lab boards, kernel 4.9.37 and 3.18.20.
1. The margin meant twice itself.
set_timeout()loaded a whole margin intoa counter the SP805 runs twice — reaching zero raises the interrupt and reloads,
and only a second zero with that interrupt still pending drives the reset. A
margin of 30 s reset the board at 58 s. Loading half a margin is what
drivers/watchdog/sp805_wdt.cdoes.2. A ping did nothing for half the cycle.
feed()readWDT_RISfirst andreturned without touching anything when no interrupt was pending, so the
deadline stayed where the previous accepted ping had put it. The honest cadence
of one ping per half-margin was therefore worth exactly one accepted ping per
cycle: miss it and a process that had been feeding the device all along got the
board reset under it. The cv500 and V5 copies were worse — even when the
interrupt was pending they only cleared it, never reloading.
3. An unexpected close handed the dog back to the driver. This is #1803.
The driver runs a kernel thread that feeds the watchdog whenever
dog_stateisSELFCLR;open()setEXTCLRso the thread stood down, butrelease()setSELFCLRagain — unconditionally. So the one event the device exists for, theguarded process dying and the kernel closing its descriptor, handed the watchdog
straight back to the driver, which then fed it for the rest of the board's life.
Nothing ever reset. The only workaround available was
nodeamon=1: no feeder atall, and therefore no cover for the module-load-to-first-open window either.
WDIOF_MAGICCLOSEhas been advertised throughout, and it means a close withouta preceding
"V"leaves the timer running. The feeder now covers the pre-openwindow and nothing else —
open()latchesEXTCLRone-way andrelease()nolonger undoes it — and a write handler makes the magic close reachable at all.
4.
open()did not start the timer. A magic close, orWDIOC_SETOPTIONSwith
WDIOS_DISABLECARD, clearsWDT_CTRL, and neither the ping inopen()nor
WDIOC_SETTIMEOUTputs it back. From the first deliberate stop the devicewas inert while looking entirely healthy: the open succeeded, the margin was
accepted and read back, the pings were accepted, and nothing would ever have
reset the board. majestic did not notice because it sends
WDIOS_ENABLECARDitself right after opening; a client that only does what the API documents
(busybox watchdog, systemd) got nothing.
5.
rmmodcould wedge the board permanently.watchdog_exit()stopped thefeeder before unregistering the device.
dog_exit()setsDOG_EXITand thenblocks in
kthread_stop()waiting for the thread to notice — and the thread'sloop condition is
dog_state, whichopen()sets straight back toEXTCLR.One open racing the unload left the thread looping for ever:
module_mutexheld, the task unkillable, and the board did not survive a cleanreboot — it needed the power pulled. Hit on a lab gk7205v200 by nothing more
exotic than running a probe against
/dev/watchdogwhile anrmmodwas inflight, and unrecoverable once lost:
release()leaves the state atSELFCLRand
open()atEXTCLR, neither of which isDOG_EXIT. Unregistering firstcloses the window. Verified on the same board — with a shell loop opening and
closing
/dev/watchdogas fast as it can,rmmodnow returns 0 in two seconds.6. hi3516cv300 had no watchdog at all.
wdt/hi3516cv300/hi_wdt.ccontainedno module entry point and its kbuild linked no init object, so
hi3516cv300_wdt.koloaded, did nothing, and the board came up with no/dev/watchdog. majestic loggedWatchdog device (/dev/watchdog) not foundand every cv300 ran unguarded. Confirmed on a lab hi3516cv300 before this
change.
Smaller things alongside:
WDIOC_GETTIMELEFT(wasEPERMeverywhere);-EINVALfor an out-of-range margin instead of clamping to one nobody askedfor;
SETTIMEOUTreading back the margin in force;-ENOTTYfor an unknownioctl; and a feeder paced on the margin in force rather than the module default,
so a caller that lowers the margin and hands the device back is not reset on a
cycle nobody configured.
The consolidation
hi3516cv500 and hi3516cv300 now build
wdt/wdt.candwdt/wdt_init.c. Whatactually differs comes from the kbuild:
WDT_BASEWDT_OSAL_HIhi_osal.hhere, notosal.hWDT_OSAL_NO_KTHREAD_STOP_FLAGosal_kthread_destory()takes the task alone and always stops itWDT_NO_OFWDT_BASEdirectlyWDT_SCTL_BASEwdt_init.ccarries both binding styles and names the device-tree compatiblesoutright rather than assembling one from
compat.h, which only knows the two V4chips; that retires
init/hi3516cv500/wtdg_init.ctoo.DOG_TIMER_MARGINmovedinto
wdt/watchdog.h, wherewdt_init.ccan see it — the parameter descriptionused to stringify the undefined token and print
(default=DOG_TIMER_MARGIN).Net −837 lines.
Verified
Four boards, both binding styles. Crash path is open,
SETTIMEOUT(30), diewithout
"V"; the board must reset one margin later and come back.GETTIMELEFTWDT_NO_OF0x120800000x120510000x120300000x12030000Soaks with majestic feeding at its own cadence, past the 300 s margin: cv300
456 s, av300 491 s, ev300 1327 s, no reboot. On cv300 majestic now logs
Detected Watchdog/Watchdog timeout set to 300 secondsfor the first time.Builds clean for hi3516cv300, hi3516cv500, hi3516ev200 (both
output-ev200andoutput-hi3516ev300) and gk7205v200 (bothoutput-gkandoutput-gk3).Not consolidated, and why
feeding path, an hrtimer,
ot_osal.h, and a command-list ioctl dispatchrather than
unlocked_ioctl. All six fixes are carried over to both in placeinstead. There is no firmware tree on this machine for either, so those two
files are reviewed but neither compiled nor run — the one untested part of
this branch, and the thing most worth a second pair of eyes.
.o; the sourcebeside it is reference and reaches no board. Benefiting needs a blob→source
switch first.
osal_v2_shimis a kernel-API compat shim for blobs, not an OSAL), and thedriver there is written against plain kernel APIs. Sharing
wdt.cwould meanwriting an OSAL for the generation first.
WDT_SCTL_BASEexists inwdt.cready for them.
Also related: OpenIPC/firmware#127 asks for the watchdog module on
hi3516cv200/hi3518ev200. It is already
insmoded there (the#nodeamon=1beside it in
load_hisiliconis a comment, not a parameter) — but that.koisa prebuilt vendor blob in the firmware package, so none of this reaches those
boards until it is built from source.
Worth a release note
cv300 goes from having no watchdog to having a working one. That is the fix, but
it does mean a cv300 whose majestic wedges will now reboot where it previously
sat there. Benign, and the point of the device, but it is a behaviour change in
the field.