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

[Nicolas Dufresne <nicolas.dufresne@collabora.com>]

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

Le lundi 27 juillet 2026 à 17:11 +0200, Francesco Saverio Pavone a écrit :
> 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.

Looking forward you v5.

Nicolas

> Thanks,
> Francesco
> 
> Il giorno ven 17 lug 2026 alle ore 17:45 Francesco Saverio Pavone
> <pavone.lawyer@gmail.com> ha scritto:
> > 
> > From: Jonas Karlman <jonas@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@kwiboo.se>
> > [fsp: wrote the commit message; the diff is unchanged]
> > Tested-by: Francesco Saverio Pavone <pavone.lawyer@gmail.com>
> > Assisted-by: Claude:claude-opus-4-8
> > Signed-off-by: Francesco Saverio Pavone <pavone.lawyer@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
> > 

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