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 application processor power
cycled while the remote processor is still operating.
The main difference in this revision is patch 11/16 (in V6). It was split
split in two part in order to simplify handling of the resource table when
the remote processor is detached or stopped. Other modifications are
detailed in the changelog of each patch.
Applies cleanly on v5.12-rc2.
Thanks,
Mathieu
Arnaud POULIQUEN (1):
remoteproc: stm32: Move memory parsing to rproc_ops
Mathieu Poirier (16):
remoteproc: Remove useless check in rproc_del()
remoteproc: Rename function rproc_actuate()
remoteproc: Add new RPROC_ATTACHED state
remoteproc: Properly represent the attached state
remoteproc: Add new get_loaded_rsc_table() to rproc_ops
remoteproc: stm32: Move resource table setup to rproc_ops
remoteproc: Add new detach() remoteproc operation
remoteproc: Introduce function __rproc_detach()
remoteproc: Introduce function rproc_detach()
remoteproc: Properly deal with the resource table when detaching
remoteproc: Properly deal with the resource table when stopping
remoteproc: Properly deal with a kernel panic when attached
remoteproc: Properly deal with a start request when attached
remoteproc: Properly deal with a stop request when attached
remoteproc: Properly deal with a detach request when attached
remoteproc: Refactor function rproc_cdev_release()
drivers/remoteproc/remoteproc_cdev.c | 21 +-
drivers/remoteproc/remoteproc_core.c | 302 ++++++++++++++++++++---
drivers/remoteproc/remoteproc_internal.h | 10 +
drivers/remoteproc/remoteproc_sysfs.c | 17 +-
drivers/remoteproc/stm32_rproc.c | 168 ++++++-------
include/linux/remoteproc.h | 21 +-
6 files changed, 401 insertions(+), 138 deletions(-)
--
2.25.1
Whether started at probe() time or thereafter from the command
line, a remote processor needs to be shut down 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 <[email protected]>
Reviewed-by: Peng Fan <[email protected]>
Reviewed-by: Arnaud Pouliquen <[email protected]>
---
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 ab150765d124..d2704501b653 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -2347,10 +2347,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
From: Arnaud POULIQUEN <[email protected]>
Some actions such as memory resources reallocation are needed when
trying to reattach a co-processor. Use the prepare() operation for
these actions.
Co-developed-by: Mathieu Poirier <[email protected]>
Signed-off-by: Mathieu Poirier <[email protected]>
Signed-off-by: Arnaud POULIQUEN <[email protected]>
---
drivers/remoteproc/remoteproc_core.c | 14 ++++++++++++--
drivers/remoteproc/stm32_rproc.c | 27 ++++++---------------------
2 files changed, 18 insertions(+), 23 deletions(-)
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index bf6f6d15b1c3..1d8bb588d996 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1582,10 +1582,17 @@ static int rproc_attach(struct rproc *rproc)
return ret;
}
+ /* Do anything that is needed to boot the remote processor */
+ ret = rproc_prepare_device(rproc);
+ if (ret) {
+ dev_err(dev, "can't prepare rproc %s: %d\n", rproc->name, ret);
+ goto disable_iommu;
+ }
+
ret = rproc_set_rsc_table(rproc);
if (ret) {
dev_err(dev, "can't load resource table: %d\n", ret);
- goto disable_iommu;
+ goto unprepare_device;
}
/* reset max_notifyid */
@@ -1602,7 +1609,7 @@ static int rproc_attach(struct rproc *rproc)
ret = rproc_handle_resources(rproc, rproc_loading_handlers);
if (ret) {
dev_err(dev, "Failed to process resources: %d\n", ret);
- goto disable_iommu;
+ goto unprepare_device;
}
/* Allocate carveout resources associated to rproc */
@@ -1621,6 +1628,9 @@ static int rproc_attach(struct rproc *rproc)
clean_up_resources:
rproc_resource_cleanup(rproc);
+unprepare_device:
+ /* release HW resources if needed */
+ rproc_unprepare_device(rproc);
disable_iommu:
rproc_disable_iommu(rproc);
return ret;
diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
index f647e565014b..3d45f51de4d0 100644
--- a/drivers/remoteproc/stm32_rproc.c
+++ b/drivers/remoteproc/stm32_rproc.c
@@ -207,16 +207,7 @@ static int stm32_rproc_mbox_idx(struct rproc *rproc, const unsigned char *name)
return -EINVAL;
}
-static int stm32_rproc_elf_load_rsc_table(struct rproc *rproc,
- const struct firmware *fw)
-{
- if (rproc_elf_load_rsc_table(rproc, fw))
- dev_warn(&rproc->dev, "no resource table found for this firmware\n");
-
- return 0;
-}
-
-static int stm32_rproc_parse_memory_regions(struct rproc *rproc)
+static int stm32_rproc_prepare(struct rproc *rproc)
{
struct device *dev = rproc->dev.parent;
struct device_node *np = dev->of_node;
@@ -274,12 +265,10 @@ static int stm32_rproc_parse_memory_regions(struct rproc *rproc)
static int stm32_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
{
- int ret = stm32_rproc_parse_memory_regions(rproc);
-
- if (ret)
- return ret;
+ if (rproc_elf_load_rsc_table(rproc, fw))
+ dev_warn(&rproc->dev, "no resource table found for this firmware\n");
- return stm32_rproc_elf_load_rsc_table(rproc, fw);
+ return 0;
}
static irqreturn_t stm32_rproc_wdg(int irq, void *data)
@@ -614,6 +603,7 @@ stm32_rproc_get_loaded_rsc_table(struct rproc *rproc, size_t *table_sz)
}
static const struct rproc_ops st_rproc_ops = {
+ .prepare = stm32_rproc_prepare,
.start = stm32_rproc_start,
.stop = stm32_rproc_stop,
.attach = stm32_rproc_attach,
@@ -796,14 +786,9 @@ static int stm32_rproc_probe(struct platform_device *pdev)
if (ret)
goto free_rproc;
- if (state == M4_STATE_CRUN) {
+ if (state == M4_STATE_CRUN)
rproc->state = RPROC_DETACHED;
- ret = stm32_rproc_parse_memory_regions(rproc);
- if (ret)
- goto free_resources;
- }
-
rproc->has_iommu = false;
ddata->workqueue = create_workqueue(dev_name(dev));
if (!ddata->workqueue) {
--
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]>
Reviewed-by: Peng Fan <[email protected]>
Reviewed-by: Arnaud Pouliquen <[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 c488b1aa6119..f6f0813dade5 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -2728,7 +2728,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
Allow a remote processor that was started by another entity to be
switched off by the remoteproc core. For that to happen a
rproc::ops::stop() operation needs to be available.
Signed-off-by: Mathieu Poirier <[email protected]>
Reviewed-by: Arnaud Pouliquen <[email protected]>
---
drivers/remoteproc/remoteproc_cdev.c | 3 ++-
drivers/remoteproc/remoteproc_core.c | 4 ++++
drivers/remoteproc/remoteproc_sysfs.c | 3 ++-
3 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
index b2cee9afb41b..0249d8f6c3f8 100644
--- a/drivers/remoteproc/remoteproc_cdev.c
+++ b/drivers/remoteproc/remoteproc_cdev.c
@@ -38,7 +38,8 @@ 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);
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index f6f0813dade5..3670b70390dd 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1801,6 +1801,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);
diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
index 66801e6fe5cd..09eb700c5e7e 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -202,7 +202,8 @@ 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);
--
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]>
Reviewed-by: Peng Fan <[email protected]>
Reviewed-by: Arnaud Pouliquen <[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 7b66e1e96e4a..8c7e9f1d50d7 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1444,7 +1444,7 @@ static int __rproc_attach(struct rproc *rproc)
goto stop_rproc;
}
- rproc->state = RPROC_RUNNING;
+ rproc->state = RPROC_ATTACHED;
dev_info(dev, "remote processor %s is now attached\n", rproc->name);
@@ -1659,14 +1659,6 @@ static int rproc_stop(struct rproc *rproc, bool crashed)
rproc->state = RPROC_OFFLINE;
- /*
- * The remote processor has been stopped and is now offline, which means
- * that the next time it is brought back online the remoteproc core will
- * be responsible to load its firmware. As such it is no longer
- * autonomous.
- */
- rproc->autonomous = false;
-
dev_info(dev, "stopped remote processor %s\n", rproc->name);
return 0;
@@ -2077,16 +2069,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 4b4aab0d4c4b..f9694def9b54 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -138,11 +138,8 @@ static ssize_t firmware_show(struct device *dev, struct device_attribute *attr,
* If the remote processor has been started by an external
* entity we have no idea of what image it is running. As such
* simply display a generic string rather then rproc->firmware.
- *
- * Here we rely on the autonomous flag because a remote processor
- * may have been attached to and currently in a running state.
*/
- if (rproc->autonomous)
+ if (rproc->state == RPROC_ATTACHED)
firmware = "unknown";
return sprintf(buf, "%s\n", firmware);
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index b0a57ff73849..6b0a0ed30a03 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -512,7 +512,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
@@ -549,7 +548,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
Add a new get_loaded_rsc_table() operation in order to support
scenarios where the remoteproc core has booted a remote processor
and detaches from it. When re-attaching to the remote processor,
the core needs to know where the resource table has been placed
in memory.
Signed-off-by: Mathieu Poirier <[email protected]>
Reviewed-by: Arnaud Pouliquen <[email protected]>
---
drivers/remoteproc/remoteproc_core.c | 32 ++++++++++++++++++++++++
drivers/remoteproc/remoteproc_internal.h | 10 ++++++++
include/linux/remoteproc.h | 6 ++++-
3 files changed, 47 insertions(+), 1 deletion(-)
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 8c7e9f1d50d7..bf6f6d15b1c3 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1537,6 +1537,32 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
return ret;
}
+static int rproc_set_rsc_table(struct rproc *rproc)
+{
+ struct resource_table *table_ptr;
+ struct device *dev = &rproc->dev;
+ size_t table_sz;
+ int ret;
+
+ table_ptr = rproc_get_loaded_rsc_table(rproc, &table_sz);
+ if (!table_ptr) {
+ /* Not having a resource table is acceptable */
+ return 0;
+ }
+
+ if (IS_ERR(table_ptr)) {
+ ret = PTR_ERR(table_ptr);
+ dev_err(dev, "can't load resource table: %d\n", ret);
+ return ret;
+ }
+
+ rproc->cached_table = NULL;
+ rproc->table_ptr = table_ptr;
+ rproc->table_sz = table_sz;
+
+ return 0;
+}
+
/*
* Attach to remote processor - similar to rproc_fw_boot() but without
* the steps that deal with the firmware image.
@@ -1556,6 +1582,12 @@ static int rproc_attach(struct rproc *rproc)
return ret;
}
+ ret = rproc_set_rsc_table(rproc);
+ if (ret) {
+ dev_err(dev, "can't load resource table: %d\n", ret);
+ goto disable_iommu;
+ }
+
/* reset max_notifyid */
rproc->max_notifyid = -1;
diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
index c34002888d2c..4f73aac7e60d 100644
--- a/drivers/remoteproc/remoteproc_internal.h
+++ b/drivers/remoteproc/remoteproc_internal.h
@@ -177,6 +177,16 @@ struct resource_table *rproc_find_loaded_rsc_table(struct rproc *rproc,
return NULL;
}
+static inline
+struct resource_table *rproc_get_loaded_rsc_table(struct rproc *rproc,
+ size_t *size)
+{
+ if (rproc->ops->get_loaded_rsc_table)
+ return rproc->ops->get_loaded_rsc_table(rproc, size);
+
+ return NULL;
+}
+
static inline
bool rproc_u64_fit_in_size_t(u64 val)
{
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 6b0a0ed30a03..51538a7d120d 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -368,7 +368,9 @@ enum rsc_handling_status {
* RSC_HANDLED if resource was handled, RSC_IGNORED if not handled and a
* negative value on error
* @load_rsc_table: load resource table from firmware image
- * @find_loaded_rsc_table: find the loaded resouce table
+ * @find_loaded_rsc_table: find the loaded resource table from firmware image
+ * @get_loaded_rsc_table: get resource table installed in memory
+ * by external entity
* @load: load firmware to memory, where the remote processor
* expects to find it
* @sanity_check: sanity check the fw image
@@ -390,6 +392,8 @@ struct rproc_ops {
int offset, int avail);
struct resource_table *(*find_loaded_rsc_table)(
struct rproc *rproc, const struct firmware *fw);
+ struct resource_table *(*get_loaded_rsc_table)(
+ struct rproc *rproc, size_t *size);
int (*load)(struct rproc *rproc, const struct firmware *fw);
int (*sanity_check)(struct rproc *rproc, const struct firmware *fw);
u64 (*get_boot_addr)(struct rproc *rproc, const struct firmware *fw);
--
2.25.1
Refactor function rproc_cdev_release() to take into account the
current state of the remote processor when choosing the state to
transition to.
Signed-off-by: Mathieu Poirier <[email protected]>
---
New for V7:
Keep the behavior of the shutdown operation in rproc_del() intact.
---
drivers/remoteproc/remoteproc_cdev.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
index 2db494816d5f..0b8a84c04f76 100644
--- a/drivers/remoteproc/remoteproc_cdev.c
+++ b/drivers/remoteproc/remoteproc_cdev.c
@@ -86,11 +86,17 @@ static long rproc_device_ioctl(struct file *filp, unsigned int ioctl, unsigned l
static int rproc_cdev_release(struct inode *inode, struct file *filp)
{
struct rproc *rproc = container_of(inode->i_cdev, struct rproc, cdev);
+ int ret = 0;
+
+ if (!rproc->cdev_put_on_release)
+ return 0;
- if (rproc->cdev_put_on_release && rproc->state == RPROC_RUNNING)
+ if (rproc->state == RPROC_RUNNING)
rproc_shutdown(rproc);
+ else if (rproc->state == RPROC_ATTACHED)
+ ret = rproc_detach(rproc);
- return 0;
+ return ret;
}
static const struct file_operations rproc_fops = {
--
2.25.1
When a remote processor that was attached to is stopped, special care
must be taken to make sure the shutdown process is similar to what
it would be had it been started by the remoteproc core.
This patch takes care of that by making a copy of the resource
table currently used by the remote processor. From that point on
the copy is used, as if the remote processor had been started by
the remoteproc core.
Signed-off-by: Mathieu Poirier <[email protected]>
---
New for V7:
New Patch, used to be part of 11/16 in V6.
---
drivers/remoteproc/remoteproc_core.c | 53 +++++++++++++++++++++++++++-
1 file changed, 52 insertions(+), 1 deletion(-)
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index e9ea2558432d..c488b1aa6119 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1634,6 +1634,52 @@ static int rproc_reset_rsc_table_on_detach(struct rproc *rproc)
return 0;
}
+static int rproc_reset_rsc_table_on_stop(struct rproc *rproc)
+{
+ struct resource_table *table_ptr;
+
+ /* A resource table was never retrieved, nothing to do here */
+ if (!rproc->table_ptr)
+ return 0;
+
+ /*
+ * If a cache table exists the remote processor was started by
+ * the remoteproc core. That cache table should be used for
+ * the rest of the shutdown process.
+ */
+ if (rproc->cached_table)
+ goto out;
+
+ /* Remember where the external entity installed the resource table */
+ table_ptr = rproc->table_ptr;
+
+ /*
+ * If we made it here the remote processor was started by another
+ * entity and a cache table doesn't exist. As such make a copy of
+ * the resource table currently used by the remote processor and
+ * use that for the rest of the shutdown process. The memory
+ * allocated here is free'd in rproc_shutdown().
+ */
+ rproc->cached_table = kmemdup(rproc->table_ptr,
+ rproc->table_sz, GFP_KERNEL);
+ if (!rproc->cached_table)
+ return -ENOMEM;
+
+ /*
+ * Since the remote processor is being switched off the clean table
+ * won't be needed. Allocated in rproc_set_rsc_table().
+ */
+ kfree(rproc->clean_table);
+
+out:
+ /*
+ * Use a copy of the resource table for the remainder of the
+ * shutdown process.
+ */
+ rproc->table_ptr = rproc->cached_table;
+ return 0;
+}
+
/*
* Attach to remote processor - similar to rproc_fw_boot() but without
* the steps that deal with the firmware image.
@@ -1759,7 +1805,12 @@ static int rproc_stop(struct rproc *rproc, bool crashed)
rproc_stop_subdevices(rproc, crashed);
/* the installed resource table is no longer accessible */
- rproc->table_ptr = rproc->cached_table;
+ ret = rproc_reset_rsc_table_on_stop(rproc);
+ if (ret) {
+ dev_err(dev, "can't reset resource table: %d\n", ret);
+ return ret;
+ }
+
/* power off the remote processor */
ret = rproc->ops->stop(rproc);
--
2.25.1
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 <[email protected]>
Reviewed-by: Peng Fan <[email protected]>
Reviewed-by: Arnaud Pouliquen <[email protected]>
---
include/linux/remoteproc.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 51538a7d120d..eff55ec72e80 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: detach from a device, leaving it powered up
* @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)
@@ -385,6 +386,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
This patch introduces the capability to detach a remote processor
that has been attached to by the remoteproc core. For that to happen
a rproc::ops::detach() operation needs to be available.
Signed-off-by: Mathieu Poirier <[email protected]>
Reviewed-by: Arnaud Pouliquen <[email protected]>
---
drivers/remoteproc/remoteproc_cdev.c | 5 +++++
drivers/remoteproc/remoteproc_sysfs.c | 5 +++++
2 files changed, 10 insertions(+)
diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
index 0249d8f6c3f8..2db494816d5f 100644
--- a/drivers/remoteproc/remoteproc_cdev.c
+++ b/drivers/remoteproc/remoteproc_cdev.c
@@ -43,6 +43,11 @@ static ssize_t rproc_cdev_write(struct file *filp, const char __user *buf, size_
return -EINVAL;
rproc_shutdown(rproc);
+ } else if (!strncmp(cmd, "detach", len)) {
+ if (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 09eb700c5e7e..ad3dd208024c 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -207,6 +207,11 @@ static ssize_t state_store(struct device *dev,
return -EINVAL;
rproc_shutdown(rproc);
+ } else if (sysfs_streq(buf, "detach")) {
+ if (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
If it is possible to detach the remote processor, keep an untouched
copy of the resource table. That way we can start from the same
resource table without having to worry about original values or what
elements the startup code has changed when re-attaching to the remote
processor.
Signed-off-by: Mathieu Poirier <[email protected]>
---
New for V7:
New Patch, used to be part of 11/16 in V6.
---
drivers/remoteproc/remoteproc_core.c | 77 ++++++++++++++++++++++++++++
include/linux/remoteproc.h | 3 ++
2 files changed, 80 insertions(+)
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 5eaa47c3ba92..e9ea2558432d 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1556,6 +1556,24 @@ static int rproc_set_rsc_table(struct rproc *rproc)
return ret;
}
+ /*
+ * If it is possible to detach the remote processor, keep an untouched
+ * copy of the resource table. That way we can start fresh again when
+ * the remote processor is re-attached, that is:
+ *
+ * DETACHED -> ATTACHED -> DETACHED -> ATTACHED
+ *
+ * Free'd in rproc_reset_rsc_table_on_detach() and
+ * rproc_reset_rsc_table_on_stop().
+ */
+ if (rproc->ops->detach) {
+ rproc->clean_table = kmemdup(table_ptr, table_sz, GFP_KERNEL);
+ if (!rproc->clean_table)
+ return -ENOMEM;
+ } else {
+ rproc->clean_table = NULL;
+ }
+
rproc->cached_table = NULL;
rproc->table_ptr = table_ptr;
rproc->table_sz = table_sz;
@@ -1563,6 +1581,59 @@ static int rproc_set_rsc_table(struct rproc *rproc)
return 0;
}
+static int rproc_reset_rsc_table_on_detach(struct rproc *rproc)
+{
+ struct resource_table *table_ptr;
+
+ /* A resource table was never retrieved, nothing to do here */
+ if (!rproc->table_ptr)
+ return 0;
+
+ /*
+ * If we made it to this point a clean_table _must_ have been
+ * allocated in rproc_set_rsc_table(). If one isn't present
+ * something went really wrong and we must complain.
+ */
+ if (WARN_ON(!rproc->clean_table))
+ return -EINVAL;
+
+ /* Remember where the external entity installed the resource table */
+ table_ptr = rproc->table_ptr;
+
+ /*
+ * If we made it here the remote processor was started by another
+ * entity and a cache table doesn't exist. As such make a copy of
+ * the resource table currently used by the remote processor and
+ * use that for the rest of the shutdown process. The memory
+ * allocated here is free'd in rproc_detach().
+ */
+ rproc->cached_table = kmemdup(rproc->table_ptr,
+ rproc->table_sz, GFP_KERNEL);
+ if (!rproc->cached_table)
+ return -ENOMEM;
+
+ /*
+ * Use a copy of the resource table for the remainder of the
+ * shutdown process.
+ */
+ rproc->table_ptr = rproc->cached_table;
+
+ /*
+ * Reset the memory area where the firmware loaded the resource table
+ * to its original value. That way when we re-attach the remote
+ * processor the resource table is clean and ready to be used again.
+ */
+ memcpy(table_ptr, rproc->clean_table, rproc->table_sz);
+
+ /*
+ * The clean resource table is no longer needed. Allocated in
+ * rproc_set_rsc_table().
+ */
+ kfree(rproc->clean_table);
+
+ return 0;
+}
+
/*
* Attach to remote processor - similar to rproc_fw_boot() but without
* the steps that deal with the firmware image.
@@ -1721,6 +1792,9 @@ static int __rproc_detach(struct rproc *rproc)
/* Stop any subdevices for the remote processor */
rproc_stop_subdevices(rproc, false);
+ /* the installed resource table is no longer accessible */
+ ret = rproc_reset_rsc_table_on_detach(rproc);
+
/* Tell the remote processor the core isn't available anymore */
ret = rproc->ops->detach(rproc);
if (ret) {
@@ -1997,6 +2071,9 @@ int rproc_detach(struct rproc *rproc)
rproc_disable_iommu(rproc);
+ /* Free the copy of the resource table */
+ kfree(rproc->cached_table);
+ rproc->cached_table = NULL;
rproc->table_ptr = NULL;
out:
mutex_unlock(&rproc->lock);
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index e1c843c19cc6..e5f52a12a650 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -514,6 +514,8 @@ struct rproc_dump_segment {
* @recovery_disabled: flag that state if recovery was disabled
* @max_notifyid: largest allocated notify id.
* @table_ptr: pointer to the resource table in effect
+ * @clean_table: copy of the resource table without modifications. Used
+ * when a remote processor is attached or detached from the core
* @cached_table: copy of the resource table
* @table_sz: size of @cached_table
* @has_iommu: flag to indicate if remote processor is behind an MMU
@@ -550,6 +552,7 @@ struct rproc {
bool recovery_disabled;
int max_notifyid;
struct resource_table *table_ptr;
+ struct resource_table *clean_table;
struct resource_table *cached_table;
size_t table_sz;
bool has_iommu;
--
2.25.1
Hi Mathieu,
I love your patch! Perhaps something to improve:
[auto build test WARNING on linux/master]
[also build test WARNING on linus/master v5.12-rc2 next-20210310]
[cannot apply to remoteproc/for-next rpmsg/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Mathieu-Poirier/remoteproc-Add-support-for-detaching-a-remote-processor/20210311-051242
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 144c79ef33536b4ecb4951e07dbc1f2b7fa99d32
config: arm-randconfig-r036-20210310 (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/6715f51e2b7ade7b5121eced81cad8065d4f5525
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Mathieu-Poirier/remoteproc-Add-support-for-detaching-a-remote-processor/20210311-051242
git checkout 6715f51e2b7ade7b5121eced81cad8065d4f5525
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
drivers/remoteproc/remoteproc_core.c: In function 'rproc_reset_rsc_table_on_stop':
>> drivers/remoteproc/remoteproc_core.c:1639:25: warning: variable 'table_ptr' set but not used [-Wunused-but-set-variable]
1639 | struct resource_table *table_ptr;
| ^~~~~~~~~
vim +/table_ptr +1639 drivers/remoteproc/remoteproc_core.c
1636
1637 static int rproc_reset_rsc_table_on_stop(struct rproc *rproc)
1638 {
> 1639 struct resource_table *table_ptr;
1640
1641 /* A resource table was never retrieved, nothing to do here */
1642 if (!rproc->table_ptr)
1643 return 0;
1644
1645 /*
1646 * If a cache table exists the remote processor was started by
1647 * the remoteproc core. That cache table should be used for
1648 * the rest of the shutdown process.
1649 */
1650 if (rproc->cached_table)
1651 goto out;
1652
1653 /* Remember where the external entity installed the resource table */
1654 table_ptr = rproc->table_ptr;
1655
1656 /*
1657 * If we made it here the remote processor was started by another
1658 * entity and a cache table doesn't exist. As such make a copy of
1659 * the resource table currently used by the remote processor and
1660 * use that for the rest of the shutdown process. The memory
1661 * allocated here is free'd in rproc_shutdown().
1662 */
1663 rproc->cached_table = kmemdup(rproc->table_ptr,
1664 rproc->table_sz, GFP_KERNEL);
1665 if (!rproc->cached_table)
1666 return -ENOMEM;
1667
1668 /*
1669 * Since the remote processor is being switched off the clean table
1670 * won't be needed. Allocated in rproc_set_rsc_table().
1671 */
1672 kfree(rproc->clean_table);
1673
1674 out:
1675 /*
1676 * Use a copy of the resource table for the remainder of the
1677 * shutdown process.
1678 */
1679 rproc->table_ptr = rproc->cached_table;
1680 return 0;
1681 }
1682
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On Wed 10 Mar 15:10 CST 2021, Mathieu Poirier wrote:
> When a remote processor that was attached to is stopped, special care
> must be taken to make sure the shutdown process is similar to what
> it would be had it been started by the remoteproc core.
>
> This patch takes care of that by making a copy of the resource
> table currently used by the remote processor. From that point on
> the copy is used, as if the remote processor had been started by
> the remoteproc core.
>
> Signed-off-by: Mathieu Poirier <[email protected]>
> ---
> New for V7:
> New Patch, used to be part of 11/16 in V6.
> ---
> drivers/remoteproc/remoteproc_core.c | 53 +++++++++++++++++++++++++++-
> 1 file changed, 52 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index e9ea2558432d..c488b1aa6119 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1634,6 +1634,52 @@ static int rproc_reset_rsc_table_on_detach(struct rproc *rproc)
> return 0;
> }
>
> +static int rproc_reset_rsc_table_on_stop(struct rproc *rproc)
> +{
> + struct resource_table *table_ptr;
> +
> + /* A resource table was never retrieved, nothing to do here */
> + if (!rproc->table_ptr)
> + return 0;
> +
> + /*
> + * If a cache table exists the remote processor was started by
> + * the remoteproc core. That cache table should be used for
> + * the rest of the shutdown process.
> + */
> + if (rproc->cached_table)
> + goto out;
> +
> + /* Remember where the external entity installed the resource table */
> + table_ptr = rproc->table_ptr;
> +
Afaict this is just a remnant from the detach case.
I think the series looks really good now, please let me know and I can
drop the local "table_ptr" as I apply the patches.
Regards,
Bjorn
> + /*
> + * If we made it here the remote processor was started by another
> + * entity and a cache table doesn't exist. As such make a copy of
> + * the resource table currently used by the remote processor and
> + * use that for the rest of the shutdown process. The memory
> + * allocated here is free'd in rproc_shutdown().
> + */
> + rproc->cached_table = kmemdup(rproc->table_ptr,
> + rproc->table_sz, GFP_KERNEL);
> + if (!rproc->cached_table)
> + return -ENOMEM;
> +
> + /*
> + * Since the remote processor is being switched off the clean table
> + * won't be needed. Allocated in rproc_set_rsc_table().
> + */
> + kfree(rproc->clean_table);
> +
> +out:
> + /*
> + * Use a copy of the resource table for the remainder of the
> + * shutdown process.
> + */
> + rproc->table_ptr = rproc->cached_table;
> + return 0;
> +}
> +
> /*
> * Attach to remote processor - similar to rproc_fw_boot() but without
> * the steps that deal with the firmware image.
> @@ -1759,7 +1805,12 @@ static int rproc_stop(struct rproc *rproc, bool crashed)
> rproc_stop_subdevices(rproc, crashed);
>
> /* the installed resource table is no longer accessible */
> - rproc->table_ptr = rproc->cached_table;
> + ret = rproc_reset_rsc_table_on_stop(rproc);
> + if (ret) {
> + dev_err(dev, "can't reset resource table: %d\n", ret);
> + return ret;
> + }
> +
>
> /* power off the remote processor */
> ret = rproc->ops->stop(rproc);
> --
> 2.25.1
>
Hi Mathieu,
Just a minor comment, with that
Reviewed-by: Arnaud Pouliquen <[email protected]>
On 3/10/21 10:10 PM, Mathieu Poirier wrote:
> If it is possible to detach the remote processor, keep an untouched
> copy of the resource table. That way we can start from the same
> resource table without having to worry about original values or what
> elements the startup code has changed when re-attaching to the remote
> processor.
>
> Signed-off-by: Mathieu Poirier <[email protected]>
> ---
> New for V7:
> New Patch, used to be part of 11/16 in V6.
> ---
> drivers/remoteproc/remoteproc_core.c | 77 ++++++++++++++++++++++++++++
> include/linux/remoteproc.h | 3 ++
> 2 files changed, 80 insertions(+)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 5eaa47c3ba92..e9ea2558432d 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1556,6 +1556,24 @@ static int rproc_set_rsc_table(struct rproc *rproc)
> return ret;
> }
>
> + /*
> + * If it is possible to detach the remote processor, keep an untouched
> + * copy of the resource table. That way we can start fresh again when
> + * the remote processor is re-attached, that is:
> + *
> + * DETACHED -> ATTACHED -> DETACHED -> ATTACHED
> + *
> + * Free'd in rproc_reset_rsc_table_on_detach() and
> + * rproc_reset_rsc_table_on_stop().
> + */
> + if (rproc->ops->detach) {
> + rproc->clean_table = kmemdup(table_ptr, table_sz, GFP_KERNEL);
> + if (!rproc->clean_table)
> + return -ENOMEM;
> + } else {
> + rproc->clean_table = NULL;
> + }
> +
> rproc->cached_table = NULL;
> rproc->table_ptr = table_ptr;
> rproc->table_sz = table_sz;
> @@ -1563,6 +1581,59 @@ static int rproc_set_rsc_table(struct rproc *rproc)
> return 0;
> }
>
> +static int rproc_reset_rsc_table_on_detach(struct rproc *rproc)
> +{
> + struct resource_table *table_ptr;
> +
> + /* A resource table was never retrieved, nothing to do here */
> + if (!rproc->table_ptr)
> + return 0;
> +
> + /*
> + * If we made it to this point a clean_table _must_ have been
> + * allocated in rproc_set_rsc_table(). If one isn't present
> + * something went really wrong and we must complain.
> + */
> + if (WARN_ON(!rproc->clean_table))
> + return -EINVAL;
> +
> + /* Remember where the external entity installed the resource table */
> + table_ptr = rproc->table_ptr;
> +
> + /*
> + * If we made it here the remote processor was started by another
> + * entity and a cache table doesn't exist. As such make a copy of
> + * the resource table currently used by the remote processor and
> + * use that for the rest of the shutdown process. The memory
> + * allocated here is free'd in rproc_detach().
> + */
> + rproc->cached_table = kmemdup(rproc->table_ptr,
> + rproc->table_sz, GFP_KERNEL);
> + if (!rproc->cached_table)
> + return -ENOMEM;
> +
> + /*
> + * Use a copy of the resource table for the remainder of the
> + * shutdown process.
> + */
> + rproc->table_ptr = rproc->cached_table;
> +
> + /*
> + * Reset the memory area where the firmware loaded the resource table
> + * to its original value. That way when we re-attach the remote
> + * processor the resource table is clean and ready to be used again.
> + */
> + memcpy(table_ptr, rproc->clean_table, rproc->table_sz);
> +
> + /*
> + * The clean resource table is no longer needed. Allocated in
> + * rproc_set_rsc_table().
> + */
> + kfree(rproc->clean_table);
> +
> + return 0;
> +}
> +
> /*
> * Attach to remote processor - similar to rproc_fw_boot() but without
> * the steps that deal with the firmware image.
> @@ -1721,6 +1792,9 @@ static int __rproc_detach(struct rproc *rproc)
> /* Stop any subdevices for the remote processor */
> rproc_stop_subdevices(rproc, false);
>
> + /* the installed resource table is no longer accessible */
> + ret = rproc_reset_rsc_table_on_detach(rproc);
> +
Something seems missing here to treat the error case.
Regards,
Arnaud
> /* Tell the remote processor the core isn't available anymore */
> ret = rproc->ops->detach(rproc);
> if (ret) {
> @@ -1997,6 +2071,9 @@ int rproc_detach(struct rproc *rproc)
>
> rproc_disable_iommu(rproc);
>
> + /* Free the copy of the resource table */
> + kfree(rproc->cached_table);
> + rproc->cached_table = NULL;
> rproc->table_ptr = NULL;
> out:
> mutex_unlock(&rproc->lock);
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index e1c843c19cc6..e5f52a12a650 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -514,6 +514,8 @@ struct rproc_dump_segment {
> * @recovery_disabled: flag that state if recovery was disabled
> * @max_notifyid: largest allocated notify id.
> * @table_ptr: pointer to the resource table in effect
> + * @clean_table: copy of the resource table without modifications. Used
> + * when a remote processor is attached or detached from the core
> * @cached_table: copy of the resource table
> * @table_sz: size of @cached_table
> * @has_iommu: flag to indicate if remote processor is behind an MMU
> @@ -550,6 +552,7 @@ struct rproc {
> bool recovery_disabled;
> int max_notifyid;
> struct resource_table *table_ptr;
> + struct resource_table *clean_table;
> struct resource_table *cached_table;
> size_t table_sz;
> bool has_iommu;
>
On 3/11/21 12:53 AM, Bjorn Andersson wrote:
> On Wed 10 Mar 15:10 CST 2021, Mathieu Poirier wrote:
>
>> When a remote processor that was attached to is stopped, special care
>> must be taken to make sure the shutdown process is similar to what
>> it would be had it been started by the remoteproc core.
>>
>> This patch takes care of that by making a copy of the resource
>> table currently used by the remote processor. From that point on
>> the copy is used, as if the remote processor had been started by
>> the remoteproc core.
>>
>> Signed-off-by: Mathieu Poirier <[email protected]>
>> ---
>> New for V7:
>> New Patch, used to be part of 11/16 in V6.
>> ---
>> drivers/remoteproc/remoteproc_core.c | 53 +++++++++++++++++++++++++++-
>> 1 file changed, 52 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>> index e9ea2558432d..c488b1aa6119 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -1634,6 +1634,52 @@ static int rproc_reset_rsc_table_on_detach(struct rproc *rproc)
>> return 0;
>> }
>>
>> +static int rproc_reset_rsc_table_on_stop(struct rproc *rproc)
>> +{
>> + struct resource_table *table_ptr;
>> +
>> + /* A resource table was never retrieved, nothing to do here */
>> + if (!rproc->table_ptr)
>> + return 0;
>> +
>> + /*
>> + * If a cache table exists the remote processor was started by
>> + * the remoteproc core. That cache table should be used for
>> + * the rest of the shutdown process.
>> + */
>> + if (rproc->cached_table)
>> + goto out;
>> +
>> + /* Remember where the external entity installed the resource table */
>> + table_ptr = rproc->table_ptr;
>> +
>
> Afaict this is just a remnant from the detach case.
>
> I think the series looks really good now, please let me know and I can
> drop the local "table_ptr" as I apply the patches.
>
Just a minor comment on patch 11, then the series LGTM also,
For this one
Reviewed-by: Arnaud Pouliquen <[email protected]>
Thanks,
Arnaud
> Regards,
> Bjorn
>
>> + /*
>> + * If we made it here the remote processor was started by another
>> + * entity and a cache table doesn't exist. As such make a copy of
>> + * the resource table currently used by the remote processor and
>> + * use that for the rest of the shutdown process. The memory
>> + * allocated here is free'd in rproc_shutdown().
>> + */
>> + rproc->cached_table = kmemdup(rproc->table_ptr,
>> + rproc->table_sz, GFP_KERNEL);
>> + if (!rproc->cached_table)
>> + return -ENOMEM;
>> +
>> + /*
>> + * Since the remote processor is being switched off the clean table
>> + * won't be needed. Allocated in rproc_set_rsc_table().
>> + */
>> + kfree(rproc->clean_table);
>> +
>> +out:
>> + /*
>> + * Use a copy of the resource table for the remainder of the
>> + * shutdown process.
>> + */
>> + rproc->table_ptr = rproc->cached_table;
>> + return 0;
>> +}
>> +
>> /*
>> * Attach to remote processor - similar to rproc_fw_boot() but without
>> * the steps that deal with the firmware image.
>> @@ -1759,7 +1805,12 @@ static int rproc_stop(struct rproc *rproc, bool crashed)
>> rproc_stop_subdevices(rproc, crashed);
>>
>> /* the installed resource table is no longer accessible */
>> - rproc->table_ptr = rproc->cached_table;
>> + ret = rproc_reset_rsc_table_on_stop(rproc);
>> + if (ret) {
>> + dev_err(dev, "can't reset resource table: %d\n", ret);
>> + return ret;
>> + }
>> +
>>
>> /* power off the remote processor */
>> ret = rproc->ops->stop(rproc);
>> --
>> 2.25.1
>>
On Wed, Mar 10, 2021 at 05:53:04PM -0600, Bjorn Andersson wrote:
> On Wed 10 Mar 15:10 CST 2021, Mathieu Poirier wrote:
>
> > When a remote processor that was attached to is stopped, special care
> > must be taken to make sure the shutdown process is similar to what
> > it would be had it been started by the remoteproc core.
> >
> > This patch takes care of that by making a copy of the resource
> > table currently used by the remote processor. From that point on
> > the copy is used, as if the remote processor had been started by
> > the remoteproc core.
> >
> > Signed-off-by: Mathieu Poirier <[email protected]>
> > ---
> > New for V7:
> > New Patch, used to be part of 11/16 in V6.
> > ---
> > drivers/remoteproc/remoteproc_core.c | 53 +++++++++++++++++++++++++++-
> > 1 file changed, 52 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > index e9ea2558432d..c488b1aa6119 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -1634,6 +1634,52 @@ static int rproc_reset_rsc_table_on_detach(struct rproc *rproc)
> > return 0;
> > }
> >
> > +static int rproc_reset_rsc_table_on_stop(struct rproc *rproc)
> > +{
> > + struct resource_table *table_ptr;
> > +
> > + /* A resource table was never retrieved, nothing to do here */
> > + if (!rproc->table_ptr)
> > + return 0;
> > +
> > + /*
> > + * If a cache table exists the remote processor was started by
> > + * the remoteproc core. That cache table should be used for
> > + * the rest of the shutdown process.
> > + */
> > + if (rproc->cached_table)
> > + goto out;
> > +
> > + /* Remember where the external entity installed the resource table */
> > + table_ptr = rproc->table_ptr;
> > +
>
> Afaict this is just a remnant from the detach case.
>
> I think the series looks really good now, please let me know and I can
> drop the local "table_ptr" as I apply the patches.
(sigh) No matter how long you stare at a patchset there is always one that gets
away. I will address Arnaud's comment and fix this in a new revision.
Thanks,
Mathieu
>
> Regards,
> Bjorn
>
> > + /*
> > + * If we made it here the remote processor was started by another
> > + * entity and a cache table doesn't exist. As such make a copy of
> > + * the resource table currently used by the remote processor and
> > + * use that for the rest of the shutdown process. The memory
> > + * allocated here is free'd in rproc_shutdown().
> > + */
> > + rproc->cached_table = kmemdup(rproc->table_ptr,
> > + rproc->table_sz, GFP_KERNEL);
> > + if (!rproc->cached_table)
> > + return -ENOMEM;
> > +
> > + /*
> > + * Since the remote processor is being switched off the clean table
> > + * won't be needed. Allocated in rproc_set_rsc_table().
> > + */
> > + kfree(rproc->clean_table);
> > +
> > +out:
> > + /*
> > + * Use a copy of the resource table for the remainder of the
> > + * shutdown process.
> > + */
> > + rproc->table_ptr = rproc->cached_table;
> > + return 0;
> > +}
> > +
> > /*
> > * Attach to remote processor - similar to rproc_fw_boot() but without
> > * the steps that deal with the firmware image.
> > @@ -1759,7 +1805,12 @@ static int rproc_stop(struct rproc *rproc, bool crashed)
> > rproc_stop_subdevices(rproc, crashed);
> >
> > /* the installed resource table is no longer accessible */
> > - rproc->table_ptr = rproc->cached_table;
> > + ret = rproc_reset_rsc_table_on_stop(rproc);
> > + if (ret) {
> > + dev_err(dev, "can't reset resource table: %d\n", ret);
> > + return ret;
> > + }
> > +
> >
> > /* power off the remote processor */
> > ret = rproc->ops->stop(rproc);
> > --
> > 2.25.1
> >