[PATCH AUTOSEL 6.17-6.1] media: amphion: Delete v4l2_fh synchronously in .release()

[Sasha Levin <sashal@kernel.org>]

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);
 
-- 
2.51.0