Linux-remoteproc Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 00/13] remoteproc: Add support for detaching from rproc
@ 2020-08-26 16:45 Mathieu Poirier
  2020-08-26 16:45 ` [PATCH 01/13] remoteproc: Re-check state in rproc_shutdown() Mathieu Poirier
                   ` (13 more replies)
  0 siblings, 14 replies; 31+ messages in thread
From: Mathieu Poirier @ 2020-08-26 16:45 UTC (permalink / raw)
  To: bjorn.andersson, ohad; +Cc: linux-remoteproc, linux-kernel

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 have not tested the solution because of the work involved in getting
a new firmware image to support the feature.  I will do so once it is
determined that this is the right approach to follow.

Applies cleanly on rproc-next (62b8f9e99329)

Thanks,
Mathieu 

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

Mathieu Poirier (13):
  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: 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: Refactor rproc delete and cdev release path
  remoteproc: Properly deal with a kernel panic when attached

 drivers/remoteproc/remoteproc_cdev.c  |  18 ++-
 drivers/remoteproc/remoteproc_core.c  | 151 +++++++++++++++++++++-----
 drivers/remoteproc/remoteproc_sysfs.c |  17 ++-
 include/linux/remoteproc.h            |  14 ++-
 4 files changed, 157 insertions(+), 43 deletions(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH 01/13] remoteproc: Re-check state in rproc_shutdown()
  2020-08-26 16:45 [PATCH 00/13] remoteproc: Add support for detaching from rproc Mathieu Poirier
@ 2020-08-26 16:45 ` Mathieu Poirier
  2020-10-15  1:28   ` Peng Fan
  2020-08-26 16:45 ` [PATCH 02/13] remoteproc: Remove useless check in rproc_del() Mathieu Poirier
                   ` (12 subsequent siblings)
  13 siblings, 1 reply; 31+ messages in thread
From: Mathieu Poirier @ 2020-08-26 16:45 UTC (permalink / raw)
  To: bjorn.andersson, ohad; +Cc: linux-remoteproc, linux-kernel

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>
---
 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 7f90eeea67e2..fb2632cbd2df 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1834,6 +1834,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


^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH 02/13] remoteproc: Remove useless check in rproc_del()
  2020-08-26 16:45 [PATCH 00/13] remoteproc: Add support for detaching from rproc Mathieu Poirier
  2020-08-26 16:45 ` [PATCH 01/13] remoteproc: Re-check state in rproc_shutdown() Mathieu Poirier
@ 2020-08-26 16:45 ` Mathieu Poirier
  2020-10-15  1:29   ` Peng Fan
  2020-08-26 16:45 ` [PATCH 03/13] remoteproc: Add new RPROC_ATTACHED state Mathieu Poirier
                   ` (11 subsequent siblings)
  13 siblings, 1 reply; 31+ messages in thread
From: Mathieu Poirier @ 2020-08-26 16:45 UTC (permalink / raw)
  To: bjorn.andersson, ohad; +Cc: linux-remoteproc, linux-kernel

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>
---
 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 fb2632cbd2df..7d78c9a9d88f 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -2260,10 +2260,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


^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH 03/13] remoteproc: Add new RPROC_ATTACHED state
  2020-08-26 16:45 [PATCH 00/13] remoteproc: Add support for detaching from rproc Mathieu Poirier
  2020-08-26 16:45 ` [PATCH 01/13] remoteproc: Re-check state in rproc_shutdown() Mathieu Poirier
  2020-08-26 16:45 ` [PATCH 02/13] remoteproc: Remove useless check in rproc_del() Mathieu Poirier
@ 2020-08-26 16:45 ` Mathieu Poirier
  2020-10-15  1:31   ` Peng Fan
  2020-08-26 16:45 ` [PATCH 04/13] remoteproc: Properly represent the attached state Mathieu Poirier
                   ` (10 subsequent siblings)
  13 siblings, 1 reply; 31+ messages in thread
From: Mathieu Poirier @ 2020-08-26 16:45 UTC (permalink / raw)
  To: bjorn.andersson, ohad; +Cc: linux-remoteproc, linux-kernel

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>
---
 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 eea514cec50e..2d575e6c9eb8 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -84,6 +84,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 2fa68bf5aa4f..4e107615121a 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


^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH 04/13] remoteproc: Properly represent the attached state
  2020-08-26 16:45 [PATCH 00/13] remoteproc: Add support for detaching from rproc Mathieu Poirier
                   ` (2 preceding siblings ...)
  2020-08-26 16:45 ` [PATCH 03/13] remoteproc: Add new RPROC_ATTACHED state Mathieu Poirier
@ 2020-08-26 16:45 ` Mathieu Poirier
  2020-10-15  1:33   ` Peng Fan
  2020-08-26 16:45 ` [PATCH 05/13] remoteproc: Add new detach() remoteproc operation Mathieu Poirier
                   ` (9 subsequent siblings)
  13 siblings, 1 reply; 31+ messages in thread
From: Mathieu Poirier @ 2020-08-26 16:45 UTC (permalink / raw)
  To: bjorn.andersson, ohad; +Cc: linux-remoteproc, linux-kernel

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>
---
 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 7d78c9a9d88f..bffaa9ea7c8f 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1421,7 +1421,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);
 
@@ -1636,14 +1636,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;
@@ -1994,16 +1986,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 2d575e6c9eb8..c152d11a4d3c 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -21,11 +21,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 4e107615121a..fe383392a821 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


^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH 05/13] remoteproc: Add new detach() remoteproc operation
  2020-08-26 16:45 [PATCH 00/13] remoteproc: Add support for detaching from rproc Mathieu Poirier
                   ` (3 preceding siblings ...)
  2020-08-26 16:45 ` [PATCH 04/13] remoteproc: Properly represent the attached state Mathieu Poirier
@ 2020-08-26 16:45 ` Mathieu Poirier
  2020-10-15  1:37   ` Peng Fan
  2020-08-26 16:45 ` [PATCH 06/13] remoteproc: Introduce function __rproc_detach() Mathieu Poirier
                   ` (8 subsequent siblings)
  13 siblings, 1 reply; 31+ messages in thread
From: Mathieu Poirier @ 2020-08-26 16:45 UTC (permalink / raw)
  To: bjorn.andersson, ohad; +Cc: linux-remoteproc, linux-kernel

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>
---
 include/linux/remoteproc.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index fe383392a821..1a57e165da2c 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


^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH 06/13] remoteproc: Introduce function __rproc_detach()
  2020-08-26 16:45 [PATCH 00/13] remoteproc: Add support for detaching from rproc Mathieu Poirier
                   ` (4 preceding siblings ...)
  2020-08-26 16:45 ` [PATCH 05/13] remoteproc: Add new detach() remoteproc operation Mathieu Poirier
@ 2020-08-26 16:45 ` Mathieu Poirier
  2020-10-15  1:39   ` Peng Fan
  2020-08-26 16:45 ` [PATCH 07/13] remoteproc: Introduce function rproc_detach() Mathieu Poirier
                   ` (7 subsequent siblings)
  13 siblings, 1 reply; 31+ messages in thread
From: Mathieu Poirier @ 2020-08-26 16:45 UTC (permalink / raw)
  To: bjorn.andersson, ohad; +Cc: linux-remoteproc, linux-kernel

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>
---
 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 bffaa9ea7c8f..7a1fc7e0620f 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1641,6 +1641,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


^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH 07/13] remoteproc: Introduce function rproc_detach()
  2020-08-26 16:45 [PATCH 00/13] remoteproc: Add support for detaching from rproc Mathieu Poirier
                   ` (5 preceding siblings ...)
  2020-08-26 16:45 ` [PATCH 06/13] remoteproc: Introduce function __rproc_detach() Mathieu Poirier
@ 2020-08-26 16:45 ` Mathieu Poirier
  2020-10-15  1:52   ` Peng Fan
  2020-08-26 16:45 ` [PATCH 08/13] remoteproc: Rename function rproc_actuate() Mathieu Poirier
                   ` (6 subsequent siblings)
  13 siblings, 1 reply; 31+ messages in thread
From: Mathieu Poirier @ 2020-08-26 16:45 UTC (permalink / raw)
  To: bjorn.andersson, ohad; +Cc: linux-remoteproc, linux-kernel

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>
---
 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 7a1fc7e0620f..f3943a1e2754 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1644,7 +1644,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;
@@ -1887,6 +1887,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 1a57e165da2c..6250491ee851 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


^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH 08/13] remoteproc: Rename function rproc_actuate()
  2020-08-26 16:45 [PATCH 00/13] remoteproc: Add support for detaching from rproc Mathieu Poirier
                   ` (6 preceding siblings ...)
  2020-08-26 16:45 ` [PATCH 07/13] remoteproc: Introduce function rproc_detach() Mathieu Poirier
@ 2020-08-26 16:45 ` Mathieu Poirier
  2020-10-15  2:15   ` Peng Fan
  2020-08-26 16:45 ` [PATCH 09/13] remoteproc: Add return value to function rproc_shutdown() Mathieu Poirier
                   ` (5 subsequent siblings)
  13 siblings, 1 reply; 31+ messages in thread
From: Mathieu Poirier @ 2020-08-26 16:45 UTC (permalink / raw)
  To: bjorn.andersson, ohad; +Cc: linux-remoteproc, linux-kernel

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>
---
 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 f3943a1e2754..c4b80ce6f22d 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1393,7 +1393,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;
@@ -1518,7 +1518,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;
@@ -1558,7 +1558,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;
 
@@ -1802,7 +1802,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);
 
@@ -1893,7 +1893,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


^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH 09/13] remoteproc: Add return value to function rproc_shutdown()
  2020-08-26 16:45 [PATCH 00/13] remoteproc: Add support for detaching from rproc Mathieu Poirier
                   ` (7 preceding siblings ...)
  2020-08-26 16:45 ` [PATCH 08/13] remoteproc: Rename function rproc_actuate() Mathieu Poirier
@ 2020-08-26 16:45 ` Mathieu Poirier
  2020-10-15  2:21   ` Peng Fan
  2020-08-26 16:45 ` [PATCH 10/13] remoteproc: Properly deal with a stop request when attached Mathieu Poirier
                   ` (4 subsequent siblings)
  13 siblings, 1 reply; 31+ messages in thread
From: Mathieu Poirier @ 2020-08-26 16:45 UTC (permalink / raw)
  To: bjorn.andersson, ohad; +Cc: linux-remoteproc, linux-kernel

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>
---
 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 c4b80ce6f22d..c6c6aba66098 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1846,7 +1846,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;
@@ -1854,15 +1854,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) {
@@ -1884,6 +1888,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 6250491ee851..40eccfbc1357 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


^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH 10/13] remoteproc: Properly deal with a stop request when attached
  2020-08-26 16:45 [PATCH 00/13] remoteproc: Add support for detaching from rproc Mathieu Poirier
                   ` (8 preceding siblings ...)
  2020-08-26 16:45 ` [PATCH 09/13] remoteproc: Add return value to function rproc_shutdown() Mathieu Poirier
@ 2020-08-26 16:45 ` Mathieu Poirier
  2020-10-15  2:29   ` Peng Fan
  2020-08-26 16:45 ` [PATCH 11/13] remoteproc: Properly deal with detach request Mathieu Poirier
                   ` (3 subsequent siblings)
  13 siblings, 1 reply; 31+ messages in thread
From: Mathieu Poirier @ 2020-08-26 16:45 UTC (permalink / raw)
  To: bjorn.andersson, ohad; +Cc: linux-remoteproc, linux-kernel

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>
---
 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 c6c6aba66098..95bb40b4ebb3 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1619,6 +1619,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);
 
@@ -1857,7 +1861,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 c152d11a4d3c..6134d2f083ce 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -113,10 +113,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


^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH 11/13] remoteproc: Properly deal with detach request
  2020-08-26 16:45 [PATCH 00/13] remoteproc: Add support for detaching from rproc Mathieu Poirier
                   ` (9 preceding siblings ...)
  2020-08-26 16:45 ` [PATCH 10/13] remoteproc: Properly deal with a stop request when attached Mathieu Poirier
@ 2020-08-26 16:45 ` Mathieu Poirier
  2020-10-15  2:30   ` Peng Fan
  2020-08-26 16:45 ` [PATCH 12/13] remoteproc: Refactor rproc delete and cdev release path Mathieu Poirier
                   ` (2 subsequent siblings)
  13 siblings, 1 reply; 31+ messages in thread
From: Mathieu Poirier @ 2020-08-26 16:45 UTC (permalink / raw)
  To: bjorn.andersson, ohad; +Cc: linux-remoteproc, linux-kernel

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>
---
 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 6134d2f083ce..013231f69837 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -118,6 +118,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


^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH 12/13] remoteproc: Refactor rproc delete and cdev release path
  2020-08-26 16:45 [PATCH 00/13] remoteproc: Add support for detaching from rproc Mathieu Poirier
                   ` (10 preceding siblings ...)
  2020-08-26 16:45 ` [PATCH 11/13] remoteproc: Properly deal with detach request Mathieu Poirier
@ 2020-08-26 16:45 ` Mathieu Poirier
  2020-10-15  2:31   ` Peng Fan
  2020-08-26 16:45 ` [PATCH 13/13] remoteproc: Properly deal with a kernel panic when attached Mathieu Poirier
  2020-09-01 16:55 ` [PATCH 00/13] remoteproc: Add support for detaching from rproc Arnaud POULIQUEN
  13 siblings, 1 reply; 31+ messages in thread
From: Mathieu Poirier @ 2020-08-26 16:45 UTC (permalink / raw)
  To: bjorn.andersson, ohad; +Cc: linux-remoteproc, linux-kernel

Refactor function rproc_del() and rproc_cdev_release() to take
into account scenarios where the remote processor has been
attached to.  If the remote processor has been started by the
remoteproc core then switch it off, and if it was attached to
detach from it. This heuristic is simple and can be enhanced
easily if there is a need to.

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

diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
index 3a3830e27050..18cffbe588c1 100644
--- a/drivers/remoteproc/remoteproc_cdev.c
+++ b/drivers/remoteproc/remoteproc_cdev.c
@@ -87,8 +87,13 @@ 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;
+
+	if (rproc->state == RPROC_RUNNING)
 		rproc_shutdown(rproc);
+	else if (rproc->state == RPROC_ATTACHED)
+		rproc_detach(rproc);
 
 	return 0;
 }
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 95bb40b4ebb3..5586582f54c5 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -2346,7 +2346,10 @@ int rproc_del(struct rproc *rproc)
 		return -EINVAL;
 
 	/* TODO: make sure this works with rproc->power > 1 */
-	rproc_shutdown(rproc);
+	if (rproc->state == RPROC_RUNNING)
+		rproc_shutdown(rproc);
+	else if (rproc->state == RPROC_ATTACHED)
+		rproc_detach(rproc);
 
 	mutex_lock(&rproc->lock);
 	rproc->state = RPROC_DELETED;
-- 
2.25.1


^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH 13/13] remoteproc: Properly deal with a kernel panic when attached
  2020-08-26 16:45 [PATCH 00/13] remoteproc: Add support for detaching from rproc Mathieu Poirier
                   ` (11 preceding siblings ...)
  2020-08-26 16:45 ` [PATCH 12/13] remoteproc: Refactor rproc delete and cdev release path Mathieu Poirier
@ 2020-08-26 16:45 ` Mathieu Poirier
  2020-10-15  2:28   ` Peng Fan
  2020-09-01 16:55 ` [PATCH 00/13] remoteproc: Add support for detaching from rproc Arnaud POULIQUEN
  13 siblings, 1 reply; 31+ messages in thread
From: Mathieu Poirier @ 2020-08-26 16:45 UTC (permalink / raw)
  To: bjorn.andersson, ohad; +Cc: linux-remoteproc, linux-kernel

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>
---
 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 5586582f54c5..54b5e3437ab5 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -2491,7 +2491,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


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 00/13] remoteproc: Add support for detaching from rproc
  2020-08-26 16:45 [PATCH 00/13] remoteproc: Add support for detaching from rproc Mathieu Poirier
                   ` (12 preceding siblings ...)
  2020-08-26 16:45 ` [PATCH 13/13] remoteproc: Properly deal with a kernel panic when attached Mathieu Poirier
@ 2020-09-01 16:55 ` Arnaud POULIQUEN
  2020-09-01 18:02   ` Mathieu Poirier
  13 siblings, 1 reply; 31+ messages in thread
From: Arnaud POULIQUEN @ 2020-09-01 16:55 UTC (permalink / raw)
  To: Mathieu Poirier, bjorn.andersson, ohad; +Cc: linux-remoteproc, linux-kernel

Hi Mathieu,

On 8/26/20 6:45 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 have not tested the solution because of the work involved in getting
> a new firmware image to support the feature.  I will do so once it is
> determined that this is the right approach to follow.

I just started watching your series. I also think that we have first to
determine the approach that match meets the requirements of all companies.

Here is my feeling, waiting more feedback from community:

If I understand your approach correctly you propose that the application
determines the firmware live-cycle. Depending on request, the remoteproc core
performs a "shutdown" ( stop + unprepare) or a "detach".
The platform driver can(or have to?) implement the stop and/or the detach.
By default a preloaded firmware is detached and a "standard" firmware is stopped
on kernel shutdown (rproc_del).

As we have seen with the rproc cdev, it might not be simple to manage this
in case of crash.
For instance you can have a Firmware started by the boot-stages but
which must be gracefully stopped in case of crash.  

Another approach would be to let the platform driver decides what should
be done on the stop and prepare ops depending on the HW context.
So the platform driver would be in charge of detaching the firmware.
In this case the issue is to determine the state after stop. the information
would be in platform driver.

I would be more in flavor of the second one, because application would not
have to be aware of the co-processor firmware life-cycle, and the firmware
could expose its own constraints for shutdown.  

A third approach (or complementary approach): 
I don't know why i didn't think of it before... The attach/detach
feature is quite similar to the regulator management.

For regulator 2 DT properties exist[1]:
- regulator-always-on: boolean, regulator should never be disabled
- regulator-boot-on: bootloader/firmware enabled regulator

It is a static configuration but could be implemented for both the attach and
the detach in the core part.
Else if a more dynamic management could be managed by the platform driver
(depending on the loaded firmware).
  
[1]https://elixir.bootlin.com/linux/v4.0/source/Documentation/devicetree/bindings/regulator/regulator.txt

Thanks,
Arnaud

> 
> Applies cleanly on rproc-next (62b8f9e99329)
> 
> Thanks,
> Mathieu 
> 
> [1]. https://lkml.org/lkml/2020/7/14/1600
> 
> Mathieu Poirier (13):
>   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: 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: Refactor rproc delete and cdev release path
>   remoteproc: Properly deal with a kernel panic when attached
> 
>  drivers/remoteproc/remoteproc_cdev.c  |  18 ++-
>  drivers/remoteproc/remoteproc_core.c  | 151 +++++++++++++++++++++-----
>  drivers/remoteproc/remoteproc_sysfs.c |  17 ++-
>  include/linux/remoteproc.h            |  14 ++-
>  4 files changed, 157 insertions(+), 43 deletions(-)
> 

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 00/13] remoteproc: Add support for detaching from rproc
  2020-09-01 16:55 ` [PATCH 00/13] remoteproc: Add support for detaching from rproc Arnaud POULIQUEN
@ 2020-09-01 18:02   ` Mathieu Poirier
  0 siblings, 0 replies; 31+ messages in thread
From: Mathieu Poirier @ 2020-09-01 18:02 UTC (permalink / raw)
  To: Arnaud POULIQUEN; +Cc: bjorn.andersson, ohad, linux-remoteproc, linux-kernel

On Tue, Sep 01, 2020 at 06:55:14PM +0200, Arnaud POULIQUEN wrote:
> Hi Mathieu,
> 
> On 8/26/20 6:45 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 have not tested the solution because of the work involved in getting
> > a new firmware image to support the feature.  I will do so once it is
> > determined that this is the right approach to follow.
> 
> I just started watching your series. I also think that we have first to
> determine the approach that match meets the requirements of all companies.
> 
> Here is my feeling, waiting more feedback from community:
> 
> If I understand your approach correctly you propose that the application
> determines the firmware live-cycle. Depending on request, the remoteproc core
> performs a "shutdown" ( stop + unprepare) or a "detach".

To be formal, /sys/class/remoteproc/remoteprocX/state takes a "stop" operation
and a newly added "detach" operation.

> The platform driver can(or have to?) implement the stop and/or the detach.

That is entirely up to the platform driver and the requirements of the system.
That is why rproc_shutdown() and rproc_detach() return an error code to be
conveyed to the sysfs mechanic.


> By default a preloaded firmware is detached and a "standard" firmware is stopped
> on kernel shutdown (rproc_del).

That is correct.  It is the simplest heuristic that I could come up with,
leaving opportunities to make enhancement as we see fit.

> 
> As we have seen with the rproc cdev, it might not be simple to manage this
> in case of crash.
> For instance you can have a Firmware started by the boot-stages but
> which must be gracefully stopped in case of crash.  
>

That is why I wanted to leave crash scenarios out of this set.  What happens
when the system crashes will follow the same heuristic we decide to enact in
rproc_del().
 
> Another approach would be to let the platform driver decides what should
> be done on the stop and prepare ops depending on the HW context.
> So the platform driver would be in charge of detaching the firmware.
> In this case the issue is to determine the state after stop. the information
> would be in platform driver.
> 

Yes, that is one way to proceed.  The downside of this approach is that platform
drivers would be responsible for setting rproc->state.  I am not totally opposed
to proceed this way but would like to explore other avenues before committing to
that solution.

On that note, did we talk about using the DT before?  That would be very easy,
simple and flexible.  Anyways, deciding what to do will take time hence keeping
this set to an absolute minimum.

> I would be more in flavor of the second one, because application would not
> have to be aware of the co-processor firmware life-cycle, and the firmware
> could expose its own constraints for shutdown.  
> 
> A third approach (or complementary approach): 
> I don't know why i didn't think of it before... The attach/detach
> feature is quite similar to the regulator management.
> 
> For regulator 2 DT properties exist[1]:
> - regulator-always-on: boolean, regulator should never be disabled
> - regulator-boot-on: bootloader/firmware enabled regulator
> 
> It is a static configuration but could be implemented for both the attach and
> the detach in the core part.

Yes, the device tree is a definite possibility.

Thanks for the input,
Mathieu

> Else if a more dynamic management could be managed by the platform driver
> (depending on the loaded firmware).
>   
> [1]https://elixir.bootlin.com/linux/v4.0/source/Documentation/devicetree/bindings/regulator/regulator.txt
> 
> Thanks,
> Arnaud
> 
> > 
> > Applies cleanly on rproc-next (62b8f9e99329)
> > 
> > Thanks,
> > Mathieu 
> > 
> > [1]. https://lkml.org/lkml/2020/7/14/1600
> > 
> > Mathieu Poirier (13):
> >   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: 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: Refactor rproc delete and cdev release path
> >   remoteproc: Properly deal with a kernel panic when attached
> > 
> >  drivers/remoteproc/remoteproc_cdev.c  |  18 ++-
> >  drivers/remoteproc/remoteproc_core.c  | 151 +++++++++++++++++++++-----
> >  drivers/remoteproc/remoteproc_sysfs.c |  17 ++-
> >  include/linux/remoteproc.h            |  14 ++-
> >  4 files changed, 157 insertions(+), 43 deletions(-)
> > 

^ permalink raw reply	[flat|nested] 31+ messages in thread

* RE: [PATCH 01/13] remoteproc: Re-check state in rproc_shutdown()
  2020-08-26 16:45 ` [PATCH 01/13] remoteproc: Re-check state in rproc_shutdown() Mathieu Poirier
@ 2020-10-15  1:28   ` Peng Fan
  0 siblings, 0 replies; 31+ messages in thread
From: Peng Fan @ 2020-10-15  1:28 UTC (permalink / raw)
  To: Mathieu Poirier, bjorn.andersson, ohad; +Cc: linux-remoteproc, linux-kernel

> Subject: [PATCH 01/13] remoteproc: Re-check state in rproc_shutdown()
> 
> 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>
> ---
>  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 7f90eeea67e2..fb2632cbd2df 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1834,6 +1834,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;
> --

Reviewed-by: Peng Fan <peng.fan@nxp.com>

^ permalink raw reply	[flat|nested] 31+ messages in thread

* RE: [PATCH 02/13] remoteproc: Remove useless check in rproc_del()
  2020-08-26 16:45 ` [PATCH 02/13] remoteproc: Remove useless check in rproc_del() Mathieu Poirier
@ 2020-10-15  1:29   ` Peng Fan
  0 siblings, 0 replies; 31+ messages in thread
From: Peng Fan @ 2020-10-15  1:29 UTC (permalink / raw)
  To: Mathieu Poirier, bjorn.andersson, ohad; +Cc: linux-remoteproc, linux-kernel

> Subject: [PATCH 02/13] remoteproc: Remove useless check in rproc_del()
> 
> 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>
> ---
>  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 fb2632cbd2df..7d78c9a9d88f 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -2260,10 +2260,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;
> --
Reviewed-by: Peng Fan <peng.fan@nxp.com>


^ permalink raw reply	[flat|nested] 31+ messages in thread

* RE: [PATCH 03/13] remoteproc: Add new RPROC_ATTACHED state
  2020-08-26 16:45 ` [PATCH 03/13] remoteproc: Add new RPROC_ATTACHED state Mathieu Poirier
@ 2020-10-15  1:31   ` Peng Fan
  0 siblings, 0 replies; 31+ messages in thread
From: Peng Fan @ 2020-10-15  1:31 UTC (permalink / raw)
  To: Mathieu Poirier, bjorn.andersson, ohad; +Cc: linux-remoteproc, linux-kernel

> Subject: [PATCH 03/13] remoteproc: Add new RPROC_ATTACHED state
> 
> 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>
> ---
>  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 eea514cec50e..2d575e6c9eb8 100644
> --- a/drivers/remoteproc/remoteproc_sysfs.c
> +++ b/drivers/remoteproc/remoteproc_sysfs.c
> @@ -84,6 +84,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
> 2fa68bf5aa4f..4e107615121a 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,
>  };
> 
>  /**
> --

Reviewed-by: Peng Fan <peng.fan@nxp.com>

^ permalink raw reply	[flat|nested] 31+ messages in thread

* RE: [PATCH 04/13] remoteproc: Properly represent the attached state
  2020-08-26 16:45 ` [PATCH 04/13] remoteproc: Properly represent the attached state Mathieu Poirier
@ 2020-10-15  1:33   ` Peng Fan
  0 siblings, 0 replies; 31+ messages in thread
From: Peng Fan @ 2020-10-15  1:33 UTC (permalink / raw)
  To: Mathieu Poirier, bjorn.andersson, ohad; +Cc: linux-remoteproc, linux-kernel

> Subject: [PATCH 04/13] remoteproc: Properly represent the attached state
> 
> 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>
> ---
>  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 7d78c9a9d88f..bffaa9ea7c8f 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1421,7 +1421,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);
> 
> @@ -1636,14 +1636,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;
> @@ -1994,16 +1986,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 2d575e6c9eb8..c152d11a4d3c 100644
> --- a/drivers/remoteproc/remoteproc_sysfs.c
> +++ b/drivers/remoteproc/remoteproc_sysfs.c
> @@ -21,11 +21,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
> 4e107615121a..fe383392a821 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;
> --

Looks good.

Reviewed-by: Peng Fan <peng.fan@nxp.com>

^ permalink raw reply	[flat|nested] 31+ messages in thread

* RE: [PATCH 05/13] remoteproc: Add new detach() remoteproc operation
  2020-08-26 16:45 ` [PATCH 05/13] remoteproc: Add new detach() remoteproc operation Mathieu Poirier
@ 2020-10-15  1:37   ` Peng Fan
  2020-10-22 21:51     ` Mathieu Poirier
  0 siblings, 1 reply; 31+ messages in thread
From: Peng Fan @ 2020-10-15  1:37 UTC (permalink / raw)
  To: Mathieu Poirier, bjorn.andersson, ohad; +Cc: linux-remoteproc, linux-kernel

> Subject: [PATCH 05/13] remoteproc: Add new detach() remoteproc operation
> 
> 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>
> ---
>  include/linux/remoteproc.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h index
> fe383392a821..1a57e165da2c 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);
> --

This look useful for i.MX8 partitioned case, M4 and A53 could reboot without
impacting the other. When A53 reboot, detach is invoked if I understand correct.

Reviewed-by: Peng Fan <peng.fan@nxp.com>

^ permalink raw reply	[flat|nested] 31+ messages in thread

* RE: [PATCH 06/13] remoteproc: Introduce function __rproc_detach()
  2020-08-26 16:45 ` [PATCH 06/13] remoteproc: Introduce function __rproc_detach() Mathieu Poirier
@ 2020-10-15  1:39   ` Peng Fan
  0 siblings, 0 replies; 31+ messages in thread
From: Peng Fan @ 2020-10-15  1:39 UTC (permalink / raw)
  To: Mathieu Poirier, bjorn.andersson, ohad; +Cc: linux-remoteproc, linux-kernel

> Subject: [PATCH 06/13] remoteproc: Introduce function __rproc_detach()
> 
> 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>
> ---
>  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 bffaa9ea7c8f..7a1fc7e0620f 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1641,6 +1641,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
> --

Reviewed-by: Peng Fan <peng.fan@nxp.com>

^ permalink raw reply	[flat|nested] 31+ messages in thread

* RE: [PATCH 07/13] remoteproc: Introduce function rproc_detach()
  2020-08-26 16:45 ` [PATCH 07/13] remoteproc: Introduce function rproc_detach() Mathieu Poirier
@ 2020-10-15  1:52   ` Peng Fan
  2020-10-22 21:55     ` Mathieu Poirier
  0 siblings, 1 reply; 31+ messages in thread
From: Peng Fan @ 2020-10-15  1:52 UTC (permalink / raw)
  To: Mathieu Poirier, bjorn.andersson, ohad; +Cc: linux-remoteproc, linux-kernel

> Subject: [PATCH 07/13] remoteproc: Introduce function rproc_detach()
> 
> 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>
> ---
>  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 7a1fc7e0620f..f3943a1e2754 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1644,7 +1644,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;
> @@ -1887,6 +1887,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
> 1a57e165da2c..6250491ee851 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,
> --


Reviewed-by: Peng Fan <peng.fan@nxp.com>

Not relevant to your patch, just see unregister_virtio_device not set device
status when reading code, should that add device status setting in
unregister_virtio_device?



^ permalink raw reply	[flat|nested] 31+ messages in thread

* RE: [PATCH 08/13] remoteproc: Rename function rproc_actuate()
  2020-08-26 16:45 ` [PATCH 08/13] remoteproc: Rename function rproc_actuate() Mathieu Poirier
@ 2020-10-15  2:15   ` Peng Fan
  0 siblings, 0 replies; 31+ messages in thread
From: Peng Fan @ 2020-10-15  2:15 UTC (permalink / raw)
  To: Mathieu Poirier, bjorn.andersson, ohad; +Cc: linux-remoteproc, linux-kernel

> Subject: [PATCH 08/13] remoteproc: Rename function rproc_actuate()
> 
> 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>
> ---
>  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 f3943a1e2754..c4b80ce6f22d 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1393,7 +1393,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;
> @@ -1518,7 +1518,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;
> @@ -1558,7 +1558,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;
> 
> @@ -1802,7 +1802,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);
> 
> @@ -1893,7 +1893,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,
> --

Reviewed-by: Peng Fan <peng.fan@nxp.com>

^ permalink raw reply	[flat|nested] 31+ messages in thread

* RE: [PATCH 09/13] remoteproc: Add return value to function rproc_shutdown()
  2020-08-26 16:45 ` [PATCH 09/13] remoteproc: Add return value to function rproc_shutdown() Mathieu Poirier
@ 2020-10-15  2:21   ` Peng Fan
  0 siblings, 0 replies; 31+ messages in thread
From: Peng Fan @ 2020-10-15  2:21 UTC (permalink / raw)
  To: Mathieu Poirier, bjorn.andersson, ohad; +Cc: linux-remoteproc, linux-kernel

> Subject: [PATCH 09/13] remoteproc: Add return value to function
> rproc_shutdown()
> 
> 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>
> ---
>  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 c4b80ce6f22d..c6c6aba66098 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1846,7 +1846,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;
> @@ -1854,15 +1854,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) {
> @@ -1884,6 +1888,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
> 6250491ee851..40eccfbc1357 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);
> --

Reviewed-by: Peng Fan <peng.fan@nxp.com>

^ permalink raw reply	[flat|nested] 31+ messages in thread

* RE: [PATCH 13/13] remoteproc: Properly deal with a kernel panic when attached
  2020-08-26 16:45 ` [PATCH 13/13] remoteproc: Properly deal with a kernel panic when attached Mathieu Poirier
@ 2020-10-15  2:28   ` Peng Fan
  0 siblings, 0 replies; 31+ messages in thread
From: Peng Fan @ 2020-10-15  2:28 UTC (permalink / raw)
  To: Mathieu Poirier, bjorn.andersson, ohad; +Cc: linux-remoteproc, linux-kernel

> Subject: [PATCH 13/13] remoteproc: Properly deal with a kernel panic when
> attached
> 
> 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>
> ---
>  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 5586582f54c5..54b5e3437ab5 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -2491,7 +2491,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);
> --

Reviewed-by: Peng Fan <peng.fan@nxp.com>


^ permalink raw reply	[flat|nested] 31+ messages in thread

* RE: [PATCH 10/13] remoteproc: Properly deal with a stop request when attached
  2020-08-26 16:45 ` [PATCH 10/13] remoteproc: Properly deal with a stop request when attached Mathieu Poirier
@ 2020-10-15  2:29   ` Peng Fan
  0 siblings, 0 replies; 31+ messages in thread
From: Peng Fan @ 2020-10-15  2:29 UTC (permalink / raw)
  To: Mathieu Poirier, bjorn.andersson, ohad; +Cc: linux-remoteproc, linux-kernel

> Subject: [PATCH 10/13] remoteproc: Properly deal with a stop request when
> attached
> 
> 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>
> ---
>  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 c6c6aba66098..95bb40b4ebb3 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1619,6 +1619,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);
> 
> @@ -1857,7 +1861,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 c152d11a4d3c..6134d2f083ce 100644
> --- a/drivers/remoteproc/remoteproc_sysfs.c
> +++ b/drivers/remoteproc/remoteproc_sysfs.c
> @@ -113,10 +113,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;
> --

Reviewed-by: Peng Fan <peng.fan@nxp.com>

^ permalink raw reply	[flat|nested] 31+ messages in thread

* RE: [PATCH 11/13] remoteproc: Properly deal with detach request
  2020-08-26 16:45 ` [PATCH 11/13] remoteproc: Properly deal with detach request Mathieu Poirier
@ 2020-10-15  2:30   ` Peng Fan
  0 siblings, 0 replies; 31+ messages in thread
From: Peng Fan @ 2020-10-15  2:30 UTC (permalink / raw)
  To: Mathieu Poirier, bjorn.andersson, ohad; +Cc: linux-remoteproc, linux-kernel

> Subject: [PATCH 11/13] remoteproc: Properly deal with detach request
> 
> 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>
> ---
>  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 6134d2f083ce..013231f69837 100644
> --- a/drivers/remoteproc/remoteproc_sysfs.c
> +++ b/drivers/remoteproc/remoteproc_sysfs.c
> @@ -118,6 +118,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;
> --

Reviewed-by: Peng Fan <peng.fan@nxp.com>

^ permalink raw reply	[flat|nested] 31+ messages in thread

* RE: [PATCH 12/13] remoteproc: Refactor rproc delete and cdev release path
  2020-08-26 16:45 ` [PATCH 12/13] remoteproc: Refactor rproc delete and cdev release path Mathieu Poirier
@ 2020-10-15  2:31   ` Peng Fan
  0 siblings, 0 replies; 31+ messages in thread
From: Peng Fan @ 2020-10-15  2:31 UTC (permalink / raw)
  To: Mathieu Poirier, bjorn.andersson, ohad; +Cc: linux-remoteproc, linux-kernel

> Subject: [PATCH 12/13] remoteproc: Refactor rproc delete and cdev release
> path
> 
> Refactor function rproc_del() and rproc_cdev_release() to take into account
> scenarios where the remote processor has been attached to.  If the remote
> processor has been started by the remoteproc core then switch it off, and if it
> was attached to detach from it. This heuristic is simple and can be enhanced
> easily if there is a need to.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  drivers/remoteproc/remoteproc_cdev.c | 7 ++++++-
> drivers/remoteproc/remoteproc_core.c | 5 ++++-
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_cdev.c
> b/drivers/remoteproc/remoteproc_cdev.c
> index 3a3830e27050..18cffbe588c1 100644
> --- a/drivers/remoteproc/remoteproc_cdev.c
> +++ b/drivers/remoteproc/remoteproc_cdev.c
> @@ -87,8 +87,13 @@ 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;
> +
> +	if (rproc->state == RPROC_RUNNING)
>  		rproc_shutdown(rproc);
> +	else if (rproc->state == RPROC_ATTACHED)
> +		rproc_detach(rproc);
> 
>  	return 0;
>  }
> diff --git a/drivers/remoteproc/remoteproc_core.c
> b/drivers/remoteproc/remoteproc_core.c
> index 95bb40b4ebb3..5586582f54c5 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -2346,7 +2346,10 @@ int rproc_del(struct rproc *rproc)
>  		return -EINVAL;
> 
>  	/* TODO: make sure this works with rproc->power > 1 */
> -	rproc_shutdown(rproc);
> +	if (rproc->state == RPROC_RUNNING)
> +		rproc_shutdown(rproc);
> +	else if (rproc->state == RPROC_ATTACHED)
> +		rproc_detach(rproc);
> 
>  	mutex_lock(&rproc->lock);
>  	rproc->state = RPROC_DELETED;
> --
Reviewed-by: Peng Fan <peng.fan@nxp.com>

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 05/13] remoteproc: Add new detach() remoteproc operation
  2020-10-15  1:37   ` Peng Fan
@ 2020-10-22 21:51     ` Mathieu Poirier
  0 siblings, 0 replies; 31+ messages in thread
From: Mathieu Poirier @ 2020-10-22 21:51 UTC (permalink / raw)
  To: Peng Fan; +Cc: bjorn.andersson, ohad, linux-remoteproc, linux-kernel

Hi Peng - I restarting work on this topic.

On Thu, Oct 15, 2020 at 01:37:42AM +0000, Peng Fan wrote:
> > Subject: [PATCH 05/13] remoteproc: Add new detach() remoteproc operation
> > 
> > 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>
> > ---
> >  include/linux/remoteproc.h | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h index
> > fe383392a821..1a57e165da2c 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);
> > --
> 
> This look useful for i.MX8 partitioned case, M4 and A53 could reboot without
> impacting the other. When A53 reboot, detach is invoked if I understand correct.

You are correct (for this patchset).  Since this heuristic is quite stringent we are
contemplating different solutions that would allow platforms to decide what they
want to do based on the usecase they care about.

Thanks,
Mathieu

> 
> Reviewed-by: Peng Fan <peng.fan@nxp.com>

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 07/13] remoteproc: Introduce function rproc_detach()
  2020-10-15  1:52   ` Peng Fan
@ 2020-10-22 21:55     ` Mathieu Poirier
  0 siblings, 0 replies; 31+ messages in thread
From: Mathieu Poirier @ 2020-10-22 21:55 UTC (permalink / raw)
  To: Peng Fan; +Cc: bjorn.andersson, ohad, linux-remoteproc, linux-kernel

On Thu, Oct 15, 2020 at 01:52:16AM +0000, Peng Fan wrote:
> > Subject: [PATCH 07/13] remoteproc: Introduce function rproc_detach()
> > 
> > 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>
> > ---
> >  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 7a1fc7e0620f..f3943a1e2754 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -1644,7 +1644,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;
> > @@ -1887,6 +1887,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
> > 1a57e165da2c..6250491ee851 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,
> > --
> 
> 
> Reviewed-by: Peng Fan <peng.fan@nxp.com>
> 
> Not relevant to your patch, just see unregister_virtio_device not set device
> status when reading code, should that add device status setting in
> unregister_virtio_device?

I must admit that I don't understand the question - would you mind rephrasing or
expanding?

Thanks,
Mathieu

> 
> 

^ permalink raw reply	[flat|nested] 31+ messages in thread

end of thread, back to index

Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-26 16:45 [PATCH 00/13] remoteproc: Add support for detaching from rproc Mathieu Poirier
2020-08-26 16:45 ` [PATCH 01/13] remoteproc: Re-check state in rproc_shutdown() Mathieu Poirier
2020-10-15  1:28   ` Peng Fan
2020-08-26 16:45 ` [PATCH 02/13] remoteproc: Remove useless check in rproc_del() Mathieu Poirier
2020-10-15  1:29   ` Peng Fan
2020-08-26 16:45 ` [PATCH 03/13] remoteproc: Add new RPROC_ATTACHED state Mathieu Poirier
2020-10-15  1:31   ` Peng Fan
2020-08-26 16:45 ` [PATCH 04/13] remoteproc: Properly represent the attached state Mathieu Poirier
2020-10-15  1:33   ` Peng Fan
2020-08-26 16:45 ` [PATCH 05/13] remoteproc: Add new detach() remoteproc operation Mathieu Poirier
2020-10-15  1:37   ` Peng Fan
2020-10-22 21:51     ` Mathieu Poirier
2020-08-26 16:45 ` [PATCH 06/13] remoteproc: Introduce function __rproc_detach() Mathieu Poirier
2020-10-15  1:39   ` Peng Fan
2020-08-26 16:45 ` [PATCH 07/13] remoteproc: Introduce function rproc_detach() Mathieu Poirier
2020-10-15  1:52   ` Peng Fan
2020-10-22 21:55     ` Mathieu Poirier
2020-08-26 16:45 ` [PATCH 08/13] remoteproc: Rename function rproc_actuate() Mathieu Poirier
2020-10-15  2:15   ` Peng Fan
2020-08-26 16:45 ` [PATCH 09/13] remoteproc: Add return value to function rproc_shutdown() Mathieu Poirier
2020-10-15  2:21   ` Peng Fan
2020-08-26 16:45 ` [PATCH 10/13] remoteproc: Properly deal with a stop request when attached Mathieu Poirier
2020-10-15  2:29   ` Peng Fan
2020-08-26 16:45 ` [PATCH 11/13] remoteproc: Properly deal with detach request Mathieu Poirier
2020-10-15  2:30   ` Peng Fan
2020-08-26 16:45 ` [PATCH 12/13] remoteproc: Refactor rproc delete and cdev release path Mathieu Poirier
2020-10-15  2:31   ` Peng Fan
2020-08-26 16:45 ` [PATCH 13/13] remoteproc: Properly deal with a kernel panic when attached Mathieu Poirier
2020-10-15  2:28   ` Peng Fan
2020-09-01 16:55 ` [PATCH 00/13] remoteproc: Add support for detaching from rproc Arnaud POULIQUEN
2020-09-01 18:02   ` Mathieu Poirier

Linux-remoteproc Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-remoteproc/0 linux-remoteproc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-remoteproc linux-remoteproc/ https://lore.kernel.org/linux-remoteproc \
		linux-remoteproc@vger.kernel.org
	public-inbox-index linux-remoteproc

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-remoteproc


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git