[PATCH] media: amphion: Stop mailbox RX before freeing the message buffer

[PATCH] media: amphion: Stop mailbox RX before freeing the message buffer

[Myeonghun Pak]

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;
+
 	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);
 	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

Re: [PATCH] media: amphion: Stop mailbox RX before freeing the message buffer

[Ming Qian(OSS)]

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

Re: [PATCH] media: amphion: Stop mailbox RX before freeing the message buffer

[Myeonghun Pak]

Hi Ming,

Thanks for the review. I’ve incorporated your suggestions and sent v2
as a separate two-patch series.

Regards,
Myeonghun

2026년 9월 23일 (수) 오전 5:44, Ming Qian(OSS) <ming.qian@oss.nxp.com>님이 작성:
>
> 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