From: "Sjur Brændeland" <sjur.brandeland@stericsson.com>
To: Fernando Guzman Lugo <fernando.lugo@ti.com>,
Ohad Ben-Cohen <ohad@wizery.com>
Cc: LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/3] remoteproc: recover a remoteproc when it has crashed
Date: Mon, 20 Aug 2012 15:07:33 +0200 [thread overview]
Message-ID: <CAJK669YTW7Uxm3HtT2ZKoZ6ScTWgc8F9jL8uHqn4p676iw_z-Q@mail.gmail.com> (raw)
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
next reply other threads:[~2012-08-20 13:07 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-08-20 13:07 Sjur Brændeland [this message]
2012-08-20 20:45 ` [PATCH 2/3] remoteproc: recover a remoteproc when it has crashed Guzman Lugo, Fernando
2012-08-22 11:57 ` Sjur BRENDELAND
-- strict thread matches above, loose matches on Subject: below --
2012-08-08 23:07 [PATCH 0/3] remoteproc: introduce rproc recovery Fernando Guzman Lugo
2012-08-08 23:07 ` [PATCH 2/3] remoteproc: recover a remoteproc when it has crashed Fernando Guzman Lugo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAJK669YTW7Uxm3HtT2ZKoZ6ScTWgc8F9jL8uHqn4p676iw_z-Q@mail.gmail.com \
--to=sjur.brandeland@stericsson.com \
--cc=fernando.lugo@ti.com \
--cc=linux-kernel@vger.kernel.org \
--cc=ohad@wizery.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).