Messages in this thread | | | Date | Mon, 20 Aug 2012 15:07:33 +0200 | Subject | Re: [PATCH 2/3] remoteproc: recover a remoteproc when it has crashed | From | Sjur Brændeland <> |
| |
Hi Fernando,
>This patch is introducing rproc_trigger_recover function which is in >charge of recovering the rproc. One way to recover the rproc after a crash >is resetting all its virtio devices. Doing that, all rpmsg drivers are >restored along with the rpmsg devices and that also causes the reset of >the remoteproc making the rpmsg communication with the remoteproc >functional again. So far, rproc_trigger_recover function is only resetting >all virtio devices, if in the future other rproc features are introduced >and need to be reset too, rproc_trigger_recover function should take care >of that.
I think you drop the driver module's ref count during recovery, because rproc_shutdown calls module_put(). Maybe you should move driver ref count handling to rproc_add and rproc_type_release, instead of rproc_boot() and rproc_shutdown()?
... >+int rproc_trigger_recover(struct rproc *rproc) >+{ >+ struct rproc_vdev *rvdev, *rvtmp; >+ >+ dev_err(&rproc->dev, "recovering %s\n", rproc->name); >+ >+ /* clean up remote vdev entries */ >+ list_for_each_entry_safe(rvdev, rvtmp, &rproc->rvdevs, node) >+ rproc_remove_virtio_dev(rvdev);
...
Is this safe? Are you guaranteed that rproc->power is counted down to zero at this point. Won't this fail if the firmware loading starts before all clients has called rproc_shutdown()?
I guess virtio drivers potentially could defer the call to del_vqs() to a work-queue. In this case you cannot be guaranteed that the rproc is shut down at this point. You may also have e.g. platform driver who has previously called rproc_boot() and calls rproc_shutdown() after calling rproc_crash().
I think you should wait for the rproc->power count to go to zero before initiating the firmware_loading. You could e.g. move firmware_loading to rproc_shutdown(), or add a completion.
BTW: I ran into a bug in unregister_virtio_device when testing your feature with SLAB_DEBUG=y. The dev->index is accessed after the device is freed. I'll submit a patch on this at some point.
After fixing that, your patch works for me!
Regards, Sjur
| |