* [PATCHv3 0/3] media: venus: close() fixes
@ 2024-10-25 3:46 Sergey Senozhatsky
2024-10-25 3:46 ` [PATCHv3 1/3] media: venus: fix enc/dec destruction order Sergey Senozhatsky
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Sergey Senozhatsky @ 2024-10-25 3:46 UTC (permalink / raw)
To: Stanimir Varbanov, Vikash Garodia, Bryan O'Donoghue
Cc: linux-media, linux-kernel, 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
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCHv3 1/3] media: venus: fix enc/dec destruction order 2024-10-25 3:46 [PATCHv3 0/3] media: venus: close() fixes Sergey Senozhatsky @ 2024-10-25 3:46 ` Sergey Senozhatsky 2024-10-25 7:20 ` Dikshita Agarwal 2024-10-25 3:46 ` [PATCHv3 2/3] media: venus: sync with threaded IRQ during inst destruction Sergey Senozhatsky 2024-10-25 3:46 ` [PATCHv3 3/3] media: venus: factor out inst destruction routine Sergey Senozhatsky 2 siblings, 1 reply; 7+ messages in thread From: Sergey Senozhatsky @ 2024-10-25 3:46 UTC (permalink / raw) To: Stanimir Varbanov, Vikash Garodia, Bryan O'Donoghue Cc: linux-media, linux-kernel, Sergey Senozhatsky, Tomasz Figa 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCHv3 1/3] media: venus: fix enc/dec destruction order 2024-10-25 3:46 ` [PATCHv3 1/3] media: venus: fix enc/dec destruction order Sergey Senozhatsky @ 2024-10-25 7:20 ` Dikshita Agarwal 2024-10-25 9:47 ` Sergey Senozhatsky 0 siblings, 1 reply; 7+ messages in thread From: Dikshita Agarwal @ 2024-10-25 7:20 UTC (permalink / raw) To: Sergey Senozhatsky, Stanimir Varbanov, Vikash Garodia, Bryan O'Donoghue Cc: linux-media, linux-kernel, Tomasz Figa 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); > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCHv3 1/3] media: venus: fix enc/dec destruction order 2024-10-25 7:20 ` Dikshita Agarwal @ 2024-10-25 9:47 ` Sergey Senozhatsky 0 siblings, 0 replies; 7+ messages in thread From: Sergey Senozhatsky @ 2024-10-25 9:47 UTC (permalink / raw) To: Dikshita Agarwal Cc: Sergey Senozhatsky, Stanimir Varbanov, Vikash Garodia, Bryan O'Donoghue, linux-media, linux-kernel, Tomasz Figa 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCHv3 2/3] media: venus: sync with threaded IRQ during inst destruction 2024-10-25 3:46 [PATCHv3 0/3] media: venus: close() fixes Sergey Senozhatsky 2024-10-25 3:46 ` [PATCHv3 1/3] media: venus: fix enc/dec destruction order Sergey Senozhatsky @ 2024-10-25 3:46 ` Sergey Senozhatsky 2024-10-25 3:46 ` [PATCHv3 3/3] media: venus: factor out inst destruction routine Sergey Senozhatsky 2 siblings, 0 replies; 7+ messages in thread From: Sergey Senozhatsky @ 2024-10-25 3:46 UTC (permalink / raw) To: Stanimir Varbanov, Vikash Garodia, Bryan O'Donoghue Cc: linux-media, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCHv3 3/3] media: venus: factor out inst destruction routine 2024-10-25 3:46 [PATCHv3 0/3] media: venus: close() fixes Sergey Senozhatsky 2024-10-25 3:46 ` [PATCHv3 1/3] media: venus: fix enc/dec destruction order Sergey Senozhatsky 2024-10-25 3:46 ` [PATCHv3 2/3] media: venus: sync with threaded IRQ during inst destruction Sergey Senozhatsky @ 2024-10-25 3:46 ` Sergey Senozhatsky 2024-10-25 8:06 ` Dikshita Agarwal 2 siblings, 1 reply; 7+ messages in thread From: Sergey Senozhatsky @ 2024-10-25 3:46 UTC (permalink / raw) To: Stanimir Varbanov, Vikash Garodia, Bryan O'Donoghue Cc: linux-media, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCHv3 3/3] media: venus: factor out inst destruction routine 2024-10-25 3:46 ` [PATCHv3 3/3] media: venus: factor out inst destruction routine Sergey Senozhatsky @ 2024-10-25 8:06 ` Dikshita Agarwal 0 siblings, 0 replies; 7+ messages in thread From: Dikshita Agarwal @ 2024-10-25 8:06 UTC (permalink / raw) To: Sergey Senozhatsky, Stanimir Varbanov, Vikash Garodia, Bryan O'Donoghue Cc: linux-media, linux-kernel 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); ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-10-25 9:47 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-10-25 3:46 [PATCHv3 0/3] media: venus: close() fixes Sergey Senozhatsky 2024-10-25 3:46 ` [PATCHv3 1/3] media: venus: fix enc/dec destruction order Sergey Senozhatsky 2024-10-25 7:20 ` Dikshita Agarwal 2024-10-25 9:47 ` Sergey Senozhatsky 2024-10-25 3:46 ` [PATCHv3 2/3] media: venus: sync with threaded IRQ during inst destruction Sergey Senozhatsky 2024-10-25 3:46 ` [PATCHv3 3/3] media: venus: factor out inst destruction routine Sergey Senozhatsky 2024-10-25 8:06 ` Dikshita Agarwal
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.