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@xxxxxxxxxxx>님이 작성:
>
> 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@xxxxxxxxxxxxxxx
> > Assisted-by: LLM
> > Co-developed-by: Ijae Kim <ae878000@xxxxxxxxx>
> > Signed-off-by: Ijae Kim <ae878000@xxxxxxxxx>
> > Signed-off-by: Myeonghun Pak <mhun512@xxxxxxxxx>
> > ---
> >  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