|
Two layers whose cost was a contract to reason about rather than an outcome.
SYSFS_UNKNOWN was a three-valued return five files had to keep apart, and
misreading unknown as absence was silent: a healthy board reported as a firmware
regression. What it guarded is real -- `serial` is served by usb_string_attr,
which takes usb_lock_device_interruptible (v6.12.96 sysfs.c:141-143), the same
lock a wedged usbfs ioctl holds -- so the BOUND stays, on every caller by
default. usb_scan reads `serial` on every device matching the VID, and hil_lock's
controller_of does that on essentially every board, so one wedged DUT would
otherwise stall every worker, not one. What goes is the third value.
read_sysfs now returns str or None, and the question the third value existed to
answer is asked directly instead, by two predicates that say which question they
answer: sysfs_stranded() is process-wide and sticky, for hil_pool_check's footer
("could anything here be the tool losing sight of healthy hardware?"), and
path_stranded(path) is per-device, which is what usbtest needs to tell a DUT
whose `serial` is held under device_lock from one that genuinely left the bus --
that difference decides whether it performs driver-registry writes that take the
uninterruptible device_lock.
Gone: _SysfsUnknown, SYSFS_UNKNOWN, sysfs_blind, sysfs_blind_note,
note_sysfs_strand, the cross-process blindness publishing and its report banner,
usb_scan's (list, bool) return, usbtest's inconclusive abort, _blind_note's slot
in the result tuple, and bounded_open, whose last caller went in the previous
commit.
The strand memo is rewritten around the one invariant that makes it safe to
reuse: it is keyed by the path's kernfs inode, captured BEFORE the read. A
busport does not change when a board returns to the same physical port, so a
path-only blacklist outlives the wedge and hil_pool_check's own recovery flow --
reset, reflash, wait_device polling that busport -- would never look at the
board again. A re-enumeration destroys the kernfs node and makes a new one, so a
changed inode is the all-clear. Two ceilings bound different things: per path
(_PATH_STRAND_MAX) for a board that flaps while still wedged, and per process
(_STRAND_MAX) as a backstop against RLIMIT_NOFILE, counted per PATH rather than
per reader because hil_pool_check runs four poll threads over one bus.
A board the pool guard never reached is now reported as run-aborted rather than
pool-timed-out, and outranks a stale board-locked cell for the same reason the
pool-timeout cell does.
Both predicates answer conservatively where they are consulted before something
irreversible. path_stranded() covers the paths read_sysfs answered None for
WITHOUT reading -- past _STRAND_MAX it declines to start another reader, and
vouching for a path nobody looked at hands usbtest's fail-CLOSED guard a
fabricated all-clear, running remove_id/unbind against a wedged device. usbtest's
startup lookup carries the same caveat hil_test's absent arm already did, because
its stderr is relayed verbatim into the report cell.
strand_note() survives the removal for the same reason master had it: every
caller that can say "not found" needs the same sentence, and the one site left to
re-invent it got missed -- a wedged-but-enumerated printer was reported as an
enumeration failure, sending a maintainer after firmware.
The two predicates are not interchangeable, and usbtest needs both. Its per-case
verdict is per-DUT -- a peer that stranded at case 2 must not make our board
report wedged at case 29 -- but the finally block's cleanup is process-wide:
remove_id plus an unbind of EVERY interface under the driver, including that
peer's, each taking the uninterruptible device_lock. So the verdict uses
path_stranded() and the global cleanup stays gated on sysfs_stranded().
USBTEST_RECOVERY_BUDGET was an independent 250s that could not actually contain
the ladder it reserved for, which is why usbtest.py carried a _time_left() gate
re-deciding before every step -- with a bare '- 35' for costs paid downstream
that nobody could re-derive. Between them the two produced a recovery that
skipped its own steps for most real hangs.
The reserve is now derived from the bounds usbtest itself declares, per flasher
and per target: a probe reset, a reflash, and the Rescue-DP POR plus retry a
wedged RP DAP needs, plus the settles and hil_util.REAP_GRACE for each bounded
step. The Rescue-DP legs are openocd-only and gated on the RP target cfg, and
a stub reset is screened out, so the reserve tracks each board's real ladder
instead of one fleet number: 390s for the two RP boards -- whose ladder the old
250 could not contain, which is exactly why the gates skipped their steps --
190s for the other seventeen probe-reset boards, and 150s for esptool and
lm4flash, whose reset is a no-op. Changing a bound in usbtest moves the
reserve with it, and a unit test asserts it covers the ladder.
With the room actually reserved, the child runs the ladder straight through:
recovery_steps, _time_left, the three per-step gates and the parsed-but-unused
--outer-timeout are gone. What stays is what decides outcomes -- the convoy_safe
gate, reset-before-reflash, the no_op screen so a stub that resets nothing is not
claimed, and wedged_pids() as the arbiter, because a clean flash only proves the
probe wrote the MCU.
hil_util.py 616 -> 514 lines.
|
|
Three readability changes with no behaviour change on the healthy path —
every pre-existing test passes untouched.
main() was 368 lines with try/finally three deep, its two abort paths
near-identical 40-line blocks; _abort_report holds that shape once, and the
controller-hint cache and pool construction move to their own helpers.
368 -> 279, test_board 180 -> 151, test_device_usbtest 164 -> 125.
test_hil_bounded.py cost 78s on every commit under test/hil/, mostly one 3s
post-flash settle paid by ten tests against a fake rig. Now 37s.
Fixes two pre-existing defects the extraction exposed: _write_failed_spec was
unguarded inside the abort path, so an OSError there replaced the caller's
RuntimeError and no report was written at all; _save_controller_hints overlaid
a startup snapshot onto the re-read cache, clobbering a concurrent job's newer
values. Also five comments that stated the opposite of the code, and both table
renderers measuring width with len() against two-column status marks.
|
