HiSilicon V4/OT: getsensorid() can gate the sensor clock off under a running pipeline

[widgetii]

Summary

On HiSilicon V4 and OT, getsensorid() can gate the sensor's clock off — permanently, underneath a consumer whose video pipeline is streaming. The sensor then answers no i2c at all, so the kernel sensor driver loses the bus and the camera's picture freezes until somebody rebuilds the pipeline.

Proven causally on a hi3516ev300 + IMX335 running OpenIPC/majestic, which calls getsensorid() from a timer.

The one bit

EV300_PERI_CRG60, physical 0x120100F0: bit 0 sensor0_cken, bit 1 sensor0_srst_req, bits 2-4 sensor0_cksel.

broken   0x00000010    sensor0_cken = 0   <- sensor clock OFF
healthy  0x00000011    sensor0_cken = 1

With 908 [sensor_i2c_write 112] i2c_master_send error, ret=-5 accumulated in dmesg:

before              crg=0x10   i2c_err=908   isp_avelum=14 (hunting 94/30/14)
devmem 0x120100F0 32 0x11
+15 s               i2c_err +0   avelum=51   exptime=33274
+35 s               i2c_err +0   avelum=51   crg=0x11
/image.jpg          a sharp, correctly exposed 2592x1944 picture

One bit restored, the error flood stops dead, the picture comes back. Nothing else touched.

Mechanism

src/hal/hisi/hal_hisi.c:

static struct EV300_PERI_CRG60 peri_crg60;
static bool crg60_changed;                   /* set once, NEVER cleared */

static void v4_ensure_sensor_enabled() {
    if (mem_reg(EV300_PERI_CRG60_ADDR, &crg60, OP_READ))
        if (!crg60.sensor0_cken) {           /* only saves when it was OFF */
            peri_crg60 = crg60;              /* i.e. saves "clock off" */
            crg60.sensor0_cken = true;
            crg60.sensor0_srst_req = false;
            mem_reg(EV300_PERI_CRG60_ADDR, &crg60, OP_WRITE);
            crg60_changed = true;
        }
}
static void v4_ensure_sensor_restored() {
    if (crg60_changed)
        mem_reg(EV300_PERI_CRG60_ADDR, &peri_crg60, OP_WRITE);
}

A probe that runs while the clock is off (boot, before the consumer's pipeline exists) saves 0x10 and latches crg60_changed. Nothing clears that flag, and a later v4_ensure_sensor_enabled() finds cken already set so it does not refresh peri_crg60. Every subsequent probe therefore restores a word saved in a different era.

hisi_hal_cleanup() reaches the restore, and hal_cleanup() is called from src/sensors.c:1177 (end of get_sensor_id_i2c()), seven places in src/i2cspi.c (i2cget/i2cset/spiget/spiset/i2cdump/spidump), and example/ipcinfo.c:268 — whose comment states "both restore paths are idempotent", which is the assumption that is false.

ot_ensure_sensor_restored() has the identical shape doubled: peri_crg8464/crg8464_changed and peri_crg8472/crg8472_changed, registers 0x11018440 and 0x11018460. v3_ensure_sensor_enabled() has no restore counterpart and is immune — that absence is load-bearing, not an oversight.

Aggravating factor: arm_sensor_clock() (src/sensors.c:1274) re-arms before every bus in the fall-through sweep, so one getsensorid() performs up to seven arm/restore pairs, and the last thing a failing sweep does is gate the clock off.

Why it is hard to see

The word written to restore is 0x10; the word the vendor SDK writes when it starts a pipeline is 0x11 — the same word this code writes when it ungates. So "the register still holds what I wrote" cannot by itself distinguish our own ungate from the SDK's.

Proposed fix

Make the save/restore an entitlement rather than a snapshot:

1. Write the register only to ungate a clock we found gated, or to undo exactly that while it still holds the word we left.
2. One arm entitles at most one undo, spent whether or not it wrote.
3. An arm that finds the register already fit for a probe takes no entitlement at all.

Rule 3 is the one that fixes this, and rule 1 cannot replace it, for the reason above.

One race remains and cannot be closed from this register: if the consumer enables the clock between our arm and our restore, writing the identical word, we still gate it. That window is one probe rather than the hours it replaces.

Patch and a hal_hisi_test covering V4, OT, the failure above, and an interleaving sweep to follow.

Related: OpenIPC/firmware#2439.

Proven on hardware

A/B on one hi3516ev300 + IMX335, same precondition both runs (devmem 0x120100F0 32 0x10 with the consumer stopped — the sensor clock gated, as a cold boot leaves it, which is what arms the latch):

UNFIXED                                   FIXED
07:44:04  pipeline up, clock 0x11         08:15:46  pipeline up, clock 0x11
08:14:04  0x00000010   GATED              08:45:46  probe due
08:14:07  i2c_master_send error -5 ...    08:46:36  0x00000011  holds, 0 errors

The consumer's probe fires 1800 s after start. Unfixed, that gated the clock and the kernel's sensor writes began failing three seconds later; fixed, the register never moves and the error count stays at zero, with the probe demonstrably having run.

[widgetii]

Consumer-side tracking, with the field report that surfaced this: widgetii/majestic#826 and OpenIPC/firmware#2439.