One firmware timeout kills the camera until the module is reloaded

[vrilutza]

When fthd_stop_channel() fails — firmware not responding — fthd_stop_streaming() does:

vb2_buffer_done(ctx->vb, VB2_BUF_STATE_DONE);
ctx->vb = NULL;

Two things are wrong. DONE tells the application the buffer holds a frame, but the hardware never wrote it; it should be ERROR.

And clearing ctx->vb is what leaves the camera dead. fthd_buffer_cleanup() looks the entry up by vb:

if (dev_priv->h2t_bufs[i].vb == vb) { ctx = ...; break; }
if (!ctx || ctx->state == BUF_FREE) return;

With vb cleared it never matches, so the entry never returns to BUF_FREE and its IOMMU mapping and DMA descriptor are never released. After that the driver has no usable buffers until the module is reloaded.

Keep ctx->vb and report the buffer as an error.

Not reproduced on hardware, and I would rather say so: it needs a firmware that stops responding, and injecting one would test the injection rather than the bug. The argument is the two code paths above.

---

The series

Seven PRs on this driver, all on master (364b1c6), all found while debugging a camera problem on a MacBookPro14,1:

#328	two small cleanups: missing break, hardcoded buffer count
#329	control values discarded at every VIDIOC_STREAMON
#330	cmd.y1 never assigned in the crop command — one line
#331	ALIGN(width, 7) aligns nothing; second commit re-does 545cb18 and should wait
#332 (this one)	one firmware timeout leaves the driver with no usable buffers
#333	the camera writes in front of any buffer that is not page aligned
#334	the auto-exposure wait: 1000 ms → 200 ms

They can be taken in any order, with one exception: the second commit of #331 should not be taken yet — reasons in that PR.

(Until 23 August there was a second exception: #328 and #334 both carried the same mdelay → msleep commit, so whichever merged first made the other conflict. I have dropped it from #328; it lives in #334. The seven are now independent of each other.)

There is one more patch not sent yet: the channel-start crop returns the top left corner of the frame at any size below the sensor. It is measured and ready, and it sits on top of #330. I am holding it back rather than adding an eighth PR to the pile.

[vrilutza]

Thanks for taking #328 and #330.

Master has moved since these were opened, so I re-checked: all five of the
remaining ones still apply cleanly on 54fb8f2, and none of them conflicts
with #337 either.

One thing I would rather say out loud before this one goes in, because it is
the only one you cannot check by reading it: I never reproduced it on
hardware. It needs a firmware that stops responding, and injecting one would
have tested my injection rather than the bug. All I have is the pair of code
paths in the description -- fthd_buffer_cleanup() looks the entry up by
ctx->vb, so clearing it is what strands the mapping.

If you would rather see it fail and recover before taking it, I can add a
module parameter that forces fthd_stop_channel() to return an error, and show
the buffers coming back afterwards.

And if anything in the other four needs changing, just say what and I will
redo them. No rush.

[patjak]

There is no need to explain how the code was fixed here. What you have written in the commit message is enough. It makes the code harder to understand and maintain, without any additional benefit. The fix itself is fine. Nice catch.

[vrilutza]

Comment dropped — the commit message carries it.