hal: an entitlement must not outlive the probe that took it

src/hal/hisi/hal_hisi.c

@@ -485,13 +485,25 @@ static void sensor_crg_enable(
 
     const uint32_t want = (cur | cken) & ~srst;
     if (want == cur) {
-        /* Already fit for a probe -- but by whose hand? If the register still
-         * holds exactly the word WE left, this is our own ungate being re-armed
-         * mid-sweep (setup_hal_hisi() arms, then arm_sensor_clock() arms again
-         * before every bus), and the undo we owe is still owed. Anything else
-         * is somebody else's clock and rule 3 disowns it. */
-        if (!(st->owed && cur == st->wrote))
-            st->owed = false;
+        /* Already fit for a probe, so this arm takes nothing -- and drops
+         * anything an earlier one held.
+         *
+         * UNCONDITIONALLY, and that is the whole point. It is tempting to keep
+         * the entitlement when the register still holds the word we wrote, on
+         * the grounds that this is our own ungate being re-armed mid-sweep.
+         * That was tried and it put the original bug straight back: the word
+         * we write is 0x11 and 0x11 is also what the vendor SDK writes, so an
+         * arm at boot that ungated a gated clock, followed by the consumer
+         * starting its pipeline, followed by a probe half an hour later, looks
+         * from this register exactly like our own ungate still standing. The
+         * cleanup then handed a streaming camera's clock back to the gated
+         * state. It reached the field before it was caught.
+         *
+         * The cost is that a probe which ungated a gated clock and then armed
+         * again may leave it running. That is deliberate: of the two ways to
+         * be wrong, a clock left on costs microamps and a clock taken away
+         * costs the picture. */
+        st->owed = false;
         return;
     }
 

src/hal/hisi/hal_hisi_test.c

@@ -218,23 +218,25 @@ int main(void) {
     hal_cleanup();
     CHECK(writes == before);
 
-    /* Qodo #2, under test before it is believed: setup ungates a gated clock,
-     * the sweep arms again before the first bus, and the cleanup must still
-     * put back what setup found. */
+    /* A second arm drops what the first held, and the clock is LEFT RUNNING.
+     * Pinned as the deliberate trade it is, not left to drift: keeping the
+     * entitlement across arms so the restore still fires is what reopened the
+     * field bug, because our own ungate and the consumer's are the same word.
+     * A clock left on costs microamps; one taken away costs the picture. */
     fresh(V4, 0x10, HISI_V4);
     setup_hal_hisi();          /* arm 1: 0x10 -> 0x11, entitled */
-    hal_enable_sensor_clock(); /* arm 2: already fit */
+    hal_enable_sensor_clock(); /* arm 2: already fit, disowns */
     hal_cleanup();
-    CHECK(peek(V4) == 0x10);
+    CHECK(peek(V4) == 0x11);
     CHECK(illegal == NULL);
 
-    /* Same, on both OT registers. */
+    /* Same on both OT registers. */
     fresh(OT, 0x00, HISI_OT);
     setup_hal_hisi();
     hal_enable_sensor_clock();
     hal_cleanup();
-    CHECK(peek(CV610_PERI_CRG8464_ADDR) == 0x00);
-    CHECK(peek(CV610_PERI_CRG8472_ADDR) == 0x00);
+    CHECK(peek(CV610_PERI_CRG8464_ADDR) & CV610_PERI_CRG_SENSOR0_CKEN);
+    CHECK(peek(CV610_PERI_CRG8472_ADDR) & CV610_PERI_CRG_SENSOR0_CKEN);
 
     /* Qodo #1: a read we could not make must not leave an entitlement from an
      * earlier arm lying about for the cleanup to spend. */
@@ -275,6 +277,22 @@ int main(void) {
     CHECK(peek(V4) == 0x11);
     CHECK(illegal == NULL);
 
+    /* THE FIELD CASE, and the one that matters most: an arm happens while the
+     * clock is gated and no probe follows it, the consumer then starts its
+     * pipeline and writes the very same word we did, and a probe arrives half
+     * an hour later. The entitlement from that first arm must not still be
+     * live, or the cleanup hands the consumer's running clock back to the
+     * gated state. Reported from the field after an earlier fix to the
+     * mid-sweep case made the entitlement outlive the probe it belonged to. */
+    fresh(V4, 0x10, HISI_V4);
+    setup_hal_hisi();          /* boot-time arm: 0x10 -> 0x11, entitled */
+    poke(V4, 0x11);            /* the SDK brings the pipeline up: same word */
+    reset_counters();
+    sweep();                   /* the telemetry probe, 30 minutes later */
+    CHECK(writes == 0);
+    CHECK(peek(V4) == 0x11);
+    CHECK(illegal == NULL);
+
     /* A register somebody else reprogrammed between our ungate and our undo
      * is theirs now. */
     fresh(V4, 0x10, HISI_V4);