[PATCHv3 0/3] media: venus: close() fixes

[PATCHv3 0/3] media: venus: close() fixes

[Sergey Senozhatsky]

A couple of fixes for venus driver close() handling
(both enc and dec).

v2->v3:
-- factored out common close() code (Bryan)

Sergey Senozhatsky (3):
  media: venus: fix enc/dec destruction order
  media: venus: sync with threaded IRQ during inst destruction
  media: venus: factor out inst destruction routine

 drivers/media/platform/qcom/venus/core.c      | 25 +++++++++++++++++++
 drivers/media/platform/qcom/venus/core.h      |  2 ++
 drivers/media/platform/qcom/venus/vdec.c      | 11 +-------
 drivers/media/platform/qcom/venus/vdec.h      |  7 +++++-
 .../media/platform/qcom/venus/vdec_ctrls.c    |  6 -----
 drivers/media/platform/qcom/venus/venc.c      | 12 +--------
 6 files changed, 35 insertions(+), 28 deletions(-)

-- 
2.47.0.163.g1226f6d8fa-goog

[PATCHv3 1/3] media: venus: fix enc/dec destruction order

[Sergey Senozhatsky]

We destroy mutex-es too early as they are still taken in
v4l2_fh_exit()->v4l2_event_unsubscribe()->v4l2_ctrl_find().

We should destroy mutex-es right before kfree().  Also
do not vdec_ctrl_deinit() before v4l2_fh_exit().

Fixes: 7472c1c69138 ("[media] media: venus: vdec: add video decoder files")
Suggested-by: Tomasz Figa <tfiga@google.com>
Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/media/platform/qcom/venus/vdec.c | 7 ++++---
 drivers/media/platform/qcom/venus/venc.c | 6 +++---
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index 6252a6b3d4ba..0013c4704f03 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -1752,13 +1752,14 @@ static int vdec_close(struct file *file)
 	cancel_work_sync(&inst->delayed_process_work);
 	v4l2_m2m_ctx_release(inst->m2m_ctx);
 	v4l2_m2m_release(inst->m2m_dev);
-	vdec_ctrl_deinit(inst);
 	ida_destroy(&inst->dpb_ids);
 	hfi_session_destroy(inst);
-	mutex_destroy(&inst->lock);
-	mutex_destroy(&inst->ctx_q_lock);
 	v4l2_fh_del(&inst->fh);
 	v4l2_fh_exit(&inst->fh);
+	vdec_ctrl_deinit(inst);
+
+	mutex_destroy(&inst->lock);
+	mutex_destroy(&inst->ctx_q_lock);
 
 	vdec_pm_put(inst, false);
 
diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
index 322a7737e2c7..6a26a6592424 100644
--- a/drivers/media/platform/qcom/venus/venc.c
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -1519,14 +1519,14 @@ static int venc_close(struct file *file)
 
 	v4l2_m2m_ctx_release(inst->m2m_ctx);
 	v4l2_m2m_release(inst->m2m_dev);
-	venc_ctrl_deinit(inst);
 	hfi_session_destroy(inst);
-	mutex_destroy(&inst->lock);
-	mutex_destroy(&inst->ctx_q_lock);
 	v4l2_fh_del(&inst->fh);
 	v4l2_fh_exit(&inst->fh);
+	venc_ctrl_deinit(inst);
 
 	inst->enc_state = VENUS_ENC_STATE_DEINIT;
+	mutex_destroy(&inst->lock);
+	mutex_destroy(&inst->ctx_q_lock);
 
 	venc_pm_put(inst, false);
 
-- 
2.47.0.163.g1226f6d8fa-goog

[PATCHv3 2/3] media: venus: sync with threaded IRQ during inst destruction

[Sergey Senozhatsky]

When destroying an inst we should make sure that we don't race
against threaded IRQ (or pending IRQ), otherwise we can concurrently
kfree() inst context and inst itself.

BUG: KASAN: slab-use-after-free in vb2_queue_error+0x80/0x90
Call trace:
dump_backtrace+0x1c4/0x1f8
show_stack+0x38/0x60
dump_stack_lvl+0x168/0x1f0
print_report+0x170/0x4c8
kasan_report+0x94/0xd0
__asan_report_load2_noabort+0x20/0x30
vb2_queue_error+0x80/0x90
venus_helper_vb2_queue_error+0x54/0x78
venc_event_notify+0xec/0x158
hfi_event_notify+0x878/0xd20
hfi_process_msg_packet+0x27c/0x4e0
venus_isr_thread+0x258/0x6e8
hfi_isr_thread+0x70/0x90
venus_isr_thread+0x34/0x50
irq_thread_fn+0x88/0x130
irq_thread+0x160/0x2c0
kthread+0x294/0x328
ret_from_fork+0x10/0x20

Allocated by task 20291:
kasan_set_track+0x4c/0x80
kasan_save_alloc_info+0x28/0x38
__kasan_kmalloc+0x84/0xa0
kmalloc_trace+0x7c/0x98
v4l2_m2m_ctx_init+0x74/0x280
venc_open+0x444/0x6d0
v4l2_open+0x19c/0x2a0
chrdev_open+0x374/0x3f0
do_dentry_open+0x710/0x10a8
vfs_open+0x88/0xa8
path_openat+0x1e6c/0x2700
do_filp_open+0x1a4/0x2e0
do_sys_openat2+0xe8/0x508
do_sys_open+0x15c/0x1a0
__arm64_sys_openat+0xa8/0xc8
invoke_syscall+0xdc/0x270
el0_svc_common+0x1ec/0x250
do_el0_svc+0x54/0x70
el0_svc+0x50/0xe8
el0t_64_sync_handler+0x48/0x120
el0t_64_sync+0x1a8/0x1b0

Rearrange inst destruction.  First remove the inst from the
core->instances list, second synchronize IRQ/IRQ-thread to
make sure that nothing else would see the inst while we take
it down.

Fixes: 7472c1c69138 ("[media] media: venus: vdec: add video decoder files")
Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/media/platform/qcom/venus/vdec.c | 12 +++++++++++-
 drivers/media/platform/qcom/venus/venc.c | 12 +++++++++++-
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index 0013c4704f03..b3192a36f388 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -1750,10 +1750,20 @@ static int vdec_close(struct file *file)
 	vdec_pm_get(inst);
 
 	cancel_work_sync(&inst->delayed_process_work);
+	/*
+	 * First, remove the inst from the ->instances list, so that
+	 * to_instance() will return NULL.
+	 */
+	hfi_session_destroy(inst);
+	/*
+	 * Second, make sure we don't have IRQ/IRQ-thread currently running
+	 * or pending execution, which would race with the inst destruction.
+	 */
+	synchronize_irq(inst->core->irq);
+
 	v4l2_m2m_ctx_release(inst->m2m_ctx);
 	v4l2_m2m_release(inst->m2m_dev);
 	ida_destroy(&inst->dpb_ids);
-	hfi_session_destroy(inst);
 	v4l2_fh_del(&inst->fh);
 	v4l2_fh_exit(&inst->fh);
 	vdec_ctrl_deinit(inst);
diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
index 6a26a6592424..36981ce448f5 100644
--- a/drivers/media/platform/qcom/venus/venc.c
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -1517,9 +1517,19 @@ static int venc_close(struct file *file)
 
 	venc_pm_get(inst);
 
+	/*
+	 * First, remove the inst from the ->instances list, so that
+	 * to_instance() will return NULL.
+	 */
+	hfi_session_destroy(inst);
+	/*
+	 * Second, make sure we don't have IRQ/IRQ-thread currently running
+	 * or pending execution, which would race with the inst destruction.
+	 */
+	synchronize_irq(inst->core->irq);
+
 	v4l2_m2m_ctx_release(inst->m2m_ctx);
 	v4l2_m2m_release(inst->m2m_dev);
-	hfi_session_destroy(inst);
 	v4l2_fh_del(&inst->fh);
 	v4l2_fh_exit(&inst->fh);
 	venc_ctrl_deinit(inst);
-- 
2.47.0.163.g1226f6d8fa-goog

[PATCHv3 3/3] media: venus: factor out inst destruction routine

[Sergey Senozhatsky]

Factor out common instance destruction code into
a common function.

Suggested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
 drivers/media/platform/qcom/venus/core.c      | 25 +++++++++++++++++++
 drivers/media/platform/qcom/venus/core.h      |  2 ++
 drivers/media/platform/qcom/venus/vdec.c      | 22 +---------------
 drivers/media/platform/qcom/venus/vdec.h      |  7 +++++-
 .../media/platform/qcom/venus/vdec_ctrls.c    |  6 -----
 drivers/media/platform/qcom/venus/venc.c      | 22 +---------------
 6 files changed, 35 insertions(+), 49 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 423deb5e94dc..4d90fc1c21fe 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -26,6 +26,7 @@
 #include "firmware.h"
 #include "pm_helpers.h"
 #include "hfi_venus_io.h"
+#include "vdec.h"
 
 static void venus_coredump(struct venus_core *core)
 {
@@ -502,6 +503,30 @@ static __maybe_unused int venus_runtime_suspend(struct device *dev)
 	return ret;
 }
 
+void venus_close_common(struct venus_inst *inst)
+{
+	/*
+	 * First, remove the inst from the ->instances list, so that
+	 * to_instance() will return NULL.
+	 */
+	hfi_session_destroy(inst);
+	/*
+	 * Second, make sure we don't have IRQ/IRQ-thread currently running
+	 * or pending execution, which would race with the inst destruction.
+	 */
+	synchronize_irq(inst->core->irq);
+
+	v4l2_m2m_ctx_release(inst->m2m_ctx);
+	v4l2_m2m_release(inst->m2m_dev);
+	v4l2_fh_del(&inst->fh);
+	v4l2_fh_exit(&inst->fh);
+	vdec_ctrl_deinit(inst);
+
+	mutex_destroy(&inst->lock);
+	mutex_destroy(&inst->ctx_q_lock);
+}
+EXPORT_SYMBOL_GPL(venus_close_common);
+
 static __maybe_unused int venus_runtime_resume(struct device *dev)
 {
 	struct venus_core *core = dev_get_drvdata(dev);
diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index 435325432922..7bb36a270e15 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -560,4 +560,6 @@ is_fw_rev_or_older(struct venus_core *core, u32 vmajor, u32 vminor, u32 vrev)
 		(core)->venus_ver.minor == vminor &&
 		(core)->venus_ver.rev <= vrev);
 }
+
+void venus_close_common(struct venus_inst *inst);
 #endif
diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index b3192a36f388..9a680402c711 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -1748,29 +1748,9 @@ static int vdec_close(struct file *file)
 	struct venus_inst *inst = to_inst(file);
 
 	vdec_pm_get(inst);
-
 	cancel_work_sync(&inst->delayed_process_work);
-	/*
-	 * First, remove the inst from the ->instances list, so that
-	 * to_instance() will return NULL.
-	 */
-	hfi_session_destroy(inst);
-	/*
-	 * Second, make sure we don't have IRQ/IRQ-thread currently running
-	 * or pending execution, which would race with the inst destruction.
-	 */
-	synchronize_irq(inst->core->irq);
-
-	v4l2_m2m_ctx_release(inst->m2m_ctx);
-	v4l2_m2m_release(inst->m2m_dev);
+	venus_close_common(inst);
 	ida_destroy(&inst->dpb_ids);
-	v4l2_fh_del(&inst->fh);
-	v4l2_fh_exit(&inst->fh);
-	vdec_ctrl_deinit(inst);
-
-	mutex_destroy(&inst->lock);
-	mutex_destroy(&inst->ctx_q_lock);
-
 	vdec_pm_put(inst, false);
 
 	kfree(inst);
diff --git a/drivers/media/platform/qcom/venus/vdec.h b/drivers/media/platform/qcom/venus/vdec.h
index 6b262d0bf561..2687255b1616 100644
--- a/drivers/media/platform/qcom/venus/vdec.h
+++ b/drivers/media/platform/qcom/venus/vdec.h
@@ -6,9 +6,14 @@
 #ifndef __VENUS_VDEC_H__
 #define __VENUS_VDEC_H__
 
+#include <media/v4l2-ctrls.h>
+
 struct venus_inst;
 
 int vdec_ctrl_init(struct venus_inst *inst);
-void vdec_ctrl_deinit(struct venus_inst *inst);
+static inline void vdec_ctrl_deinit(struct venus_inst *inst)
+{
+	v4l2_ctrl_handler_free(&inst->ctrl_handler);
+}
 
 #endif
diff --git a/drivers/media/platform/qcom/venus/vdec_ctrls.c b/drivers/media/platform/qcom/venus/vdec_ctrls.c
index 7e0f29bf7fae..fa034a7fdbed 100644
--- a/drivers/media/platform/qcom/venus/vdec_ctrls.c
+++ b/drivers/media/platform/qcom/venus/vdec_ctrls.c
@@ -4,7 +4,6 @@
  * Copyright (C) 2017 Linaro Ltd.
  */
 #include <linux/types.h>
-#include <media/v4l2-ctrls.h>
 
 #include "core.h"
 #include "helpers.h"
@@ -187,8 +186,3 @@ int vdec_ctrl_init(struct venus_inst *inst)
 
 	return 0;
 }
-
-void vdec_ctrl_deinit(struct venus_inst *inst)
-{
-	v4l2_ctrl_handler_free(&inst->ctrl_handler);
-}
diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
index 36981ce448f5..2ae22ba156bb 100644
--- a/drivers/media/platform/qcom/venus/venc.c
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -1516,28 +1516,8 @@ static int venc_close(struct file *file)
 	struct venus_inst *inst = to_inst(file);
 
 	venc_pm_get(inst);
-
-	/*
-	 * First, remove the inst from the ->instances list, so that
-	 * to_instance() will return NULL.
-	 */
-	hfi_session_destroy(inst);
-	/*
-	 * Second, make sure we don't have IRQ/IRQ-thread currently running
-	 * or pending execution, which would race with the inst destruction.
-	 */
-	synchronize_irq(inst->core->irq);
-
-	v4l2_m2m_ctx_release(inst->m2m_ctx);
-	v4l2_m2m_release(inst->m2m_dev);
-	v4l2_fh_del(&inst->fh);
-	v4l2_fh_exit(&inst->fh);
-	venc_ctrl_deinit(inst);
-
+	venus_close_common(inst);
 	inst->enc_state = VENUS_ENC_STATE_DEINIT;
-	mutex_destroy(&inst->lock);
-	mutex_destroy(&inst->ctx_q_lock);
-
 	venc_pm_put(inst, false);
 
 	kfree(inst);
-- 
2.47.0.163.g1226f6d8fa-goog

Re: [PATCHv3 1/3] media: venus: fix enc/dec destruction order

[Dikshita Agarwal]

On 10/25/2024 9:16 AM, Sergey Senozhatsky wrote:
> We destroy mutex-es too early as they are still taken in
> v4l2_fh_exit()->v4l2_event_unsubscribe()->v4l2_ctrl_find().
> 
> We should destroy mutex-es right before kfree().  Also
> do not vdec_ctrl_deinit() before v4l2_fh_exit().
> 
> Fixes: 7472c1c69138 ("[media] media: venus: vdec: add video decoder files")
> Suggested-by: Tomasz Figa <tfiga@google.com>
> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  drivers/media/platform/qcom/venus/vdec.c | 7 ++++---
>  drivers/media/platform/qcom/venus/venc.c | 6 +++---
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> index 6252a6b3d4ba..0013c4704f03 100644
> --- a/drivers/media/platform/qcom/venus/vdec.c
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -1752,13 +1752,14 @@ static int vdec_close(struct file *file)
>  	cancel_work_sync(&inst->delayed_process_work);
>  	v4l2_m2m_ctx_release(inst->m2m_ctx);
>  	v4l2_m2m_release(inst->m2m_dev);
> -	vdec_ctrl_deinit(inst);
>  	ida_destroy(&inst->dpb_ids);
>  	hfi_session_destroy(inst);
> -	mutex_destroy(&inst->lock);
> -	mutex_destroy(&inst->ctx_q_lock);
>  	v4l2_fh_del(&inst->fh);
>  	v4l2_fh_exit(&inst->fh);
> +	vdec_ctrl_deinit(inst);
Why vdec_ctrl_deinit ->v4l2_ctrl_handler_free(&inst->ctrl_handler) needs to
be called after v4l2_fh_exit?
Ideally it should be before v4l2_fh_exit.

Thanks,
Dikshita
> +
> +	mutex_destroy(&inst->lock);
> +	mutex_destroy(&inst->ctx_q_lock);
>  
>  	vdec_pm_put(inst, false);
>  
> diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
> index 322a7737e2c7..6a26a6592424 100644
> --- a/drivers/media/platform/qcom/venus/venc.c
> +++ b/drivers/media/platform/qcom/venus/venc.c
> @@ -1519,14 +1519,14 @@ static int venc_close(struct file *file)
>  
>  	v4l2_m2m_ctx_release(inst->m2m_ctx);
>  	v4l2_m2m_release(inst->m2m_dev);
> -	venc_ctrl_deinit(inst);
>  	hfi_session_destroy(inst);
> -	mutex_destroy(&inst->lock);
> -	mutex_destroy(&inst->ctx_q_lock);
>  	v4l2_fh_del(&inst->fh);
>  	v4l2_fh_exit(&inst->fh);
> +	venc_ctrl_deinit(inst);
>  
>  	inst->enc_state = VENUS_ENC_STATE_DEINIT;
> +	mutex_destroy(&inst->lock);
> +	mutex_destroy(&inst->ctx_q_lock);
>  
>  	venc_pm_put(inst, false);
>  

Re: [PATCHv3 3/3] media: venus: factor out inst destruction routine

[Dikshita Agarwal]

On 10/25/2024 9:16 AM, Sergey Senozhatsky wrote:
> Factor out common instance destruction code into
> a common function.
> 
> Suggested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> ---
>  drivers/media/platform/qcom/venus/core.c      | 25 +++++++++++++++++++
>  drivers/media/platform/qcom/venus/core.h      |  2 ++
>  drivers/media/platform/qcom/venus/vdec.c      | 22 +---------------
>  drivers/media/platform/qcom/venus/vdec.h      |  7 +++++-
>  .../media/platform/qcom/venus/vdec_ctrls.c    |  6 -----
>  drivers/media/platform/qcom/venus/venc.c      | 22 +---------------
>  6 files changed, 35 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index 423deb5e94dc..4d90fc1c21fe 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -26,6 +26,7 @@
>  #include "firmware.h"
>  #include "pm_helpers.h"
>  #include "hfi_venus_io.h"
> +#include "vdec.h"
>  
>  static void venus_coredump(struct venus_core *core)
>  {
> @@ -502,6 +503,30 @@ static __maybe_unused int venus_runtime_suspend(struct device *dev)
>  	return ret;
>  }
>  
> +void venus_close_common(struct venus_inst *inst)
> +{
> +	/*
> +	 * First, remove the inst from the ->instances list, so that
> +	 * to_instance() will return NULL.
> +	 */
> +	hfi_session_destroy(inst);
> +	/*
> +	 * Second, make sure we don't have IRQ/IRQ-thread currently running
> +	 * or pending execution, which would race with the inst destruction.
> +	 */
> +	synchronize_irq(inst->core->irq);
> +
> +	v4l2_m2m_ctx_release(inst->m2m_ctx);
> +	v4l2_m2m_release(inst->m2m_dev);
> +	v4l2_fh_del(&inst->fh);
> +	v4l2_fh_exit(&inst->fh);
> +	vdec_ctrl_deinit(inst);
vdec specific APIs shouldn't be invoked from common API.
Pls call v4l2_ctrl_handler_free(&inst->ctrl_handler) directly from here.
and remove vdec/venc_ctrl_init.
> +
> +	mutex_destroy(&inst->lock);
> +	mutex_destroy(&inst->ctx_q_lock);
> +}
> +EXPORT_SYMBOL_GPL(venus_close_common);
> +
>  static __maybe_unused int venus_runtime_resume(struct device *dev)
>  {
>  	struct venus_core *core = dev_get_drvdata(dev);
> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> index 435325432922..7bb36a270e15 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -560,4 +560,6 @@ is_fw_rev_or_older(struct venus_core *core, u32 vmajor, u32 vminor, u32 vrev)
>  		(core)->venus_ver.minor == vminor &&
>  		(core)->venus_ver.rev <= vrev);
>  }
> +
> +void venus_close_common(struct venus_inst *inst);
>  #endif
> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> index b3192a36f388..9a680402c711 100644
> --- a/drivers/media/platform/qcom/venus/vdec.c
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -1748,29 +1748,9 @@ static int vdec_close(struct file *file)
>  	struct venus_inst *inst = to_inst(file);
>  
>  	vdec_pm_get(inst);
> -
>  	cancel_work_sync(&inst->delayed_process_work);
> -	/*
> -	 * First, remove the inst from the ->instances list, so that
> -	 * to_instance() will return NULL.
> -	 */
> -	hfi_session_destroy(inst);
> -	/*
> -	 * Second, make sure we don't have IRQ/IRQ-thread currently running
> -	 * or pending execution, which would race with the inst destruction.
> -	 */
> -	synchronize_irq(inst->core->irq);
> -
> -	v4l2_m2m_ctx_release(inst->m2m_ctx);
> -	v4l2_m2m_release(inst->m2m_dev);
> +	venus_close_common(inst);
>  	ida_destroy(&inst->dpb_ids);
> -	v4l2_fh_del(&inst->fh);
> -	v4l2_fh_exit(&inst->fh);
> -	vdec_ctrl_deinit(inst);
> -
> -	mutex_destroy(&inst->lock);
> -	mutex_destroy(&inst->ctx_q_lock);
> -
>  	vdec_pm_put(inst, false);
>  
>  	kfree(inst);
> diff --git a/drivers/media/platform/qcom/venus/vdec.h b/drivers/media/platform/qcom/venus/vdec.h
> index 6b262d0bf561..2687255b1616 100644
> --- a/drivers/media/platform/qcom/venus/vdec.h
> +++ b/drivers/media/platform/qcom/venus/vdec.h
> @@ -6,9 +6,14 @@
>  #ifndef __VENUS_VDEC_H__
>  #define __VENUS_VDEC_H__
>  
> +#include <media/v4l2-ctrls.h>
> +
>  struct venus_inst;
>  
>  int vdec_ctrl_init(struct venus_inst *inst);
> -void vdec_ctrl_deinit(struct venus_inst *inst);
> +static inline void vdec_ctrl_deinit(struct venus_inst *inst)
> +{
> +	v4l2_ctrl_handler_free(&inst->ctrl_handler);
> +}
>  
>  #endif
> diff --git a/drivers/media/platform/qcom/venus/vdec_ctrls.c b/drivers/media/platform/qcom/venus/vdec_ctrls.c
> index 7e0f29bf7fae..fa034a7fdbed 100644
> --- a/drivers/media/platform/qcom/venus/vdec_ctrls.c
> +++ b/drivers/media/platform/qcom/venus/vdec_ctrls.c
> @@ -4,7 +4,6 @@
>   * Copyright (C) 2017 Linaro Ltd.
>   */
>  #include <linux/types.h>
> -#include <media/v4l2-ctrls.h>
>  
>  #include "core.h"
>  #include "helpers.h"
> @@ -187,8 +186,3 @@ int vdec_ctrl_init(struct venus_inst *inst)
>  
>  	return 0;
>  }
> -
> -void vdec_ctrl_deinit(struct venus_inst *inst)
> -{
> -	v4l2_ctrl_handler_free(&inst->ctrl_handler);
> -}
> diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
> index 36981ce448f5..2ae22ba156bb 100644
> --- a/drivers/media/platform/qcom/venus/venc.c
> +++ b/drivers/media/platform/qcom/venus/venc.c
> @@ -1516,28 +1516,8 @@ static int venc_close(struct file *file)
>  	struct venus_inst *inst = to_inst(file);
>  
>  	venc_pm_get(inst);
> -
> -	/*
> -	 * First, remove the inst from the ->instances list, so that
> -	 * to_instance() will return NULL.
> -	 */
> -	hfi_session_destroy(inst);
> -	/*
> -	 * Second, make sure we don't have IRQ/IRQ-thread currently running
> -	 * or pending execution, which would race with the inst destruction.
> -	 */
> -	synchronize_irq(inst->core->irq);
> -
> -	v4l2_m2m_ctx_release(inst->m2m_ctx);
> -	v4l2_m2m_release(inst->m2m_dev);
> -	v4l2_fh_del(&inst->fh);
> -	v4l2_fh_exit(&inst->fh);
> -	venc_ctrl_deinit(inst);
> -
> +	venus_close_common(inst);
>  	inst->enc_state = VENUS_ENC_STATE_DEINIT;
> -	mutex_destroy(&inst->lock);
> -	mutex_destroy(&inst->ctx_q_lock);
> -
>  	venc_pm_put(inst, false);
>  
>  	kfree(inst);

Re: [PATCHv3 1/3] media: venus: fix enc/dec destruction order

[Sergey Senozhatsky]

On (24/10/25 12:50), Dikshita Agarwal wrote:
> > diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> > index 6252a6b3d4ba..0013c4704f03 100644
> > --- a/drivers/media/platform/qcom/venus/vdec.c
> > +++ b/drivers/media/platform/qcom/venus/vdec.c
> > @@ -1752,13 +1752,14 @@ static int vdec_close(struct file *file)
> >  	cancel_work_sync(&inst->delayed_process_work);
> >  	v4l2_m2m_ctx_release(inst->m2m_ctx);
> >  	v4l2_m2m_release(inst->m2m_dev);
> > -	vdec_ctrl_deinit(inst);
> >  	ida_destroy(&inst->dpb_ids);
> >  	hfi_session_destroy(inst);
> > -	mutex_destroy(&inst->lock);
> > -	mutex_destroy(&inst->ctx_q_lock);
> >  	v4l2_fh_del(&inst->fh);
> >  	v4l2_fh_exit(&inst->fh);
> > +	vdec_ctrl_deinit(inst);
> Why vdec_ctrl_deinit ->v4l2_ctrl_handler_free(&inst->ctrl_handler) needs to
> be called after v4l2_fh_exit?
> Ideally it should be before v4l2_fh_exit.

Because ->fh holds a pointer to ->ctrl_handler

	inst->fh.ctrl_handler = &inst->ctrl_handler

so after vdec_ctrl_deinit() fh holds stale (released) data.
In general destruction in reverse order of initialization is
safer.

init:
	init ctrl
	init fh     // using ctrl

de-init:
	release fh
	release ctrl