[PATCHv2 0/3] remoteproc: introduce rproc recovery

[PATCHv2 0/3] remoteproc: introduce rproc recovery

[Fernando Guzman Lugo]

These set of patches make possible the remoteproc recover after a crash.
This is a hard recovery, that means the remoteproc is reset and it will
start from the beginning. When a crash happen all the virtio devices are
destroyed. Therefore, both rpmsg drivers and devices are gracefully
removed which also cause rproc users become 0 and the remoteproc is turned
off. After the virtio devices are destroyed the crash handler function
will read the virtio information from the firmware in order to recreate
the virtio devices that will boot the remoteproc and everything will be
functional again.

-Version 2
* Wait untill there is no more rproc users before realoding the FW again
  in order to make sure the recovery will work fine. As suggested by
  Sjur BRENDELAND

Fernando Guzman Lugo (3):
  remoteproc: add rproc_report_crash function to notify rproc crashes
  remoteproc: recover a remoteproc when it has crashed
  remoteproc: create debugfs entry to disable/enable recovery
    dynamically

 Documentation/remoteproc.txt             |    7 ++
 drivers/remoteproc/remoteproc_core.c     |  116 ++++++++++++++++++++++++++++--
 drivers/remoteproc/remoteproc_debugfs.c  |   83 +++++++++++++++++++++
 drivers/remoteproc/remoteproc_internal.h |    1 +
 include/linux/remoteproc.h               |   22 ++++++
 5 files changed, 222 insertions(+), 7 deletions(-)

[PATCHv2 1/3] remoteproc: add rproc_report_crash function to notify rproc crashes

[Fernando Guzman Lugo]

This patch is exporting the rproc_report_crash function which can be used
to report a rproc crash to the remoteproc core. This function is specially
thought to be called by low-level remoteproc driver code in case of
detecting a crash (remoteproc is not functional anymore). Using this
function from another driver (non rproc driver) should be analyzed very
carefully most of the time that will be considered wrong.

rproc_report_crash function can be called from any context, that means,
it can be called from atomic context without any problem. The reporter
function is creating a new thread (workqueue work) in charge of handling
the crash (if possible).

Creating this new thread is done for two main reasons. First reason is
to be able to call it from atomic context, due to the fact that many
crashes trigger an interrupt, so this function can be called directly
from ISR context. Second reason is avoid any deadlock condition which
could happen if the rproc_report_crash function is called from a
function which is indirectly holding a rproc lock.

The reporter function is scheduling the crash handler task. This task
is thought to have some features like:

-remoteproc register dump
-remoteproc stack dump
-remoteproc core dump
-Saving of the remoteproc traces in order to be visible after the crash
-Reseting the remoteproc in order to make it functional again (hard recovery)

Right now, it is only printing the crash type which was detected. The
types of crashes are represented by an enum. I have only added mmufault
crash type. Remoteproc low-level drivers can add more types when needed.

Signed-off-by: Fernando Guzman Lugo <fernando.lugo@ti.com>
---
 Documentation/remoteproc.txt         |    7 +++
 drivers/remoteproc/remoteproc_core.c |   80 +++++++++++++++++++++++++++++++---
 include/linux/remoteproc.h           |   18 ++++++++
 3 files changed, 98 insertions(+), 7 deletions(-)

diff --git a/Documentation/remoteproc.txt b/Documentation/remoteproc.txt
index 23a09b8..e6469fd 100644
--- a/Documentation/remoteproc.txt
+++ b/Documentation/remoteproc.txt
@@ -129,6 +129,13 @@ int dummy_rproc_example(struct rproc *my_rproc)
 
       Returns 0 on success and -EINVAL if @rproc isn't valid.
 
+  void rproc_report_crash(struct rproc *rproc, enum rproc_crash_type type)
+    - Report a crash in a remoteproc
+      This function must be called every time a crash is detected by the
+      platform specific rproc implementation. This should not be called from a
+      non-remoteproc driver. This function can be called from atomic/interrupt
+      context.
+
 5. Implementation callbacks
 
 These callbacks should be provided by platform-specific remoteproc
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index d5c2dbf..3a6f1a1 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -50,6 +50,18 @@ typedef int (*rproc_handle_resource_t)(struct rproc *rproc, void *, int avail);
 /* Unique indices for remoteproc devices */
 static DEFINE_IDA(rproc_dev_index);
 
+static const char * const rproc_crash_names[] = {
+	[RPROC_MMUFAULT]	= "mmufault",
+};
+
+/* translate rproc_crash_type to string */
+static const char *rproc_crash_to_string(enum rproc_crash_type type)
+{
+	if (type < ARRAY_SIZE(rproc_crash_names))
+		return rproc_crash_names[type];
+	return "unkown";
+}
+
 /*
  * This is the IOMMU fault handler we register with the IOMMU API
  * (when relevant; not all remote processors access memory through
@@ -57,19 +69,17 @@ static DEFINE_IDA(rproc_dev_index);
  *
  * IOMMU core will invoke this handler whenever the remote processor
  * will try to access an unmapped device address.
- *
- * Currently this is mostly a stub, but it will be later used to trigger
- * the recovery of the remote processor.
  */
 static int rproc_iommu_fault(struct iommu_domain *domain, struct device *dev,
 		unsigned long iova, int flags, void *token)
 {
+	struct rproc *rproc = token;
+
 	dev_err(dev, "iommu fault: da 0x%lx flags 0x%x\n", iova, flags);
 
-	/*
-	 * Let the iommu core know we're not really handling this fault;
-	 * we just plan to use this as a recovery trigger.
-	 */
+	rproc_report_crash(rproc, RPROC_MMUFAULT);
+
+	/* Let the iommu core know we're not really handling this fault; */
 	return -ENOSYS;
 }
 
@@ -872,6 +882,34 @@ out:
 }
 
 /**
+ * rproc_crash_handler_work() - handle a crash
+ *
+ * This function needs to handle everything related to a crash, like cpu
+ * registers and stack dump, information to help to debug the fatal error, etc.
+ */
+static void rproc_crash_handler_work(struct work_struct *work)
+{
+	struct rproc *rproc = container_of(work, struct rproc, crash_handler);
+	struct device *dev = &rproc->dev;
+
+	dev_dbg(dev, "enter %s\n", __func__);
+
+	mutex_lock(&rproc->lock);
+	if (rproc->state == RPROC_CRASHED || rproc->state == RPROC_OFFLINE) {
+		/* handle only the first crash detected */
+		mutex_unlock(&rproc->lock);
+		return;
+	}
+
+	rproc->state = RPROC_CRASHED;
+	dev_err(&rproc->dev, "handling crash #%u in %s\n",
+		++rproc->crash_cnt, rproc->name);
+	mutex_unlock(&rproc->lock);
+
+	/* TODO: handle crash */
+}
+
+/**
  * rproc_boot() - boot a remote processor
  * @rproc: handle of a remote processor
  *
@@ -1165,6 +1203,8 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
 	INIT_LIST_HEAD(&rproc->traces);
 	INIT_LIST_HEAD(&rproc->rvdevs);
 
+	INIT_WORK(&rproc->crash_handler, rproc_crash_handler_work);
+
 	rproc->state = RPROC_OFFLINE;
 
 	return rproc;
@@ -1221,6 +1261,32 @@ int rproc_del(struct rproc *rproc)
 }
 EXPORT_SYMBOL(rproc_del);
 
+/**
+ * rproc_report_crash() - rproc crash reporter function
+ * @rproc: remote processor
+ * @type: crash type
+ *
+ * This function must be called every time a crash is detected by the low-level
+ * drivers implementing a specific remoteproc. This should not be called from a
+ * non-remoteproc driver.
+ *
+ * This function can be called from atomic/interrupt context.
+ */
+void rproc_report_crash(struct rproc *rproc, enum rproc_crash_type type)
+{
+	if (!rproc) {
+		pr_err("NULL rproc pointer\n");
+		return;
+	}
+
+	dev_err(&rproc->dev, "crash detected in %s: type %s\n",
+		rproc->name, rproc_crash_to_string(type));
+
+	/* create a new task to handle the error */
+	schedule_work(&rproc->crash_handler);
+}
+EXPORT_SYMBOL(rproc_report_crash);
+
 static int __init remoteproc_init(void)
 {
 	rproc_init_debugfs();
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 131b539..a46ed27 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -361,6 +361,19 @@ enum rproc_state {
 };
 
 /**
+ * enum rproc_crash_type - remote processor crash types
+ * @RPROC_MMUFAULT:	iommu fault
+ *
+ * Each element of the enum is used as an array index. So that, the value of
+ * the elements should be always something sane.
+ *
+ * Feel free to add more types when needed.
+ */
+enum rproc_crash_type {
+	RPROC_MMUFAULT,
+};
+
+/**
  * struct rproc - represents a physical remote processor device
  * @node: klist node of this rproc object
  * @domain: iommu domain
@@ -383,6 +396,8 @@ enum rproc_state {
  * @rvdevs: list of remote virtio devices
  * @notifyids: idr for dynamically assigning rproc-wide unique notify ids
  * @index: index of this rproc device
+ * @crash_handler: workqueue for handling a crash
+ * @crash_cnt: crash counter
  */
 struct rproc {
 	struct klist_node node;
@@ -406,6 +421,8 @@ struct rproc {
 	struct list_head rvdevs;
 	struct idr notifyids;
 	int index;
+	struct work_struct crash_handler;
+	unsigned crash_cnt;
 };
 
 /* we currently support only two vrings per rvdev */
@@ -460,6 +477,7 @@ int rproc_del(struct rproc *rproc);
 
 int rproc_boot(struct rproc *rproc);
 void rproc_shutdown(struct rproc *rproc);
+void rproc_report_crash(struct rproc *rproc, enum rproc_crash_type type);
 
 static inline struct rproc_vdev *vdev_to_rvdev(struct virtio_device *vdev)
 {
-- 
1.7.1

[PATCHv2 2/3] remoteproc: recover a remoteproc when it has crashed

[Fernando Guzman Lugo]

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.

Signed-off-by: Fernando Guzman Lugo <fernando.lugo@ti.com>
---
 drivers/remoteproc/remoteproc_core.c     |   37 +++++++++++++++++++++++++++++-
 drivers/remoteproc/remoteproc_internal.h |    1 +
 include/linux/remoteproc.h               |    2 +
 3 files changed, 39 insertions(+), 1 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 3a6f1a1..9fbd364 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -882,6 +882,36 @@ out:
 }
 
 /**
+ * rproc_trigger_recover() - recover a remoteproc
+ * @rproc: the remote processor
+ *
+ * The recovery is done by reseting all the virtio devices, that way all the
+ * rpmsg drivers will be reseted along with the remote processor making the
+ * remoteproc functional again.
+ *
+ * This function can sleep, so that it cannot be called from atomic context.
+ */
+int rproc_trigger_recover(struct rproc *rproc)
+{
+	struct rproc_vdev *rvdev, *rvtmp;
+
+	dev_err(&rproc->dev, "recovering %s\n", rproc->name);
+
+	init_completion(&rproc->crash_comp);
+	/* clean up remote vdev entries */
+	list_for_each_entry_safe(rvdev, rvtmp, &rproc->rvdevs, node)
+		rproc_remove_virtio_dev(rvdev);
+
+	/* wait until there is no more rproc users */
+	wait_for_completion(&rproc->crash_comp);
+
+	/* run rproc_fw_config_virtio to create vdevs again */
+	return request_firmware_nowait(THIS_MODULE, FW_ACTION_HOTPLUG,
+			rproc->firmware, &rproc->dev, GFP_KERNEL,
+			rproc, rproc_fw_config_virtio);
+}
+
+/**
  * rproc_crash_handler_work() - handle a crash
  *
  * This function needs to handle everything related to a crash, like cpu
@@ -906,7 +936,7 @@ static void rproc_crash_handler_work(struct work_struct *work)
 		++rproc->crash_cnt, rproc->name);
 	mutex_unlock(&rproc->lock);
 
-	/* TODO: handle crash */
+	rproc_trigger_recover(rproc);
 }
 
 /**
@@ -1030,6 +1060,10 @@ void rproc_shutdown(struct rproc *rproc)
 
 	rproc_disable_iommu(rproc);
 
+	/* if in crash state, unlock crash handler */
+	if (rproc->state == RPROC_CRASHED)
+		complete_all(&rproc->crash_comp);
+
 	rproc->state = RPROC_OFFLINE;
 
 	dev_info(dev, "stopped remote processor %s\n", rproc->name);
@@ -1204,6 +1238,7 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
 	INIT_LIST_HEAD(&rproc->rvdevs);
 
 	INIT_WORK(&rproc->crash_handler, rproc_crash_handler_work);
+	init_completion(&rproc->crash_comp);
 
 	rproc->state = RPROC_OFFLINE;
 
diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
index a690ebe..d9c0730 100644
--- a/drivers/remoteproc/remoteproc_internal.h
+++ b/drivers/remoteproc/remoteproc_internal.h
@@ -63,6 +63,7 @@ void rproc_free_vring(struct rproc_vring *rvring);
 int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);
 
 void *rproc_da_to_va(struct rproc *rproc, u64 da, int len);
+int rproc_trigger_recover(struct rproc *rproc);
 
 static inline
 int rproc_fw_sanity_check(struct rproc *rproc, const struct firmware *fw)
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index a46ed27..0c1a2f9 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -398,6 +398,7 @@ enum rproc_crash_type {
  * @index: index of this rproc device
  * @crash_handler: workqueue for handling a crash
  * @crash_cnt: crash counter
+ * @crash_comp: completion used to sync crash handler and the rproc reload
  */
 struct rproc {
 	struct klist_node node;
@@ -423,6 +424,7 @@ struct rproc {
 	int index;
 	struct work_struct crash_handler;
 	unsigned crash_cnt;
+	struct completion crash_comp;
 };
 
 /* we currently support only two vrings per rvdev */
-- 
1.7.1

[PATCHv2 3/3] remoteproc: create debugfs entry to disable/enable recovery dynamically

[Fernando Guzman Lugo]

Add a debugfs entry (named recovery) so that recovery can be disabled
dynamically at runtime. This entry is very useful when you are trying to
debug a rproc crash. Without this a recovery will take place making
impossible to debug the issue.

Original idea from Ohad Ben-Cohen and contributions from
Subramaniam Chanderashekarapuram

Example:
-disabling recovery:
$ echo disabled > <debugfs>/remoteproc/remoteproc0/recovery

-enabling recovery:
$ echo enabled > <debugfs>/remoteproc/remoteproc0/recovery

-in case you have disabled recovery and you want to continue
 debugging you can recover the remoteproc once using recover.
 This will not change the state of the recovery entry, it will
 only recovery the rproc if its state is RPROC_CRASHED
$ echo recover > <debugfs>/remoteproc/remoteproc0/recovery

Signed-off-by: Fernando Guzman Lugo <fernando.lugo@ti.com>
---
 drivers/remoteproc/remoteproc_core.c    |    3 +-
 drivers/remoteproc/remoteproc_debugfs.c |   83 +++++++++++++++++++++++++++++++
 include/linux/remoteproc.h              |    2 +
 3 files changed, 87 insertions(+), 1 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 9fbd364..16e20f7 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -936,7 +936,8 @@ static void rproc_crash_handler_work(struct work_struct *work)
 		++rproc->crash_cnt, rproc->name);
 	mutex_unlock(&rproc->lock);
 
-	rproc_trigger_recover(rproc);
+	if (!rproc->recovery_disabled)
+		rproc_trigger_recover(rproc);
 }
 
 /**
diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c
index 0383385..aa95cde 100644
--- a/drivers/remoteproc/remoteproc_debugfs.c
+++ b/drivers/remoteproc/remoteproc_debugfs.c
@@ -28,6 +28,9 @@
 #include <linux/debugfs.h>
 #include <linux/remoteproc.h>
 #include <linux/device.h>
+#include <linux/uaccess.h>
+
+#include "remoteproc_internal.h"
 
 /* remoteproc debugfs parent dir */
 static struct dentry *rproc_dbg;
@@ -111,6 +114,84 @@ static const struct file_operations rproc_name_ops = {
 	.llseek	= generic_file_llseek,
 };
 
+/* expose recovery flag via debugfs */
+static ssize_t rproc_recovery_read(struct file *filp, char __user *userbuf,
+		size_t count, loff_t *ppos)
+{
+	struct rproc *rproc = filp->private_data;
+	char *buf = rproc->recovery_disabled ? "disabled\n" : "enabled\n";
+
+	return simple_read_from_buffer(userbuf, count, ppos, buf, strlen(buf));
+}
+
+
+/*
+ * Writing to the recovey debugfs entry we can change the behavior of the
+ * recovery dynamically. The default value of this entry is "enabled".
+ *
+ * There are 3 possible options you can write to the recovery debug entry:
+ * "enabled", "disabled" and "recover"
+ *
+ * enabled:	In this case recovery will be enabled, every time there is a
+ *		rproc crashed the rproc will be recovered. If recovery has been
+ *		disabled and it crashed and you enable recovery it will be
+ *		recover as soon as you enable recovery.
+ * disabled:	In this case recovery will be disabled, that means if a rproc
+ *		crashes it will remain in crashed state. Therefore the rproc
+ *		won't be functional any more. But this option is used for
+ *		debugging purposes. Otherwise, debugging a crash would not be
+ *		possible.
+ * recover:	This function will trigger a recovery without taking care of
+ *		the recovery state (enabled/disabled) and without changing it.
+ *		This useful for the cases when you are debugging a crash and
+ *		after enabling recovery you get another crash immediately. As
+ *		the recovery state will be enabled it will recover the rproc
+ *		without let you debug the new crash. So, it is recommended to
+ *		disabled recovery, then starting debugging and use "recovery"
+ *		command while still debugging and when you are done then you
+ *		case use enabled command.
+ */
+static ssize_t rproc_recovery_write(struct file *filp,
+		const char __user *user_buf, size_t count, loff_t *ppos)
+{
+	struct rproc *rproc = filp->private_data;
+	char buf[10];
+	int ret;
+
+	if (count > sizeof(buf))
+		return count;
+
+	ret = copy_from_user(buf, user_buf, count);
+	if (ret)
+		return ret;
+
+	/* remove end of line */
+	if (buf[count - 1] == '\n')
+		buf[count - 1] = '\0';
+
+	if (!strncmp(buf, "enabled", count)) {
+		rproc->recovery_disabled = false;
+		/* if rproc has crashed trigger recovery */
+		if (rproc->state == RPROC_CRASHED)
+			rproc_trigger_recover(rproc);
+	} else if (!strncmp(buf, "disabled", count)) {
+		rproc->recovery_disabled = true;
+	} else if (!strncmp(buf, "recover", count)) {
+		/* if rproc has crashed trigger recovery */
+		if (rproc->state == RPROC_CRASHED)
+			rproc_trigger_recover(rproc);
+	}
+
+	return count;
+}
+
+static const struct file_operations rproc_recovery_ops = {
+	.read = rproc_recovery_read,
+	.write = rproc_recovery_write,
+	.open = simple_open,
+	.llseek = generic_file_llseek,
+};
+
 void rproc_remove_trace_file(struct dentry *tfile)
 {
 	debugfs_remove(tfile);
@@ -154,6 +235,8 @@ void rproc_create_debug_dir(struct rproc *rproc)
 					rproc, &rproc_name_ops);
 	debugfs_create_file("state", 0400, rproc->dbg_dir,
 					rproc, &rproc_state_ops);
+	debugfs_create_file("recovery", 0400, rproc->dbg_dir,
+					rproc, &rproc_recovery_ops);
 }
 
 void __init rproc_init_debugfs(void)
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 0c1a2f9..2ccc3fe 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -399,6 +399,7 @@ enum rproc_crash_type {
  * @crash_handler: workqueue for handling a crash
  * @crash_cnt: crash counter
  * @crash_comp: completion used to sync crash handler and the rproc reload
+ * @recovery_disabled: flag that state if recovery was disabled
  */
 struct rproc {
 	struct klist_node node;
@@ -425,6 +426,7 @@ struct rproc {
 	struct work_struct crash_handler;
 	unsigned crash_cnt;
 	struct completion crash_comp;
+	bool recovery_disabled;
 };
 
 /* we currently support only two vrings per rvdev */
-- 
1.7.1

Re: [PATCHv2 0/3] remoteproc: introduce rproc recovery

[Ohad Ben-Cohen]

Hi Fernando,

On Thu, Aug 30, 2012 at 9:26 PM, Fernando Guzman Lugo
<fernando.lugo@ti.com> wrote:
> These set of patches make possible the remoteproc recover after a crash.
> This is a hard recovery, that means the remoteproc is reset and it will
> start from the beginning. When a crash happen all the virtio devices are
> destroyed. Therefore, both rpmsg drivers and devices are gracefully
> removed which also cause rproc users become 0 and the remoteproc is turned
> off. After the virtio devices are destroyed the crash handler function
> will read the virtio information from the firmware in order to recreate
> the virtio devices that will boot the remoteproc and everything will be
> functional again.

Applied to remoteproc's for-next branch with a few changes (so we
don't miss the merge window).

The changes are detailed in the commit logs of the patches, please let
me know if there's any issue.

Thanks!
Ohad.

Re: [PATCHv2 0/3] remoteproc: introduce rproc recovery

[Fernando Lugo]

Hi Ohad,

On Tue, Sep 18, 2012 at 4:59 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> Hi Fernando,
>
> On Thu, Aug 30, 2012 at 9:26 PM, Fernando Guzman Lugo
> <fernando.lugo@ti.com> wrote:
>> These set of patches make possible the remoteproc recover after a crash.
>> This is a hard recovery, that means the remoteproc is reset and it will
>> start from the beginning. When a crash happen all the virtio devices are
>> destroyed. Therefore, both rpmsg drivers and devices are gracefully
>> removed which also cause rproc users become 0 and the remoteproc is turned
>> off. After the virtio devices are destroyed the crash handler function
>> will read the virtio information from the firmware in order to recreate
>> the virtio devices that will boot the remoteproc and everything will be
>> functional again.
>
> Applied to remoteproc's for-next branch with a few changes (so we
> don't miss the merge window).
>
> The changes are detailed in the commit logs of the patches, please let
> me know if there's any issue.

That sounds good. Thanks.

Regards,
Fernando.

>
> Thanks!
> Ohad.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html