wdt: the same fixes for the hi3516cv200 generation, where "V" meant the opposite - #235
Conversation
PR Summary by QodoFix watchdog resets and magic-close behavior on cv200-era chips
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Code Review by Qodo
1. Shutdown can leave the watchdog armed
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| hidog_start(); | ||
| hidog_set_heartbeat(cur_margin); |
There was a problem hiding this comment.
1. Shutdown can leave the watchdog armed 🐞 Bug ☼ Reliability
hidog_open() now calls hidog_start() without serializing it with the reboot notifier's hidog_stop(). If an open starts after the notifier stops the timer, it re-arms the watchdog during shutdown; if the notifier stops it between the new start and heartbeat calls, the open instead succeeds with the timer disabled.
Agent Prompt
## Issue description
A concurrent open can re-arm the watchdog after the reboot notifier stops it, or return with the timer disabled when the notifier intervenes between start and heartbeat.
## Fix Focus Areas
- kernel/wdt/hi3516cv200/hi_wdt.c[280-294]
- kernel/wdt/hi3516cv200/hi_wdt.c[480-486]
- kernel/wdt/hi3516av100/hi_wdt.c[280-294]
- kernel/wdt/hi3520dv200/hi_wdt.c[280-294]
## Recommended Fix
Coordinate the open transition and reboot-notifier stop so shutdown cannot be followed by a new start, and a successful open cannot finish with the timer stopped. Apply the same coordination to all three copies.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| expect_close = 0; | ||
| module_put(THIS_MODULE); |
There was a problem hiding this comment.
2. Unloading the module silences an armed watchdog 🐞 Bug ≡ Correctness
hidog_release() calls module_put(THIS_MODULE) even when it leaves the watchdog armed after an unexpected close or a close with nowayout set. Once the descriptor closes, rmmod can reach hidog_exit(), which unconditionally stops the timer, so the watchdog no longer resets the system after the guarded process dies.
Agent Prompt
## Issue description
`hidog_release()` drops the module reference even when a close leaves the watchdog armed. Module removal can then reach `hidog_exit()` and stop the timer, including when `nowayout` is set.
## Fix Focus Areas
- kernel/wdt/hi3516cv200/hi_wdt.c[264-331]
- kernel/wdt/hi3516cv200/hi_wdt.c[659-696]
- kernel/wdt/hi3516av100/hi_wdt.c[264-331]
- kernel/wdt/hi3520dv200/hi_wdt.c[264-331]
## Recommended Fix
Apply the same lifecycle handling to all three copies. Call `module_put()` on close only when `hidog_stop()` was invoked—an expected close with `!nowayout`. Otherwise, retain the reference and record it in an orphan flag; on the next `hidog_open()`, clear that flag instead of calling `__module_get()` again. When `nowayout` is set, take a permanent reference at init, as the watchdog core does, so module removal cannot silently disarm an armed watchdog.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
All three findings were real, two of them things this PR introduced. Fixed in the amended head ( 3 — header edits have no effect. Correct, and the most useful of the three. It also explains an earlier measurement cleanly: stock answered the mainline 1 — shutdown can leave the watchdog armed. Correct, and newly possible because of this PR: before it, 2 — unloading silences an armed watchdog. Correct on the part that matters. For the default case I am leaving the behaviour as it is: the module reference is symmetric, and a watchdog driver stopping its hardware when it unloads is what the watchdog core does too. Reinstating the old pin is what made But the Re-verified after the fixes
Scope is unchanged: only hi3516cv200 is measured; hi3520dv200 is build-verified and hi3516av100 is syntax-checked only, having no tree here. |
826b3bb to
a82b9ad
Compare
|
Re-review still lists 1 and 2 against head 1 — addressed, by shutdown state rather than a lock. The finding asked for "a lock or shutdown state [to] order the complete open against the notifier". It is the latter: hidog_start();
hidog_set_heartbeat(cur_margin);
if (shutting_down)
hidog_stop();That covers both orderings the finding names, and the third:
Every interleaving ends with the timer off, which is the outcome a shutdown wants. The open does still return success with the timer disabled in that window; during a shutdown that is the harmless half of the finding, and failing the open instead would be a larger behaviour change for a path the system is about to leave anyway. On ordering: these parts are single-core ( 2 — adopted for if (!nowayout) {
hidog_set_timeout(0);
hidog_stop();
}For the default case I am keeping the symmetric module reference and letting unload stop the timer, because that is what the watchdog core itself does — If the project would rather CI is green: 35 passed, 0 failed. |
a82b9ad to
2ec4ddf
Compare
…he opposite Fixes OpenIPC/firmware#127. The module is already insmod'ed on cv200/hi3518ev200 and openhisilicon's build is what runs there -- hisilicon-opensdk installs open_wdt.ko over the vendor blob as wdt.ko -- so what #127 is missing is not the module, it is that this copy of the driver never got the fixes the rest of the tree has. Confirmed rather than assumed: a build of the unmodified tree source answers ioctls identically to the module the firmware ships. Measured on a lab hi3518ev200, stock module: a 30 s margin armed, the owner died without "V", and the board ran on for 106 s with CTRL = 0x00000003 enabled, RESEN set LOAD = 0x055D4A80 90,000,000 = 30 x 3 MHz, i.e. twice the margin VALUE = 0x0033F866 counting Armed, running, and never firing, because hidog_release() handed the device to the driver's own kernel thread the moment userspace let go. This generation also had the magic close exactly inverted. hidog_write() assigned the *nowayout module parameter*, so writing "V" -- the documented way to say "I am stopping it deliberately" -- took the branch that left the dog running, while not writing it took the branch that stopped it. A single "V" also flipped nowayout for the rest of the boot, after which every WDIOC_SETOPTIONS was refused with -WDIOS_UNKNOWN, which is -(-1) = +1: a positive ioctl return, so userspace reads the refusal as success. The majestic in the field writes "V" before closing -- it logs "magic close supported" -- so one clean S95majestic stop was enough to disable WDIOC_SETOPTIONS for good. Both observed on the board. Carried over from wdt.c, one for one: the margin is the time to the reset; a ping reloads the counter rather than only clearing a pending interrupt; an unexpected close leaves the dog armed and unfed; opening the device starts the timer and restores the configured margin; WDIOC_GETTIMELEFT; -EINVAL for an out-of-range margin instead of clamping to one nobody asked for; SETTIMEOUT reading the margin back; and a feeder paced on the margin in force rather than the module default. The ioctl numbers all resolve from <linux/watchdog.h>, which is what hi_wdt.c includes. The per-chip watchdog.h sitting beside it is on no include path -- an angle-bracket linux/watchdog.h cannot be shadowed by a file named watchdog.h, and two of the three kbuilds add no -I for the directory at all -- so it is dead and is left alone here rather than edited to look alive. nowayout goes back to meaning what it says: the device cannot be stopped. WDIOS_DISABLECARD answers -EBUSY when it is set, as the watchdog core does, and hidog_exit() no longer stops the hardware either -- otherwise rmmod would be a way to silence a dog already counting down on a process that has died. One module reference per open, dropped on every close: the pin the old code left behind on one of its two paths made rmmod return EAGAIN for the rest of the boot, and nothing else needs it. The orphan_timer bit that went with it is gone. Because open() now starts the hardware, it has to lose a race it could not previously enter: the reboot notifier stops the timer on the way down, and an open landing either side of that could re-arm the watchdog behind the shutdown's back -- or come back holding a device whose timer the notifier had just turned off. The register writes have dog_lock, but arming is start-then-heartbeat and the stop can land between them, so a mutex orders the whole transition instead: the notifier takes it to set a flag and stop, and open takes it to arm, refusing with -ENODEV when the flag is already set. Either the open completes and the notifier stops the timer afterwards, or the notifier runs first and the open refuses. Both sides can sleep, so a mutex is available where the spinlock is not. Two of the commands do not carry the numbers <linux/watchdog.h> gives them on the later SDKs -- WDIOC_SETOPTIONS is _IOWR there and WDIOC_KEEPALIVE is _IO, and an ioctl number encodes direction and argument size -- so a client written for either spelling missed. Only the encoding ever differed, the command number being 4 and 5 on both sides, so the switch normalises on _IOC_NR() and both reach the same case. hi3520dv200 additionally had "if (IS_ERR(p_dog) < 0)", which is never true, so its feeder-creation failure was never detected and p_dog was used as a pointer regardless. hi3516av100 loses two stray debug printks that were the only thing separating its WDIOC_SETOPTIONS from the other two copies. Measured on hi3516cv200, kernel console on UART: - crash and recover: 18 of 18 cycles across four builds. A 30 s margin bit 30 s later (MARK 13:54:38, reset 13:55:09 on the console clock, not the 60 s the old arithmetic gave) and the board came back unattended in 17-28 s every time. - the SP805's two expiries are visible in WDT_VALUE while unfed: it counts to zero, reloads once (0x00E78369 -> 0x19D2B0EE), runs a second full count, and only then resets. That is what the rate/2 load is for, and the default margin reads back as LOAD=0x055D4A7F, 60 x 1,500,000 - 1. - ioctl spellings, after majestic had run and been stopped: stock answered one of four (-ENOTTY for the vendor keepalive, a bogus +1 for SETOPTIONS, -ENOTTY for its vendor spelling); this answers all four. - fourteen clean stop/start cycles leave the module reference at 0 after each stop and 2 after each start, and the device opens throughout -- the shutdown refusal does not bleed into ordinary opens, and repeated reboots under the notifier's mutex shut down without a BUG, a hung task or a reset. - majestic soaked 521 s against its 300 s margin, WDT_VALUE sawtoothing on every 150 s keepalive, no reboot. - clean reboots still shut down rather than resetting under the notifier. hi3516av100 and hi3520dv200 are the same vendor driver and get the same treatment, but neither has hardware here: hi3520dv200 is build-verified only and hi3516av100 has no tree to build against and is syntax-checked only. Only hi3516cv200 is measured above.
2ec4ddf to
0018f1f
Compare
|
Merging at head 1 — Shutdown can leave the watchdog armed: fixed, by construction. The two cannot interleave at all, so neither outcome the finding names is reachable: a shutdown cannot be followed by a new start, and a successful open cannot finish with the timer stopped. Both sides may sleep, so a mutex is available where 2 — Unloading the module silences an armed watchdog: adopted for For the default case the behaviour is intentional and I am not changing it. What actually gates this change is the DUT rather than the static review: 18 of 18 crash-and-recover cycles across four builds, a bite measured at 30 s for a 30 s margin on the UART clock, the SP805's two expiries visible in |
…he board (#2517) Closes #127. OpenIPC/openhisilicon#235. 0f80bb8..ecbc855 is exactly one commit -- the watchdog change -- and nothing else rides along. The watchdog on the hi3516cv200 generation (cv200, hi3518ev200/ev201, and the av100 and 3520dv200 copies of the same driver) was armed, enabled and counting, and still never reset anything: hidog_release() handed the device to the driver's own kernel thread the moment userspace let go, so the one event the watchdog exists for ended with the driver feeding the dog for the rest of the board's life. #127 asks for "the watchdog module" on these parts. It was already insmod'ed, and this package installs open_wdt.ko over the vendor blob as wdt.ko, so openhisilicon's build is what has been running all along -- confirmed by building the unmodified source and watching it answer ioctls exactly as the shipped module does. The module was never the missing piece; the fixes were. Measured on a lab hi3518ev200 before, stock module: a 30 s margin armed, the owner killed, and the board ran on for 106 s with CTRL=0x00000003, LOAD=0x055D4A80 (90,000,000 = 30 x 3 MHz, twice the margin) and VALUE counting down. After: 18 of 18 crash-and-recover cycles, the bite 30 s after arming on a UART clock, the board back unattended in 17-28 s, and a 521 s majestic soak against its 300 s margin. Two more worth knowing about, both specific to this generation: - the magic close was inverted. hidog_write() assigned the nowayout module parameter, so writing "V" left the dog running and not writing it stopped it. A single "V" also flipped nowayout for the rest of the boot, after which every WDIOC_SETOPTIONS was refused with -WDIOS_UNKNOWN, which is -(-1) = +1 -- a positive ioctl return that userspace reads as success. majestic writes "V" before closing, so one clean `S95majestic stop` disabled that ioctl for good. - hi3520dv200 had "if (IS_ERR(p_dog) < 0)", never true, so its feeder creation failure went undetected. Worth knowing before this ships: cv200-era cameras go from a watchdog that never fired to one that does, so a camera whose majestic wedges will now reboot where it previously sat there unguarded. That is the fix, and it is the first thing anyone will notice. Only hi3516cv200 is measured. hi3520dv200 is build-verified and hi3516av100 syntax-checked only, neither having hardware in the lab.
1 participant
Fixes #127 (OpenIPC/firmware).
The watchdog on the hi3516cv200 generation was armed, enabled and counting, and
still never reset anything. This is the same set of defects already fixed in
wdt/wdt.c, applied to the copy that generation builds — plus two that arespecific to it, one of which has a nasty consequence on current firmware.
Verified on a lab hi3518ev200 with the kernel console on UART and a
switchable supply, so every claim below is a measurement.
The premise, checked rather than assumed
#127 asks for "the watchdog module on hi3516cv200/hi3518ev200". It is already
insmoded there, andhisilicon-opensdkinstallsopen_wdt.koover thevendor blob as
wdt.ko— so openhisilicon's build is what runs. I confirmedthat by building the unmodified tree source and showing it answers ioctls
identically to the module the firmware ships. So fixing this source does reach
those boards, and the issue is about the driver, not the packaging.
Stock, measured
A 30 s margin armed, the owner died without
"V", the board ran on for 106 s:Armed, running, never firing —
hidog_release()handed the device to thedriver's own kernel thread the moment userspace let go.
The magic close was inverted
hidog_write()assigned thenowayoutmodule parameter. So writing"V",the documented way to say I am stopping it deliberately, took the branch that
left the dog running, and not writing it took the branch that stopped it.
It gets worse: a single
"V"flipsnowayoutfor the rest of the boot, afterwhich every
WDIOC_SETOPTIONSis refused with-WDIOS_UNKNOWN— which is-(-1)=+1. A positive ioctl return, so userspace reads the refusal assuccess. The majestic in the field writes
"V"before closing (it logsmagic close supported), so one cleanS95majestic stopdisablesWDIOC_SETOPTIONSpermanently. Both observed on the board.What is carried over from
wdt.cThe margin is the time to the reset; a ping reloads the counter instead of only
clearing a pending interrupt; an unexpected close leaves the dog armed and
unfed; opening the device starts the timer and restores the configured margin;
WDIOC_GETTIMELEFT;-EINVALfor an out-of-range margin rather than clampingto one nobody asked for;
SETTIMEOUTreading the margin back; and a feederpaced on the margin in force rather than the module default.
All the ioctl numbers resolve from
<linux/watchdog.h>, which is whathi_wdt.cincludes. The per-chipwatchdog.hbeside it is on no include pathand is not compiled, so it is left alone rather than edited to look alive.
nowayoutgoes back to meaning what it says: the device cannot be stopped.WDIOS_DISABLECARDanswers-EBUSYwhen it is set, as the watchdog core does,and
hidog_exit()no longer stops the hardware either — otherwisermmodwould be a loophole for silencing a dog already counting down on a process that
has died. One module reference per open, dropped on every close; the pin the old
code left on one of its two paths made
rmmodreturnEAGAINfor the rest ofthe boot.
Because
open()now starts the hardware it has to lose a race it could notpreviously enter: the reboot notifier stops the timer on the way down, so the
notifier and a complete open are serialised by a mutex — the notifier takes it to set a flag and stop,
open()takes it to arm and refuses with-ENODEVwhen the flag is set — so the two cannot interleave at all.Two commands also do not carry the numbers
<linux/watchdog.h>gives them onthe later SDKs (
WDIOC_SETOPTIONSis_IOWRthere,WDIOC_KEEPALIVEis_IO, and an ioctl number encodes direction and argument size). Only theencoding ever differed — the command number is 4 and 5 on both sides — so the
switch normalises on
_IOC_NR()and either spelling reaches the same case.And
hi3520dv200hadif (IS_ERR(p_dog) < 0), which is never true, so itsfeeder-creation failure was never detected and
p_dogwas used as a pointerregardless.
Measured after
13:54:38, reset13:55:09) — not the 60 s the old arithmetic gaveWDT_VALUE: counts to zero, reloads once (0x00E78369→0x19D2B0EE), runs a second full count, then resets — which is what therate/2load is for-ENOTTY, one bogus+1, one-ENOTTY); this answers 4 of 40after each stop,2after each start, device opens throughoutWDT_VALUEsawtoothing on every 150 s keepalive, no rebootThe module this PR builds for hi3516cv200 is byte-identical (
a47253c3) tothe one all of the above was run against.
Scope, and what is not tested
hi3516av100andhi3520dv200are the same vendor driver and get the sametreatment, but neither has hardware here:
loses two stray debug printks that were the only thing separating its
WDIOC_SETOPTIONSfrom the other two copies.Only hi3516cv200 is measured. I have deliberately left out of this PR the
same
_IOC_NR()normalisation forwdt/wdt.cand the two later-generationcopies: that code is untested on the chips it would affect, and
hi3516cv6xxcannot even be compiled here. It can follow once there is hardware for it.
One note for reviewers on test design: the clean-shutdown path and the
crash path exercise opposite branches of
hidog_release(), and a run ofSIGKILL cycles will pass while the
"V"path is broken. Both are worthkeeping in any future drill.
Firmware
No firmware pin bump is included.
hisilicon-opensdkin OpenIPC/firmware stillpoints at the previous revision, so nothing changes for built images until that
is bumped separately.