[PATCH] media: verisilicon: suspend the device before unregistering it on remove

[PATCH] media: verisilicon: suspend the device before unregistering it on remove

[Sergei Khomenkov via B4 Relay]

From: Sergei Khomenkov <shomenkow@gmail.com>

The clocks are enabled and disabled by the runtime PM callbacks.
hantro_remove() calls v4l2_device_unregister() and clk_bulk_unprepare()
first and only then pm_runtime_dont_use_autosuspend(), which suspends
an active device right away. By then v4l2_device_unregister() has
cleared the driver data (it points to the v4l2_device, the first member
of struct hantro_dev), so hantro_runtime_suspend() dereferences NULL.

The device is always active at that point: the driver core resumes it
before calling remove(), and its put only starts the 100 ms autosuspend
timer. So every unbind and every module unload, with or without a
decode before it, gives an "Unpreparing enabled" warning for each clock
and then that oops.

Stop runtime PM before the unregister and the unprepare. If runtime PM
is forbidden through sysfs (power/control set to "on"), the device is
still active after that, so disable its clocks and mark it suspended by
hand.

The probe error path unwinds in the same order. It can only hit this if
a job runs between the encoder node registration and a later probe
failure, so it is left for a follow-up.

Found with an LLM coding assistant while porting mainline Linux to a
Rockchip RK3026 board (G1, ID 0x6731); the assistant also drafted this
change. Tested on that board. Without this change, unloading the idle
module (runtime status "suspended") gives the warnings and the oops.
With it, the idle module unloads with no warning or oops, and so it does
right after an H.264 or VP8 decode with the autosuspend delay raised to
10 s through sysfs; the clock enable and prepare counts are back to
their values before the load. The same holds with power/control set to
"on", over three load, decode and unload cycles on one boot. kmemleak
reports nothing after five load, decode and unload cycles. Built with
W=1 with and without CONFIG_PM. Not tested on other SoCs or with a
power domain.

Fixes: 556ab0b2741d ("media: hantro: release runtime resources when device_run fails")
Cc: stable@vger.kernel.org
Signed-off-by: Sergei Khomenkov <shomenkow@gmail.com>
---
 drivers/media/platform/verisilicon/hantro_drv.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
index 26e5d5547466..8af47375d471 100644
--- a/drivers/media/platform/verisilicon/hantro_drv.c
+++ b/drivers/media/platform/verisilicon/hantro_drv.c
@@ -1286,10 +1286,16 @@ static void hantro_remove(struct platform_device *pdev)
 	hantro_remove_enc_func(vpu);
 	media_device_cleanup(&vpu->mdev);
 	v4l2_m2m_put(vpu->m2m_dev);
-	v4l2_device_unregister(&vpu->v4l2_dev);
-	clk_bulk_unprepare(vpu->variant->num_clocks, vpu->clocks);
+	/* Suspend first: v4l2_device_unregister() clears the driver data. */
 	pm_runtime_dont_use_autosuspend(vpu->dev);
 	pm_runtime_disable(vpu->dev);
+	/* Runtime PM forbidden through sysfs: the device is still active. */
+	if (IS_ENABLED(CONFIG_PM) && !pm_runtime_status_suspended(vpu->dev)) {
+		clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
+		pm_runtime_set_suspended(vpu->dev);
+	}
+	v4l2_device_unregister(&vpu->v4l2_dev);
+	clk_bulk_unprepare(vpu->variant->num_clocks, vpu->clocks);
 	reset_control_assert(vpu->resets);
 }
 

---
base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
change-id: 20261106-hantro-remove-9afbc9b601f9

Best regards,
-- 
Sergei Khomenkov <shomenkow@gmail.com>

Re: [PATCH] media: verisilicon: suspend the device before unregistering it on remove

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 4210 bytes --]

Le vendredi 06 novembre 2026 à 12:04 +0300, Sergei Khomenkov via B4 Relay a écrit :
> From: Sergei Khomenkov <shomenkow@gmail.com>
> 
> The clocks are enabled and disabled by the runtime PM callbacks.
> hantro_remove() calls v4l2_device_unregister() and clk_bulk_unprepare()
> first and only then pm_runtime_dont_use_autosuspend(), which suspends
> an active device right away. By then v4l2_device_unregister() has
> cleared the driver data (it points to the v4l2_device, the first member
> of struct hantro_dev), so hantro_runtime_suspend() dereferences NULL.
> 
> The device is always active at that point: the driver core resumes it
> before calling remove(), and its put only starts the 100 ms autosuspend
> timer. So every unbind and every module unload, with or without a
> decode before it, gives an "Unpreparing enabled" warning for each clock
> and then that oops.
> 
> Stop runtime PM before the unregister and the unprepare. If runtime PM
> is forbidden through sysfs (power/control set to "on"), the device is
> still active after that, so disable its clocks and mark it suspended by
> hand.
> 
> The probe error path unwinds in the same order. It can only hit this if
> a job runs between the encoder node registration and a later probe
> failure, so it is left for a follow-up.
> 
> Found with an LLM coding assistant while porting mainline Linux to a
> Rockchip RK3026 board (G1, ID 0x6731); the assistant also drafted this
> change. Tested on that board. Without this change, unloading the idle
> module (runtime status "suspended") gives the warnings and the oops.
> With it, the idle module unloads with no warning or oops, and so it does
> right after an H.264 or VP8 decode with the autosuspend delay raised to
> 10 s through sysfs; the clock enable and prepare counts are back to
> their values before the load. The same holds with power/control set to
> "on", over three load, decode and unload cycles on one boot. kmemleak
> reports nothing after five load, decode and unload cycles. Built with
> W=1 with and without CONFIG_PM. Not tested on other SoCs or with a
> power domain.
> 
> Fixes: 556ab0b2741d ("media: hantro: release runtime resources when device_run fails")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sergei Khomenkov <shomenkow@gmail.com>
> ---
>  drivers/media/platform/verisilicon/hantro_drv.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
> index 26e5d5547466..8af47375d471 100644
> --- a/drivers/media/platform/verisilicon/hantro_drv.c
> +++ b/drivers/media/platform/verisilicon/hantro_drv.c
> @@ -1286,10 +1286,16 @@ static void hantro_remove(struct platform_device *pdev)
>  	hantro_remove_enc_func(vpu);
>  	media_device_cleanup(&vpu->mdev);
>  	v4l2_m2m_put(vpu->m2m_dev);
> -	v4l2_device_unregister(&vpu->v4l2_dev);
> -	clk_bulk_unprepare(vpu->variant->num_clocks, vpu->clocks);
> +	/* Suspend first: v4l2_device_unregister() clears the driver data. */
>  	pm_runtime_dont_use_autosuspend(vpu->dev);
>  	pm_runtime_disable(vpu->dev);
> +	/* Runtime PM forbidden through sysfs: the device is still active. */
> +	if (IS_ENABLED(CONFIG_PM) && !pm_runtime_status_suspended(vpu->dev)) {
> +		clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
> +		pm_runtime_set_suspended(vpu->dev);
> +	}
> +	v4l2_device_unregister(&vpu->v4l2_dev);

To be verified, but its not entirely logical here. My impression was that you
want to unregister the dev entry sooner, so that it prevent newly open, and turn
existing fh into a mode where all ioctl will fail.

Can you do some extra code inspection and report why you strictly need to modify
the clock states before you unregister the device?

p.s. are you sure outside of test build, that PM can be disabled on any of the
platform hantro driver runs ?

> +	clk_bulk_unprepare(vpu->variant->num_clocks, vpu->clocks);
>  	reset_control_assert(vpu->resets);
>  }
>  
> 
> ---
> base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
> change-id: 20261106-hantro-remove-9afbc9b601f9
> 
> Best regards,

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]