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

[Bryan O'Donoghue]

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