From 5ebfbfcca71dcf2ac8ec2bd19670875659d6621c Mon Sep 17 00:00:00 2001 From: Dmitry Ilyin <6576495+widgetii@users.noreply.github.com> Date: Sun, 20 Sep 2026 17:04:46 +0300 Subject: [PATCH] hal: an entitlement must not outlive the probe that took it #219 taught rule 3 to keep the entitlement when the register still held the word we wrote, so that a second arm inside one sweep would not drop the undo the first one owed. That was wrong, and it reached the field. The word we write is 0x11. The word the vendor SDK writes when it starts a pipeline is also 0x11. So "the register still holds what I wrote" cannot tell our own ungate still standing from the consumer's clock running, and this sequence gated a streaming camera: boot arm finds 0x10, ungates to 0x11, entitled pipeline up the SDK writes 0x11 -- the same word t+30 min a probe arms: want == cur, and cur == wrote, so the entitlement was kept cleanup re-reads, sees wrote, writes found = 0x10 Reported on a second camera running nightly-20260919, which carries this fix: black picture thirty minutes after every start, while the camera running the build from before #219's review held 0x00000011 for twenty-four hours (OpenIPC/firmware#2439). So rule 3 disowns unconditionally again. The cost is the thing #219's review asked to avoid: a probe that ungated a gated clock and then armed again leaves it running. That is the deliberate trade and it is now pinned by test rather than left to drift -- of the two ways to be wrong, a clock left on costs microamps and a clock taken away costs the picture. The failed-read half of #219's review stands: a read we could not make still drops the entitlement, for the same reason. Three cases added, the important one being the field sequence above: arm while gated, consumer writes the same word, probe half an hour later, assert nothing is written. It fails against the shipped code. --- src/hal/hisi/hal_hisi.c | 26 +++++++++++++++++++------- src/hal/hisi/hal_hisi_test.c | 34 ++++++++++++++++++++++++++-------- 2 files changed, 45 insertions(+), 15 deletions(-) diff --git a/src/hal/hisi/hal_hisi.c b/src/hal/hisi/hal_hisi.c index 075eaa6..1a96dd1 100644 --- a/src/hal/hisi/hal_hisi.c +++ b/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; } diff --git a/src/hal/hisi/hal_hisi_test.c b/src/hal/hisi/hal_hisi_test.c index c4063b1..832a282 100644 --- a/src/hal/hisi/hal_hisi_test.c +++ b/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);