Re: [PATCH v4 2/2] media: venus: disable recovery work before HFI teardown

[Myeonghun Pak]

Hi Bryan,

Thanks for the review.

In v5, I removed the disable_delayed_work_sync() call from
venus_hfi_destroy() and kept it only at the start of venus_remove().
The work is now disabled exactly once during removal.

Thanks,
Myeonghun

2026년 10월 1일 (목) 오후 4:10, Bryan O'Donoghue <bod@xxxxxxxxxx>님이 작성:

>
> On 01/10/2026 18:55, Myeonghun Pak wrote:
> > venus_remove() cancels core->work before the IRQ is disabled. An IRQ
> > thread can queue the work again after cancellation. The work can then
> > access HFI state after venus_hfi_destroy() frees it. The work also
> > requeues itself when recovery fails, so cancelling an already running
> > instance alone does not close the race.
> >
> > Disable and drain the work at the start of remove, before other resources
> > are dismantled. Do the same in venus_hfi_destroy() for paths that bypass
> > remove, including probe unwind. Disabling the work prevents both IRQ
> > handlers and the work itself from requeuing it. Drain the work before
> > disabling the IRQ so an active recovery can finish any IRQ based
> > completion waits. Then synchronize the IRQ before freeing HFI state.
> >
> > Fixes: af2c3834c8ca ("[media] media: venus: adding core part and helper functions")
> > Reported-by: Sashiko <sashiko-bot@xxxxxxxxxx>
> > Link: https://lore.kernel.org/all/20260730153912.BAC5E1F00A3D@xxxxxxxxxxxxxxx/
> > Cc: stable@xxxxxxxxxxxxxxx
> > Assisted-by: LLM
> > Co-developed-by: Ijae Kim <ae878000@xxxxxxxxx>
> > Signed-off-by: Ijae Kim <ae878000@xxxxxxxxx>
> > Signed-off-by: Myeonghun Pak <mhun512@xxxxxxxxx>
> > ---
> >   drivers/media/platform/qcom/venus/core.c      | 2 +-
> >   drivers/media/platform/qcom/venus/hfi_venus.c | 1 +
> >   2 files changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> > index 7087af32060f..6e495bc04691 100644
> > --- a/drivers/media/platform/qcom/venus/core.c
> > +++ b/drivers/media/platform/qcom/venus/core.c
> > @@ -596,7 +596,7 @@ static void venus_remove(struct platform_device *pdev)
> >       struct device *dev = core->dev;
> >       int ret;
> >
> > -     cancel_delayed_work_sync(&core->work);
> > +     disable_delayed_work_sync(&core->work);
> >       ret = pm_runtime_get_sync(dev);
> >       WARN_ON(ret < 0);
> >
> > diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
> > index e7e4e78a186a..20b8ba1e62f1 100644
> > --- a/drivers/media/platform/qcom/venus/hfi_venus.c
> > +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
> > @@ -1689,6 +1689,7 @@ void venus_hfi_destroy(struct venus_core *core)
> >   {
> >       struct venus_hfi_device *hdev = to_hfi_priv(core);
> >
> > +     disable_delayed_work_sync(&core->work);
> >       disable_irq(core->irq);
> >       core->priv = NULL;
> >       venus_interface_queues_release(hdev);
> > --
> > 2.53.0
> >
>
> You shouldn't have to disable the work queue twice.
>
> Does a path actually exist where hfi_destroy() runs but venus_remove()
> does not ?
>
> The commit text is vague about that - sure hfi_destroy() may be callable
> from the error path of probe() but is any work scheduled in that case ?
>
> I mean lets just look at venus_hfi_destroy()
>
>
> static void venus_remove(struct platform_device *pdev)
> {
>          struct venus_core *core = platform_get_drvdata(pdev);
>          const struct venus_pm_ops *pm_ops = core->pm_ops;
>          struct device *dev = core->dev;
>          int ret;
>
>          cancel_delayed_work_sync(&core->work);
>          ret = pm_runtime_get_sync(dev);
>          WARN_ON(ret < 0);
>
>          ret = hfi_core_deinit(core, true);
>          WARN_ON(ret);
>
>          venus_shutdown(core);
>          of_platform_depopulate(dev);
>
>          venus_firmware_deinit(core);
>
>          venus_remove_dynamic_nodes(core);
>
>          pm_runtime_put_sync(dev);
>          pm_runtime_disable(dev);
>
>          if (pm_ops->core_put)
>                  pm_ops->core_put(core);
>
>          v4l2_device_unregister(&core->v4l2_dev);
>
>          hfi_destroy(core);
>
>          mutex_destroy(&core->pm_lock);
>          mutex_destroy(&core->lock);
>          venus_dbgfs_deinit(core);
> }
>
> Your patch will call disable_delayed_work_sync() twice on this path once
> in venus_remove() per your patch and then again in hfi_destroy()...
>
> NAK - can't be right.
>
> ---
> bod