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

[widgetii]

Follows #219, and corrects a mistake I made in its review round. Field regression — please take it quickly.

What went wrong

#219's review asked that a second arm inside one sweep should not drop the undo the first arm owed. I made rule 3 keep the entitlement when the register still held the word we wrote. That is exactly the distinction the register cannot support:

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 simply running, and this sequence gated a live 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   <- gated

The evidence

Two cameras, same model, reported together:

build	outcome
the build from before #219's review round	0x00000011 held for 24 hours, clean
nightly-20260919, which carries #219 as merged	black 30 minutes after every start

That is as clean a bisect of my own mistake as I could ask for.

The fix

Rule 3 disowns unconditionally again.

The cost is the thing the review round 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 other half of that review round stands: a read we could not make still drops the entitlement, for the same reason.

Tests

Three cases, the important one being the field sequence: arm while gated, consumer writes the same word, probe half an hour later, assert nothing is written. Verified it fails against the shipped code before fixing — and the two cases the review round added are re-aimed to assert the deliberate leave-it-running behaviour rather than a restore that must not happen.

ipcinfo unchanged at 1063400 bytes.

Field report: OpenIPC/firmware#2439.

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Prevent stale sensor-clock entitlements from gating live cameras

🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

AI Description

• Disowns stale restore entitlements whenever a later arm finds the sensor clock enabled.
• Prevents telemetry cleanup from gating clocks now owned by active vendor pipelines.
• Adds regression coverage for field timing and deliberate leave-running behavior.

Diagram

sequenceDiagram
    actor Boot as Boot Setup
    participant HAL as HAL Clock Logic
    participant CRG as Sensor Register
    participant SDK as Vendor SDK
    actor Probe as Telemetry Probe
    Boot->>HAL: Arm clock
    HAL->>CRG: Ungate to 0x11
    SDK->>CRG: Start pipeline at 0x11
    Probe->>HAL: Re-arm clock
    HAL->>CRG: Read state
    CRG-->>HAL: Already enabled
    HAL->>HAL: Disown stale restore
    Probe->>HAL: Finish probe
    Note over HAL,CRG: Cleanup skips restore

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Probe-scoped ownership tokens

• ➕ Could preserve restoration across repeated arms within one explicitly bounded probe.
• ➕ Would model ownership independently from ambiguous register contents.
• ➖ Requires broader lifecycle and caller API changes.
• ➖ Incorrect session boundaries could still retain ownership after the SDK starts.
• ➖ Too invasive for an urgent field regression.

2. Never restore sensor clocks

• ➕ Eliminates the risk of gating an active camera pipeline.
• ➕ Simplifies ownership handling substantially.
• ➖ Leaves every temporarily enabled clock running indefinitely.
• ➖ Increases power use beyond the narrower tradeoff in this patch.

Recommendation: Use the PR's unconditional disowning behavior for this field fix. Register contents cannot identify ownership because both the HAL and SDK write 0x11, so availability should take precedence over restoring a potentially stale gated state. Probe-scoped ownership could be explored separately only with reliable lifecycle boundaries.

Files changed (2) +45 / -15

Bug fix (1) +19 / -7

hal_hisi.cDisown restore entitlement when the clock is already enabled +19/-7

Disown restore entitlement when the clock is already enabled

• Changes rule 3 so every arm that finds the desired register state unconditionally clears any earlier restore entitlement. This prevents cleanup from gating a consumer-owned clock when the vendor SDK writes the same value as the HAL, deliberately accepting that some clocks may remain enabled.

src/hal/hisi/hal_hisi.c

Tests (1) +26 / -8

hal_hisi_test.cCover stale entitlement field regression and leave-running policy +26/-8

Cover stale entitlement field regression and leave-running policy

• Updates V4 and OT expectations so a second arm disowns restoration and leaves the clock enabled. Adds the reported boot-arm, SDK-start, delayed-probe sequence and verifies that the probe performs no write or illegal clock gating.

src/hal/hisi/hal_hisi_test.c

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Tip of the day

💡 Did you know, you can turn on the rule miner and Qodo learns your standards from review history

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗