[PATCH v2 00/14] remoteproc: Add support for detaching from rproc

[PATCH v2 00/14] remoteproc: Add support for detaching from rproc

[Mathieu Poirier]

Following the work done here [1], this set provides support for the
remoteproc core to release resources associated with a remote processor
without having to switch it off. That way a platform driver can be removed
or the applcation processor power cycled while the remote processor is
still operating.

The only thing that changes in this revision are the last two patches where
device tree bindings to control how to handle attached remote processors have
been added.  More specifically two bindings are being proposed:

"autonomous_on_core_reboot": When rproc_cdev_release() or rproc_del() are called
and the remote processor has been attached to, it will be detached from (rather
than turned off) if "autonomous_on_core_reboot" is specified in the DT.

"autonomous_on_remote_crash": When a remote processor that has been attached to
crashes, it will be detached from if "autonomous_on_remote_crash" is specified
in the DT. It is _not_ used in this set and presented to show how I intend to 
organise things. 

I spent a fair amount of time coming up with the name for the bindings and would
welcome other ideas.  I will write a proper yaml file and CC the linux-kernel
mailing list once we have an agreement on the naming convention.

Applies cleanly on v5.10-rc1

Thanks,
Mathieu

[1]. https://lkml.org/lkml/2020/7/14/1600

Mathieu Poirier (14):
  remoteproc: Re-check state in rproc_shutdown()
  remoteproc: Remove useless check in rproc_del()
  remoteproc: Add new RPROC_ATTACHED state
  remoteproc: Properly represent the attached state
  remoteproc: Properly deal with a kernel panic when attached
  remoteproc: Add new detach() remoteproc operation
  remoteproc: Introduce function __rproc_detach()
  remoteproc: Introduce function rproc_detach()
  remoteproc: Rename function rproc_actuate()
  remoteproc: Add return value to function rproc_shutdown()
  remoteproc: Properly deal with a stop request when attached
  remoteproc: Properly deal with detach request
  remoteproc: Add automation flags
  remoteproc: Refactor rproc delete and cdev release path

 drivers/remoteproc/remoteproc_cdev.c  |  24 +++-
 drivers/remoteproc/remoteproc_core.c  | 183 +++++++++++++++++++++-----
 drivers/remoteproc/remoteproc_sysfs.c |  17 ++-
 include/linux/remoteproc.h            |  19 ++-
 4 files changed, 199 insertions(+), 44 deletions(-)

-- 
2.25.1

[PATCH v2 01/14] remoteproc: Re-check state in rproc_shutdown()

[Mathieu Poirier]

The state of the remote processor may have changed between the
time a call to rproc_shutdown() was made and the time it is
executed.  To avoid moving forward with an operation that may
have been cancelled, recheck while holding the mutex.

Cc: <stable@vger.kernel.org>
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Reviewed-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/remoteproc_core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index dab2c0f5caf0..e55568d1e7e2 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1857,6 +1857,9 @@ void rproc_shutdown(struct rproc *rproc)
 		return;
 	}
 
+	if (rproc->state != RPROC_RUNNING)
+		goto out;
+
 	/* if the remote proc is still needed, bail out */
 	if (!atomic_dec_and_test(&rproc->power))
 		goto out;
-- 
2.25.1

[PATCH v2 02/14] remoteproc: Remove useless check in rproc_del()

[Mathieu Poirier]

Whether started at probe() time or thereafter from the command
line, a remote processor needs to be shutdown before the final
cleanup phases can happen.  Otherwise the system may be left in
an unpredictable state where the remote processor is expecting
the remoteproc core to be providing services when in fact it
no longer exist.

Invariably calling rproc_shutdown() is fine since it will return
immediately if the remote processor has already been switched
off.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Reviewed-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/remoteproc_core.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index e55568d1e7e2..f36786b47a4f 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -2283,10 +2283,8 @@ int rproc_del(struct rproc *rproc)
 	if (!rproc)
 		return -EINVAL;
 
-	/* if rproc is marked always-on, rproc_add() booted it */
 	/* TODO: make sure this works with rproc->power > 1 */
-	if (rproc->auto_boot)
-		rproc_shutdown(rproc);
+	rproc_shutdown(rproc);
 
 	mutex_lock(&rproc->lock);
 	rproc->state = RPROC_DELETED;
-- 
2.25.1

[PATCH v2 03/14] remoteproc: Add new RPROC_ATTACHED state

[Mathieu Poirier]

Add a new RPROC_ATTACHED state to take into account scenarios
where the remoteproc core needs to attach to a remote processor
that is booted by another entity.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Reviewed-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/remoteproc_sysfs.c | 1 +
 include/linux/remoteproc.h            | 7 +++++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
index d1cf7bf277c4..1167adcf8741 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -201,6 +201,7 @@ static const char * const rproc_state_string[] = {
 	[RPROC_RUNNING]		= "running",
 	[RPROC_CRASHED]		= "crashed",
 	[RPROC_DELETED]		= "deleted",
+	[RPROC_ATTACHED]	= "attached",
 	[RPROC_DETACHED]	= "detached",
 	[RPROC_LAST]		= "invalid",
 };
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 3fa3ba6498e8..4564c4665a2c 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -403,6 +403,8 @@ struct rproc_ops {
  * @RPROC_RUNNING:	device is up and running
  * @RPROC_CRASHED:	device has crashed; need to start recovery
  * @RPROC_DELETED:	device is deleted
+ * @RPROC_ATTACHED:	device has been booted by another entity and the core
+ *			has attached to it
  * @RPROC_DETACHED:	device has been booted by another entity and waiting
  *			for the core to attach to it
  * @RPROC_LAST:		just keep this one at the end
@@ -419,8 +421,9 @@ enum rproc_state {
 	RPROC_RUNNING	= 2,
 	RPROC_CRASHED	= 3,
 	RPROC_DELETED	= 4,
-	RPROC_DETACHED	= 5,
-	RPROC_LAST	= 6,
+	RPROC_ATTACHED	= 5,
+	RPROC_DETACHED	= 6,
+	RPROC_LAST	= 7,
 };
 
 /**
-- 
2.25.1

[PATCH v2 04/14] remoteproc: Properly represent the attached state

[Mathieu Poirier]

There is a need to know when a remote processor has been attached
to rather than booted by the remoteproc core.  In order to avoid
manipulating two variables, i.e rproc::autonomous and
rproc::state, get rid of the former and simply use the newly
introduced RPROC_ATTACHED state.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Reviewed-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/remoteproc_core.c  | 20 +-------------------
 drivers/remoteproc/remoteproc_sysfs.c |  5 +----
 include/linux/remoteproc.h            |  2 --
 3 files changed, 2 insertions(+), 25 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index f36786b47a4f..63fba1b61840 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1444,7 +1444,7 @@ static int rproc_attach(struct rproc *rproc)
 		goto stop_rproc;
 	}
 
-	rproc->state = RPROC_RUNNING;
+	rproc->state = RPROC_ATTACHED;
 
 	dev_info(dev, "remote processor %s is now attached\n", rproc->name);
 
@@ -1659,14 +1659,6 @@ static int rproc_stop(struct rproc *rproc, bool crashed)
 
 	rproc->state = RPROC_OFFLINE;
 
-	/*
-	 * The remote processor has been stopped and is now offline, which means
-	 * that the next time it is brought back online the remoteproc core will
-	 * be responsible to load its firmware.  As such it is no longer
-	 * autonomous.
-	 */
-	rproc->autonomous = false;
-
 	dev_info(dev, "stopped remote processor %s\n", rproc->name);
 
 	return 0;
@@ -2017,16 +2009,6 @@ int rproc_add(struct rproc *rproc)
 	if (ret < 0)
 		return ret;
 
-	/*
-	 * Remind ourselves the remote processor has been attached to rather
-	 * than booted by the remoteproc core.  This is important because the
-	 * RPROC_DETACHED state will be lost as soon as the remote processor
-	 * has been attached to.  Used in firmware_show() and reset in
-	 * rproc_stop().
-	 */
-	if (rproc->state == RPROC_DETACHED)
-		rproc->autonomous = true;
-
 	/* if rproc is marked always-on, request it to boot */
 	if (rproc->auto_boot) {
 		ret = rproc_trigger_auto_boot(rproc);
diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
index 1167adcf8741..99ff51fd9707 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -138,11 +138,8 @@ static ssize_t firmware_show(struct device *dev, struct device_attribute *attr,
 	 * If the remote processor has been started by an external
 	 * entity we have no idea of what image it is running.  As such
 	 * simply display a generic string rather then rproc->firmware.
-	 *
-	 * Here we rely on the autonomous flag because a remote processor
-	 * may have been attached to and currently in a running state.
 	 */
-	if (rproc->autonomous)
+	if (rproc->state == RPROC_ATTACHED)
 		firmware = "unknown";
 
 	return sprintf(buf, "%s\n", firmware);
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 4564c4665a2c..3fe2ae0bd1ca 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -510,7 +510,6 @@ struct rproc_dump_segment {
  * @table_sz: size of @cached_table
  * @has_iommu: flag to indicate if remote processor is behind an MMU
  * @auto_boot: flag to indicate if remote processor should be auto-started
- * @autonomous: true if an external entity has booted the remote processor
  * @dump_segments: list of segments in the firmware
  * @nb_vdev: number of vdev currently handled by rproc
  * @char_dev: character device of the rproc
@@ -547,7 +546,6 @@ struct rproc {
 	size_t table_sz;
 	bool has_iommu;
 	bool auto_boot;
-	bool autonomous;
 	struct list_head dump_segments;
 	int nb_vdev;
 	u8 elf_class;
-- 
2.25.1

[PATCH v2 05/14] remoteproc: Properly deal with a kernel panic when attached

[Mathieu Poirier]

The panic handler operation of registered remote processors
should also be called when remote processors have been
attached to.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Reviewed-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/remoteproc_core.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 63fba1b61840..ed1f9ca4248b 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -2408,7 +2408,11 @@ static int rproc_panic_handler(struct notifier_block *nb, unsigned long event,
 
 	rcu_read_lock();
 	list_for_each_entry_rcu(rproc, &rproc_list, node) {
-		if (!rproc->ops->panic || rproc->state != RPROC_RUNNING)
+		if (!rproc->ops->panic)
+			continue;
+
+		if (rproc->state != RPROC_RUNNING &&
+		    rproc->state != RPROC_ATTACHED)
 			continue;
 
 		d = rproc->ops->panic(rproc);
-- 
2.25.1

[PATCH v2 06/14] remoteproc: Add new detach() remoteproc operation

[Mathieu Poirier]

Add an new detach() operation in order to support scenarios where
the remoteproc core is going away but the remote processor is
kept operating.  This could be the case when the system is
rebooted or when the platform driver is removed.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Reviewed-by: Peng Fan <peng.fan@nxp.com>
---
 include/linux/remoteproc.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 3fe2ae0bd1ca..3faff9bb4fb8 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -361,6 +361,7 @@ enum rsc_handling_status {
  * @start:	power on the device and boot it
  * @stop:	power off the device
  * @attach:	attach to a device that his already powered up
+ * @detach:	tell the remote processor that the core is going away
  * @kick:	kick a virtqueue (virtqueue id given as a parameter)
  * @da_to_va:	optional platform hook to perform address translations
  * @parse_fw:	parse firmware to extract information (e.g. resource table)
@@ -382,6 +383,7 @@ struct rproc_ops {
 	int (*start)(struct rproc *rproc);
 	int (*stop)(struct rproc *rproc);
 	int (*attach)(struct rproc *rproc);
+	int (*detach)(struct rproc *rproc);
 	void (*kick)(struct rproc *rproc, int vqid);
 	void * (*da_to_va)(struct rproc *rproc, u64 da, size_t len);
 	int (*parse_fw)(struct rproc *rproc, const struct firmware *fw);
-- 
2.25.1

[PATCH v2 07/14] remoteproc: Introduce function __rproc_detach()

[Mathieu Poirier]

Introduce function __rproc_detach() to perform the same kind of
operation as rproc_stop(), but instead of switching off the
remote processor using rproc->ops->stop(), it uses
rproc->ops->detach().  That way it is possible for the core
to release the resources associated with a remote processor while
the latter is kept operating.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Reviewed-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/remoteproc_core.c | 31 ++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index ed1f9ca4248b..62e88ff65009 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1664,6 +1664,37 @@ static int rproc_stop(struct rproc *rproc, bool crashed)
 	return 0;
 }
 
+/*
+ * __rproc_detach(): Does the opposite of rproc_attach()
+ */
+static int __maybe_unused __rproc_detach(struct rproc *rproc)
+{
+	struct device *dev = &rproc->dev;
+	int ret;
+
+	/* No need to continue if a detach() operation has not been provided */
+	if (!rproc->ops->detach)
+		return -EINVAL;
+
+	/* Stop any subdevices for the remote processor */
+	rproc_stop_subdevices(rproc, false);
+
+	/* Tell the remote processor the core isn't available anymore */
+	ret = rproc->ops->detach(rproc);
+	if (ret) {
+		dev_err(dev, "can't detach from rproc: %d\n", ret);
+		rproc_start_subdevices(rproc);
+		return ret;
+	}
+
+	rproc_unprepare_subdevices(rproc);
+
+	rproc->state = RPROC_DETACHED;
+
+	dev_info(dev, "detached remote processor %s\n", rproc->name);
+
+	return 0;
+}
 
 /**
  * rproc_trigger_recovery() - recover a remoteproc
-- 
2.25.1

[PATCH v2 08/14] remoteproc: Introduce function rproc_detach()

[Mathieu Poirier]

Introduce function rproc_detach() to enable the remoteproc
core to release the resources associated with a remote processor
without stopping its operation.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Reviewed-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/remoteproc_core.c | 65 +++++++++++++++++++++++++++-
 include/linux/remoteproc.h           |  1 +
 2 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 62e88ff65009..6b33a83960d2 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1667,7 +1667,7 @@ static int rproc_stop(struct rproc *rproc, bool crashed)
 /*
  * __rproc_detach(): Does the opposite of rproc_attach()
  */
-static int __maybe_unused __rproc_detach(struct rproc *rproc)
+static int __rproc_detach(struct rproc *rproc)
 {
 	struct device *dev = &rproc->dev;
 	int ret;
@@ -1910,6 +1910,69 @@ void rproc_shutdown(struct rproc *rproc)
 }
 EXPORT_SYMBOL(rproc_shutdown);
 
+/**
+ * rproc_detach() - Detach the remote processor from the
+ * remoteproc core
+ *
+ * @rproc: the remote processor
+ *
+ * Detach a remote processor (previously attached to with rproc_actuate()).
+ *
+ * In case @rproc is still being used by an additional user(s), then
+ * this function will just decrement the power refcount and exit,
+ * without disconnecting the device.
+ *
+ * Function rproc_detach() calls __rproc_detach() in order to let a remote
+ * processor know that services provided by the application processor are
+ * no longer available.  From there it should be possible to remove the
+ * platform driver and even power cycle the application processor (if the HW
+ * supports it) without needing to switch off the remote processor.
+ */
+int rproc_detach(struct rproc *rproc)
+{
+	struct device *dev = &rproc->dev;
+	int ret;
+
+	ret = mutex_lock_interruptible(&rproc->lock);
+	if (ret) {
+		dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, ret);
+		return ret;
+	}
+
+	if (rproc->state != RPROC_RUNNING && rproc->state != RPROC_ATTACHED) {
+		ret = -EPERM;
+		goto out;
+	}
+
+	/* if the remote proc is still needed, bail out */
+	if (!atomic_dec_and_test(&rproc->power)) {
+		ret = -EBUSY;
+		goto out;
+	}
+
+	ret = __rproc_detach(rproc);
+	if (ret) {
+		atomic_inc(&rproc->power);
+		goto out;
+	}
+
+	/* clean up all acquired resources */
+	rproc_resource_cleanup(rproc);
+
+	rproc_disable_iommu(rproc);
+
+	/*
+	 * Set the remote processor's table pointer to NULL.  Since mapping
+	 * of the resource table to a virtual address is done in the platform
+	 * driver, unmapping should also be done there.
+	 */
+	rproc->table_ptr = NULL;
+out:
+	mutex_unlock(&rproc->lock);
+	return ret;
+}
+EXPORT_SYMBOL(rproc_detach);
+
 /**
  * rproc_get_by_phandle() - find a remote processor by phandle
  * @phandle: phandle to the rproc
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 3faff9bb4fb8..6713faab6959 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -656,6 +656,7 @@ rproc_of_resm_mem_entry_init(struct device *dev, u32 of_resm_idx, size_t len,
 
 int rproc_boot(struct rproc *rproc);
 void rproc_shutdown(struct rproc *rproc);
+int rproc_detach(struct rproc *rproc);
 void rproc_report_crash(struct rproc *rproc, enum rproc_crash_type type);
 int rproc_coredump_add_segment(struct rproc *rproc, dma_addr_t da, size_t size);
 int rproc_coredump_add_custom_segment(struct rproc *rproc,
-- 
2.25.1

[PATCH v2 09/14] remoteproc: Rename function rproc_actuate()

[Mathieu Poirier]

Align what was done for rproc_detach() by renaming function
rproc_actuate().  That way it is easier to figure out the
opposite of each functions.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Reviewed-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/remoteproc_core.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 6b33a83960d2..de5a5720d1e8 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1416,7 +1416,7 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
 	return ret;
 }
 
-static int rproc_attach(struct rproc *rproc)
+static int __rproc_attach(struct rproc *rproc)
 {
 	struct device *dev = &rproc->dev;
 	int ret;
@@ -1541,7 +1541,7 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
  * Attach to remote processor - similar to rproc_fw_boot() but without
  * the steps that deal with the firmware image.
  */
-static int rproc_actuate(struct rproc *rproc)
+static int rproc_attach(struct rproc *rproc)
 {
 	struct device *dev = &rproc->dev;
 	int ret;
@@ -1581,7 +1581,7 @@ static int rproc_actuate(struct rproc *rproc)
 		goto clean_up_resources;
 	}
 
-	ret = rproc_attach(rproc);
+	ret = __rproc_attach(rproc);
 	if (ret)
 		goto clean_up_resources;
 
@@ -1825,7 +1825,7 @@ int rproc_boot(struct rproc *rproc)
 	if (rproc->state == RPROC_DETACHED) {
 		dev_info(dev, "attaching to %s\n", rproc->name);
 
-		ret = rproc_actuate(rproc);
+		ret = rproc_attach(rproc);
 	} else {
 		dev_info(dev, "powering up %s\n", rproc->name);
 
@@ -1916,7 +1916,7 @@ EXPORT_SYMBOL(rproc_shutdown);
  *
  * @rproc: the remote processor
  *
- * Detach a remote processor (previously attached to with rproc_actuate()).
+ * Detach a remote processor (previously attached to with rproc_attach()).
  *
  * In case @rproc is still being used by an additional user(s), then
  * this function will just decrement the power refcount and exit,
-- 
2.25.1

[PATCH v2 10/14] remoteproc: Add return value to function rproc_shutdown()

[Mathieu Poirier]

Add a return value to function rproc_shutdown() in order to
properly deal with error conditions that may occur.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Reviewed-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/remoteproc_core.c | 13 +++++++++----
 include/linux/remoteproc.h           |  2 +-
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index de5a5720d1e8..f58475f6dcab 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1869,7 +1869,7 @@ EXPORT_SYMBOL(rproc_boot);
  *   returns, and users can still use it with a subsequent rproc_boot(), if
  *   needed.
  */
-void rproc_shutdown(struct rproc *rproc)
+int rproc_shutdown(struct rproc *rproc)
 {
 	struct device *dev = &rproc->dev;
 	int ret;
@@ -1877,15 +1877,19 @@ void rproc_shutdown(struct rproc *rproc)
 	ret = mutex_lock_interruptible(&rproc->lock);
 	if (ret) {
 		dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, ret);
-		return;
+		return ret;
 	}
 
-	if (rproc->state != RPROC_RUNNING)
+	if (rproc->state != RPROC_RUNNING) {
+		ret = -EPERM;
 		goto out;
+	}
 
 	/* if the remote proc is still needed, bail out */
-	if (!atomic_dec_and_test(&rproc->power))
+	if (!atomic_dec_and_test(&rproc->power)) {
+		ret = -EBUSY;
 		goto out;
+	}
 
 	ret = rproc_stop(rproc, false);
 	if (ret) {
@@ -1907,6 +1911,7 @@ void rproc_shutdown(struct rproc *rproc)
 	rproc->table_ptr = NULL;
 out:
 	mutex_unlock(&rproc->lock);
+	return ret;
 }
 EXPORT_SYMBOL(rproc_shutdown);
 
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 6713faab6959..71d4d4873164 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -655,7 +655,7 @@ rproc_of_resm_mem_entry_init(struct device *dev, u32 of_resm_idx, size_t len,
 			     u32 da, const char *name, ...);
 
 int rproc_boot(struct rproc *rproc);
-void rproc_shutdown(struct rproc *rproc);
+int rproc_shutdown(struct rproc *rproc);
 int rproc_detach(struct rproc *rproc);
 void rproc_report_crash(struct rproc *rproc, enum rproc_crash_type type);
 int rproc_coredump_add_segment(struct rproc *rproc, dma_addr_t da, size_t size);
-- 
2.25.1

[PATCH v2 11/14] remoteproc: Properly deal with a stop request when attached

[Mathieu Poirier]

This patch introduces the capability to stop a remote processor
that has been attached to by the remoteproc core.  For that to
happen a rproc::ops::stop() operation need to be available.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Reviewed-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/remoteproc_cdev.c  | 5 +++--
 drivers/remoteproc/remoteproc_core.c  | 6 +++++-
 drivers/remoteproc/remoteproc_sysfs.c | 5 +++--
 3 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
index b19ea3057bde..d06f8d4919c7 100644
--- a/drivers/remoteproc/remoteproc_cdev.c
+++ b/drivers/remoteproc/remoteproc_cdev.c
@@ -37,10 +37,11 @@ static ssize_t rproc_cdev_write(struct file *filp, const char __user *buf, size_
 
 		ret = rproc_boot(rproc);
 	} else if (!strncmp(cmd, "stop", len)) {
-		if (rproc->state != RPROC_RUNNING)
+		if (rproc->state != RPROC_RUNNING &&
+		    rproc->state != RPROC_ATTACHED)
 			return -EINVAL;
 
-		rproc_shutdown(rproc);
+		ret = rproc_shutdown(rproc);
 	} else {
 		dev_err(&rproc->dev, "Unrecognized option\n");
 		ret = -EINVAL;
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index f58475f6dcab..229fa2cad0bd 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1642,6 +1642,10 @@ static int rproc_stop(struct rproc *rproc, bool crashed)
 	struct device *dev = &rproc->dev;
 	int ret;
 
+	/* No need to continue if a stop() operation has not been provided */
+	if (!rproc->ops->stop)
+		return -EINVAL;
+
 	/* Stop any subdevices for the remote processor */
 	rproc_stop_subdevices(rproc, crashed);
 
@@ -1880,7 +1884,7 @@ int rproc_shutdown(struct rproc *rproc)
 		return ret;
 	}
 
-	if (rproc->state != RPROC_RUNNING) {
+	if (rproc->state != RPROC_RUNNING && rproc->state != RPROC_ATTACHED) {
 		ret = -EPERM;
 		goto out;
 	}
diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
index 99ff51fd9707..96751c087585 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -230,10 +230,11 @@ static ssize_t state_store(struct device *dev,
 		if (ret)
 			dev_err(&rproc->dev, "Boot failed: %d\n", ret);
 	} else if (sysfs_streq(buf, "stop")) {
-		if (rproc->state != RPROC_RUNNING)
+		if (rproc->state != RPROC_RUNNING &&
+		    rproc->state != RPROC_ATTACHED)
 			return -EINVAL;
 
-		rproc_shutdown(rproc);
+		ret = rproc_shutdown(rproc);
 	} else {
 		dev_err(&rproc->dev, "Unrecognised option: %s\n", buf);
 		ret = -EINVAL;
-- 
2.25.1

[PATCH v2 12/14] remoteproc: Properly deal with detach request

[Mathieu Poirier]

This patch introduces the capability to detach a remote processor
that has been attached to or booted by the remoteproc core.  For
that to happen a rproc::ops::detach() operation need to be
available.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Reviewed-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/remoteproc_cdev.c  | 6 ++++++
 drivers/remoteproc/remoteproc_sysfs.c | 6 ++++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
index d06f8d4919c7..3a3830e27050 100644
--- a/drivers/remoteproc/remoteproc_cdev.c
+++ b/drivers/remoteproc/remoteproc_cdev.c
@@ -42,6 +42,12 @@ static ssize_t rproc_cdev_write(struct file *filp, const char __user *buf, size_
 			return -EINVAL;
 
 		ret = rproc_shutdown(rproc);
+	} else if (!strncmp(cmd, "detach", len)) {
+		if (rproc->state != RPROC_RUNNING &&
+		    rproc->state != RPROC_ATTACHED)
+			return -EINVAL;
+
+		ret = rproc_detach(rproc);
 	} else {
 		dev_err(&rproc->dev, "Unrecognized option\n");
 		ret = -EINVAL;
diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
index 96751c087585..b05fad2178bb 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -235,6 +235,12 @@ static ssize_t state_store(struct device *dev,
 			return -EINVAL;
 
 		ret = rproc_shutdown(rproc);
+	} else if (sysfs_streq(buf, "detach")) {
+		if (rproc->state != RPROC_RUNNING &&
+		    rproc->state != RPROC_ATTACHED)
+			return -EINVAL;
+
+		ret = rproc_detach(rproc);
 	} else {
 		dev_err(&rproc->dev, "Unrecognised option: %s\n", buf);
 		ret = -EINVAL;
-- 
2.25.1

[RFC v2 13/14] remoteproc: Add automation flags

[Mathieu Poirier]

Adding flags to dictate how to handle a platform driver being removed
or the remote processor crashing while in RPROC_ATTACHED state.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/remoteproc/remoteproc_core.c | 25 +++++++++++++++++++++++++
 include/linux/remoteproc.h           |  5 +++++
 2 files changed, 30 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 229fa2cad0bd..d024367c63e5 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -2227,6 +2227,29 @@ static int rproc_alloc_ops(struct rproc *rproc, const struct rproc_ops *ops)
 	return 0;
 }
 
+static void rproc_set_automation_flags(struct rproc *rproc)
+{
+	struct device *dev = rproc->dev.parent;
+	struct device_node *np = dev->of_node;
+	bool core_reboot, remote_crash;
+
+	/*
+	 * When function rproc_cdev_release() or rproc_del() are called and
+	 * the remote processor has been attached to, it will be detached from
+	 * (rather than turned off) if "autonomous_on_core_reboot" is specified
+	 * in the DT.
+	 */
+	core_reboot = of_property_read_bool(np, "autonomous_on_core_reboot");
+	rproc->autonomous_on_core_reboot = core_reboot;
+
+	/*
+	 * When the remote processor crashes it will be detached from, and
+	 * attached to, if "autonomous_on_remote_crash" is specified in the DT.
+	 */
+	remote_crash = of_property_read_bool(np, "autonomous_on_remote_crash");
+	rproc->autonomous_on_core_reboot = core_reboot;
+}
+
 /**
  * rproc_alloc() - allocate a remote processor handle
  * @dev: the underlying device
@@ -2285,6 +2308,8 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
 	if (rproc_alloc_ops(rproc, ops))
 		goto put_device;
 
+	rproc_set_automation_flags(rproc);
+
 	/* Assign a unique device index and name */
 	rproc->index = ida_simple_get(&rproc_dev_index, 0, 0, GFP_KERNEL);
 	if (rproc->index < 0) {
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 71d4d4873164..9a6e79ef35d7 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -516,6 +516,9 @@ struct rproc_dump_segment {
  * @nb_vdev: number of vdev currently handled by rproc
  * @char_dev: character device of the rproc
  * @cdev_put_on_release: flag to indicate if remoteproc should be shutdown on @char_dev release
+ * @autonomous_on_core_reboot: true if the remote processor should be detached from
+ *			       (rather than turned off) when the remoteproc core
+ *			       goes away.
  */
 struct rproc {
 	struct list_head node;
@@ -554,6 +557,8 @@ struct rproc {
 	u16 elf_machine;
 	struct cdev cdev;
 	bool cdev_put_on_release;
+	bool autonomous_on_core_reboot	: 1,
+	     autonomous_on_remote_crash	: 1;
 };
 
 /**
-- 
2.25.1

[RFC v2 14/14] remoteproc: Refactor rproc delete and cdev release path

[Mathieu Poirier]

Refactor function rproc_del() and rproc_cdev_release() to take
into account the policy specified in the device tree.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/remoteproc/remoteproc_cdev.c | 13 ++++++++++++-
 drivers/remoteproc/remoteproc_core.c | 12 ++++++++++--
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
index 3a3830e27050..2e3f85920e5b 100644
--- a/drivers/remoteproc/remoteproc_cdev.c
+++ b/drivers/remoteproc/remoteproc_cdev.c
@@ -87,7 +87,18 @@ static int rproc_cdev_release(struct inode *inode, struct file *filp)
 {
 	struct rproc *rproc = container_of(inode->i_cdev, struct rproc, cdev);
 
-	if (rproc->cdev_put_on_release && rproc->state == RPROC_RUNNING)
+	if (!rproc->cdev_put_on_release)
+		return 0;
+
+	/*
+	 * The application has crashed or is releasing its file handle.  Detach
+	 * or shutdown the remote processor based on the policy specified in the
+	 * DT.  No need to check rproc->state right away, it will be done
+	 * in either rproc_detach() or rproc_shutdown().
+	 */
+	if (rproc->autonomous_on_core_reboot)
+		rproc_detach(rproc);
+	else
 		rproc_shutdown(rproc);
 
 	return 0;
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index d024367c63e5..9652e543c1ed 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -2393,8 +2393,16 @@ int rproc_del(struct rproc *rproc)
 	if (!rproc)
 		return -EINVAL;
 
-	/* TODO: make sure this works with rproc->power > 1 */
-	rproc_shutdown(rproc);
+	/*
+	 * TODO: make sure this works with rproc->power > 1
+	 *
+	 * No need to check rproc->state right away, it will be done in either
+	 * rproc_detach() or rproc_shutdown().
+	 */
+	if (rproc->autonomous_on_core_reboot)
+		rproc_detach(rproc);
+	else
+		rproc_shutdown(rproc);
 
 	mutex_lock(&rproc->lock);
 	rproc->state = RPROC_DELETED;
-- 
2.25.1

Re: [RFC v2 13/14] remoteproc: Add automation flags

[Arnaud POULIQUEN]

Hi Mathieu,

I applied your series on my branch to start the review and test it.
Compiler(W=1) complains and highlights an issue.

Please find comment below.

On 10/30/20 8:57 PM, Mathieu Poirier wrote:
> Adding flags to dictate how to handle a platform driver being removed
> or the remote processor crashing while in RPROC_ATTACHED state.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  drivers/remoteproc/remoteproc_core.c | 25 +++++++++++++++++++++++++
>  include/linux/remoteproc.h           |  5 +++++
>  2 files changed, 30 insertions(+)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 229fa2cad0bd..d024367c63e5 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -2227,6 +2227,29 @@ static int rproc_alloc_ops(struct rproc *rproc, const struct rproc_ops *ops)
>  	return 0;
>  }
>  
> +static void rproc_set_automation_flags(struct rproc *rproc)
> +{
> +	struct device *dev = rproc->dev.parent;
> +	struct device_node *np = dev->of_node;
> +	bool core_reboot, remote_crash;
> +
> +	/*
> +	 * When function rproc_cdev_release() or rproc_del() are called and
> +	 * the remote processor has been attached to, it will be detached from
> +	 * (rather than turned off) if "autonomous_on_core_reboot" is specified
> +	 * in the DT.
> +	 */
> +	core_reboot = of_property_read_bool(np, "autonomous_on_core_reboot");
> +	rproc->autonomous_on_core_reboot = core_reboot;
> +
> +	/*
> +	 * When the remote processor crashes it will be detached from, and
> +	 * attached to, if "autonomous_on_remote_crash" is specified in the DT.
> +	 */
> +	remote_crash = of_property_read_bool(np, "autonomous_on_remote_crash");
> +	rproc->autonomous_on_core_reboot = core_reboot;

copy/past issue :)
rproc->autonomous_on_remote_crash = remote_crash;

Regards,
Arnaud

> +}
> +
>  /**
>   * rproc_alloc() - allocate a remote processor handle
>   * @dev: the underlying device
> @@ -2285,6 +2308,8 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
>  	if (rproc_alloc_ops(rproc, ops))
>  		goto put_device;
>  
> +	rproc_set_automation_flags(rproc);
> +
>  	/* Assign a unique device index and name */
>  	rproc->index = ida_simple_get(&rproc_dev_index, 0, 0, GFP_KERNEL);
>  	if (rproc->index < 0) {
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 71d4d4873164..9a6e79ef35d7 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -516,6 +516,9 @@ struct rproc_dump_segment {
>   * @nb_vdev: number of vdev currently handled by rproc
>   * @char_dev: character device of the rproc
>   * @cdev_put_on_release: flag to indicate if remoteproc should be shutdown on @char_dev release
> + * @autonomous_on_core_reboot: true if the remote processor should be detached from
> + *			       (rather than turned off) when the remoteproc core
> + *			       goes away.
>   */
>  struct rproc {
>  	struct list_head node;
> @@ -554,6 +557,8 @@ struct rproc {
>  	u16 elf_machine;
>  	struct cdev cdev;
>  	bool cdev_put_on_release;
> +	bool autonomous_on_core_reboot	: 1,
> +	     autonomous_on_remote_crash	: 1;
>  };
>  
>  /**
> 

Re: [RFC v2 13/14] remoteproc: Add automation flags

[Mathieu Poirier]

On Fri, Nov 06, 2020 at 03:38:22PM +0100, Arnaud POULIQUEN wrote:
> Hi Mathieu,
> 
> I applied your series on my branch to start the review and test it.
> Compiler(W=1) complains and highlights an issue.
> 
> Please find comment below.
> 
> On 10/30/20 8:57 PM, Mathieu Poirier wrote:
> > Adding flags to dictate how to handle a platform driver being removed
> > or the remote processor crashing while in RPROC_ATTACHED state.
> > 
> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > ---
> >  drivers/remoteproc/remoteproc_core.c | 25 +++++++++++++++++++++++++
> >  include/linux/remoteproc.h           |  5 +++++
> >  2 files changed, 30 insertions(+)
> > 
> > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > index 229fa2cad0bd..d024367c63e5 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -2227,6 +2227,29 @@ static int rproc_alloc_ops(struct rproc *rproc, const struct rproc_ops *ops)
> >  	return 0;
> >  }
> >  
> > +static void rproc_set_automation_flags(struct rproc *rproc)
> > +{
> > +	struct device *dev = rproc->dev.parent;
> > +	struct device_node *np = dev->of_node;
> > +	bool core_reboot, remote_crash;
> > +
> > +	/*
> > +	 * When function rproc_cdev_release() or rproc_del() are called and
> > +	 * the remote processor has been attached to, it will be detached from
> > +	 * (rather than turned off) if "autonomous_on_core_reboot" is specified
> > +	 * in the DT.
> > +	 */
> > +	core_reboot = of_property_read_bool(np, "autonomous_on_core_reboot");
> > +	rproc->autonomous_on_core_reboot = core_reboot;
> > +
> > +	/*
> > +	 * When the remote processor crashes it will be detached from, and
> > +	 * attached to, if "autonomous_on_remote_crash" is specified in the DT.
> > +	 */
> > +	remote_crash = of_property_read_bool(np, "autonomous_on_remote_crash");
> > +	rproc->autonomous_on_core_reboot = core_reboot;
> 
> copy/past issue :)
> rproc->autonomous_on_remote_crash = remote_crash;

What a doozy... Thanks for pointing it out.

> 
> Regards,
> Arnaud
> 
> > +}
> > +
> >  /**
> >   * rproc_alloc() - allocate a remote processor handle
> >   * @dev: the underlying device
> > @@ -2285,6 +2308,8 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
> >  	if (rproc_alloc_ops(rproc, ops))
> >  		goto put_device;
> >  
> > +	rproc_set_automation_flags(rproc);
> > +
> >  	/* Assign a unique device index and name */
> >  	rproc->index = ida_simple_get(&rproc_dev_index, 0, 0, GFP_KERNEL);
> >  	if (rproc->index < 0) {
> > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > index 71d4d4873164..9a6e79ef35d7 100644
> > --- a/include/linux/remoteproc.h
> > +++ b/include/linux/remoteproc.h
> > @@ -516,6 +516,9 @@ struct rproc_dump_segment {
> >   * @nb_vdev: number of vdev currently handled by rproc
> >   * @char_dev: character device of the rproc
> >   * @cdev_put_on_release: flag to indicate if remoteproc should be shutdown on @char_dev release
> > + * @autonomous_on_core_reboot: true if the remote processor should be detached from
> > + *			       (rather than turned off) when the remoteproc core
> > + *			       goes away.
> >   */
> >  struct rproc {
> >  	struct list_head node;
> > @@ -554,6 +557,8 @@ struct rproc {
> >  	u16 elf_machine;
> >  	struct cdev cdev;
> >  	bool cdev_put_on_release;
> > +	bool autonomous_on_core_reboot	: 1,
> > +	     autonomous_on_remote_crash	: 1;
> >  };
> >  
> >  /**
> > 

Re: [PATCH v2 06/14] remoteproc: Add new detach() remoteproc operation

[Arnaud POULIQUEN]

On 10/30/20 8:57 PM, Mathieu Poirier wrote:
> Add an new detach() operation in order to support scenarios where
> the remoteproc core is going away but the remote processor is
> kept operating.  This could be the case when the system is
> rebooted or when the platform driver is removed.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> Reviewed-by: Peng Fan <peng.fan@nxp.com>
> ---
>  include/linux/remoteproc.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 3fe2ae0bd1ca..3faff9bb4fb8 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -361,6 +361,7 @@ enum rsc_handling_status {
>   * @start:	power on the device and boot it
>   * @stop:	power off the device
>   * @attach:	attach to a device that his already powered up
> + * @detach:	tell the remote processor that the core is going away

This comment seems to me rather ambiguous...
"tell the remote processor" could means communication with the remote processor.
The term "remote processor" is used for this op and "device" for the other ops.
Proposal:
 detach from a device by leaving it power-up.

Regards,
Arnaud
 
>   * @kick:	kick a virtqueue (virtqueue id given as a parameter)
>   * @da_to_va:	optional platform hook to perform address translations
>   * @parse_fw:	parse firmware to extract information (e.g. resource table)
> @@ -382,6 +383,7 @@ struct rproc_ops {
>  	int (*start)(struct rproc *rproc);
>  	int (*stop)(struct rproc *rproc);
>  	int (*attach)(struct rproc *rproc);
> +	int (*detach)(struct rproc *rproc);
>  	void (*kick)(struct rproc *rproc, int vqid);
>  	void * (*da_to_va)(struct rproc *rproc, u64 da, size_t len);
>  	int (*parse_fw)(struct rproc *rproc, const struct firmware *fw);
> 

Re: [PATCH v2 07/14] remoteproc: Introduce function __rproc_detach()

[Arnaud POULIQUEN]

On 10/30/20 8:57 PM, Mathieu Poirier wrote:
> Introduce function __rproc_detach() to perform the same kind of
> operation as rproc_stop(), but instead of switching off the
> remote processor using rproc->ops->stop(), it uses
> rproc->ops->detach().  That way it is possible for the core
> to release the resources associated with a remote processor while
> the latter is kept operating.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> Reviewed-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/remoteproc/remoteproc_core.c | 31 ++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index ed1f9ca4248b..62e88ff65009 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1664,6 +1664,37 @@ static int rproc_stop(struct rproc *rproc, bool crashed)
>  	return 0;
>  }
>  
> +/*
> + * __rproc_detach(): Does the opposite of rproc_attach()
> + */
> +static int __maybe_unused __rproc_detach(struct rproc *rproc)
> +{
> +	struct device *dev = &rproc->dev;
> +	int ret;
> +
> +	/* No need to continue if a detach() operation has not been provided */
> +	if (!rproc->ops->detach)
> +		return -EINVAL;
> +
> +	/* Stop any subdevices for the remote processor */
> +	rproc_stop_subdevices(rproc, false);

How to determine whether a subdevice should be stopped or detached? 
For instance, in ST, we have a resource manager subdev which maintains clocks and regulators
for peripherals used by the remote processor.
In case of detachment we would need to maintain clock and regulators.

> +
> +	/* Tell the remote processor the core isn't available anymore */
> +	ret = rproc->ops->detach(rproc);
> +	if (ret) {
> +		dev_err(dev, "can't detach from rproc: %d\n", ret);
> +		rproc_start_subdevices(rproc);
> +		return ret;
> +	}
> +
> +	rproc_unprepare_subdevices(rproc);

Same here, is prepare/unprepare can depend on the operation?

Seems that adding rproc_attach_subdevices/rproc_detach_subdevices could be not sufficient
to address prepare/unprepare.
Alternative could be:
- extra parameter for the subdev ops to indicate attach/detach action...?
- intermediate rproc state : ATTACHING, DETACHING
- other?

That's said, I don't think that it is blocking for the ST resource manager.
In this particular case, regulators and clocks can be permanently activated
as a back-up solution (always-on).

So, if no other company has a problem with that, we can keep this implementation for now.

Regards,
Arnaud

> +
> +	rproc->state = RPROC_DETACHED;
> +
> +	dev_info(dev, "detached remote processor %s\n", rproc->name);
> +
> +	return 0;
> +}
>  
>  /**
>   * rproc_trigger_recovery() - recover a remoteproc
> 

Re: [PATCH v2 08/14] remoteproc: Introduce function rproc_detach()

[Arnaud POULIQUEN]

Hi Mathieu,

On 10/30/20 8:57 PM, Mathieu Poirier wrote:
> Introduce function rproc_detach() to enable the remoteproc
> core to release the resources associated with a remote processor
> without stopping its operation.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> Reviewed-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/remoteproc/remoteproc_core.c | 65 +++++++++++++++++++++++++++-
>  include/linux/remoteproc.h           |  1 +
>  2 files changed, 65 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 62e88ff65009..6b33a83960d2 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1667,7 +1667,7 @@ static int rproc_stop(struct rproc *rproc, bool crashed)
>  /*
>   * __rproc_detach(): Does the opposite of rproc_attach()
>   */
> -static int __maybe_unused __rproc_detach(struct rproc *rproc)
> +static int __rproc_detach(struct rproc *rproc)
>  {
>  	struct device *dev = &rproc->dev;
>  	int ret;
> @@ -1910,6 +1910,69 @@ void rproc_shutdown(struct rproc *rproc)
>  }
>  EXPORT_SYMBOL(rproc_shutdown);
>  
> +/**
> + * rproc_detach() - Detach the remote processor from the
> + * remoteproc core
> + *
> + * @rproc: the remote processor
> + *
> + * Detach a remote processor (previously attached to with rproc_actuate()).
> + *
> + * In case @rproc is still being used by an additional user(s), then
> + * this function will just decrement the power refcount and exit,
> + * without disconnecting the device.
> + *
> + * Function rproc_detach() calls __rproc_detach() in order to let a remote
> + * processor know that services provided by the application processor are
> + * no longer available.  From there it should be possible to remove the
> + * platform driver and even power cycle the application processor (if the HW
> + * supports it) without needing to switch off the remote processor.
> + */
> +int rproc_detach(struct rproc *rproc)
> +{
> +	struct device *dev = &rproc->dev;
> +	int ret;
> +
> +	ret = mutex_lock_interruptible(&rproc->lock);
> +	if (ret) {
> +		dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, ret);
> +		return ret;
> +	}
> +
> +	if (rproc->state != RPROC_RUNNING && rproc->state != RPROC_ATTACHED) {
> +		ret = -EPERM;
> +		goto out;
> +	}
> +
> +	/* if the remote proc is still needed, bail out */
> +	if (!atomic_dec_and_test(&rproc->power)) {
> +		ret = -EBUSY;
> +		goto out;
> +	}
> +
> +	ret = __rproc_detach(rproc);
> +	if (ret) {
> +		atomic_inc(&rproc->power);
> +		goto out;
> +	}
> +
> +	/* clean up all acquired resources */
> +	rproc_resource_cleanup(rproc);
> +
> +	rproc_disable_iommu(rproc);

I'm not an IOMMU expert,so maybe this remark is not relevant...
As IOMMU manage the access of the remote device to its memory,
could this lead to a remote device crash?

Regards,
Arnaud

> +
> +	/*
> +	 * Set the remote processor's table pointer to NULL.  Since mapping
> +	 * of the resource table to a virtual address is done in the platform
> +	 * driver, unmapping should also be done there.
> +	 */
> +	rproc->table_ptr = NULL;
> +out:
> +	mutex_unlock(&rproc->lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL(rproc_detach);
> +
>  /**
>   * rproc_get_by_phandle() - find a remote processor by phandle
>   * @phandle: phandle to the rproc
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 3faff9bb4fb8..6713faab6959 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -656,6 +656,7 @@ rproc_of_resm_mem_entry_init(struct device *dev, u32 of_resm_idx, size_t len,
>  
>  int rproc_boot(struct rproc *rproc);
>  void rproc_shutdown(struct rproc *rproc);
> +int rproc_detach(struct rproc *rproc);
>  void rproc_report_crash(struct rproc *rproc, enum rproc_crash_type type);
>  int rproc_coredump_add_segment(struct rproc *rproc, dma_addr_t da, size_t size);
>  int rproc_coredump_add_custom_segment(struct rproc *rproc,
> 

Re: [PATCH v2 10/14] remoteproc: Add return value to function rproc_shutdown()

[Arnaud POULIQUEN]

On 10/30/20 8:57 PM, Mathieu Poirier wrote:
> Add a return value to function rproc_shutdown() in order to
> properly deal with error conditions that may occur.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> Reviewed-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/remoteproc/remoteproc_core.c | 13 +++++++++----
>  include/linux/remoteproc.h           |  2 +-
>  2 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index de5a5720d1e8..f58475f6dcab 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1869,7 +1869,7 @@ EXPORT_SYMBOL(rproc_boot);
>   *   returns, and users can still use it with a subsequent rproc_boot(), if
>   *   needed.
>   */
> -void rproc_shutdown(struct rproc *rproc)
> +int rproc_shutdown(struct rproc *rproc)
>  {
>  	struct device *dev = &rproc->dev;
>  	int ret;
> @@ -1877,15 +1877,19 @@ void rproc_shutdown(struct rproc *rproc)
>  	ret = mutex_lock_interruptible(&rproc->lock);
>  	if (ret) {
>  		dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, ret);
> -		return;
> +		return ret;
>  	}
>  
> -	if (rproc->state != RPROC_RUNNING)
> +	if (rproc->state != RPROC_RUNNING) {
> +		ret = -EPERM;
>  		goto out;
> +	}
>  
>  	/* if the remote proc is still needed, bail out */
> -	if (!atomic_dec_and_test(&rproc->power))
> +	if (!atomic_dec_and_test(&rproc->power)) {
> +		ret = -EBUSY;
>  		goto out;
> +	}
>  
>  	ret = rproc_stop(rproc, false);
>  	if (ret) {

Few lines after in source code rproc_unprepare_device could also be tested

Regards,
Arnaud

> @@ -1907,6 +1911,7 @@ void rproc_shutdown(struct rproc *rproc)
>  	rproc->table_ptr = NULL;
>  out:
>  	mutex_unlock(&rproc->lock);
> +	return ret;
>  }
>  EXPORT_SYMBOL(rproc_shutdown);
>  
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 6713faab6959..71d4d4873164 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -655,7 +655,7 @@ rproc_of_resm_mem_entry_init(struct device *dev, u32 of_resm_idx, size_t len,
>  			     u32 da, const char *name, ...);
>  
>  int rproc_boot(struct rproc *rproc);
> -void rproc_shutdown(struct rproc *rproc);
> +int rproc_shutdown(struct rproc *rproc);
>  int rproc_detach(struct rproc *rproc);
>  void rproc_report_crash(struct rproc *rproc, enum rproc_crash_type type);
>  int rproc_coredump_add_segment(struct rproc *rproc, dma_addr_t da, size_t size);
> 

Re: [PATCH v2 01/14] remoteproc: Re-check state in rproc_shutdown()

[Arnaud POULIQUEN]

On 10/30/20 8:57 PM, Mathieu Poirier wrote:
> The state of the remote processor may have changed between the
> time a call to rproc_shutdown() was made and the time it is
> executed.  To avoid moving forward with an operation that may
> have been cancelled, recheck while holding the mutex.
> 
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> Reviewed-by: Peng Fan <peng.fan@nxp.com>

Reviewed-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>

Thanks,
Arnaud

> ---
>  drivers/remoteproc/remoteproc_core.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index dab2c0f5caf0..e55568d1e7e2 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1857,6 +1857,9 @@ void rproc_shutdown(struct rproc *rproc)
>  		return;
>  	}
>  
> +	if (rproc->state != RPROC_RUNNING)
> +		goto out;
> +
>  	/* if the remote proc is still needed, bail out */
>  	if (!atomic_dec_and_test(&rproc->power))
>  		goto out;
> 

Re: [PATCH v2 02/14] remoteproc: Remove useless check in rproc_del()

[Arnaud POULIQUEN]

On 10/30/20 8:57 PM, Mathieu Poirier wrote:
> Whether started at probe() time or thereafter from the command
> line, a remote processor needs to be shutdown before the final
> cleanup phases can happen.  Otherwise the system may be left in
> an unpredictable state where the remote processor is expecting
> the remoteproc core to be providing services when in fact it
> no longer exist.
> 
> Invariably calling rproc_shutdown() is fine since it will return
> immediately if the remote processor has already been switched
> off.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> Reviewed-by: Peng Fan <peng.fan@nxp.com>


Reviewed-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>

Thanks,
Arnaud

> ---
>  drivers/remoteproc/remoteproc_core.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index e55568d1e7e2..f36786b47a4f 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -2283,10 +2283,8 @@ int rproc_del(struct rproc *rproc)
>  	if (!rproc)
>  		return -EINVAL;
>  
> -	/* if rproc is marked always-on, rproc_add() booted it */
>  	/* TODO: make sure this works with rproc->power > 1 */
> -	if (rproc->auto_boot)
> -		rproc_shutdown(rproc);
> +	rproc_shutdown(rproc);
>  
>  	mutex_lock(&rproc->lock);
>  	rproc->state = RPROC_DELETED;
> 

Re: [PATCH v2 03/14] remoteproc: Add new RPROC_ATTACHED state

[Arnaud POULIQUEN]

On 10/30/20 8:57 PM, Mathieu Poirier wrote:
> Add a new RPROC_ATTACHED state to take into account scenarios
> where the remoteproc core needs to attach to a remote processor
> that is booted by another entity.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> Reviewed-by: Peng Fan <peng.fan@nxp.com>


Reviewed-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>

Thanks,
Arnaud

> ---
>  drivers/remoteproc/remoteproc_sysfs.c | 1 +
>  include/linux/remoteproc.h            | 7 +++++--
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
> index d1cf7bf277c4..1167adcf8741 100644
> --- a/drivers/remoteproc/remoteproc_sysfs.c
> +++ b/drivers/remoteproc/remoteproc_sysfs.c
> @@ -201,6 +201,7 @@ static const char * const rproc_state_string[] = {
>  	[RPROC_RUNNING]		= "running",
>  	[RPROC_CRASHED]		= "crashed",
>  	[RPROC_DELETED]		= "deleted",
> +	[RPROC_ATTACHED]	= "attached",
>  	[RPROC_DETACHED]	= "detached",
>  	[RPROC_LAST]		= "invalid",
>  };
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 3fa3ba6498e8..4564c4665a2c 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -403,6 +403,8 @@ struct rproc_ops {
>   * @RPROC_RUNNING:	device is up and running
>   * @RPROC_CRASHED:	device has crashed; need to start recovery
>   * @RPROC_DELETED:	device is deleted
> + * @RPROC_ATTACHED:	device has been booted by another entity and the core
> + *			has attached to it
>   * @RPROC_DETACHED:	device has been booted by another entity and waiting
>   *			for the core to attach to it
>   * @RPROC_LAST:		just keep this one at the end
> @@ -419,8 +421,9 @@ enum rproc_state {
>  	RPROC_RUNNING	= 2,
>  	RPROC_CRASHED	= 3,
>  	RPROC_DELETED	= 4,
> -	RPROC_DETACHED	= 5,
> -	RPROC_LAST	= 6,
> +	RPROC_ATTACHED	= 5,
> +	RPROC_DETACHED	= 6,
> +	RPROC_LAST	= 7,
>  };
>  
>  /**
> 

Re: [PATCH v2 04/14] remoteproc: Properly represent the attached state

[Arnaud POULIQUEN]

On 10/30/20 8:57 PM, Mathieu Poirier wrote:
> There is a need to know when a remote processor has been attached
> to rather than booted by the remoteproc core.  In order to avoid
> manipulating two variables, i.e rproc::autonomous and
> rproc::state, get rid of the former and simply use the newly
> introduced RPROC_ATTACHED state.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> Reviewed-by: Peng Fan <peng.fan@nxp.com>

Reviewed-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>

Thanks,
Arnaud

> ---
>  drivers/remoteproc/remoteproc_core.c  | 20 +-------------------
>  drivers/remoteproc/remoteproc_sysfs.c |  5 +----
>  include/linux/remoteproc.h            |  2 --
>  3 files changed, 2 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index f36786b47a4f..63fba1b61840 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1444,7 +1444,7 @@ static int rproc_attach(struct rproc *rproc)
>  		goto stop_rproc;
>  	}
>  
> -	rproc->state = RPROC_RUNNING;
> +	rproc->state = RPROC_ATTACHED;
>  
>  	dev_info(dev, "remote processor %s is now attached\n", rproc->name);
>  
> @@ -1659,14 +1659,6 @@ static int rproc_stop(struct rproc *rproc, bool crashed)
>  
>  	rproc->state = RPROC_OFFLINE;
>  
> -	/*
> -	 * The remote processor has been stopped and is now offline, which means
> -	 * that the next time it is brought back online the remoteproc core will
> -	 * be responsible to load its firmware.  As such it is no longer
> -	 * autonomous.
> -	 */
> -	rproc->autonomous = false;
> -
>  	dev_info(dev, "stopped remote processor %s\n", rproc->name);
>  
>  	return 0;
> @@ -2017,16 +2009,6 @@ int rproc_add(struct rproc *rproc)
>  	if (ret < 0)
>  		return ret;
>  
> -	/*
> -	 * Remind ourselves the remote processor has been attached to rather
> -	 * than booted by the remoteproc core.  This is important because the
> -	 * RPROC_DETACHED state will be lost as soon as the remote processor
> -	 * has been attached to.  Used in firmware_show() and reset in
> -	 * rproc_stop().
> -	 */
> -	if (rproc->state == RPROC_DETACHED)
> -		rproc->autonomous = true;
> -
>  	/* if rproc is marked always-on, request it to boot */
>  	if (rproc->auto_boot) {
>  		ret = rproc_trigger_auto_boot(rproc);
> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
> index 1167adcf8741..99ff51fd9707 100644
> --- a/drivers/remoteproc/remoteproc_sysfs.c
> +++ b/drivers/remoteproc/remoteproc_sysfs.c
> @@ -138,11 +138,8 @@ static ssize_t firmware_show(struct device *dev, struct device_attribute *attr,
>  	 * If the remote processor has been started by an external
>  	 * entity we have no idea of what image it is running.  As such
>  	 * simply display a generic string rather then rproc->firmware.
> -	 *
> -	 * Here we rely on the autonomous flag because a remote processor
> -	 * may have been attached to and currently in a running state.
>  	 */
> -	if (rproc->autonomous)
> +	if (rproc->state == RPROC_ATTACHED)
>  		firmware = "unknown";
>  
>  	return sprintf(buf, "%s\n", firmware);
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 4564c4665a2c..3fe2ae0bd1ca 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -510,7 +510,6 @@ struct rproc_dump_segment {
>   * @table_sz: size of @cached_table
>   * @has_iommu: flag to indicate if remote processor is behind an MMU
>   * @auto_boot: flag to indicate if remote processor should be auto-started
> - * @autonomous: true if an external entity has booted the remote processor
>   * @dump_segments: list of segments in the firmware
>   * @nb_vdev: number of vdev currently handled by rproc
>   * @char_dev: character device of the rproc
> @@ -547,7 +546,6 @@ struct rproc {
>  	size_t table_sz;
>  	bool has_iommu;
>  	bool auto_boot;
> -	bool autonomous;
>  	struct list_head dump_segments;
>  	int nb_vdev;
>  	u8 elf_class;
> 

Re: [PATCH v2 05/14] remoteproc: Properly deal with a kernel panic when attached

[Arnaud POULIQUEN]

On 10/30/20 8:57 PM, Mathieu Poirier wrote:
> The panic handler operation of registered remote processors
> should also be called when remote processors have been
> attached to.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> Reviewed-by: Peng Fan <peng.fan@nxp.com>

Reviewed-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>

Thanks,
Arnaud

> ---
>  drivers/remoteproc/remoteproc_core.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 63fba1b61840..ed1f9ca4248b 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -2408,7 +2408,11 @@ static int rproc_panic_handler(struct notifier_block *nb, unsigned long event,
>  
>  	rcu_read_lock();
>  	list_for_each_entry_rcu(rproc, &rproc_list, node) {
> -		if (!rproc->ops->panic || rproc->state != RPROC_RUNNING)
> +		if (!rproc->ops->panic)
> +			continue;
> +
> +		if (rproc->state != RPROC_RUNNING &&
> +		    rproc->state != RPROC_ATTACHED)
>  			continue;
>  
>  		d = rproc->ops->panic(rproc);
> 

Re: [PATCH v2 09/14] remoteproc: Rename function rproc_actuate()

[Arnaud POULIQUEN]

On 10/30/20 8:57 PM, Mathieu Poirier wrote:
> Align what was done for rproc_detach() by renaming function
> rproc_actuate().  That way it is easier to figure out the
> opposite of each functions.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> Reviewed-by: Peng Fan <peng.fan@nxp.com>

Reviewed-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>

Thanks,
Arnaud

> ---
>  drivers/remoteproc/remoteproc_core.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 6b33a83960d2..de5a5720d1e8 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1416,7 +1416,7 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
>  	return ret;
>  }
>  
> -static int rproc_attach(struct rproc *rproc)
> +static int __rproc_attach(struct rproc *rproc)
>  {
>  	struct device *dev = &rproc->dev;
>  	int ret;
> @@ -1541,7 +1541,7 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>   * Attach to remote processor - similar to rproc_fw_boot() but without
>   * the steps that deal with the firmware image.
>   */
> -static int rproc_actuate(struct rproc *rproc)
> +static int rproc_attach(struct rproc *rproc)
>  {
>  	struct device *dev = &rproc->dev;
>  	int ret;
> @@ -1581,7 +1581,7 @@ static int rproc_actuate(struct rproc *rproc)
>  		goto clean_up_resources;
>  	}
>  
> -	ret = rproc_attach(rproc);
> +	ret = __rproc_attach(rproc);
>  	if (ret)
>  		goto clean_up_resources;
>  
> @@ -1825,7 +1825,7 @@ int rproc_boot(struct rproc *rproc)
>  	if (rproc->state == RPROC_DETACHED) {
>  		dev_info(dev, "attaching to %s\n", rproc->name);
>  
> -		ret = rproc_actuate(rproc);
> +		ret = rproc_attach(rproc);
>  	} else {
>  		dev_info(dev, "powering up %s\n", rproc->name);
>  
> @@ -1916,7 +1916,7 @@ EXPORT_SYMBOL(rproc_shutdown);
>   *
>   * @rproc: the remote processor
>   *
> - * Detach a remote processor (previously attached to with rproc_actuate()).
> + * Detach a remote processor (previously attached to with rproc_attach()).
>   *
>   * In case @rproc is still being used by an additional user(s), then
>   * this function will just decrement the power refcount and exit,
> 

Re: [PATCH v2 11/14] remoteproc: Properly deal with a stop request when attached

[Arnaud POULIQUEN]

On 10/30/20 8:57 PM, Mathieu Poirier wrote:
> This patch introduces the capability to stop a remote processor
> that has been attached to by the remoteproc core.  For that to
> happen a rproc::ops::stop() operation need to be available.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> Reviewed-by: Peng Fan <peng.fan@nxp.com>


Reviewed-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>

Thanks,
Arnaud

> ---
>  drivers/remoteproc/remoteproc_cdev.c  | 5 +++--
>  drivers/remoteproc/remoteproc_core.c  | 6 +++++-
>  drivers/remoteproc/remoteproc_sysfs.c | 5 +++--
>  3 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
> index b19ea3057bde..d06f8d4919c7 100644
> --- a/drivers/remoteproc/remoteproc_cdev.c
> +++ b/drivers/remoteproc/remoteproc_cdev.c
> @@ -37,10 +37,11 @@ static ssize_t rproc_cdev_write(struct file *filp, const char __user *buf, size_
>  
>  		ret = rproc_boot(rproc);
>  	} else if (!strncmp(cmd, "stop", len)) {
> -		if (rproc->state != RPROC_RUNNING)
> +		if (rproc->state != RPROC_RUNNING &&
> +		    rproc->state != RPROC_ATTACHED)
>  			return -EINVAL;
>  
> -		rproc_shutdown(rproc);
> +		ret = rproc_shutdown(rproc);
>  	} else {
>  		dev_err(&rproc->dev, "Unrecognized option\n");
>  		ret = -EINVAL;
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index f58475f6dcab..229fa2cad0bd 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1642,6 +1642,10 @@ static int rproc_stop(struct rproc *rproc, bool crashed)
>  	struct device *dev = &rproc->dev;
>  	int ret;
>  
> +	/* No need to continue if a stop() operation has not been provided */
> +	if (!rproc->ops->stop)
> +		return -EINVAL;
> +
>  	/* Stop any subdevices for the remote processor */
>  	rproc_stop_subdevices(rproc, crashed);
>  
> @@ -1880,7 +1884,7 @@ int rproc_shutdown(struct rproc *rproc)
>  		return ret;
>  	}
>  
> -	if (rproc->state != RPROC_RUNNING) {
> +	if (rproc->state != RPROC_RUNNING && rproc->state != RPROC_ATTACHED) {
>  		ret = -EPERM;
>  		goto out;
>  	}
> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
> index 99ff51fd9707..96751c087585 100644
> --- a/drivers/remoteproc/remoteproc_sysfs.c
> +++ b/drivers/remoteproc/remoteproc_sysfs.c
> @@ -230,10 +230,11 @@ static ssize_t state_store(struct device *dev,
>  		if (ret)
>  			dev_err(&rproc->dev, "Boot failed: %d\n", ret);
>  	} else if (sysfs_streq(buf, "stop")) {
> -		if (rproc->state != RPROC_RUNNING)
> +		if (rproc->state != RPROC_RUNNING &&
> +		    rproc->state != RPROC_ATTACHED)
>  			return -EINVAL;
>  
> -		rproc_shutdown(rproc);
> +		ret = rproc_shutdown(rproc);
>  	} else {
>  		dev_err(&rproc->dev, "Unrecognised option: %s\n", buf);
>  		ret = -EINVAL;
> 

Re: [RFC v2 13/14] remoteproc: Add automation flags

[Arnaud POULIQUEN]

Hi Mathieu,

Thanks for initiating the discussion!

Waiting feedback from other, please find my feedback on our proposal below.

On 10/30/20 8:57 PM, Mathieu Poirier wrote:
> Adding flags to dictate how to handle a platform driver being removed
> or the remote processor crashing while in RPROC_ATTACHED state.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  drivers/remoteproc/remoteproc_core.c | 25 +++++++++++++++++++++++++
>  include/linux/remoteproc.h           |  5 +++++
>  2 files changed, 30 insertions(+)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 229fa2cad0bd..d024367c63e5 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -2227,6 +2227,29 @@ static int rproc_alloc_ops(struct rproc *rproc, const struct rproc_ops *ops)
>  	return 0;
>  }
>  
> +static void rproc_set_automation_flags(struct rproc *rproc)
> +{
> +	struct device *dev = rproc->dev.parent;
> +	struct device_node *np = dev->of_node;
> +	bool core_reboot, remote_crash;
> +
> +	/*
> +	 * When function rproc_cdev_release() or rproc_del() are called and
> +	 * the remote processor has been attached to, it will be detached from
> +	 * (rather than turned off) if "autonomous_on_core_reboot" is specified
> +	 * in the DT.
> +	 */
> +	core_reboot = of_property_read_bool(np, "autonomous_on_core_reboot");
> +	rproc->autonomous_on_core_reboot = core_reboot;
> +
> +	/*
> +	 * When the remote processor crashes it will be detached from, and
> +	 * attached to, if "autonomous_on_remote_crash" is specified in the DT.
> +	 */
> +	remote_crash = of_property_read_bool(np, "autonomous_on_remote_crash");
> +	rproc->autonomous_on_core_reboot = core_reboot;
> +}
> +

I wonder if the naming is not too restrictive.

I think here we probably need first to identify the use cases we want to support
to determine which use cases should be addressed and deduce DT fields.

Please find my view below:

1) Attach to a remote processor on boot.
This is the "attach" you introduced in a previous series. I wonder here if a DT
field should not be introduce for platform which are not able to dynamically
determines the remote processor state. Something like "remote-boot-on" or
"autonomous-boot-on".

2) Detach from a remote processor on Linux kernel shutdown
Two possible actions: shutdown the remote processor or detach from it.
A DT field could be used to determine the expected behavior.

3) Linux core reboot on crash
Two possible actions: shutdown and restart the remote processor or
detach/re-attach from/to it.
Is same DT field than 2) can be used for this . Or should be determine by a
new sysfs recovery option [1]?

[1]
https://elixir.bootlin.com/linux/v5.10-rc2/source/drivers/remoteproc/remoteproc_sysfs.c#L45

4) The remote processor need to reboot on crash.
3 possible actions:
 - shutdown and restart the remote processor
 - detach and re-attach from/to it.
 - Just shutdown, as no recovery possible without a system reset.

5) Detach/re-attach on Linux suspend/resume
Perhaps better to manage this in platform drivers without a generic DT field?

If i try to apply this on the remote proc boot and shutdown sequences:

1) on remoteproc device add:
- Need to determine if the remote processor is already running:
   - started by another entity
   - Linux reboot after crash

2) On remoteproc device release.
- Need to determine if the remote processor need to be shutdown or detached:
   - Linux kernel crash
   - Linux kernel graceful shutdown with remote processor keeping ON.

3) On remote processor crash
- Need to determine if the remote processor will be restarted by an external
entity or by the remoteproc framework, or if simply not possible to recover
without a system reset.

Regarding these use cases here is an alternative proposal(inspired by regulator
framework):
- "remote-boot-on": determine on probe if the remoteproc firmware is already
booted. This field is optional, use by a platform driver which can not
determine the state of the remote processor. Could be dynamically updated by the
platform driver to manage Kernel crash...

- "remote-always-on": means that the detach has to be privileged on
shutdown. Need also to be managed by platform driver as it can be
compared to the remote processor current state.

- "remoteproc-crash-recovery": crash recovery mode:
   possible value: "SHUTDOWN", "DETACH", "DISABLED"


Regards,
Arnaud

>  /**
>   * rproc_alloc() - allocate a remote processor handle
>   * @dev: the underlying device
> @@ -2285,6 +2308,8 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
>  	if (rproc_alloc_ops(rproc, ops))
>  		goto put_device;
>  
> +	rproc_set_automation_flags(rproc);
> +
>  	/* Assign a unique device index and name */
>  	rproc->index = ida_simple_get(&rproc_dev_index, 0, 0, GFP_KERNEL);
>  	if (rproc->index < 0) {
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 71d4d4873164..9a6e79ef35d7 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -516,6 +516,9 @@ struct rproc_dump_segment {
>   * @nb_vdev: number of vdev currently handled by rproc
>   * @char_dev: character device of the rproc
>   * @cdev_put_on_release: flag to indicate if remoteproc should be shutdown on @char_dev release
> + * @autonomous_on_core_reboot: true if the remote processor should be detached from
> + *			       (rather than turned off) when the remoteproc core
> + *			       goes away.
>   */
>  struct rproc {
>  	struct list_head node;
> @@ -554,6 +557,8 @@ struct rproc {
>  	u16 elf_machine;
>  	struct cdev cdev;
>  	bool cdev_put_on_release;
> +	bool autonomous_on_core_reboot	: 1,
> +	     autonomous_on_remote_crash	: 1;
>  };
>  
>  /**
> 

Re: [PATCH v2 11/14] remoteproc: Properly deal with a stop request when attached

[Arnaud POULIQUEN]

Hi Mathieu,


On 10/30/20 8:57 PM, Mathieu Poirier wrote:
> This patch introduces the capability to stop a remote processor
> that has been attached to by the remoteproc core.  For that to
> happen a rproc::ops::stop() operation need to be available.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> Reviewed-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/remoteproc/remoteproc_cdev.c  | 5 +++--
>  drivers/remoteproc/remoteproc_core.c  | 6 +++++-
>  drivers/remoteproc/remoteproc_sysfs.c | 5 +++--
>  3 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
> index b19ea3057bde..d06f8d4919c7 100644
> --- a/drivers/remoteproc/remoteproc_cdev.c
> +++ b/drivers/remoteproc/remoteproc_cdev.c
> @@ -37,10 +37,11 @@ static ssize_t rproc_cdev_write(struct file *filp, const char __user *buf, size_
>  
>  		ret = rproc_boot(rproc);
>  	} else if (!strncmp(cmd, "stop", len)) {
> -		if (rproc->state != RPROC_RUNNING)
> +		if (rproc->state != RPROC_RUNNING &&
> +		    rproc->state != RPROC_ATTACHED)
>  			return -EINVAL;
>  
> -		rproc_shutdown(rproc);
> +		ret = rproc_shutdown(rproc);
>  	} else {
>  		dev_err(&rproc->dev, "Unrecognized option\n");
>  		ret = -EINVAL;
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index f58475f6dcab..229fa2cad0bd 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1642,6 +1642,10 @@ static int rproc_stop(struct rproc *rproc, bool crashed)
>  	struct device *dev = &rproc->dev;
>  	int ret;
>  
> +	/* No need to continue if a stop() operation has not been provided */
> +	if (!rproc->ops->stop)
> +		return -EINVAL;
> +
>  	/* Stop any subdevices for the remote processor */
>  	rproc_stop_subdevices(rproc, crashed);
>  
> @@ -1880,7 +1884,7 @@ int rproc_shutdown(struct rproc *rproc)
>  		return ret;
>  	}
>  
> -	if (rproc->state != RPROC_RUNNING) {
> +	if (rproc->state != RPROC_RUNNING && rproc->state != RPROC_ATTACHED) {
>  		ret = -EPERM;
>  		goto out;
>  	}
> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
> index 99ff51fd9707..96751c087585 100644
> --- a/drivers/remoteproc/remoteproc_sysfs.c
> +++ b/drivers/remoteproc/remoteproc_sysfs.c

I've started some tests, it's working pretty well!
I found an issue when trying to start the remote proc while already attached:

@@ -106,7 +106,8 @@ static ssize_t state_store(struct device *dev,
 	int ret = 0;

 	if (sysfs_streq(buf, "start")) {
-		if (rproc->state == RPROC_RUNNING)
+		if (rproc->state == RPROC_RUNNING ||
+		    rproc->state == RPROC_ATTACHED)
 			return -EBUSY;

 		ret = rproc_boot(rproc);

To fix it also in cdev...

Regards,
Arnaud

> @@ -230,10 +230,11 @@ static ssize_t state_store(struct device *dev,
>  		if (ret)
>  			dev_err(&rproc->dev, "Boot failed: %d\n", ret);
>  	} else if (sysfs_streq(buf, "stop")) {
> -		if (rproc->state != RPROC_RUNNING)
> +		if (rproc->state != RPROC_RUNNING &&
> +		    rproc->state != RPROC_ATTACHED)
>  			return -EINVAL;
>  
> -		rproc_shutdown(rproc);
> +		ret = rproc_shutdown(rproc);
>  	} else {
>  		dev_err(&rproc->dev, "Unrecognised option: %s\n", buf);
>  		ret = -EINVAL;
> 

Re: [PATCH v2 00/14] remoteproc: Add support for detaching from rproc

[Arnaud POULIQUEN]

Hi Mathieu,


On 10/30/20 8:56 PM, Mathieu Poirier wrote:
> Following the work done here [1], this set provides support for the
> remoteproc core to release resources associated with a remote processor
> without having to switch it off. That way a platform driver can be removed
> or the applcation processor power cycled while the remote processor is
> still operating.

I performed some non-regression tests on firmware attachment + few tests on
detach. I don't see any major problem introduced by this patchset (except the
minor problem I reported in patch 11/14:remoteproc: correctly process a stop
request when attached).

My concern is that without the bindings, we still have a problem on the recovery.
If a crash occurs while attached to a remote processor, the remote framework is
in an unexpected state, which requires a system reset to recover it.

To reproduce the issue, simply generate the crash :
 cat 1 >/sys/kernel/debug/remoteproc/remoteproc0/crash

At the end of the mail, I attached a temporary patch to apply on top of this
series, waiting for the bindings management. The patch shutdowns the attached
remote processor instead of trying to recover it.

I wonder if we should fix this for version 4.10 based on the current
implementation (if the patch window is not closed)...
Please tell me what would be the best strategy. If it's not too late, I can
prepare and send a patch tomorrow for v5.10.

Regards
Arnaud
> 
> The only thing that changes in this revision are the last two patches where
> device tree bindings to control how to handle attached remote processors have
> been added.  More specifically two bindings are being proposed:
> 
> "autonomous_on_core_reboot": When rproc_cdev_release() or rproc_del() are called
> and the remote processor has been attached to, it will be detached from (rather
> than turned off) if "autonomous_on_core_reboot" is specified in the DT.
> 
> "autonomous_on_remote_crash": When a remote processor that has been attached to
> crashes, it will be detached from if "autonomous_on_remote_crash" is specified
> in the DT. It is _not_ used in this set and presented to show how I intend to 
> organise things. 


> 
> I spent a fair amount of time coming up with the name for the bindings and would
> welcome other ideas.  I will write a proper yaml file and CC the linux-kernel
> mailing list once we have an agreement on the naming convention.
> 
> Applies cleanly on v5.10-rc1
> 
> Thanks,
> Mathieu
> 
> [1]. https://lkml.org/lkml/2020/7/14/1600
> 
> Mathieu Poirier (14):
>   remoteproc: Re-check state in rproc_shutdown()
>   remoteproc: Remove useless check in rproc_del()
>   remoteproc: Add new RPROC_ATTACHED state
>   remoteproc: Properly represent the attached state
>   remoteproc: Properly deal with a kernel panic when attached
>   remoteproc: Add new detach() remoteproc operation
>   remoteproc: Introduce function __rproc_detach()
>   remoteproc: Introduce function rproc_detach()
>   remoteproc: Rename function rproc_actuate()
>   remoteproc: Add return value to function rproc_shutdown()
>   remoteproc: Properly deal with a stop request when attached
>   remoteproc: Properly deal with detach request
>   remoteproc: Add automation flags
>   remoteproc: Refactor rproc delete and cdev release path
> 
>  drivers/remoteproc/remoteproc_cdev.c  |  24 +++-
>  drivers/remoteproc/remoteproc_core.c  | 183 +++++++++++++++++++++-----
>  drivers/remoteproc/remoteproc_sysfs.c |  17 ++-
>  include/linux/remoteproc.h            |  19 ++-
>  4 files changed, 199 insertions(+), 44 deletions(-)
> 

From a04866cf0add0020b65e9ab80d62d44290a1d695 Mon Sep 17 00:00:00 2001
From: Arnaud Pouliquen <arnaud.pouliquen@st.com>
Date: Thu, 12 Nov 2020 18:25:52 +0100
Subject: [PATCH] remoteproc: core fix unexpected state on crash for attached
 firmware

The recovery falls in an unexpected state when attached to a remote
processor.
As no firmware to load is found, the remote processor has
just been stopped but associated resources are not free.
As consequence rproc->power is remaining at 1 and it s no more
possible to recover the remote processor.
This patch shutdown the attached remote processor instead of trying
to recover it.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c
b/drivers/remoteproc/remoteproc_core.c
index a0611d494758..a38209dd782c 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1739,6 +1739,8 @@ 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;
+	unsigned int rproc_attached = false;
+

 	dev_dbg(dev, "enter %s\n", __func__);

@@ -1750,15 +1752,21 @@ static void rproc_crash_handler_work(struct work_struct
*work)
 		return;
 	}

+	if (rproc->state == RPROC_ATTACHED)
+		rproc_attached = true;
+
 	rproc->state = RPROC_CRASHED;
 	dev_err(dev, "handling crash #%u in %s\n", ++rproc->crash_cnt,
 		rproc->name);

 	mutex_unlock(&rproc->lock);

-	if (!rproc->recovery_disabled)
-		rproc_trigger_recovery(rproc);
-
+	if (!rproc->recovery_disabled) {
+		if (!rproc_attached)
+			rproc_trigger_recovery(rproc);
+		else
+			rproc_shutdown(rproc);
+	}
 	pm_relax(rproc->dev.parent);
 }

@@ -1862,7 +1870,8 @@ int rproc_shutdown(struct rproc *rproc)
 		return ret;
 	}

-	if (rproc->state != RPROC_RUNNING && rproc->state != RPROC_ATTACHED) {
+	if (rproc->state != RPROC_RUNNING && rproc->state != RPROC_ATTACHED &&
+	    rproc->state != RPROC_CRASHED) {
 		ret = -EPERM;
 		goto out;
 	}
-- 
2.17.1

Re: [PATCH v2 00/14] remoteproc: Add support for detaching from rproc

[Mathieu Poirier]

Good day,

On Thu, Nov 12, 2020 at 07:20:49PM +0100, Arnaud POULIQUEN wrote:
> Hi Mathieu,
> 
> 
> On 10/30/20 8:56 PM, Mathieu Poirier wrote:
> > Following the work done here [1], this set provides support for the
> > remoteproc core to release resources associated with a remote processor
> > without having to switch it off. That way a platform driver can be removed
> > or the applcation processor power cycled while the remote processor is
> > still operating.
> 
> I performed some non-regression tests on firmware attachment + few tests on
> detach. I don't see any major problem introduced by this patchset (except the
> minor problem I reported in patch 11/14:remoteproc: correctly process a stop
> request when attached).
> 

Your assessment is correct - I will fix that.

> My concern is that without the bindings, we still have a problem on the recovery.

At this time (and while this feature is being worked on) we have problems with
shutdown and recovery.  The up side is that 1) we know about them and 2) we are
actively working on closing the gab.  As such I am weary of introducting
temporary measures visible to users that will have to be maintained in the
future.  I'd rather simply acknowledge that something is broken.

I will take into account your comments on the bindings (which I haven't looked
at yet).  From there I expect to spin off another revision early to middle of
next week.  If we get this part in for the v5.11 cycle then only crash scenarios
will be left to address.

Thanks for the feedback,
Mathieu

> If a crash occurs while attached to a remote processor, the remote framework is
> in an unexpected state, which requires a system reset to recover it.
> 
> To reproduce the issue, simply generate the crash :
>  cat 1 >/sys/kernel/debug/remoteproc/remoteproc0/crash
> 
> At the end of the mail, I attached a temporary patch to apply on top of this
> series, waiting for the bindings management. The patch shutdowns the attached
> remote processor instead of trying to recover it.
> 
> I wonder if we should fix this for version 4.10 based on the current
> implementation (if the patch window is not closed)...
> Please tell me what would be the best strategy. If it's not too late, I can
> prepare and send a patch tomorrow for v5.10.
> 
> Regards
> Arnaud
> > 
> > The only thing that changes in this revision are the last two patches where
> > device tree bindings to control how to handle attached remote processors have
> > been added.  More specifically two bindings are being proposed:
> > 
> > "autonomous_on_core_reboot": When rproc_cdev_release() or rproc_del() are called
> > and the remote processor has been attached to, it will be detached from (rather
> > than turned off) if "autonomous_on_core_reboot" is specified in the DT.
> > 
> > "autonomous_on_remote_crash": When a remote processor that has been attached to
> > crashes, it will be detached from if "autonomous_on_remote_crash" is specified
> > in the DT. It is _not_ used in this set and presented to show how I intend to 
> > organise things. 
> 
> 
> > 
> > I spent a fair amount of time coming up with the name for the bindings and would
> > welcome other ideas.  I will write a proper yaml file and CC the linux-kernel
> > mailing list once we have an agreement on the naming convention.
> > 
> > Applies cleanly on v5.10-rc1
> > 
> > Thanks,
> > Mathieu
> > 
> > [1]. https://lkml.org/lkml/2020/7/14/1600
> > 
> > Mathieu Poirier (14):
> >   remoteproc: Re-check state in rproc_shutdown()
> >   remoteproc: Remove useless check in rproc_del()
> >   remoteproc: Add new RPROC_ATTACHED state
> >   remoteproc: Properly represent the attached state
> >   remoteproc: Properly deal with a kernel panic when attached
> >   remoteproc: Add new detach() remoteproc operation
> >   remoteproc: Introduce function __rproc_detach()
> >   remoteproc: Introduce function rproc_detach()
> >   remoteproc: Rename function rproc_actuate()
> >   remoteproc: Add return value to function rproc_shutdown()
> >   remoteproc: Properly deal with a stop request when attached
> >   remoteproc: Properly deal with detach request
> >   remoteproc: Add automation flags
> >   remoteproc: Refactor rproc delete and cdev release path
> > 
> >  drivers/remoteproc/remoteproc_cdev.c  |  24 +++-
> >  drivers/remoteproc/remoteproc_core.c  | 183 +++++++++++++++++++++-----
> >  drivers/remoteproc/remoteproc_sysfs.c |  17 ++-
> >  include/linux/remoteproc.h            |  19 ++-
> >  4 files changed, 199 insertions(+), 44 deletions(-)
> > 
> 
> From a04866cf0add0020b65e9ab80d62d44290a1d695 Mon Sep 17 00:00:00 2001
> From: Arnaud Pouliquen <arnaud.pouliquen@st.com>
> Date: Thu, 12 Nov 2020 18:25:52 +0100
> Subject: [PATCH] remoteproc: core fix unexpected state on crash for attached
>  firmware
> 
> The recovery falls in an unexpected state when attached to a remote
> processor.
> As no firmware to load is found, the remote processor has
> just been stopped but associated resources are not free.
> As consequence rproc->power is remaining at 1 and it s no more
> possible to recover the remote processor.
> This patch shutdown the attached remote processor instead of trying
> to recover it.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
> ---
>  drivers/remoteproc/remoteproc_core.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c
> b/drivers/remoteproc/remoteproc_core.c
> index a0611d494758..a38209dd782c 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1739,6 +1739,8 @@ 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;
> +	unsigned int rproc_attached = false;
> +
> 
>  	dev_dbg(dev, "enter %s\n", __func__);
> 
> @@ -1750,15 +1752,21 @@ static void rproc_crash_handler_work(struct work_struct
> *work)
>  		return;
>  	}
> 
> +	if (rproc->state == RPROC_ATTACHED)
> +		rproc_attached = true;
> +
>  	rproc->state = RPROC_CRASHED;
>  	dev_err(dev, "handling crash #%u in %s\n", ++rproc->crash_cnt,
>  		rproc->name);
> 
>  	mutex_unlock(&rproc->lock);
> 
> -	if (!rproc->recovery_disabled)
> -		rproc_trigger_recovery(rproc);
> -
> +	if (!rproc->recovery_disabled) {
> +		if (!rproc_attached)
> +			rproc_trigger_recovery(rproc);
> +		else
> +			rproc_shutdown(rproc);
> +	}
>  	pm_relax(rproc->dev.parent);
>  }
> 
> @@ -1862,7 +1870,8 @@ int rproc_shutdown(struct rproc *rproc)
>  		return ret;
>  	}
> 
> -	if (rproc->state != RPROC_RUNNING && rproc->state != RPROC_ATTACHED) {
> +	if (rproc->state != RPROC_RUNNING && rproc->state != RPROC_ATTACHED &&
> +	    rproc->state != RPROC_CRASHED) {
>  		ret = -EPERM;
>  		goto out;
>  	}
> -- 
> 2.17.1
> 
> 
> 

Re: [RFC v2 13/14] remoteproc: Add automation flags

[Mathieu Poirier]

Hi,

On Thu, Nov 12, 2020 at 02:56:20PM +0100, Arnaud POULIQUEN wrote:
> Hi Mathieu,
> 
> Thanks for initiating the discussion!
> 
> Waiting feedback from other, please find my feedback on our proposal below.

The first version of this set has been released on August 26th and since then,
only you and Peng have given me feedback.  As such I suggest that we move
forward with the decision you and I settle on.  As usual with open source
development, people can submit new patches to enhance our solution as they see
fit.

> 
> On 10/30/20 8:57 PM, Mathieu Poirier wrote:
> > Adding flags to dictate how to handle a platform driver being removed
> > or the remote processor crashing while in RPROC_ATTACHED state.
> > 
> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > ---
> >  drivers/remoteproc/remoteproc_core.c | 25 +++++++++++++++++++++++++
> >  include/linux/remoteproc.h           |  5 +++++
> >  2 files changed, 30 insertions(+)
> > 
> > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > index 229fa2cad0bd..d024367c63e5 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -2227,6 +2227,29 @@ static int rproc_alloc_ops(struct rproc *rproc, const struct rproc_ops *ops)
> >  	return 0;
> >  }
> >  
> > +static void rproc_set_automation_flags(struct rproc *rproc)
> > +{
> > +	struct device *dev = rproc->dev.parent;
> > +	struct device_node *np = dev->of_node;
> > +	bool core_reboot, remote_crash;
> > +
> > +	/*
> > +	 * When function rproc_cdev_release() or rproc_del() are called and
> > +	 * the remote processor has been attached to, it will be detached from
> > +	 * (rather than turned off) if "autonomous_on_core_reboot" is specified
> > +	 * in the DT.
> > +	 */
> > +	core_reboot = of_property_read_bool(np, "autonomous_on_core_reboot");
> > +	rproc->autonomous_on_core_reboot = core_reboot;
> > +
> > +	/*
> > +	 * When the remote processor crashes it will be detached from, and
> > +	 * attached to, if "autonomous_on_remote_crash" is specified in the DT.
> > +	 */
> > +	remote_crash = of_property_read_bool(np, "autonomous_on_remote_crash");
> > +	rproc->autonomous_on_core_reboot = core_reboot;
> > +}
> > +
> 
> I wonder if the naming is not too restrictive.

I'm happy to have this conversation, which is really the point of this second
revision.  I turned names and ideas around in my head for a long time and the above is
the best I came up with.  Your insight gave me food for thought - see below.

> 
> I think here we probably need first to identify the use cases we want to support
> to determine which use cases should be addressed and deduce DT fields.
> 
> Please find my view below:
> 
> 1) Attach to a remote processor on boot.
> This is the "attach" you introduced in a previous series. I wonder here if a DT
> field should not be introduce for platform which are not able to dynamically
> determines the remote processor state. Something like "remote-boot-on" or
> "autonomous-boot-on".

Right - I think "autonomous-on-core-boot" would be best as it really spells out
what is going on. I did not include it in the "attach" patchset because there
wasn't a need for it.  Both ST and NXP are able to determine the state of the
remote processor from a platform driver.  My initial strategy was to introduce
the functionality when the need for it comes up.  I can revisit if you feel
strongly about adding it immediately. 

> 
> 2) Detach from a remote processor on Linux kernel shutdown
> Two possible actions: shutdown the remote processor or detach from it.
> A DT field could be used to determine the expected behavior.
>

That is what the "autonomous-on-core-reboot" was for but reading your
description I think "autonomous-on-core-shutdown" is best to describe the
scenario.
 
> 3) Linux core reboot on crash
> Two possible actions: shutdown and restart the remote processor or
> detach/re-attach from/to it.
> Is same DT field than 2) can be used for this . Or should be determine by a
> new sysfs recovery option [1]?

As far as I can tell nothing happens to drivers when the kernel crashes.  To
take action when the kernel crashes each driver needs to register explicitly
with the panic notifier, which the remoteproc doesn't currently do.  That's a
different feature that I would like to delay for another time.

If and when that time comes we can either reuse "autonomous-on-core-shutdown"
or introduce "autonomous-on-core-crash", depending on the level of granularity
needed.

> 
> [1]
> https://elixir.bootlin.com/linux/v5.10-rc2/source/drivers/remoteproc/remoteproc_sysfs.c#L45
> 
> 4) The remote processor need to reboot on crash.
> 3 possible actions:
>  - shutdown and restart the remote processor

That is currently the default behavior _if_ recovery is enabled.

>  - detach and re-attach from/to it.

That is how I intend to use "autonomous-on-remote-crash", _if_ recovery is
enabled.

>  - Just shutdown, as no recovery possible without a system reset.

That is the current behavior if recovery is _not_ enabled.

Dealing with crash scenarios is a little more complex and requires some
refactoring.  That is why I wanted to solely concentrate on the shutdown scenario
in this set.

> 
> 5) Detach/re-attach on Linux suspend/resume
> Perhaps better to manage this in platform drivers without a generic DT field?

I think that falls in the same category as power management and is too specific
to be handled in the remoteproc core.  As you suggest, it is probably best to
leave that to platform drivers for the time being. 

> 
> If i try to apply this on the remote proc boot and shutdown sequences:
> 
> 1) on remoteproc device add:
> - Need to determine if the remote processor is already running:
>    - started by another entity
>    - Linux reboot after crash
> 
> 2) On remoteproc device release.
> - Need to determine if the remote processor need to be shutdown or detached:
>    - Linux kernel crash
>    - Linux kernel graceful shutdown with remote processor keeping ON.
> 
> 3) On remote processor crash
> - Need to determine if the remote processor will be restarted by an external
> entity or by the remoteproc framework, or if simply not possible to recover
> without a system reset.
> 
> Regarding these use cases here is an alternative proposal(inspired by regulator
> framework):
> - "remote-boot-on": determine on probe if the remoteproc firmware is already
> booted. This field is optional, use by a platform driver which can not
> determine the state of the remote processor. Could be dynamically updated by the
> platform driver to manage Kernel crash...
> 
> - "remote-always-on": means that the detach has to be privileged on
> shutdown. Need also to be managed by platform driver as it can be
> compared to the remote processor current state.
> 
> - "remoteproc-crash-recovery": crash recovery mode:
>    possible value: "SHUTDOWN", "DETACH", "DISABLED"

I think all of the above scenarios can be managed with a combination of the
proposed bindings , i.e "autonomous-on-core-shutdown" and
"autonomous-on-remote-crash".  The latter would be used in conjuction with the
recovery mechanic already available. 

Let me know what you think.

Mathieu

> 
> 
> Regards,
> Arnaud
> 
> >  /**
> >   * rproc_alloc() - allocate a remote processor handle
> >   * @dev: the underlying device
> > @@ -2285,6 +2308,8 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
> >  	if (rproc_alloc_ops(rproc, ops))
> >  		goto put_device;
> >  
> > +	rproc_set_automation_flags(rproc);
> > +
> >  	/* Assign a unique device index and name */
> >  	rproc->index = ida_simple_get(&rproc_dev_index, 0, 0, GFP_KERNEL);
> >  	if (rproc->index < 0) {
> > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > index 71d4d4873164..9a6e79ef35d7 100644
> > --- a/include/linux/remoteproc.h
> > +++ b/include/linux/remoteproc.h
> > @@ -516,6 +516,9 @@ struct rproc_dump_segment {
> >   * @nb_vdev: number of vdev currently handled by rproc
> >   * @char_dev: character device of the rproc
> >   * @cdev_put_on_release: flag to indicate if remoteproc should be shutdown on @char_dev release
> > + * @autonomous_on_core_reboot: true if the remote processor should be detached from
> > + *			       (rather than turned off) when the remoteproc core
> > + *			       goes away.
> >   */
> >  struct rproc {
> >  	struct list_head node;
> > @@ -554,6 +557,8 @@ struct rproc {
> >  	u16 elf_machine;
> >  	struct cdev cdev;
> >  	bool cdev_put_on_release;
> > +	bool autonomous_on_core_reboot	: 1,
> > +	     autonomous_on_remote_crash	: 1;
> >  };
> >  
> >  /**
> > 

Re: [RFC v2 13/14] remoteproc: Add automation flags

[Arnaud POULIQUEN]

Hi Mathieu,

On 11/13/20 10:27 PM, Mathieu Poirier wrote:
> Hi,
> 
> On Thu, Nov 12, 2020 at 02:56:20PM +0100, Arnaud POULIQUEN wrote:
>> Hi Mathieu,
>>
>> Thanks for initiating the discussion!
>>
>> Waiting feedback from other, please find my feedback on our proposal below.
> 
> The first version of this set has been released on August 26th and since then,
> only you and Peng have given me feedback.  As such I suggest that we move
> forward with the decision you and I settle on.  As usual with open source
> development, people can submit new patches to enhance our solution as they see
> fit.
> 
>>
>> On 10/30/20 8:57 PM, Mathieu Poirier wrote:
>>> Adding flags to dictate how to handle a platform driver being removed
>>> or the remote processor crashing while in RPROC_ATTACHED state.
>>>
>>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>>> ---
>>>  drivers/remoteproc/remoteproc_core.c | 25 +++++++++++++++++++++++++
>>>  include/linux/remoteproc.h           |  5 +++++
>>>  2 files changed, 30 insertions(+)
>>>
>>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>>> index 229fa2cad0bd..d024367c63e5 100644
>>> --- a/drivers/remoteproc/remoteproc_core.c
>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>> @@ -2227,6 +2227,29 @@ static int rproc_alloc_ops(struct rproc *rproc, const struct rproc_ops *ops)
>>>  	return 0;
>>>  }
>>>  
>>> +static void rproc_set_automation_flags(struct rproc *rproc)
>>> +{
>>> +	struct device *dev = rproc->dev.parent;
>>> +	struct device_node *np = dev->of_node;
>>> +	bool core_reboot, remote_crash;
>>> +
>>> +	/*
>>> +	 * When function rproc_cdev_release() or rproc_del() are called and
>>> +	 * the remote processor has been attached to, it will be detached from
>>> +	 * (rather than turned off) if "autonomous_on_core_reboot" is specified
>>> +	 * in the DT.
>>> +	 */
>>> +	core_reboot = of_property_read_bool(np, "autonomous_on_core_reboot");
>>> +	rproc->autonomous_on_core_reboot = core_reboot;
>>> +
>>> +	/*
>>> +	 * When the remote processor crashes it will be detached from, and
>>> +	 * attached to, if "autonomous_on_remote_crash" is specified in the DT.
>>> +	 */
>>> +	remote_crash = of_property_read_bool(np, "autonomous_on_remote_crash");
>>> +	rproc->autonomous_on_core_reboot = core_reboot;
>>> +}
>>> +
>>
>> I wonder if the naming is not too restrictive.
> 
> I'm happy to have this conversation, which is really the point of this second
> revision.  I turned names and ideas around in my head for a long time and the above is
> the best I came up with.  Your insight gave me food for thought - see below.


> 
>>
>> I think here we probably need first to identify the use cases we want to support
>> to determine which use cases should be addressed and deduce DT fields.
>>
>> Please find my view below:
>>
>> 1) Attach to a remote processor on boot.
>> This is the "attach" you introduced in a previous series. I wonder here if a DT
>> field should not be introduce for platform which are not able to dynamically
>> determines the remote processor state. Something like "remote-boot-on" or
>> "autonomous-boot-on".
> 
> Right - I think "autonomous-on-core-boot" would be best as it really spells out
> what is going on. I did not include it in the "attach" patchset because there
> wasn't a need for it.  Both ST and NXP are able to determine the state of the
> remote processor from a platform driver.  My initial strategy was to introduce
> the functionality when the need for it comes up.  I can revisit if you feel
> strongly about adding it immediately. 

No problem to add it later. I mentioned it here only trying to have a complete
view of the use cases.

> 
>>
>> 2) Detach from a remote processor on Linux kernel shutdown
>> Two possible actions: shutdown the remote processor or detach from it.
>> A DT field could be used to determine the expected behavior.
>>
> 
> That is what the "autonomous-on-core-reboot" was for but reading your
> description I think "autonomous-on-core-shutdown" is best to describe the
> scenario.
>  
>> 3) Linux core reboot on crash
>> Two possible actions: shutdown and restart the remote processor or
>> detach/re-attach from/to it.
>> Is same DT field than 2) can be used for this . Or should be determine by a
>> new sysfs recovery option [1]?
> 
> As far as I can tell nothing happens to drivers when the kernel crashes.  To
> take action when the kernel crashes each driver needs to register explicitly
> with the panic notifier, which the remoteproc doesn't currently do.  That's a
> different feature that I would like to delay for another time.
> 
> If and when that time comes we can either reuse "autonomous-on-core-shutdown"
> or introduce "autonomous-on-core-crash", depending on the level of granularity
> needed.

Right, i don't know if it possible to determine boot reason on remoteproc probe.
(first boot or re-boot after crash). This information will be needed to
determine if the firmware has to be re-attached or re-started.

> 
>>
>> [1]
>> https://elixir.bootlin.com/linux/v5.10-rc2/source/drivers/remoteproc/remoteproc_sysfs.c#L45
>>
>> 4) The remote processor need to reboot on crash.
>> 3 possible actions:
>>  - shutdown and restart the remote processor
> 
> That is currently the default behavior _if_ recovery is enabled.
> 
>>  - detach and re-attach from/to it.
> 
> That is how I intend to use "autonomous-on-remote-crash", _if_ recovery is
> enabled.
> 
>>  - Just shutdown, as no recovery possible without a system reset.
> 
> That is the current behavior if recovery is _not_ enabled.
> 

The recovery is a user interface, for my pov the recovery sysfs/debugfs is used
to allow userland to chose behavior on crash
- automatic recovery
- asynchronous recovery ( recovery trigged by the userspace)
- no recovery

As the userspace should be hardware independent, the platform driver should
provide information on the capabilities I list above.
That's why i proposed to not have a boolean but an enum to handle the crash

> Dealing with crash scenarios is a little more complex and requires some
> refactoring.  That is why I wanted to solely concentrate on the shutdown scenario
> in this set.

Yes, my feedback was mainly to determine if the properties allow to cover the
use cases, to avoid to refactor them later. let start with the shutdown.

> 
>>
>> 5) Detach/re-attach on Linux suspend/resume
>> Perhaps better to manage this in platform drivers without a generic DT field?
> 
> I think that falls in the same category as power management and is too specific
> to be handled in the remoteproc core.  As you suggest, it is probably best to
> leave that to platform drivers for the time being. 
> 
>>
>> If i try to apply this on the remote proc boot and shutdown sequences:
>>
>> 1) on remoteproc device add:
>> - Need to determine if the remote processor is already running:
>>    - started by another entity
>>    - Linux reboot after crash
>>
>> 2) On remoteproc device release.
>> - Need to determine if the remote processor need to be shutdown or detached:
>>    - Linux kernel crash
>>    - Linux kernel graceful shutdown with remote processor keeping ON.
>>
>> 3) On remote processor crash
>> - Need to determine if the remote processor will be restarted by an external
>> entity or by the remoteproc framework, or if simply not possible to recover
>> without a system reset.
>>
>> Regarding these use cases here is an alternative proposal(inspired by regulator
>> framework):
>> - "remote-boot-on": determine on probe if the remoteproc firmware is already
>> booted. This field is optional, use by a platform driver which can not
>> determine the state of the remote processor. Could be dynamically updated by the
>> platform driver to manage Kernel crash...
>>
>> - "remote-always-on": means that the detach has to be privileged on
>> shutdown. Need also to be managed by platform driver as it can be
>> compared to the remote processor current state.
>>
>> - "remoteproc-crash-recovery": crash recovery mode:
>>    possible value: "SHUTDOWN", "DETACH", "DISABLED"
> 
> I think all of the above scenarios can be managed with a combination of the
> proposed bindings , i.e "autonomous-on-core-shutdown" and
> "autonomous-on-remote-crash".  The latter would be used in conjuction with the
> recovery mechanic already available. 
> 
> Let me know what you think.

"autonomous-on-core-shutdown" is fine and sufficient for this series. So as you
suggest let's start with that.

As explained above for the "autonomous-on-remote-crash" property i would prefer
an enum to also be able to prohibit the recovering as well, especially if the
recovering is migrating to the sysfs. let's discuss this in a second step.

Thanks,
Arnaud

> 
> Mathieu
> 
>>
>>
>> Regards,
>> Arnaud
>>
>>>  /**
>>>   * rproc_alloc() - allocate a remote processor handle
>>>   * @dev: the underlying device
>>> @@ -2285,6 +2308,8 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
>>>  	if (rproc_alloc_ops(rproc, ops))
>>>  		goto put_device;
>>>  
>>> +	rproc_set_automation_flags(rproc);
>>> +
>>>  	/* Assign a unique device index and name */
>>>  	rproc->index = ida_simple_get(&rproc_dev_index, 0, 0, GFP_KERNEL);
>>>  	if (rproc->index < 0) {
>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>>> index 71d4d4873164..9a6e79ef35d7 100644
>>> --- a/include/linux/remoteproc.h
>>> +++ b/include/linux/remoteproc.h
>>> @@ -516,6 +516,9 @@ struct rproc_dump_segment {
>>>   * @nb_vdev: number of vdev currently handled by rproc
>>>   * @char_dev: character device of the rproc
>>>   * @cdev_put_on_release: flag to indicate if remoteproc should be shutdown on @char_dev release
>>> + * @autonomous_on_core_reboot: true if the remote processor should be detached from
>>> + *			       (rather than turned off) when the remoteproc core
>>> + *			       goes away.
>>>   */
>>>  struct rproc {
>>>  	struct list_head node;
>>> @@ -554,6 +557,8 @@ struct rproc {
>>>  	u16 elf_machine;
>>>  	struct cdev cdev;
>>>  	bool cdev_put_on_release;
>>> +	bool autonomous_on_core_reboot	: 1,
>>> +	     autonomous_on_remote_crash	: 1;
>>>  };
>>>  
>>>  /**
>>>

Re: [PATCH v2 06/14] remoteproc: Add new detach() remoteproc operation

[Mathieu Poirier]

On Fri, Nov 06, 2020 at 06:31:30PM +0100, Arnaud POULIQUEN wrote:
> On 10/30/20 8:57 PM, Mathieu Poirier wrote:
> > Add an new detach() operation in order to support scenarios where
> > the remoteproc core is going away but the remote processor is
> > kept operating.  This could be the case when the system is
> > rebooted or when the platform driver is removed.
> > 
> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > Reviewed-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  include/linux/remoteproc.h | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > index 3fe2ae0bd1ca..3faff9bb4fb8 100644
> > --- a/include/linux/remoteproc.h
> > +++ b/include/linux/remoteproc.h
> > @@ -361,6 +361,7 @@ enum rsc_handling_status {
> >   * @start:	power on the device and boot it
> >   * @stop:	power off the device
> >   * @attach:	attach to a device that his already powered up
> > + * @detach:	tell the remote processor that the core is going away
> 
> This comment seems to me rather ambiguous...
> "tell the remote processor" could means communication with the remote processor.
> The term "remote processor" is used for this op and "device" for the other ops.
> Proposal:
>  detach from a device by leaving it power-up.

I agree - now that I'm looking at it again it is quite obvious.

> 
> Regards,
> Arnaud
>  
> >   * @kick:	kick a virtqueue (virtqueue id given as a parameter)
> >   * @da_to_va:	optional platform hook to perform address translations
> >   * @parse_fw:	parse firmware to extract information (e.g. resource table)
> > @@ -382,6 +383,7 @@ struct rproc_ops {
> >  	int (*start)(struct rproc *rproc);
> >  	int (*stop)(struct rproc *rproc);
> >  	int (*attach)(struct rproc *rproc);
> > +	int (*detach)(struct rproc *rproc);
> >  	void (*kick)(struct rproc *rproc, int vqid);
> >  	void * (*da_to_va)(struct rproc *rproc, u64 da, size_t len);
> >  	int (*parse_fw)(struct rproc *rproc, const struct firmware *fw);
> > 

Re: [PATCH v2 07/14] remoteproc: Introduce function __rproc_detach()

[Mathieu Poirier]

On Fri, Nov 06, 2020 at 06:43:05PM +0100, Arnaud POULIQUEN wrote:
> 
> 
> On 10/30/20 8:57 PM, Mathieu Poirier wrote:
> > Introduce function __rproc_detach() to perform the same kind of
> > operation as rproc_stop(), but instead of switching off the
> > remote processor using rproc->ops->stop(), it uses
> > rproc->ops->detach().  That way it is possible for the core
> > to release the resources associated with a remote processor while
> > the latter is kept operating.
> > 
> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > Reviewed-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/remoteproc/remoteproc_core.c | 31 ++++++++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> > 
> > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > index ed1f9ca4248b..62e88ff65009 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -1664,6 +1664,37 @@ static int rproc_stop(struct rproc *rproc, bool crashed)
> >  	return 0;
> >  }
> >  
> > +/*
> > + * __rproc_detach(): Does the opposite of rproc_attach()
> > + */
> > +static int __maybe_unused __rproc_detach(struct rproc *rproc)
> > +{
> > +	struct device *dev = &rproc->dev;
> > +	int ret;
> > +
> > +	/* No need to continue if a detach() operation has not been provided */
> > +	if (!rproc->ops->detach)
> > +		return -EINVAL;
> > +
> > +	/* Stop any subdevices for the remote processor */
> > +	rproc_stop_subdevices(rproc, false);
> 
> How to determine whether a subdevice should be stopped or detached? 
> For instance, in ST, we have a resource manager subdev which maintains clocks and regulators
> for peripherals used by the remote processor.
> In case of detachment we would need to maintain clock and regulators.
> 
> > +
> > +	/* Tell the remote processor the core isn't available anymore */
> > +	ret = rproc->ops->detach(rproc);
> > +	if (ret) {
> > +		dev_err(dev, "can't detach from rproc: %d\n", ret);
> > +		rproc_start_subdevices(rproc);
> > +		return ret;
> > +	}
> > +
> > +	rproc_unprepare_subdevices(rproc);
> 
> Same here, is prepare/unprepare can depend on the operation?
> 
> Seems that adding rproc_attach_subdevices/rproc_detach_subdevices could be not sufficient
> to address prepare/unprepare.
> Alternative could be:
> - extra parameter for the subdev ops to indicate attach/detach action...?
> - intermediate rproc state : ATTACHING, DETACHING
> - other?

These are really good points.  I did not think about that kind of scenario and
the best solution isn't obvious to me either.

> 
> That's said, I don't think that it is blocking for the ST resource manager.
> In this particular case, regulators and clocks can be permanently activated
> as a back-up solution (always-on).
> 
> So, if no other company has a problem with that, we can keep this implementation for now.

As I wrote above the path forward isn't clear to me and as such will opt to
address this in another patchset...  But it has to be fixed. 

> 
> Regards,
> Arnaud
> 
> > +
> > +	rproc->state = RPROC_DETACHED;
> > +
> > +	dev_info(dev, "detached remote processor %s\n", rproc->name);
> > +
> > +	return 0;
> > +}
> >  
> >  /**
> >   * rproc_trigger_recovery() - recover a remoteproc
> > 

Re: [PATCH v2 08/14] remoteproc: Introduce function rproc_detach()

[Mathieu Poirier]

On Mon, Nov 09, 2020 at 10:05:35AM +0100, Arnaud POULIQUEN wrote:
> Hi Mathieu,
> 
> On 10/30/20 8:57 PM, Mathieu Poirier wrote:
> > Introduce function rproc_detach() to enable the remoteproc
> > core to release the resources associated with a remote processor
> > without stopping its operation.
> > 
> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > Reviewed-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/remoteproc/remoteproc_core.c | 65 +++++++++++++++++++++++++++-
> >  include/linux/remoteproc.h           |  1 +
> >  2 files changed, 65 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > index 62e88ff65009..6b33a83960d2 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -1667,7 +1667,7 @@ static int rproc_stop(struct rproc *rproc, bool crashed)
> >  /*
> >   * __rproc_detach(): Does the opposite of rproc_attach()
> >   */
> > -static int __maybe_unused __rproc_detach(struct rproc *rproc)
> > +static int __rproc_detach(struct rproc *rproc)
> >  {
> >  	struct device *dev = &rproc->dev;
> >  	int ret;
> > @@ -1910,6 +1910,69 @@ void rproc_shutdown(struct rproc *rproc)
> >  }
> >  EXPORT_SYMBOL(rproc_shutdown);
> >  
> > +/**
> > + * rproc_detach() - Detach the remote processor from the
> > + * remoteproc core
> > + *
> > + * @rproc: the remote processor
> > + *
> > + * Detach a remote processor (previously attached to with rproc_actuate()).
> > + *
> > + * In case @rproc is still being used by an additional user(s), then
> > + * this function will just decrement the power refcount and exit,
> > + * without disconnecting the device.
> > + *
> > + * Function rproc_detach() calls __rproc_detach() in order to let a remote
> > + * processor know that services provided by the application processor are
> > + * no longer available.  From there it should be possible to remove the
> > + * platform driver and even power cycle the application processor (if the HW
> > + * supports it) without needing to switch off the remote processor.
> > + */
> > +int rproc_detach(struct rproc *rproc)
> > +{
> > +	struct device *dev = &rproc->dev;
> > +	int ret;
> > +
> > +	ret = mutex_lock_interruptible(&rproc->lock);
> > +	if (ret) {
> > +		dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, ret);
> > +		return ret;
> > +	}
> > +
> > +	if (rproc->state != RPROC_RUNNING && rproc->state != RPROC_ATTACHED) {
> > +		ret = -EPERM;
> > +		goto out;
> > +	}
> > +
> > +	/* if the remote proc is still needed, bail out */
> > +	if (!atomic_dec_and_test(&rproc->power)) {
> > +		ret = -EBUSY;
> > +		goto out;
> > +	}
> > +
> > +	ret = __rproc_detach(rproc);
> > +	if (ret) {
> > +		atomic_inc(&rproc->power);
> > +		goto out;
> > +	}
> > +
> > +	/* clean up all acquired resources */
> > +	rproc_resource_cleanup(rproc);
> > +
> > +	rproc_disable_iommu(rproc);
> 
> I'm not an IOMMU expert,so maybe this remark is not relevant...
> As IOMMU manage the access of the remote device to its memory,
> could this lead to a remote device crash?

Right - that is the kind of thing that tormented me when I wrote this set.  On
some system it might cause a crash but on others it is needed.  As such I
decided to include it in the detach() process and let people implement the
functionality should they need to.  We can remove it but it is guaranteed that
someone will post a patch to add it again.

Based on the scenario it may have to be something like rproc_detach_iommu().
Since we have no clue on what the requirements will be I decided to simply keep
the current call.  Let me know if you really disagree with my approach.

> 
> Regards,
> Arnaud
> 
> > +
> > +	/*
> > +	 * Set the remote processor's table pointer to NULL.  Since mapping
> > +	 * of the resource table to a virtual address is done in the platform
> > +	 * driver, unmapping should also be done there.
> > +	 */
> > +	rproc->table_ptr = NULL;
> > +out:
> > +	mutex_unlock(&rproc->lock);
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL(rproc_detach);
> > +
> >  /**
> >   * rproc_get_by_phandle() - find a remote processor by phandle
> >   * @phandle: phandle to the rproc
> > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > index 3faff9bb4fb8..6713faab6959 100644
> > --- a/include/linux/remoteproc.h
> > +++ b/include/linux/remoteproc.h
> > @@ -656,6 +656,7 @@ rproc_of_resm_mem_entry_init(struct device *dev, u32 of_resm_idx, size_t len,
> >  
> >  int rproc_boot(struct rproc *rproc);
> >  void rproc_shutdown(struct rproc *rproc);
> > +int rproc_detach(struct rproc *rproc);
> >  void rproc_report_crash(struct rproc *rproc, enum rproc_crash_type type);
> >  int rproc_coredump_add_segment(struct rproc *rproc, dma_addr_t da, size_t size);
> >  int rproc_coredump_add_custom_segment(struct rproc *rproc,
> > 

Re: [PATCH v2 10/14] remoteproc: Add return value to function rproc_shutdown()

[Mathieu Poirier]

On Mon, Nov 09, 2020 at 10:14:22AM +0100, Arnaud POULIQUEN wrote:
> 
> 
> On 10/30/20 8:57 PM, Mathieu Poirier wrote:
> > Add a return value to function rproc_shutdown() in order to
> > properly deal with error conditions that may occur.
> > 
> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > Reviewed-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/remoteproc/remoteproc_core.c | 13 +++++++++----
> >  include/linux/remoteproc.h           |  2 +-
> >  2 files changed, 10 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > index de5a5720d1e8..f58475f6dcab 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -1869,7 +1869,7 @@ EXPORT_SYMBOL(rproc_boot);
> >   *   returns, and users can still use it with a subsequent rproc_boot(), if
> >   *   needed.
> >   */
> > -void rproc_shutdown(struct rproc *rproc)
> > +int rproc_shutdown(struct rproc *rproc)
> >  {
> >  	struct device *dev = &rproc->dev;
> >  	int ret;
> > @@ -1877,15 +1877,19 @@ void rproc_shutdown(struct rproc *rproc)
> >  	ret = mutex_lock_interruptible(&rproc->lock);
> >  	if (ret) {
> >  		dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, ret);
> > -		return;
> > +		return ret;
> >  	}
> >  
> > -	if (rproc->state != RPROC_RUNNING)
> > +	if (rproc->state != RPROC_RUNNING) {
> > +		ret = -EPERM;
> >  		goto out;
> > +	}
> >  
> >  	/* if the remote proc is still needed, bail out */
> > -	if (!atomic_dec_and_test(&rproc->power))
> > +	if (!atomic_dec_and_test(&rproc->power)) {
> > +		ret = -EBUSY;
> >  		goto out;
> > +	}
> >  
> >  	ret = rproc_stop(rproc, false);
> >  	if (ret) {
> 
> Few lines after in source code rproc_unprepare_device could also be tested

You are correct

> 
> Regards,
> Arnaud
> 
> > @@ -1907,6 +1911,7 @@ void rproc_shutdown(struct rproc *rproc)
> >  	rproc->table_ptr = NULL;
> >  out:
> >  	mutex_unlock(&rproc->lock);
> > +	return ret;
> >  }
> >  EXPORT_SYMBOL(rproc_shutdown);
> >  
> > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > index 6713faab6959..71d4d4873164 100644
> > --- a/include/linux/remoteproc.h
> > +++ b/include/linux/remoteproc.h
> > @@ -655,7 +655,7 @@ rproc_of_resm_mem_entry_init(struct device *dev, u32 of_resm_idx, size_t len,
> >  			     u32 da, const char *name, ...);
> >  
> >  int rproc_boot(struct rproc *rproc);
> > -void rproc_shutdown(struct rproc *rproc);
> > +int rproc_shutdown(struct rproc *rproc);
> >  int rproc_detach(struct rproc *rproc);
> >  void rproc_report_crash(struct rproc *rproc, enum rproc_crash_type type);
> >  int rproc_coredump_add_segment(struct rproc *rproc, dma_addr_t da, size_t size);
> > 

Re: [RFC v2 13/14] remoteproc: Add automation flags

[Mathieu Poirier]

On Mon, Nov 16, 2020 at 11:21:02AM +0100, Arnaud POULIQUEN wrote:
> Hi Mathieu,
> 
> On 11/13/20 10:27 PM, Mathieu Poirier wrote:
> > Hi,
> > 
> > On Thu, Nov 12, 2020 at 02:56:20PM +0100, Arnaud POULIQUEN wrote:
> >> Hi Mathieu,
> >>
> >> Thanks for initiating the discussion!
> >>
> >> Waiting feedback from other, please find my feedback on our proposal below.
> > 
> > The first version of this set has been released on August 26th and since then,
> > only you and Peng have given me feedback.  As such I suggest that we move
> > forward with the decision you and I settle on.  As usual with open source
> > development, people can submit new patches to enhance our solution as they see
> > fit.
> > 
> >>
> >> On 10/30/20 8:57 PM, Mathieu Poirier wrote:
> >>> Adding flags to dictate how to handle a platform driver being removed
> >>> or the remote processor crashing while in RPROC_ATTACHED state.
> >>>
> >>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> >>> ---
> >>>  drivers/remoteproc/remoteproc_core.c | 25 +++++++++++++++++++++++++
> >>>  include/linux/remoteproc.h           |  5 +++++
> >>>  2 files changed, 30 insertions(+)
> >>>
> >>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> >>> index 229fa2cad0bd..d024367c63e5 100644
> >>> --- a/drivers/remoteproc/remoteproc_core.c
> >>> +++ b/drivers/remoteproc/remoteproc_core.c
> >>> @@ -2227,6 +2227,29 @@ static int rproc_alloc_ops(struct rproc *rproc, const struct rproc_ops *ops)
> >>>  	return 0;
> >>>  }
> >>>  
> >>> +static void rproc_set_automation_flags(struct rproc *rproc)
> >>> +{
> >>> +	struct device *dev = rproc->dev.parent;
> >>> +	struct device_node *np = dev->of_node;
> >>> +	bool core_reboot, remote_crash;
> >>> +
> >>> +	/*
> >>> +	 * When function rproc_cdev_release() or rproc_del() are called and
> >>> +	 * the remote processor has been attached to, it will be detached from
> >>> +	 * (rather than turned off) if "autonomous_on_core_reboot" is specified
> >>> +	 * in the DT.
> >>> +	 */
> >>> +	core_reboot = of_property_read_bool(np, "autonomous_on_core_reboot");
> >>> +	rproc->autonomous_on_core_reboot = core_reboot;
> >>> +
> >>> +	/*
> >>> +	 * When the remote processor crashes it will be detached from, and
> >>> +	 * attached to, if "autonomous_on_remote_crash" is specified in the DT.
> >>> +	 */
> >>> +	remote_crash = of_property_read_bool(np, "autonomous_on_remote_crash");
> >>> +	rproc->autonomous_on_core_reboot = core_reboot;
> >>> +}
> >>> +
> >>
> >> I wonder if the naming is not too restrictive.
> > 
> > I'm happy to have this conversation, which is really the point of this second
> > revision.  I turned names and ideas around in my head for a long time and the above is
> > the best I came up with.  Your insight gave me food for thought - see below.
> 
> 
> > 
> >>
> >> I think here we probably need first to identify the use cases we want to support
> >> to determine which use cases should be addressed and deduce DT fields.
> >>
> >> Please find my view below:
> >>
> >> 1) Attach to a remote processor on boot.
> >> This is the "attach" you introduced in a previous series. I wonder here if a DT
> >> field should not be introduce for platform which are not able to dynamically
> >> determines the remote processor state. Something like "remote-boot-on" or
> >> "autonomous-boot-on".
> > 
> > Right - I think "autonomous-on-core-boot" would be best as it really spells out
> > what is going on. I did not include it in the "attach" patchset because there
> > wasn't a need for it.  Both ST and NXP are able to determine the state of the
> > remote processor from a platform driver.  My initial strategy was to introduce
> > the functionality when the need for it comes up.  I can revisit if you feel
> > strongly about adding it immediately. 
> 
> No problem to add it later. I mentioned it here only trying to have a complete
> view of the use cases.
> 
> > 
> >>
> >> 2) Detach from a remote processor on Linux kernel shutdown
> >> Two possible actions: shutdown the remote processor or detach from it.
> >> A DT field could be used to determine the expected behavior.
> >>
> > 
> > That is what the "autonomous-on-core-reboot" was for but reading your
> > description I think "autonomous-on-core-shutdown" is best to describe the
> > scenario.
> >  
> >> 3) Linux core reboot on crash
> >> Two possible actions: shutdown and restart the remote processor or
> >> detach/re-attach from/to it.
> >> Is same DT field than 2) can be used for this . Or should be determine by a
> >> new sysfs recovery option [1]?
> > 
> > As far as I can tell nothing happens to drivers when the kernel crashes.  To
> > take action when the kernel crashes each driver needs to register explicitly
> > with the panic notifier, which the remoteproc doesn't currently do.  That's a
> > different feature that I would like to delay for another time.
> > 
> > If and when that time comes we can either reuse "autonomous-on-core-shutdown"
> > or introduce "autonomous-on-core-crash", depending on the level of granularity
> > needed.
> 
> Right, i don't know if it possible to determine boot reason on remoteproc probe.
> (first boot or re-boot after crash). This information will be needed to
> determine if the firmware has to be re-attached or re-started.
> 
> > 
> >>
> >> [1]
> >> https://elixir.bootlin.com/linux/v5.10-rc2/source/drivers/remoteproc/remoteproc_sysfs.c#L45
> >>
> >> 4) The remote processor need to reboot on crash.
> >> 3 possible actions:
> >>  - shutdown and restart the remote processor
> > 
> > That is currently the default behavior _if_ recovery is enabled.
> > 
> >>  - detach and re-attach from/to it.
> > 
> > That is how I intend to use "autonomous-on-remote-crash", _if_ recovery is
> > enabled.
> > 
> >>  - Just shutdown, as no recovery possible without a system reset.
> > 
> > That is the current behavior if recovery is _not_ enabled.
> > 
> 
> The recovery is a user interface, for my pov the recovery sysfs/debugfs is used
> to allow userland to chose behavior on crash
> - automatic recovery
> - asynchronous recovery ( recovery trigged by the userspace)
> - no recovery
> 
> As the userspace should be hardware independent, the platform driver should
> provide information on the capabilities I list above.
> That's why i proposed to not have a boolean but an enum to handle the crash
> 
> > Dealing with crash scenarios is a little more complex and requires some
> > refactoring.  That is why I wanted to solely concentrate on the shutdown scenario
> > in this set.
> 
> Yes, my feedback was mainly to determine if the properties allow to cover the
> use cases, to avoid to refactor them later. let start with the shutdown.
> 
> > 
> >>
> >> 5) Detach/re-attach on Linux suspend/resume
> >> Perhaps better to manage this in platform drivers without a generic DT field?
> > 
> > I think that falls in the same category as power management and is too specific
> > to be handled in the remoteproc core.  As you suggest, it is probably best to
> > leave that to platform drivers for the time being. 
> > 
> >>
> >> If i try to apply this on the remote proc boot and shutdown sequences:
> >>
> >> 1) on remoteproc device add:
> >> - Need to determine if the remote processor is already running:
> >>    - started by another entity
> >>    - Linux reboot after crash
> >>
> >> 2) On remoteproc device release.
> >> - Need to determine if the remote processor need to be shutdown or detached:
> >>    - Linux kernel crash
> >>    - Linux kernel graceful shutdown with remote processor keeping ON.
> >>
> >> 3) On remote processor crash
> >> - Need to determine if the remote processor will be restarted by an external
> >> entity or by the remoteproc framework, or if simply not possible to recover
> >> without a system reset.
> >>
> >> Regarding these use cases here is an alternative proposal(inspired by regulator
> >> framework):
> >> - "remote-boot-on": determine on probe if the remoteproc firmware is already
> >> booted. This field is optional, use by a platform driver which can not
> >> determine the state of the remote processor. Could be dynamically updated by the
> >> platform driver to manage Kernel crash...
> >>
> >> - "remote-always-on": means that the detach has to be privileged on
> >> shutdown. Need also to be managed by platform driver as it can be
> >> compared to the remote processor current state.
> >>
> >> - "remoteproc-crash-recovery": crash recovery mode:
> >>    possible value: "SHUTDOWN", "DETACH", "DISABLED"
> > 
> > I think all of the above scenarios can be managed with a combination of the
> > proposed bindings , i.e "autonomous-on-core-shutdown" and
> > "autonomous-on-remote-crash".  The latter would be used in conjuction with the
> > recovery mechanic already available. 
> > 
> > Let me know what you think.
> 
> "autonomous-on-core-shutdown" is fine and sufficient for this series. So as you
> suggest let's start with that.

Ok - I'll spin off another revision with this binding and a proper yaml file.

> 
> As explained above for the "autonomous-on-remote-crash" property i would prefer
> an enum to also be able to prohibit the recovering as well, especially if the
> recovering is migrating to the sysfs. let's discuss this in a second step.
> 

I understand where you are coming from.  The problem with an enumeration is that
it conflicts with rproc::recovery, which still has to be taken into account.

Platform drivers can access and set rproc::recovery so any specific case can be
handled there.  And with regards to sysfs, changing the setting requires root
access.  Someone with this kind of previleges should know what they are
doing - if they don't then they'll learn very quickly.

Have a good weekend,
Mathieu

> Thanks,
> Arnaud
> 
> > 
> > Mathieu
> > 
> >>
> >>
> >> Regards,
> >> Arnaud
> >>
> >>>  /**
> >>>   * rproc_alloc() - allocate a remote processor handle
> >>>   * @dev: the underlying device
> >>> @@ -2285,6 +2308,8 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
> >>>  	if (rproc_alloc_ops(rproc, ops))
> >>>  		goto put_device;
> >>>  
> >>> +	rproc_set_automation_flags(rproc);
> >>> +
> >>>  	/* Assign a unique device index and name */
> >>>  	rproc->index = ida_simple_get(&rproc_dev_index, 0, 0, GFP_KERNEL);
> >>>  	if (rproc->index < 0) {
> >>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> >>> index 71d4d4873164..9a6e79ef35d7 100644
> >>> --- a/include/linux/remoteproc.h
> >>> +++ b/include/linux/remoteproc.h
> >>> @@ -516,6 +516,9 @@ struct rproc_dump_segment {
> >>>   * @nb_vdev: number of vdev currently handled by rproc
> >>>   * @char_dev: character device of the rproc
> >>>   * @cdev_put_on_release: flag to indicate if remoteproc should be shutdown on @char_dev release
> >>> + * @autonomous_on_core_reboot: true if the remote processor should be detached from
> >>> + *			       (rather than turned off) when the remoteproc core
> >>> + *			       goes away.
> >>>   */
> >>>  struct rproc {
> >>>  	struct list_head node;
> >>> @@ -554,6 +557,8 @@ struct rproc {
> >>>  	u16 elf_machine;
> >>>  	struct cdev cdev;
> >>>  	bool cdev_put_on_release;
> >>> +	bool autonomous_on_core_reboot	: 1,
> >>> +	     autonomous_on_remote_crash	: 1;
> >>>  };
> >>>  
> >>>  /**
> >>>