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

[Francesco Saverio Pavone]

Self-review after running this on a Rock 5B+. The reorder closes the
clk leak, but it opens a window I missed when I sent it.
Moving the PM teardown ahead of rkvdec_v4l2_cleanup() is necessary,
for the drvdata reason in the commit message. The side effect is that
video_unregister_device() is now the last thing remove() does, so
/dev/videoN stays open-able while the empty IOMMU domain is already
freed.
A job queued in that window reaches rkvdec_iommu_restore() and touches
the freed domain. Before the reorder, cleanup ran first and the window
did not exist.
Splitting the helper keeps both properties: unregister the media and
video devices first, which stops new jobs, then tear down PM, then
release the m2m and V4L2 device. Only that last step clears drvdata,
so rkvdec_runtime_suspend() still finds its state.

   cancel_delayed_work_sync(&rkvdec->watchdog_work);

   rkvdec_v4l2_unregister(rkvdec); /* media + video unregister */

   pm_runtime_dont_use_autosuspend(&pdev->dev);

   if (rkvdec->empty_domain)
     iommu_domain_free(rkvdec->empty_domain);

   pm_runtime_disable(&pdev->dev);
   rkvdec_v4l2_release(rkvdec); /* media cleanup, m2m, v4l2 */

Running here on 7.2-rc5: unbind then rebind returns the rkvdec clock
enable counts to their pre-unbind values and /dev/video0 comes back.
I can send this as a v5 carrying both changes, or as a follow-up patch
on top of v4, whichever you prefer.
Thanks,
Francesco

Il giorno ven 17 lug 2026 alle ore 17:45 Francesco Saverio Pavone
<pavone.lawyer at gmail.com> ha scritto:
>
> From: Jonas Karlman <jonas at kwiboo.se>
>
> 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 at kwiboo.se>
> [fsp: wrote the commit message; the diff is unchanged]
> Tested-by: Francesco Saverio Pavone <pavone.lawyer at gmail.com>
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Francesco Saverio Pavone <pavone.lawyer at gmail.com>
> ---
> Changes in v4:
>  - No functional change. Resent as its own thread (no In-Reply-To on v2),
>    per Nicolas's note that a new version should be its own thread for
>    patchwork tracking. Same diff and same commit message as v3.
>
> 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@gmail.com/
> Link to v2: https://lore.kernel.org/all/20260518145414.64514-1-pavone.lawyer@gmail.com/
> Link to v3: https://lore.kernel.org/all/20260717150440.77079-1-pavone.lawyer@gmail.com/
>  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
> --
> 2.54.0
>