* Re: [PATCH] media: amphion: Stop mailbox RX before freeing the message buffer
2026-09-23 5:19 [PATCH] media: amphion: Stop mailbox RX before freeing the message buffer Myeonghun Pak
@ 2026-09-23 9:44 ` Ming Qian(OSS)
0 siblings, 0 replies; 2+ messages in thread
From: Ming Qian(OSS) @ 2026-09-23 9:44 UTC (permalink / raw)
To: Myeonghun Pak
Cc: Ming Qian, Zhou Peng, Mauro Carvalho Chehab, linux-media,
linux-kernel, stable, Ijae Kim
Hi Myeonghun, Ijae,
Thanks for the patch.
On Wed, Sep 23, 2026 at 01:19:50AM -0400, Myeonghun Pak wrote:
> vpu_core_unregister() frees core->msg_buffer and clears core->vpu
> before cancelling the message workers. The fifo keeps its own pointer
> to that buffer. The core workers read it and queue inst->msg_work,
> which uses core->vpu and may send on TX.
>
> vpu_isr() fills the fifo from the mailbox RX callback. i.MX MU
> free_irq() waits for that callback, not for the queued work. An open
> instance holds a runtime-PM reference, so remove can skip runtime
> suspend and leave RX active.
>
> Stop RX, then drain the core workers and the instance workers they
> queued with vpu_core_cancel_work(), while core->vpu and TX are still
> valid. Free the buffer and clear core->vpu after that drain. Drop
> the runtime reference only then: runtime suspend frees every channel.
>
> Fixes: 9f599f351e86 ("media: amphion: add vpu core driver")
> Cc: stable@vger.kernel.org
> Assisted-by: LLM
> Co-developed-by: Ijae Kim <ae878000@gmail.com>
> Signed-off-by: Ijae Kim <ae878000@gmail.com>
> Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
> ---
> drivers/media/platform/amphion/vpu_core.c | 28 +++++++++++++++++++---------
> drivers/media/platform/amphion/vpu_mbox.c | 9 +++++++--
> drivers/media/platform/amphion/vpu_mbox.h | 1 +
> 3 files changed, 27 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/media/platform/amphion/vpu_core.c b/drivers/media/platform/amphion/vpu_core.c
> index 85cc4a14f8ed..000000000000 100644
> --- a/drivers/media/platform/amphion/vpu_core.c
> +++ b/drivers/media/platform/amphion/vpu_core.c
> @@ -299,21 +299,23 @@ static void vpu_core_put_vpu(struct vpu_core *core)
> core->vpu->put_vpu(core->vpu);
> }
>
> +static void vpu_core_cancel_work(struct vpu_core *core);
> +
> static int vpu_core_unregister(struct device *dev, struct vpu_core *core)
> {
> list_del_init(&core->list);
>
> vpu_core_put_vpu(core);
> - core->vpu = NULL;
> - kfree(core->msg_buffer);
> - core->msg_buffer = NULL;
>
> if (core->workqueue) {
> - cancel_work_sync(&core->msg_work);
> - cancel_delayed_work_sync(&core->msg_delayed_work);
> + vpu_core_cancel_work(core);
> destroy_workqueue(core->workqueue);
> core->workqueue = NULL;
> }
>
> + kfree(core->msg_buffer);
> + core->msg_buffer = NULL;
> + core->vpu = NULL;
> +
This looks correct to me. One suggestion: please move vpu_core_put_vpu() below
the drain as well, so that the whole release sequence sits after
vpu_core_cancel_work(). That would also match vpu_core_suspend().
> return 0;
> }
> @@ -700,10 +702,18 @@ static void vpu_core_remove(struct platform_device *pdev)
> WARN_ON(ret < 0);
>
> vpu_core_shutdown(core);
> - pm_runtime_put_sync(dev);
> - pm_runtime_disable(dev);
> -
> - vpu_core_unregister(core->parent, core);
> + /*
> + * Runtime suspend frees every mailbox channel, including TX,
> + * and is skipped while another runtime-PM reference remains.
> + * Stop RX first. vpu_core_unregister() then drains the core
> + * workers and the instance workers they queue, which may still
> + * send on TX, before the fifo buffer and core->vpu are released.
> + */
> + vpu_mbox_free_rx(core);
> + vpu_core_unregister(core->parent, core);
> + pm_runtime_put_sync(dev);
> + pm_runtime_disable(dev);
> + vpu_mbox_free(core);
This part, though, I don't think we need.
The driver assumes that all instances are already closed by the time remove()
runs. struct vpu_core is devm_kzalloc()'d on the core platform device, and
inst->core is just a plain pointer taken in vpu_inst_register(). So if an
instance is still open when the core device goes away, inst->core is left
dangling, and the next ioctl - or close() -> vpu_inst_unregister() ->
mutex_lock(&core->lock) - will panic. get_device(core->dev) doesn't help here;
it pins the struct device, not the driver data.
As long as that assumption holds, the existing order is fine: the runtime-PM
reference is dropped before vpu_core_unregister(), so
vpu_core_runtime_suspend() -> vpu_mbox_free() has already released RX along with
its irq, and vpu_isr() can no longer fill the fifo. So I'd drop this part, along
with vpu_mbox_free_rx() and the vpu_mbox.h change. That also keeps the fix small,
which is nicer for stable.
That said, the hole you're pointing at is real: remove() can be reached today
with an instance still open. I just don't think the right answer is to harden
this path. rmmod is already safe, because vdec_fops/venc_fops set
.owner = THIS_MODULE and an open /dev/videoX holds the module reference. sysfs
unbind isn't. So how about closing that instead, as a separate patch:
- .suppress_bind_attrs = true on amphion_vpu_core_driver, and on
amphion_vpu_driver too - otherwise unbinding the parent ends up in the same
place.
- WARN_ON(!list_empty(&core->instances)) at the beginning of
vpu_core_remove(), so the assumption is stated in the code rather
than implied.
Regards,
Ming
> memunmap(core->fw.virt);
> memunmap(core->rpc.virt);
> mutex_destroy(&core->lock);
> diff --git a/drivers/media/platform/amphion/vpu_mbox.c b/drivers/media/platform/amphion/vpu_mbox.c
> index b2ac8de6a2d9..521fd47f1d90 100644
> --- a/drivers/media/platform/amphion/vpu_mbox.c
> +++ b/drivers/media/platform/amphion/vpu_mbox.c
> @@ -88,14 +88,19 @@ error:
> return ret;
> }
>
> +void vpu_mbox_free_rx(struct vpu_core *core)
> +{
> + mbox_free_channel(core->rx.ch);
> + core->rx.ch = NULL;
> +}
> +
> void vpu_mbox_free(struct vpu_core *core)
> {
> mbox_free_channel(core->tx_type.ch);
> mbox_free_channel(core->tx_data.ch);
> - mbox_free_channel(core->rx.ch);
> core->tx_type.ch = NULL;
> core->tx_data.ch = NULL;
> - core->rx.ch = NULL;
> + vpu_mbox_free_rx(core);
> dev_dbg(core->dev, "%s free mbox\n", vpu_core_type_desc(core->type));
> }
>
> diff --git a/drivers/media/platform/amphion/vpu_mbox.h b/drivers/media/platform/amphion/vpu_mbox.h
> index 8b7aea4f606c..a934fe61b294 100644
> --- a/drivers/media/platform/amphion/vpu_mbox.h
> +++ b/drivers/media/platform/amphion/vpu_mbox.h
> @@ -8,6 +8,7 @@
>
> int vpu_mbox_init(struct vpu_core *core);
> int vpu_mbox_request(struct vpu_core *core);
> +void vpu_mbox_free_rx(struct vpu_core *core);
> void vpu_mbox_free(struct vpu_core *core);
> void vpu_mbox_send_msg(struct vpu_core *core, u32 type, u32 data);
> void vpu_mbox_send_type(struct vpu_core *core, u32 type);
> base-commit: 238650ef6c7c7cca08e032527329424c9fbd70e5
> --
> 2.47.1
^ permalink raw reply [flat|nested] 2+ messages in thread