[26.04_linux-nvidia-bos] NVIDIA: SAUCE: watchdog: sbsa_gwdt: stop the watchdog across the whole system-sleep transition - #569
Conversation
BaseOS Kernel ReviewWarning
|
PR Validation ReportPatchscan ✅ No Missing FixesAll cherry-picked commits checked — no missing upstream fixes found. PR Lint ✅ All checks passedDetailsChecking 1 commits... Cherry-pick digest: ┌──────────────┬──────────────────────────────────────────────────────────────────┬────────────┬─────────┬───────────────────────────┐ │ Local │ Referenced upstream / Patch subject │ Patch-ID │ Subject │ SoB chain │ ├──────────────┼──────────────────────────────────────────────────────────────────┼────────────┼─────────┼───────────────────────────┤ │ 1f857bc3ba59 │ [SAUCE] watchdog: sbsa_gwdt: stop the watchdog across the whole │ N/A │ N/A │ dcemin │ └──────────────┴──────────────────────────────────────────────────────────────────┴────────────┴─────────┴───────────────────────────┘ Lint: all checks passed. |
|
@dcemin-nv I created the Launchpad bug for this change: Could you please add this link to the PR description? |
|
Tracking info: internal NVbug 6611666 (watchdog initiated reset during suspend entry on the N1x stress runs). Upstream plan: the patch is prepared against current mainline and is being posted to linux-watchdog (Wim Van Sebroeck, Guenter Roeck cc linux-watchdog@vger.kernel.org); I will add the lore link here once it is archived. |
1237c06 to
a8318b7
Compare
|
I reviewed the current v2 heads of this PR and the paired PR #568. The existing BaseOS Reviewer comments already cover the functional issues. I found three additional commit/metadata items:
|
…e system-sleep transition The driver stops a running watchdog in its own device suspend callback and restarts it in its resume callback. That leaves the watchdog armed, with nobody refreshing it, for the entire early part of suspend entry: userspace freeze, kernel thread freeze, and every device suspend callback that runs before this device's own. The same window exists at the tail end of resume. When the watchdog is running from boot (early_enable=1, previously force_enable=1 downstream; 10 s default timeout) and any device stalls its suspend callback past the timeout, the watchdog resets the system in the middle of suspend entry. On N1x (Yukon) this fired on about 7% of suspend attempts in a randomized stress run (9 resets in 124 suspends; the serial console shows the board dropping into the boot ROM mid-entry with the watchdog reset status set in NONRST_REG2). Two elimination runs confirm the mechanism: the identical stress matrix with the parameter off produced zero resets in 118 suspends, and with this patch applied and the watchdog force-enabled, zero resets in 198 suspends across four runs (the baseline rate predicts about 14). Stop the watchdog from a PM notifier at the *_PREPARE events, before tasks are frozen and device callbacks run, and restart it at the PM_POST_* events, after everything has resumed. Drop the per-device dev_pm_ops: the notifier calls the driver stop path directly, so WDOG_HW_RUNNING stays set and the old resume callback would re-arm the watchdog during device resume, before PM_POST_SUSPEND, leaving it armed and unrefreshed through the thaw. The notifier is the only stop and start point. This is also upstream-relevant as a companion to the early_enable parameter: the armed-during-entry window exists for any system running the SBSA watchdog from boot. Fixes: 57d2caa ("Watchdog: introduce ARM SBSA watchdog driver") Signed-off-by: David Cemin <dcemin@nvidia.com>
a8318b7 to
1f857bc
Compare
|
v2 pushed: 1f857bc (568: e28c4a4). Fixes: 57d2caa trailer added, Change-Id line removed, description updated to the v2 behaviour (no dev_pm_ops, the notifier is the sole stop and start point). On the retest question: the change relative to v1 only removes the redundant device callbacks, so the v1 soak result is claimed for v2. |
nirmoy
left a comment
nirmoy
left a comment
There was a problem hiding this comment.
Codex found the same two issues as the existing Boro review on 1f857bc3ba59. The rendered Boro report places its comments after whole diff hunks, making their alignment with the affected statements misleading. These inline comments attach the findings directly to the relevant code.
Codex rates the restart race P1 and the notifier setup failure P2. Whitespace and strict checkpatch checks passed; no local build or hardware tests were run.
| if (watchdog_hw_running(&gwdt->wdd)) | ||
| sbsa_gwdt_start(&gwdt->wdd); |
There was a problem hiding this comment.
[P1] Synchronize PM restart with userspace watchdog control
PM_POST_SUSPEND runs after processes are thawed, so this check and direct hardware restart can race with a successful userspace disable or magic close (nowayout=false). The core can clear its running/active state and stop the hardware before the notifier re-enables it using the stale check. Hardware is then armed while the core considers it stopped and no longer refreshes it, allowing an unexpected reset.
Please synchronize PM handling with watchdog-core operations. Also account for a userspace start after PREPARE and before task freezing; locking only the callback leaves that entry window open.
| if (ret) | ||
| dev_warn(dev, "Failed to register PM notifier: %d\n", ret); |
There was a problem hiding this comment.
[P2] Treat PM notifier setup as a required probe dependency
If devm_add_action_or_reset() cannot allocate its cleanup record, it immediately calls sbsa_gwdt_unregister_pm_notifier(). This error path then only warns and probe returns success. Since this patch removes the device PM callbacks, the bound driver has no remaining path to stop a running watchdog for system sleep, allowing a reset when sleep exceeds the timeout.
Please make notifier setup mandatory and unwind failures safely, including any watchdog started by early_enable.
3 participants
BugLink: https://bugs.launchpad.net/ubuntu/+source/linux-nvidia/+bug/2166302
Fix for a watchdog initiated system reset during suspend entry, found and root caused on the N1x laptop program.
The driver stops a running watchdog only in its device suspend callback, which leaves it armed with nobody refreshing it through userspace freeze, kernel thread freeze, and every device suspend callback that runs earlier (same window at the tail of resume). With the watchdog running from boot and a 10 second timeout, any slow device suspend resets the system mid entry. On N1x this fired on about 7 percent of suspend attempts in randomized stress runs (9 resets in 124 suspends); two elimination runs confirm the mechanism (0 resets in 118 with the watchdog off, 0 in 198 with this fix and the watchdog force enabled, where the baseline rate predicts about 14).
The fix moves the stop to a PM notifier at the PREPARE events and the restart to PM_POST. v2 removes SET_SYSTEM_SLEEP_PM_OPS entirely: the notifier calls the driver stop path directly, so WDOG_HW_RUNNING stays set and a device resume callback would re-arm the watchdog during dpm_resume, before PM_POST_SUSPEND, leaving it armed and unrefreshed through the thaw. The notifier is the only stop and start point. The commit carries Fixes: 57d2caa ("Watchdog: introduce ARM SBSA watchdog driver").
Validation: the 198 suspend elimination matrix and the 200 cycle confirmation run were performed on v1 (notifier plus dev_pm_ops). v2 is compile verified on both branches; the change relative to v1 only removes the redundant device callbacks, so the v1 soak result is claimed for v2. Shipping in the N1x FastOS kernel.
Notes for review: