From: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com
[ Upstream commit 19fb9c5b815f70eb90d5b545f65b83bc9c490ecd ]
The v4l2_fh initialized and added in vpu_v4l2_open() is delete and cleaned up when the last reference to the vpu_inst is released. This may happen later than at vpu_v4l2_close() time.
Not deleting and cleaning up the v4l2_fh when closing the file handle to the video device is not ideal, as the v4l2_fh will still be present in the video device's fh_list, and will store a copy of events queued to the video device. There may also be other side effects of keeping alive an object that represents an open file handle after the file handle is closed.
The v4l2_fh instance is embedded in the vpu_inst structure, and is accessed in two different ways:
- in vpu_notify_eos() and vpu_notify_source_change(), to queue V4L2 events to the file handle ; and
- through the driver to access the v4l2_fh.m2m_ctx pointer.
The v4l2_fh.m2m_ctx pointer is not touched by v4l2_fh_del() and v4l2_fh_exit(). It is set to NULL by the driver when closing the file handle, in vpu_v4l2_close().
The vpu_notify_eos() and vpu_notify_source_change() functions are called in vpu_set_last_buffer_dequeued() and vdec_handle_resolution_change() respectively, only if the v4l2_fh.m2m_ctx pointer is not NULL. There is therefore a guarantee that no new event will be queued to the v4l2_fh after vpu_v4l2_close() destroys the m2m_ctx.
The vpu_notify_eos() function is also called from vpu_vb2_buf_finish(), which is guaranteed to be called for all queued buffers when vpu_v4l2_close() calls v4l2_m2m_ctx_release(), and will not be called later.
It is therefore safe to assume that the driver will not touch the v4l2_fh, except to check the m2m_ctx pointer, after vpu_v4l2_close() destroys the m2m_ctx. We can safely delete and cleanup the v4l2_fh synchronously in vpu_v4l2_close().
Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com Reviewed-by: Ming Qian ming.qian@oss.nxp.com Signed-off-by: Hans Verkuil hverkuil+cisco@kernel.org Signed-off-by: Sasha Levin sashal@kernel.org ---
LLM Generated explanations, may be completely bogus:
YES
- What it fixes - Today the driver deletes the `v4l2_fh` only when the last reference to the instance is put, not when the file is closed. In current code, `v4l2_fh_del()` and `v4l2_fh_exit()` are called from the instance release path `vpu_v4l2_release()` (drivers/media/platform/amphion/vpu_v4l2.c:712-713), not from the file `.release()` (close) path (drivers/media/platform/amphion/vpu_v4l2.c:786-804). - That means after a user closes the device file, the `v4l2_fh` object still remains on the `video_device`’s `fh_list` and can accumulate device events until the last kref is dropped. This is undesirable (stale file-handle state, latent memory consumption and confusing semantics), and it also diverges from how most V4L2 mem2mem drivers behave and from the documented expectation that `v4l2_fh_del/exit` are called in the v4l2 file `.release()`.
- What changes and why it’s safe - The patch moves `v4l2_fh_del()`/`v4l2_fh_exit()` out of the instance release path and into the file `.release()` (`vpu_v4l2_close()`), and also adds them to the open error path: - Remove from instance release: currently called in `vpu_v4l2_release()` at drivers/media/platform/amphion/vpu_v4l2.c:712-713. - Add to close path: after the driver releases the mem2mem context in `vpu_v4l2_close()` (drivers/media/platform/amphion/vpu_v4l2.c:793-799), it will now call `v4l2_fh_del()`/`v4l2_fh_exit()` and only then proceed to unregister and put the instance. - Add to the open error label: currently the `error:` path lacks `v4l2_fh_del/exit` (drivers/media/platform/amphion/vpu_v4l2.c:781-783); the patch adds them there to avoid leaving an fh briefly on the device list after a failed open. - Safety argument (from code): - After close, `vpu_v4l2_close()` already destroys the mem2mem context (`v4l2_m2m_ctx_release`) before anything else of interest (drivers/media/platform/amphion/vpu_v4l2.c:793-799). This is critical: it ensures the driver no longer queues new events to the `v4l2_fh`. - Calls that queue events check `m2m_ctx` first: - `vpu_set_last_buffer_dequeued()` returns if `inst->fh.m2m_ctx == NULL` (drivers/media/platform/amphion/vpu_v4l2.c:110). - The decoder’s resolution-change path (`vdec_handle_resolution_change()`) also returns early if `inst->fh.m2m_ctx == NULL` (drivers/media/platform/amphion/vdec.c:357-366) before calling `vpu_notify_source_change()`. - `vpu_vb2_buf_finish()` may call `vpu_notify_eos(inst)`, but buffer-finish callbacks are guaranteed to flush during `v4l2_m2m_ctx_release()` and not after it returns, so there are no post-close event queues to an already exited `fh`. - With `m2m_ctx` destroyed first, no code path will call `v4l2_event_queue_fh()` after `v4l2_fh_exit()` sets `fh->vdev = NULL`. This avoids the risk of dereferencing a NULL `fh->vdev` in the core event code (see drivers/media/v4l2-core/v4l2-event.c:173-179 and drivers/media/v4l2-core/v4l2-fh.c:87-114).
- Why this is a good stable backport - Bug fix that affects users: prevents stale `fh` objects from staying on the device’s `fh_list` after close, which can accumulate events and resources and misrepresent the state of “open” file handles. - Small and contained: only changes `drivers/media/platform/amphion/vpu_v4l2.c`, moving two calls and adding them to an error path. No API or architectural changes. - Aligns with V4L2 expectations and common driver practice: many V4L2 mem2mem drivers delete and exit the `v4l2_fh` in their file `.release()`; the V4L2 API documentation for `v4l2_fh_del/exit` indicates they should be called in the `.release()` handler (see include/media/v4l2-fh.h). - Low regression risk: the mem2mem context is released at close time already (drivers/media/platform/amphion/vpu_v4l2.c:793-799), and all event-queuing paths are guarded by `m2m_ctx != NULL`, ensuring no events are queued after `fh` is deleted/exited. - Extra robustness: adding `v4l2_fh_del/exit` to the open error path ensures no transient fhs linger on the device list if open fails after `v4l2_fh_add`.
- Preconditions for backporting - Ensure the target stable branch matches the current behavior where `vpu_v4l2_close()` releases `inst->fh.m2m_ctx` (drivers/media/platform/amphion/vpu_v4l2.c:793-799). Earlier versions temporarily released `m2m_ctx` in the instance release path; this patch’s safety relies on doing it in `.close()`. If the branch still releases `m2m_ctx` in the instance release routine, this patch should be combined with or preceded by the change that moves `m2m_ctx` release into `.close()`.
Given the above, this change is a clear, minimal correctness fix with low risk and should be backported.
drivers/media/platform/amphion/vpu_v4l2.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/media/platform/amphion/vpu_v4l2.c b/drivers/media/platform/amphion/vpu_v4l2.c index 74668fa362e24..1c3740baf6942 100644 --- a/drivers/media/platform/amphion/vpu_v4l2.c +++ b/drivers/media/platform/amphion/vpu_v4l2.c @@ -718,8 +718,6 @@ static int vpu_v4l2_release(struct vpu_inst *inst)
v4l2_ctrl_handler_free(&inst->ctrl_handler); mutex_destroy(&inst->lock); - v4l2_fh_del(&inst->fh); - v4l2_fh_exit(&inst->fh);
call_void_vop(inst, cleanup);
@@ -788,6 +786,8 @@ int vpu_v4l2_open(struct file *file, struct vpu_inst *inst)
return 0; error: + v4l2_fh_del(&inst->fh); + v4l2_fh_exit(&inst->fh); vpu_inst_put(inst); return ret; } @@ -807,6 +807,9 @@ int vpu_v4l2_close(struct file *file) call_void_vop(inst, release); vpu_inst_unlock(inst);
+ v4l2_fh_del(&inst->fh); + v4l2_fh_exit(&inst->fh); + vpu_inst_unregister(inst); vpu_inst_put(inst);