test/hil: run the HID echo in a child, which is the only bound that worksclaude/hil-hid-child

test_device_hid_generic_inout was the last unbounded blocking IO in the file.
hidapi's hidraw backend reads `manufacturer` and `product` via udev for each
device passing the VID filter, and both are usb_string_attr -- served under the
device lock a wedged usbfs ioctl holds (v6.12.96 sysfs.c:141-143). Every DUT on
this rig is VID cafe, so a wedged SIBLING board passes that filter and stalls
the walk. hid_open calls hid_enumerate internally, and hid_close takes
down_write(&minors_rwsem) -- the same rwsem hidraw_open needs, so a stuck close
blocks opens of every hidraw node.

A PROCESS, not a thread, and not as a matter of taste. cython-hidapi wraps
hid_enumerate in `with nogil` but calls hid_open and hid_close BARE (hidapi 0.15.0 hid.pyx),
so those hold the GIL for the whole blocking call. A daemon thread cannot bound
that: the waiter parks off-GIL but must reacquire the GIL to return, which the
stuck thread never yields, so an in-process bound is inert exactly where it is
needed -- and the whole worker freezes, not just the call. Measured: a 1.0s
thread bound against a GIL-holding stall never returned at all. run_cmd's killpg
reaches a child regardless of what it is doing.

run_cmd grows an argv form for it. A multi-line `python3 -c` body cannot survive
a trip through the shell, and shell=True is now conditional on cmd being a
string, so every existing caller is untouched. Its banners render an argv
through _cmd_label so a program body does not land in the CI log.

Tests drive a scripted hid stub through the child. Two of its stall modes use
ctypes.PyDLL, which -- unlike CDLL -- does NOT release the GIL, so they are the
shape hid_open and hid_close actually have: a thread-based guard passes them
while doing nothing, and the child bound genuinely kills them. The remaining
modes cover the absent device and both bad-echo shapes.

The walk filters on BOTH ids, not just the VID -- and that does more than
skip non-matches. Read against hidapi 0.15.0 linux/hid.c: the id pre-check is
guarded by `if (vendor_id != 0 || product_id != 0)` (:962), so an UNFILTERED
walk sends every device straight to the locked reads. The pre-check itself is
free (parse_hid_vid_pid_from_sysfs parses <sysfs_path>/device/uevent, :532) and
both `continue`s precede create_device_info_for_device (:966-970 before :976),
which is where copy_udev_string reads manufacturer/product (:739-740). hid_open
passes the same ids through to hid_enumerate internally (:1030), so the filter
narrows that walk too. Six
examples in this tree expose a HID interface under VID cafe, so a VID-only walk
stalls on any of them wedged on a peer board; with hid_generic_inout's own
0x4012 too, only a peer running THIS example matches. That narrows the blast
radius -- it does not remove the need for the child, since hid_open
re-enumerates internally and such a peer still matches both ids.

HID_INOUT_PID is pinned against the example's own descriptor rather than
restated: a silent drift would widen the walk back to every cafe: HID device
and nothing else would fail.

5 files changed, 259 insertions, 36 deletions

diff --git a/test/hil/helper/hil_util.py b/test/hil/helper/hil_util.py
index dfd37467a..a0279fd4c 100644
--- a/test/hil/helper/hil_util.py
+++ b/test/hil/helper/hil_util.py
@@ -499,9 +499,26 @@ def run_alongside(argv: list, work, timeout: int) -> subprocess.CompletedProcess
     return _reap()
 
 
-def run_cmd(cmd: str, cwd: str | None = None, timeout: int | None = None,
+def _cmd_label(cmd) -> str:
+    """A one-line name for a banner. An argv whose payload is a `python3 -c` program would
+    otherwise dump the whole body into the CI log, where run_cmd's banners are already the
+    noisiest thing in a failing row."""
+    if isinstance(cmd, str):
+        return cmd
+    parts = [a if len(a) <= 60 else f'<{len(a)}-char program>' for a in cmd]
+    return ' '.join(parts)
+
+
+def run_cmd(cmd: str | list, cwd: str | None = None, timeout: int | None = None,
             binary: bool = False, split_stderr: bool = False,
             quiet: bool = False) -> subprocess.CompletedProcess:
+    """Bounded subprocess: own session, killpg on expiry, rc 124 when it had to be killed.
+
+    `cmd` is a shell STRING or an argv LIST. argv exists for a program that cannot survive
+    a trip through the shell -- a multi-line `python3 -c` body -- which is how the harness
+    runs a library call that no in-process bound can contain. A daemon thread cannot bound
+    a C call that holds the GIL, so for those the child process IS the bound.
+    """
     if timeout is None:
         timeout = CMD_TIMEOUT
     # binary: raw bytes (text mode's errors='replace' mangles non-UTF-8 file content).
@@ -510,7 +527,8 @@ def run_cmd(cmd: str, cwd: str | None = None, timeout: int | None = None,
     # still print: a killed child is always noteworthy).
     popen_kwargs = {
         'cwd': cwd,
-        'shell': True,
+        # a list goes straight to execve; only a string needs a shell to parse it
+        'shell': isinstance(cmd, str),
         'stdout': subprocess.PIPE,
         'stderr': subprocess.PIPE if split_stderr else subprocess.STDOUT,
     }
@@ -563,7 +581,7 @@ def run_cmd(cmd: str, cwd: str | None = None, timeout: int | None = None,
         timeout_err = _typed(err if err is not None else ex.stderr)
         if split_stderr and timeout_err is None:
             timeout_err = b'' if binary else ''
-        _print_banner(f'COMMAND TIMEOUT ({timeout}s): {cmd}', timeout_out, timeout_err)
+        _print_banner(f'COMMAND TIMEOUT ({timeout}s): {_cmd_label(cmd)}', timeout_out, timeout_err)
         return subprocess.CompletedProcess(args=cmd, returncode=124, stdout=timeout_out, stderr=timeout_err)
     except BaseException:
         # BaseException, not Exception (as in CPython's own subprocess.run):
@@ -582,7 +600,7 @@ def run_cmd(cmd: str, cwd: str | None = None, timeout: int | None = None,
         raise
 
     if r.returncode != 0 and not quiet:
-        _print_banner(f'COMMAND FAILED: {cmd}', r.stdout, r.stderr)
+        _print_banner(f'COMMAND FAILED: {_cmd_label(cmd)}', r.stdout, r.stderr)
     elif verbose:
         print(cmd)
         print(cmd_stdout_text(r.stdout))
diff --git a/test/hil/hil_test.py b/test/hil/hil_test.py
index 140860677..908befece 100755
--- a/test/hil/hil_test.py
+++ b/test/hil/hil_test.py
@@ -320,6 +320,59 @@ LP_READER = (
     '    buf += chunk\n'
     'sys.stdout.buffer.write(buf)\n'
 )
+# Runs under hil_util.run_cmd as `python3 -c`, argv so the body needs no shell quoting.
+# A PROCESS, not a thread, and not optional: cython-hidapi wraps hid_enumerate in
+# `with nogil` but calls hid_open and hid_close BARE (hidapi 0.15.0 hid.pyx), so those hold
+# the GIL for their whole blocking call. A daemon thread cannot bound that -- the waiter
+# parks off-GIL but must reacquire the GIL to return, which the stuck thread never yields
+# -- so an in-process bound is inert exactly where it is needed, and the whole worker
+# freezes rather than just the call. killpg reaches a child regardless.
+#
+# What blocks: hidapi's hidraw backend reads `manufacturer` and `product` via udev for each
+# device that reaches create_device_info_for_device, via copy_udev_string(usb_dev,
+# "manufacturer"/"product") -- both usb_string_attr, served under the device lock a wedged
+# usbfs ioctl holds (v6.12.96 sysfs.c:141-143).
+#
+# Passing BOTH ids is what keeps a wedged peer out of that path, and it does more than skip
+# non-matches: hidapi only runs the cheap pre-check `if (vendor_id != 0 || product_id != 0)`
+# (0.15.0 linux/hid.c:962), so an unfiltered walk sends EVERY device straight to the locked
+# reads. The pre-check itself is free -- parse_hid_vid_pid_from_sysfs parses
+# <sysfs_path>/device/uevent (:532) -- and both `continue`s precede
+# create_device_info_for_device (:966-970 before :976). Six examples in this tree expose a
+# HID interface under VID cafe, so a VID-only walk would stall on any of them wedged on a
+# peer. hid_open passes the same ids through to hid_enumerate internally (:1030), so the
+# filter narrows that walk too -- but a peer running THIS example still matches both ids,
+# which is why the child process, not the filter, is what bounds this.
+HID_ECHO = (
+    'import hid, random, sys, time\n'
+    'uid, deadline = sys.argv[1], time.monotonic() + float(sys.argv[2])\n'
+    'dev = None\n'
+    'while dev is None:\n'
+    '    for d in hid.enumerate(0xCafe, int(sys.argv[3], 16)):\n'
+    '        if d["serial_number"] == uid:\n'
+    '            dev = d\n'
+    '            break\n'
+    '    if dev is not None or time.monotonic() >= deadline:\n'
+    '        break\n'
+    '    time.sleep(1)\n'
+    'if dev is None:\n'
+    '    sys.exit(f"HID device not found for {uid}")\n'
+    'h = hid.device()\n'
+    'h.open(dev["vendor_id"], dev["product_id"], uid)\n'
+    'try:\n'
+    '    for size in (8, 32, 63):\n'
+    # Report ID (0) + payload, padded to 64 bytes
+    '        payload = bytes(random.randint(1, 255) for _ in range(size))\n'
+    '        h.write(bytes([0]) + payload + bytes(64 - size))\n'
+    '        echo = h.read(64, 2000)\n'
+    '        if not echo or len(echo) < size:\n'
+    '            sys.exit(f"HID echo timeout or short read ({size} bytes)")\n'
+    '        if bytes(echo[:size]) != payload:\n'
+    '            sys.exit(f"HID echo wrong data ({size} bytes): sent {payload.hex()} "\n'
+    '                     f"received {bytes(echo[:size]).hex()}")\n'
+    'finally:\n'
+    '    h.close()\n'
+)
 MTYPE_TIMEOUT = 30  # a README-sized read is <1 s; bounds a D-state hang on a wedged device
 
 
@@ -358,6 +411,13 @@ def read_disk_file(uid: str, lun: int, fname: str) -> bytes:
 # ~5 KB of transfers plus libmtp setup takes seconds, not minutes; a larger value makes a
 # wedged MTP board cost that much on every retry, all charged to the pool guard.
 MTP_SESSION_MARGIN = 30  # transfer budget after enumeration; past it the session is killed
+# room past the child's OWN enumeration budget for the echo exchange (3 x write + a 2000ms
+# hidapi read) and interpreter start-up, so the outer kill only fires on a real stall
+HID_ECHO_MARGIN = 30
+# hid_generic_inout's own idProduct. Pinned against the example's descriptor by
+# HidEchoRunsInAChild.test_the_pid_matches_the_example, because a silent drift here would
+# widen the walk back to every cafe: HID device without failing anything.
+HID_INOUT_PID = 0x4012
 
 
 def get_printer_dev(id: str, vendor_str, product_str, ifnum: int):
@@ -1304,38 +1364,19 @@ def test_device_audio_test_freertos(board):
 
 
 def test_device_hid_generic_inout(board):
+    # The whole exchange runs in a child (see HID_ECHO): hidapi's blocking calls hold the
+    # GIL, so nothing in-process can bound them. run_cmd's killpg can.
     uid = board['uid']
-    import hid  # cython-hidapi (pip: hidapi, apt: python3-hid)
-
-    timeout = enum_timeout()
-    dev = None
-    while timeout > 0:
-        for d in hid.enumerate(0xCafe):
-            if d['serial_number'] == uid:
-                dev = d
-                break
-        if dev:
-            break
-        time.sleep(1)
-        timeout -= 1
-    assert dev is not None, f'HID device not found for {uid}'
-
-    h = hid.device()
-    h.open(dev['vendor_id'], dev['product_id'], uid)
-    try:
-        for size in [8, 32, 63]:
-            # Report ID (0) + payload, padded to 64 bytes
-            payload = bytes([random.randint(1, 255) for _ in range(size)])
-            report = bytes([0]) + payload + bytes(64 - size)
-            h.write(report)
-            echo = h.read(64, 2000)
-            assert echo and len(echo) >= size, (
-                f'HID echo timeout or short read ({size} bytes)')
-            assert bytes(echo[:size]) == payload, (
-                f'HID echo wrong data ({size} bytes):\n'
-                f'  expected: {payload.hex()}\n  received: {bytes(echo[:size]).hex()}')
-    finally:
-        h.close()
+    r = hil_util.run_cmd(
+        [sys.executable, '-c', HID_ECHO, uid, str(enum_timeout()), f'{HID_INOUT_PID:#06x}'],
+        timeout=enum_timeout() + HID_ECHO_MARGIN, split_stderr=True)
+    # rc 124 is run_cmd's kill: the child was still inside a hidapi call, which is the
+    # wedge this runs in a child FOR -- and stderr is empty there, so say so rather than
+    # render a bare trailing colon
+    detail = hil_util.cmd_stdout_text(r.stderr).strip()[:300]
+    assert r.returncode == 0, (f'hid_generic_inout: {detail}' if detail else
+                               f'hid_generic_inout: the child was killed on its bound '
+                               f'(rc {r.returncode}) -- a hidapi call did not return')
 
 
 def test_device_usbtest(board):
diff --git a/test/hil/test/stubs/hid.py b/test/hil/test/stubs/hid.py
new file mode 100644
index 000000000..819a7a98d
--- /dev/null
+++ b/test/hil/test/stubs/hid.py
@@ -0,0 +1,64 @@
+# SPDX-License-Identifier: MIT
+"""Scripted stand-in for cython-hidapi, for the HID_ECHO child tests.
+
+A real wedge cannot be manufactured on demand, so the failure modes are scripted here and
+selected with FAKE_HID_MODE. Mirrors test/stubs/pymtp.py, which does the same for libmtp.
+"""
+import ctypes
+import os
+import time
+
+_MODE = os.environ.get('FAKE_HID_MODE', 'ok')
+_UID = os.environ.get('FAKE_HID_UID', 'CAFE01')
+_PID = int(os.environ.get('FAKE_HID_PID', '0x4012'), 16)
+
+
+def enumerate(vid=0, pid=0):
+    """Real hid.enumerate(vid, pid) filters on both ids -- 0 means "any" -- and returns a
+    'path' key too. The filters are applied BEFORE the locked manufacturer/product reads,
+    which is why passing both narrows what a wedged peer can stall."""
+    if _MODE == 'wedged_enumerate':
+        # hidapi's hidraw backend reads `manufacturer`/`product` for every device it
+        # lists, both served under the device lock -- this is that stall.
+        while True:
+            time.sleep(3600)
+    if _MODE == 'absent':
+        return []
+    if vid not in (0, 0xCafe) or pid not in (0, _PID):
+        return []
+    return [{'serial_number': _UID, 'vendor_id': 0xCafe, 'product_id': _PID,
+             'path': b'/dev/hidraw0'}]
+
+
+class device:
+    def __init__(self):
+        self._last = b''
+
+    def open(self, vid, pid, serial):
+        if _MODE == 'wedged_open':
+            while True:
+                time.sleep(3600)
+        if _MODE == 'wedged_open_gil':
+            # PyDLL, not CDLL: it does NOT release the GIL, which is the shape cython-
+            # hidapi's bare hid_open() call has. A thread-based bound is inert against
+            # this; only killing the process works.
+            ctypes.PyDLL('libc.so.6').sleep(3600)
+
+    def write(self, report):
+        self._last = bytes(report)
+
+    def read(self, size, timeout_ms):
+        if _MODE == 'wedged_read':
+            while True:
+                time.sleep(3600)
+        if _MODE == 'short_read':
+            return list(self._last[1:4])
+        if _MODE == 'wrong_data':
+            return list(bytes(b ^ 0xFF for b in self._last[1:]))
+        return list(self._last[1:])       # the device echoes the payload, minus report ID
+
+    def close(self):
+        if _MODE == 'wedged_close':
+            # also GIL-holding in cython-hidapi, and it runs in HID_ECHO's finally on
+            # every failure path
+            ctypes.PyDLL('libc.so.6').sleep(3600)
diff --git a/test/hil/test/test_ci_select.py b/test/hil/test/test_ci_select.py
index f3e000cb4..ace230246 100644
--- a/test/hil/test/test_ci_select.py
+++ b/test/hil/test/test_ci_select.py
@@ -955,7 +955,8 @@ class TestTheHarnessTestsAreNotTheHarness(unittest.TestCase):
 
     def test_the_harness_own_tests_select_nothing_on_either_axis(self):
         for p in ('test/hil/test/test_ci_select.py', 'test/hil/test/test_ci_metrics.py',
-                  'test/hil/test/test_hil_bounded.py', 'test/hil/test/stubs/pymtp.py'):
+                  'test/hil/test/test_hil_bounded.py', 'test/hil/test/stubs/pymtp.py',
+                  'test/hil/test/stubs/hid.py'):
             s = ci_select.classify([p], REPO, ROSTERS)
             self.assertFalse(s['full'], p)
             self.assertFalse(s['boards'], p)
@@ -979,6 +980,7 @@ class TestTheHarnessTestsAreNotTheHarness(unittest.TestCase):
         out = subprocess.run(['git', 'ls-files', 'test/hil/test'], cwd=REPO,
                              capture_output=True, text=True, check=True)
         self.assertEqual(sorted(out.stdout.split()), [
+            'test/hil/test/stubs/hid.py',
             'test/hil/test/stubs/pymtp.py',
             'test/hil/test/test_ci_metrics.py',
             'test/hil/test/test_ci_select.py',
diff --git a/test/hil/test/test_hil_bounded.py b/test/hil/test/test_hil_bounded.py
index 2230ef422..2b8a24b2e 100644
--- a/test/hil/test/test_hil_bounded.py
+++ b/test/hil/test/test_hil_bounded.py
@@ -1930,5 +1930,103 @@ class WedgedBoardCannotReportAPass(unittest.TestCase):
         self.assertIn('30/30', cell)
 
 
+class HidEchoRunsInAChild(unittest.TestCase):
+    """hidapi's blocking calls hold the GIL -- cython-hidapi wraps hid_enumerate in
+    `with nogil` but calls hid_open and hid_close bare -- so a daemon thread cannot bound
+    them: the waiter parks off-GIL but must reacquire the GIL to return, which the stuck
+    thread never yields. Only a child process can be killed regardless, which is what
+    run_cmd's killpg does."""
+
+    def _run(self, mode, uid='CAFE01', budget='0', timeout=20, pid=None):
+        saved = {k: os.environ.get(k) for k in ('FAKE_HID_MODE', 'FAKE_HID_UID',
+                                                'FAKE_HID_PID', 'PYTHONPATH',
+                                                'PYTHONSAFEPATH')}
+
+        def restore():
+            for k, v in saved.items():
+                os.environ.pop(k, None) if v is None else os.environ.__setitem__(k, v)
+        self.addCleanup(restore)
+        os.environ['FAKE_HID_MODE'] = mode
+        os.environ['FAKE_HID_UID'] = uid
+        stubs = os.path.join(TEST_DIR, 'stubs')
+        pp = saved['PYTHONPATH']
+        os.environ['PYTHONPATH'] = stubs if not pp else f'{stubs}:{pp}'
+        # `python3 -c` puts the cwd at sys.path[0], AHEAD of PYTHONPATH, so any hid.py
+        # reachable from the suite's cwd would displace the stub and every mode-driven
+        # test below would pass or fail for the wrong reason. Safe-path mode drops it --
+        # the same practice _MtpFakeRig documents.
+        os.environ['PYTHONSAFEPATH'] = '1'
+        from helper import hil_util
+        want = pid or f'{hil_test.HID_INOUT_PID:#06x}'
+        return hil_util.run_cmd(
+            [sys.executable, '-c', hil_test.HID_ECHO, uid, budget, want],
+            timeout=timeout, split_stderr=True, quiet=True)
+
+    def _stderr(self, r):
+        from helper import hil_util
+        return hil_util.cmd_stdout_text(r.stderr)
+
+    def test_a_healthy_device_passes(self):
+        r = self._run('ok')
+        self.assertEqual(r.returncode, 0, self._stderr(r))
+
+    def test_the_pid_matches_the_example(self):
+        """The walk filters on BOTH ids, and hidapi applies them before the locked
+        manufacturer/product reads. Six examples in this tree expose a HID interface under
+        VID cafe, so a stale PID here silently widens the walk back to all of them -- and
+        nothing else would fail. Pinned against the descriptor rather than restated."""
+        import re
+        src = (Path(TEST_DIR).parents[2]
+               / 'examples/device/hid_generic_inout/src/usb_descriptors.c').read_text()
+        m = re.search(r'#define\s+USB_PID\s+(0x[0-9a-fA-F]+)', src)
+        self.assertIsNotNone(m, 'hid_generic_inout no longer defines USB_PID')
+        self.assertEqual(hil_test.HID_INOUT_PID, int(m.group(1), 16),
+                         'HID_INOUT_PID drifted from the example descriptor')
+
+    def test_a_peer_running_another_example_is_filtered_out(self):
+        """The point of the PID filter: a wedged sibling on a different example never
+        reaches the locked reads at all."""
+        r = self._run('ok', pid='0x400f')      # hid_composite, not ours
+        self.assertNotEqual(r.returncode, 0)
+        self.assertIn('HID device not found', self._stderr(r))
+
+    def test_a_gil_holding_stall_is_still_killed(self):
+        """THE case an in-process bound cannot cover. hid_open is not `with nogil`, so a
+        thread-based guard is inert there; the child is killed anyway."""
+        t0 = time.monotonic()
+        r = self._run('wedged_open_gil', timeout=2)
+        self.assertEqual(r.returncode, 124,
+                         'a GIL-holding hidapi stall must still be killed on the bound')
+        self.assertLess(time.monotonic() - t0, 20, 'run_cmd did not bound the child')
+
+    def test_a_wedged_enumerate_is_killed_on_the_bound(self):
+        r = self._run('wedged_enumerate', timeout=2)
+        self.assertEqual(r.returncode, 124)
+
+    def test_a_wedged_close_is_killed_on_the_bound(self):
+        """close() runs in the child's finally on EVERY failure path and is also
+        GIL-holding; hidraw_release takes the same rwsem hidraw_open needs."""
+        r = self._run('wedged_close', timeout=3)
+        self.assertEqual(r.returncode, 124)
+
+    def test_an_absent_device_reports_why(self):
+        r = self._run('absent')
+        self.assertNotEqual(r.returncode, 0)
+        self.assertIn('HID device not found', self._stderr(r))
+
+    def test_a_bad_echo_reports_both_payloads(self):
+        r = self._run('wrong_data')
+        self.assertNotEqual(r.returncode, 0)
+        msg = self._stderr(r)
+        self.assertIn('wrong data', msg)
+        self.assertIn('sent', msg)
+        self.assertIn('received', msg)
+
+    def test_a_short_echo_is_not_read_as_a_pass(self):
+        r = self._run('short_read')
+        self.assertNotEqual(r.returncode, 0)
+        self.assertIn('short read', self._stderr(r))
+
+
 if __name__ == '__main__':
     unittest.main()