RE: [PATCH V2] remoteproc: virtio: Fix wdg cannot recovery remote processor

[Joakim Zhang]

Hello Arnaud,

> -----Original Message-----
> From: Arnaud POULIQUEN <arnaud.pouliquen@xxxxxxxxxxx>
> Sent: Saturday, December 16, 2023 12:56 AM
> To: Joakim Zhang <joakim.zhang@xxxxxxxxxxx>; andersson@xxxxxxxxxx;
> mathieu.poirier@xxxxxxxxxx
> Cc: linux-remoteproc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; cix-
> kernel-upstream <cix-kernel-upstream@xxxxxxxxxxx>
> Subject: Re: [PATCH V2] remoteproc: virtio: Fix wdg cannot recovery remote
> processor
> 
> Hello  Joakim,
> 
> On 12/15/23 15:50, joakim.zhang@xxxxxxxxxxx wrote:
> > From: Joakim Zhang <joakim.zhang@xxxxxxxxxxx>
> >
> > Recovery remote processor failed when wdg irq received:
> > [    0.842574] remoteproc remoteproc0: crash detected in cix-dsp-rproc:
> type watchdog
> > [    0.842750] remoteproc remoteproc0: handling crash #1 in cix-dsp-rproc
> > [    0.842824] remoteproc remoteproc0: recovering cix-dsp-rproc
> > [    0.843342] remoteproc remoteproc0: stopped remote processor cix-dsp-
> rproc
> > [    0.847901] rproc-virtio rproc-virtio.0.auto: Failed to associate buffer
> > [    0.847979] remoteproc remoteproc0: failed to probe subdevices for cix-
> dsp-rproc: -16
> >
> > The reason is that dma coherent mem would not be released when
> > recovering the remote processor, due to rproc_virtio_remove() would
> > not be called, where the mem released. It will fail when it try to
> > allocate and associate buffer again.
> >
> > We can see that dma coherent mem allocated from
> > rproc_add_virtio_dev(), so should release it from
> > rproc_remove_virtio_dev(). These functions should appear symmetrically:
> > -rproc_vdev_do_start()->rproc_add_virtio_dev()-
> >dma_declare_coherent_m
> > emory()
> > -rproc_vdev_do_stop()->rproc_remove_virtio_dev()-
> >dma_release_coherent
> > _memory()
> >
> > The same for of_reserved_mem_device_init_by_idx() and
> of_reserved_mem_device_release().
> >
> > Fixes: 1d7b61c06dc3 ("remoteproc: virtio: Create platform device for
> > the remoteproc_virtio")
> > Signed-off-by: Joakim Zhang <joakim.zhang@xxxxxxxxxxx>
> > ---
> > ChangeLogs:
> > V1->V2:
> >         * the same for of_reserved_mem_device_release()
> > ---
> >  drivers/remoteproc/remoteproc_virtio.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/remoteproc/remoteproc_virtio.c
> > b/drivers/remoteproc/remoteproc_virtio.c
> > index 83d76915a6ad..e877ee78740d 100644
> > --- a/drivers/remoteproc/remoteproc_virtio.c
> > +++ b/drivers/remoteproc/remoteproc_virtio.c
> > @@ -465,8 +465,12 @@ static int rproc_add_virtio_dev(struct rproc_vdev
> > *rvdev, int id)  static int rproc_remove_virtio_dev(struct device
> > *dev, void *data)  {
> >         struct virtio_device *vdev = dev_to_virtio(dev);
> > +       struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
> >
> >         unregister_virtio_device(vdev);
> > +       of_reserved_mem_device_release(&rvdev->pdev->dev);
> > +       dma_release_coherent_memory(&rvdev->pdev->dev);
> > +
> 
> At this step, the virtio device may not be released and may still be using the
> memory.
> Do you try to move this in rproc_virtio_dev_release?

Oh, yes, thanks for the hint, I tested, and it can fix the issue, I will send v3 soon.

Joakim
> Regards,
> Arnaud
> 
> >         return 0;
> >  }
> >
> > @@ -584,9 +588,6 @@ static void rproc_virtio_remove(struct
> platform_device *pdev)
> >         rproc_remove_subdev(rproc, &rvdev->subdev);
> >         rproc_remove_rvdev(rvdev);
> >
> > -       of_reserved_mem_device_release(&pdev->dev);
> > -       dma_release_coherent_memory(&pdev->dev);
> > -
> >         put_device(&rproc->dev);
> >  }
> >
> > --
> > 2.25.1