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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
Add a return value to function rproc_shutdown() in order to
properly deal with error conditions that may occur.
Signed-off-by: Mathieu Poirier <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
The panic handler operation of registered remote processors
should also be called when remote processors have been
attached to.
Signed-off-by: Mathieu Poirier <[email protected]>
---
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
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: <[email protected]>
Signed-off-by: Mathieu Poirier <[email protected]>
---
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
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(-)
>
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(-)
> >
> 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: <[email protected]>
> Signed-off-by: Mathieu Poirier <[email protected]>
> ---
> 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 <[email protected]>
> 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 <[email protected]>
> ---
> 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 <[email protected]>
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?
> 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 <[email protected]>
> ---
> 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 <[email protected]>
> 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 <[email protected]>
> ---
> 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 <[email protected]>
> 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 <[email protected]>
> ---
> 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 <[email protected]>
> 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 <[email protected]>
> ---
> 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 <[email protected]>
> 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 <[email protected]>
> ---
> 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 <[email protected]>
> 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 <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
> > ---
> > 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 <[email protected]>
>
> 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
>
>