Restrict deadman and runtime-PM targets and add command timeouts

.github/workflows/build.yaml

@@ -8,7 +8,6 @@ on:
       - v*
     paths-ignore:
       - LICENSE
-      - README.md
   pull_request:
     branches:
       - main

Makefile

@@ -52,7 +52,7 @@ check-dist:  ## check python sdist and wheel with check-dist
 check-types:  ## check python types with ty
 	ty check --python $$(which python)
 
-checks: check-dist
+checks: check-dist check-types
 
 # Alias
 check: checks
@@ -101,7 +101,7 @@ dist-check:  ## run python dist checker with twine
 
 dist: clean dist-build dist-check  ## build all dists
 
-publish: dist  ## publish python assets
+publish: dist  ## build the distributions a release would publish (publishing is a separate step)
 
 #########
 # CLEAN #

README.md

@@ -5,14 +5,37 @@ Host utilities for FPGA bench machines
 [![Build Status](https://github.com/dau-dev/dau-utils/actions/workflows/build.yaml/badge.svg?branch=main&event=push)](https://github.com/dau-dev/dau-utils/actions/workflows/build.yaml)
 [![codecov](https://codecov.io/gh/dau-dev/dau-utils/branch/main/graph/badge.svg)](https://codecov.io/gh/dau-dev/dau-utils)
 [![License](https://img.shields.io/github/license/dau-dev/dau-utils)](https://github.com/dau-dev/dau-utils)
-[![PyPI](https://img.shields.io/pypi/v/dau-utils.svg)](https://pypi.python.org/pypi/dau-utils)
 
 ## Overview
 
-Two small command-line tools for the Linux machine an FPGA card is plugged
-into. Each one does a single thing and leaves the decisions (which devices,
-when to arm) to the caller. Higher layers such as `dau-build` run them
-from their hardware command plans; you can also run them by hand.
+Small command-line tools for the Linux machine an FPGA card is plugged into.
+Each one does a single thing and leaves the decisions (which devices, when to
+arm) to the caller. Higher layers such as `dau-build` run them from their
+hardware command plans; you can also run them by hand. Both tools print what
+they would do with `--dry-run`, which needs no privilege, and refuse an
+ambiguous request rather than guessing.
+
+## Requirements
+
+- Linux with systemd (the deadman schedules a transient timer) and the
+  `sysrq` trigger enabled for `b` (`kernel.sysrq` must allow it).
+- `sudo` without a password prompt for the invoking user, for `systemd-run`
+  and `systemctl`: the deadman runs them with `sudo -n` and treats a prompt
+  as a failure. The runtime-PM tool writes sysfs directly and is run under
+  `sudo` itself.
+- `pciutils` (`lspci`) for pattern-based device discovery.
+- Python 3.11 or later.
+
+## Installation
+
+There is no package index release yet. Install from a clone, or straight from
+GitHub:
+
+```bash
+pip install git+https://github.com/dau-dev/dau-utils.git
+```
+
+For development, clone and run `make develop`.
 
 ## Deadman: recover from a hung PCIe device
 
@@ -29,12 +52,26 @@ dau-utils-deadman disarm                # operation returned: cancel the reboot
 dau-utils-deadman status
 ```
 
+The timeout is a policy choice: long enough for the operation to finish,
+short enough that a wedged host comes back without anyone present. The
+default is 180 seconds.
+
 If the host hangs before `disarm`, the timer fires `sysrq-b`
 (`emergency_restart()`), which resets the machine without going through the
-stuck driver. The same operations are available from Python:
-`dau_utils.deadman.arm`, `disarm`, `is_armed`, and an `armed()` context
-manager. Failures raise `DeadmanError`; if you see one, assume the host is
-not protected.
+stuck driver. `arm` confirms the timer is active before it reports success,
+and `disarm` confirms it is gone; a missing command, a failed `systemd-run`,
+or a call that does not answer within fifteen seconds raises `DeadmanError`.
+If you see one, assume the host is not protected, or not yet safe.
+
+The unit name is the tool's own (`dau-deadman`, or `dau-deadman-<suffix>` to
+run more than one); it will not stop or reset any other unit.
+
+The same operations are available from Python: `dau_utils.deadman.arm`,
+`disarm`, `is_armed`, and an `armed()` context manager. The context manager
+disarms on the way out whatever the block did, including on an exception,
+which is right for Python work. A hardware plan that fails a step must leave
+the timer armed, so that the host reboots if the step wedged it; the plans in
+`dau-build` call `arm` and `disarm` directly for that reason.
 
 Arm it before every PCIe rescan, flash or register probe against a device
 that has hung before. Synthesis and other work that does not touch PCIe does
@@ -43,23 +80,29 @@ not need it.
 ## Runtime PM: hold and release PCIe power management
 
 `dau-utils-pci-runtime-pm` holds or releases Linux PCI runtime power
-management for devices matched against `lspci -Dnn`. You choose the patterns;
-the tool does the sysfs writes:
+management for devices named by PCI address or matched against `lspci -Dnn`
+by text pattern. You choose the devices; the tool writes `power/control` and
+`d3cold_allowed` under the sysfs device root:
 
 ```bash
-dau-utils-pci-runtime-pm hold    --pattern Thunderbolt --pattern 10ee:7011
-dau-utils-pci-runtime-pm release --pattern Thunderbolt --pattern 10ee:7011
+sudo dau-utils-pci-runtime-pm hold    --device 0000:04:00.0
+sudo dau-utils-pci-runtime-pm hold    --pattern "Downstream Bridge" --pattern abcd:ef01
+sudo dau-utils-pci-runtime-pm release --pattern "Downstream Bridge" --pattern abcd:ef01
 ```
 
-`--dry-run` prints the writes it would make without touching sysfs.
+A device must be a PCI address (`dddd:bb:dd.f`), which is the only form that
+stays inside the sysfs root. `--device` and `--pattern` are alternatives, not
+a combination. Every attribute in the plan is checked before the first write,
+so a device is never left half held. A pattern that matches nothing is
+tolerated, and said on stderr, because the device being recovered may not
+enumerate.
+
+`--dry-run` prints the writes without touching sysfs and needs no privilege.
 
 ## Development
 
 ```bash
-pip install -e .[develop]
+make develop
 python -m pytest dau_utils/tests
 python -m ruff check dau_utils
 ```
-
-> [!NOTE]
-> This library was generated using [copier](https://copier.readthedocs.io/en/stable/) from the [Base Python Project Template repository](https://github.com/python-project-templates/base).

dau_utils/deadman.py

@@ -1,28 +1,44 @@
 """Prime a forced reboot before a risky PCIe operation, cancel it on success.
 
-The dpv1 wedge class that needs a power cycle is a driver/PCIe hang where the
-kernel and systemd stay alive -- they keep petting the hardware watchdog, so
-systemd's ``RuntimeWatchdogSec`` never fires, yet ``systemctl reboot`` hangs on
-the stuck device. The recovery is ``sysrq-b`` (``emergency_restart()``), which
+The hang this recovers from is a driver or PCIe wedge where the kernel and
+systemd stay alive: they keep petting the hardware watchdog, so systemd's
+``RuntimeWatchdogSec`` never fires, yet ``systemctl reboot`` blocks on the
+stuck device. The recovery is ``sysrq-b`` (``emergency_restart()``), which
 resets immediately without touching the wedged driver.
 
 ``arm`` schedules that reset as a transient systemd timer so it survives the
 controlling SSH session; ``disarm`` cancels it. Run ``arm`` before a rescan,
 flash, or register probe and ``disarm`` once it returns cleanly. If the box
 wedges before ``disarm``, the timer fires and reboots it. A full kernel lock
 (systemd itself dead) is still caught by systemd's own hardware watchdog.
+
+Every call into systemd is bounded and non-interactive (``sudo -n``), and a
+call that fails, hangs or is missing surfaces as :class:`DeadmanError`: the
+caller must not treat the host as protected, or as safe, on anything less
+than a confirmed answer.
 """
 
 from __future__ import annotations
 
 import argparse
+import re
+import shlex
 import subprocess
 import sys
-from collections.abc import Iterator, Sequence
+from collections.abc import Generator, Sequence
 from contextlib import contextmanager
 
 DEFAULT_UNIT = "dau-deadman"
+# policy: long enough for a reprogram and rescan to complete, short enough
+# that a wedged host comes back without anyone present
 DEFAULT_TIMEOUT_S = 180
+# systemd answers in well under a second; a call that takes this long is
+# the hang this tool exists to survive, and must not hang the tool itself
+COMMAND_TIMEOUT_S = 15
+
+# the only units this tool may create, stop or reset: its own namespace. An
+# arbitrary --unit would let `disarm` stop any service on the host.
+_UNIT = re.compile(r"dau-deadman(-[a-z0-9]+)*")
 
 # sysrq 'b' = emergency_restart(): reboot past a wedged driver, where a clean
 # `systemctl reboot` would block on it. Best-effort sync first so a live
@@ -39,12 +55,21 @@ class DeadmanError(RuntimeError):
     """A deadman operation could not be confirmed -- treat the host as unsafe."""
 
 
+def check_unit(unit: str) -> str:
+    """The unit name, if it is in this tool's namespace."""
+    if not _UNIT.fullmatch(unit):
+        raise ValueError(f"deadman unit must be dau-deadman or dau-deadman-<suffix>, got {unit!r}")
+    return unit
+
+
 def arm_command(timeout_s: int = DEFAULT_TIMEOUT_S, *, unit: str = DEFAULT_UNIT) -> tuple[str, ...]:
     """The ``systemd-run`` invocation that schedules the reset ``timeout_s`` from now."""
+    check_unit(unit)
     if timeout_s < 1:
         raise ValueError(f"deadman timeout must be at least 1 second, got {timeout_s}")
     return (
         "sudo",
+        "-n",
         "systemd-run",
         f"--unit={unit}",
         f"--on-active={timeout_s}",
@@ -58,71 +83,120 @@ def arm_command(timeout_s: int = DEFAULT_TIMEOUT_S, *, unit: str = DEFAULT_UNIT)
 
 def disarm_commands(*, unit: str = DEFAULT_UNIT) -> tuple[tuple[str, ...], ...]:
     """Stop the pending timer/service and clear any failed state, idempotently."""
+    check_unit(unit)
     return (
-        ("sudo", "systemctl", "stop", f"{unit}.timer", f"{unit}.service"),
-        ("sudo", "systemctl", "reset-failed", f"{unit}.timer", f"{unit}.service"),
+        ("sudo", "-n", "systemctl", "stop", f"{unit}.timer", f"{unit}.service"),
+        ("sudo", "-n", "systemctl", "reset-failed", f"{unit}.timer", f"{unit}.service"),
     )
 
 
 def status_command(*, unit: str = DEFAULT_UNIT) -> tuple[str, ...]:
     """List the pending deadman timer, if armed."""
+    check_unit(unit)
     return ("systemctl", "list-timers", "--all", f"{unit}.timer")
 
 
+def _run(command: Sequence[str], *, check: bool) -> subprocess.CompletedProcess:
+    """One bounded, captured call; every failure mode is a DeadmanError."""
+    try:
+        return subprocess.run(tuple(command), check=check, capture_output=True, text=True, timeout=COMMAND_TIMEOUT_S)
+    except FileNotFoundError as error:
+        raise DeadmanError(f"{command[0]} is not installed: {error}") from error
+    except subprocess.TimeoutExpired as error:
+        raise DeadmanError(f"{shlex.join(command)} did not answer within {COMMAND_TIMEOUT_S}s") from error
+    except subprocess.CalledProcessError as error:
+        detail = (error.stderr or error.stdout or "").strip()
+        raise DeadmanError(f"{shlex.join(command)} failed with exit {error.returncode}: {detail}") from error
+    except OSError as error:
+        raise DeadmanError(f"{shlex.join(command)} could not run: {error}") from error
+
+
 def _is_active(name: str) -> bool:
-    """True only if systemctl positively reports ``name`` running. A failed
-    query (D-Bus down, sudo denied) is treated as active -- we cannot claim a
-    unit is stopped unless systemctl confirms it."""
-    result = subprocess.run(("systemctl", "is-active", name), check=False, capture_output=True, text=True)
+    """True only if systemctl positively reports ``name`` not running. A failed
+    query (D-Bus down, sudo denied, a hang) is treated as active -- we cannot
+    claim a unit is stopped unless systemctl confirms it."""
+    try:
+        result = _run(("systemctl", "is-active", name), check=False)
+    except DeadmanError:
+        return True
     return result.stdout.strip() not in _INACTIVE_STATES
 
 
 def is_armed(*, unit: str = DEFAULT_UNIT) -> bool:
     """True if a deadman timer or its service is still live for ``unit``."""
+    check_unit(unit)
     return _is_active(f"{unit}.timer") or _is_active(f"{unit}.service")
 
 
 def arm(timeout_s: int = DEFAULT_TIMEOUT_S, *, unit: str = DEFAULT_UNIT) -> None:
-    """Schedule the forced reset. Refuses to stomp an already-armed timer so
-    concurrent callers cannot silently cancel each other's protection; clears
-    only inactive stale state before scheduling."""
+    """Schedule the forced reset and confirm the timer is live. Refuses to
+    stomp an already-armed timer so concurrent callers cannot silently cancel
+    each other's protection; clears only inactive stale state before
+    scheduling."""
+    command = arm_command(timeout_s, unit=unit)
     if is_armed(unit=unit):
         raise DeadmanError(f"{unit} is already armed; disarm it before arming again")
-    subprocess.run(("sudo", "systemctl", "reset-failed", f"{unit}.timer", f"{unit}.service"), check=False, capture_output=True)
-    subprocess.run(arm_command(timeout_s, unit=unit), check=True)
+    _run(disarm_commands(unit=unit)[1], check=False)
+    _run(command, check=True)
+    if not is_armed(unit=unit):
+        raise DeadmanError(f"systemd-run returned but {unit}.timer is not active; the host is NOT protected")
 
 
 def disarm(*, unit: str = DEFAULT_UNIT) -> None:
     """Cancel the pending reset and confirm it is gone. Raises ``DeadmanError``
     if the timer cannot be verified inactive -- the caller must not treat the
     host as safe until this returns cleanly."""
-    subprocess.run(("sudo", "systemctl", "stop", f"{unit}.timer", f"{unit}.service"), check=False, capture_output=True)
+    stop, reset_failed = disarm_commands(unit=unit)
+    _run(stop, check=False)
     if is_armed(unit=unit):
         raise DeadmanError(f"{unit} still armed after stop; reset may still fire -- intervene before trusting the host")
-    subprocess.run(("sudo", "systemctl", "reset-failed", f"{unit}.timer", f"{unit}.service"), check=False, capture_output=True)
+    _run(reset_failed, check=False)
+
+
+def status(*, unit: str = DEFAULT_UNIT) -> int:
+    """Print the timer listing; return systemctl's exit code."""
+    result = _run(status_command(unit=unit), check=False)
+    sys.stdout.write(result.stdout)
+    sys.stderr.write(result.stderr)
+    return result.returncode
 
 
 @contextmanager
-def armed(timeout_s: int = DEFAULT_TIMEOUT_S, *, unit: str = DEFAULT_UNIT) -> Iterator[None]:
-    """Arm around a risky block; disarm on the way out, success or exception."""
+def armed(timeout_s: int = DEFAULT_TIMEOUT_S, *, unit: str = DEFAULT_UNIT) -> Generator[None, None, None]:
+    """Arm around a risky block; disarm on the way out, success or exception.
+
+    This is for Python callers whose failure is a Python exception. A hardware
+    plan that fails a step must leave the timer ARMED so the host reboots if
+    the step wedged it; use ``arm``/``disarm`` directly there.
+    """
     arm(timeout_s, unit=unit)
     try:
         yield
     finally:
         disarm(unit=unit)
 
 
-def main(argv: Sequence[str] | None = None) -> int:
+def _parser() -> argparse.ArgumentParser:
     parser = argparse.ArgumentParser(description="Prime a forced reboot before a risky PCIe op; cancel it on success")
-    parser.add_argument("action", choices=("arm", "disarm", "status"), help="Deadman action")
-    parser.add_argument("--timeout", type=int, default=DEFAULT_TIMEOUT_S, help="Seconds before the reset fires when arming")
-    parser.add_argument("--unit", default=DEFAULT_UNIT, help="Transient systemd unit name")
-    parser.add_argument("--dry-run", action="store_true", help="Print the command(s) without running them")
-    args = parser.parse_args(argv)
+    actions = parser.add_subparsers(dest="action", required=True)
+    arm_parser = actions.add_parser("arm", help="schedule the reset")
+    arm_parser.add_argument("--timeout", type=int, default=DEFAULT_TIMEOUT_S, help="seconds before the reset fires")
+    arm_parser.add_argument("--unit", default=DEFAULT_UNIT, type=check_unit, help="transient systemd unit, dau-deadman or dau-deadman-<suffix>")
+    arm_parser.add_argument("--dry-run", action="store_true", help="print the command without running it")
+    disarm_parser = actions.add_parser("disarm", help="cancel the pending reset")
+    disarm_parser.add_argument("--unit", default=DEFAULT_UNIT, type=check_unit, help="transient systemd unit, dau-deadman or dau-deadman-<suffix>")
+    disarm_parser.add_argument("--dry-run", action="store_true", help="print the commands without running them")
+    status_parser = actions.add_parser("status", help="list the pending timer")
+    status_parser.add_argument("--unit", default=DEFAULT_UNIT, type=check_unit, help="transient systemd unit, dau-deadman or dau-deadman-<suffix>")
+    return parser
+
+
+def main(argv: Sequence[str] | None = None) -> int:
+    args = _parser().parse_args(argv)
 
     if args.action == "arm":
         if args.dry_run:
-            print(" ".join(arm_command(args.timeout, unit=args.unit)))
+            print(shlex.join(arm_command(args.timeout, unit=args.unit)))
             return 0
         try:
             arm(args.timeout, unit=args.unit)
@@ -135,7 +209,7 @@ def main(argv: Sequence[str] | None = None) -> int:
     if args.action == "disarm":
         if args.dry_run:
             for command in disarm_commands(unit=args.unit):
-                print(" ".join(command))
+                print(shlex.join(command))
             return 0
         try:
             disarm(unit=args.unit)
@@ -145,8 +219,11 @@ def main(argv: Sequence[str] | None = None) -> int:
         print(f"deadman disarmed (unit {args.unit})")
         return 0
 
-    subprocess.run(status_command(unit=args.unit), check=False)
-    return 0
+    try:
+        return status(unit=args.unit)
+    except DeadmanError as error:
+        print(f"deadman status unavailable: {error}", file=sys.stderr)
+        return 1
 
 
 if __name__ == "__main__":