Re: [PATCH v3] media: rkvdec: fix clk reference leak on unbind

[Nicolas Dufresne]

Hi Francesco,


This was sent as a reply to v2, best practice is that new version should be its
own thread. This convention is best for patchwork tracking and other tools.

cheers,
Nicolas

Le vendredi 17 juillet 2026 à 17:04 +0200, Francesco Saverio Pavone a écrit :
> From: Jonas Karlman <jonas@xxxxxxxxx>
> 
> remove() calls pm_runtime_disable() before
> pm_runtime_dont_use_autosuspend(), so the second call can never suspend
> the device: it reaches rpm_idle(), which returns -EACCES once PM runtime
> is disabled. The probe error path has had the two the other way round
> since the driver was merged.
> 
> This shows up when the device is unbound while the 100ms autosuspend
> window is still open, which is what an rmmod right after a decode does.
> device_release_driver() calls pm_runtime_put_sync() before .remove(),
> and rpm_idle() adds RPM_AUTO on its own, so that put only arms the
> autosuspend timer. pm_runtime_disable() then cancels the timer, and
> pm_runtime_reinit() relabels the device suspended without calling the
> driver back. The clk_bulk reference taken by rkvdec_runtime_resume() is
> never dropped, and a later probe does not reclaim it, so every such
> unbind leaks one enable count.
> 
> Drop autosuspend first, so the callback still runs and releases the
> clocks.
> 
> The PM calls also have to move ahead of rkvdec_v4l2_cleanup() rather
> than just swap with each other. rkvdec_runtime_suspend() looks its state
> up with dev_get_drvdata(), and v4l2_device_unregister() clears it:
> struct rkvdec_dev has v4l2_device as its first member, so
> &rkvdec->v4l2_dev and rkvdec are the same address and the check in
> v4l2_device_disconnect() matches. That is harmless today because
> pm_runtime_disable() suppresses the callback, but once the callback can
> run, a suspend after the V4L2 teardown dereferences NULL. Swapping only
> the two PM calls oopses on every unbind.
> 
> Fixes: cd33c830448b ("media: rkvdec: Add the rkvdec driver")
> Signed-off-by: Jonas Karlman <jonas@xxxxxxxxx>
> [fsp: wrote the commit message; the diff is unchanged]
> Tested-by: Francesco Saverio Pavone <pavone.lawyer@xxxxxxxxx>
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Francesco Saverio Pavone <pavone.lawyer@xxxxxxxxx>
> ---
> Changes in v3:
>  - Rewrote the commit message, and dropped the VP9 claim from v1 and v2.
>    Those said this fixed a VP9 inter-prediction bug on RK3588, green chroma
>    from the second ALTREF frame onward. The bug is real, but this is not
>    what fixes it, and I should have established that before sending v1.
> 
>    What happened: I took this patch out of chewitt's tree along with two
>    others and tested the three as a batch. The green is fixed by "media:
>    rkvdec: implement reset controls" from Alex Bee, which adds the
>    reset_control handling that recovers the VDPU381 after a transient error
>    (COLMV_REF_ERR_STA and friends) instead of leaving it dirty for the next
>    inter frame. Randy Li's PMU idle export goes with it. This patch was the
>    third one in that batch and got the credit.
> 
>    Retested this week on the same Rock 5B+ with an unpatched driver: a VP9
>    Profile 0 1080p clip with alt-ref frames decodes byte-identical to the
>    libvpx reference, across five rmmod/insmod cycles and after an unbind
>    inside the autosuspend window. The green does not come back, because the
>    reset_control work is in the tree I test on. Sorry for the review and the
>    testing you spent on that basis.
> 
>  - Worth flagging separately: mainline rkvdec has no reset_control support
>    at all, so the VDPU381 is never recovered after a transient error. That
>    is a real gap, it is just not this patch. I can write it up properly if
>    that is useful.
> 
>  - The diff is unchanged from v1 and v2. It is Jonas's 2020 commit verbatim,
>    and his original one-line subject already described exactly what it does.
>    The wrong story was mine, not his.
> 
>  - The subject changed with the message: "media: rkvdec: fix PM runtime
>    teardown ordering in remove" in v1 and v2, "media: rkvdec: fix clk
>    reference leak on unbind" here, since that is what it actually fixes.
> 
>  - What is left is measured. With a dev_info() at the top of
>    rkvdec_runtime_suspend(), autosuspend_delay raised to 60s to take the
>    timer out of the race, and unbind driven through sysfs:
>        unpatched: 0 suspend callbacks, aclk_rkvdec0 enable_count 1 -> 2
>        patched:   1 suspend callback,  enable_count 1 -> 1
>    The leak survives rmmod and accumulates one per unbind. With
>    autosuspend_delay=0 both orders suspend once, which is the control: the
>    difference only exists inside the window.
> 
>  - Fixes: was wrong in v1 and v2. ff8c5622f9f7 has the two pm_runtime calls
>    as context and only added iommu_domain_free(). cd33c830448b added remove()
>    with the reversed order, and the probe error path with the right one, so
>    the tag points there now.
> 
>  - Dropped Cc: stable. A clk reference leaked on unbind is not backport
>    material, and the tag was only there for the VP9 claim.
> 
>  - Dropped your Reviewed-by and Tested-by from v2: they were given for a fix
>    to something else.
> 
>  - Not included, happy to send as follow-ups: clearing empty_domain after
>    iommu_domain_free(), and hoisting the unregisters to the top of remove()
>    as you suggested on v2.
> 
> Tested on a Radxa Rock 5B+ (RK3588) on a 7.1 tree where the six calls in
> rkvdec_v4l2_cleanup() are open-coded; the executed sequence is the one this
> patch produces. VP9 decode stays byte-identical to libvpx, and five
> rmmod/insmod cycles leave dmesg clean.
> 
> Link to v1:
> https://lore.kernel.org/all/20260518105413.42147-1-pavone.lawyer@xxxxxxxxx/
> Link to v2:
> https://lore.kernel.org/all/20260518145414.64514-1-pavone.lawyer@xxxxxxxxx/
>  drivers/media/platform/rockchip/rkvdec/rkvdec.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/rockchip/rkvdec/rkvdec.c
> b/drivers/media/platform/rockchip/rkvdec/rkvdec.c
> index 1d1e9bfef8e9..0ec3fca9cccc 100644
> --- a/drivers/media/platform/rockchip/rkvdec/rkvdec.c
> +++ b/drivers/media/platform/rockchip/rkvdec/rkvdec.c
> @@ -1869,12 +1869,13 @@ static void rkvdec_remove(struct platform_device
> *pdev)
>  
>  	cancel_delayed_work_sync(&rkvdec->watchdog_work);
>  
> -	rkvdec_v4l2_cleanup(rkvdec);
> -	pm_runtime_disable(&pdev->dev);
>  	pm_runtime_dont_use_autosuspend(&pdev->dev);
>  
>  	if (rkvdec->empty_domain)
>  		iommu_domain_free(rkvdec->empty_domain);
> +
> +	pm_runtime_disable(&pdev->dev);
> +	rkvdec_v4l2_cleanup(rkvdec);
>  }
>  
>  #ifdef CONFIG_PM

Attachment: signature.asc
Description: This is a digitally signed message part