2020-06-01 17:53:56

by Mathieu Poirier

Subject: [PATCH v4 0/9] remoteproc: Add support for attaching with rproc

This fourth iteration implements a solution that is fairly different from
what was proposed in V3 and earlier versions. Three aspects have been
revisited:

1) Only the scenario where the remoteproc core is attaching to the remote
processor is implemented. Other scenarios where actions need to be
taken when the remote processor is stopped or crashes will be
considered in subsequent versions.

2) The introduction of a new RPROC_DETACHED state to be set by platform
drivers when needing to attach to an already running remote processor.

3) New functions are introduced to replicate the functionality provided by
rproc_fw_boot() and rproc_start(), minus operations related to firmware
management.

Enhancement to the documentation has been left out intentionally until it
is agreed to move forward with this implementation.

Applies cleanly on rproc-next(7dcef3988eed) and will be rebased on v5.8-rc1
when it comes out in two weeks.

Thanks,
Mathieu

Mathieu Poirier (9):
remoteproc: Add new RPROC_DETACHED state
remoteproc: Add new attach() remoteproc operation
remoteproc: Introducing function rproc_attach()
remoteproc: Introducing function rproc_actuate()
remoteproc: Introducing function rproc_validate()
remoteproc: Refactor function rproc_boot()
remoteproc: Refactor function rproc_trigger_auto_boot()
remoteproc: Refactor function rproc_free_vring()
remoteproc: Properly handle firmware name when attaching

drivers/remoteproc/remoteproc_core.c | 226 +++++++++++++++++++++--
drivers/remoteproc/remoteproc_internal.h | 8 +
drivers/remoteproc/remoteproc_sysfs.c | 17 +-
include/linux/remoteproc.h | 9 +-
4 files changed, 243 insertions(+), 17 deletions(-)

--
2.20.1

2020-06-01 17:54:00

by Mathieu Poirier

Subject: [PATCH v4 2/9] remoteproc: Add new attach() remoteproc operation

Add an new attach() operation in order to properly deal with
scenarios where the remoteproc core needs to attach to a
remote processor that has been booted by another entity.

Signed-off-by: Mathieu Poirier <[email protected]>
---
drivers/remoteproc/remoteproc_internal.h | 8 ++++++++
include/linux/remoteproc.h | 2 ++
2 files changed, 10 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
index 4ba7cb59d3e8..fc710866f8ce 100644
--- a/drivers/remoteproc/remoteproc_internal.h
+++ b/drivers/remoteproc/remoteproc_internal.h
@@ -79,6 +79,14 @@ static inline int rproc_unprepare_device(struct rproc *rproc)
return 0;
}

+static inline int rproc_attach_device(struct rproc *rproc)
+{
+ if (rproc->ops->attach)
+ return rproc->ops->attach(rproc);
+
+ return 0;
+}
+
static inline
int rproc_fw_sanity_check(struct rproc *rproc, const struct firmware *fw)
{
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 21182ad2d059..bf6a310ba870 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -359,6 +359,7 @@ enum rsc_handling_status {
* @unprepare: unprepare device after stop
* @start: power on the device and boot it
* @stop: power off the device
+ * @attach: attach to a device that his already 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)
@@ -379,6 +380,7 @@ struct rproc_ops {
int (*unprepare)(struct rproc *rproc);
int (*start)(struct rproc *rproc);
int (*stop)(struct rproc *rproc);
+ int (*attach)(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.20.1

2020-06-01 17:54:17

by Mathieu Poirier

Subject: [PATCH v4 9/9] remoteproc: Properly handle firmware name when attaching

This patch prevents the firmware image name from being displayed when
the remoteproc core is attaching to a remote processor. This is needed
needed since there is no guarantee about the nature of the firmware
image that is loaded by the external entity.

Signed-off-by: Mathieu Poirier <[email protected]>
---
drivers/remoteproc/remoteproc_core.c | 18 ++++++++++++++++++
drivers/remoteproc/remoteproc_sysfs.c | 16 ++++++++++++++--
include/linux/remoteproc.h | 2 ++
3 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 0e23284fbd25..a8adc712e7f6 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1642,6 +1642,14 @@ 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;
@@ -2166,6 +2174,16 @@ int rproc_add(struct rproc *rproc)
/* create debugfs entries */
rproc_create_debug_dir(rproc);

+ /*
+ * 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 8b462c501465..4ee158431f67 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -14,8 +14,20 @@ static ssize_t firmware_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
struct rproc *rproc = to_rproc(dev);
-
- return sprintf(buf, "%s\n", rproc->firmware);
+ const char *firmware = rproc->firmware;
+
+ /*
+ * 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)
+ firmware = "unknown";
+
+ return sprintf(buf, "%s\n", firmware);
}

/* Change firmware name via sysfs */
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index bf6a310ba870..cf5e31556780 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -491,6 +491,7 @@ 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
*/
@@ -524,6 +525,7 @@ 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.20.1

2020-06-01 17:54:20

by Mathieu Poirier

Subject: [PATCH v4 8/9] remoteproc: Refactor function rproc_free_vring()

When function rproc_free_vring() clears the virtio device section
it does so on the cached resource table rather than the one
installed in the remote processor memory. When a remote processor
has been booted by another entity there is no need to use a cached
table and as such, no need to clear the virtio device section in
it.

Signed-off-by: Mathieu Poirier <[email protected]>
---
drivers/remoteproc/remoteproc_core.c | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index d32ac8f0c872..0e23284fbd25 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -411,10 +411,22 @@ void rproc_free_vring(struct rproc_vring *rvring)

idr_remove(&rproc->notifyids, rvring->notifyid);

- /* reset resource entry info */
- rsc = (void *)rproc->table_ptr + rvring->rvdev->rsc_offset;
- rsc->vring[idx].da = 0;
- rsc->vring[idx].notifyid = -1;
+ /*
+ * At this point rproc_stop() has been called and the installed resource
+ * table in the remote processor memory may no longer be accessible. As
+ * such and as per rproc_stop(), rproc->table_ptr points to the cached
+ * resource table (rproc->cached_table). The cached resource table is
+ * only available when a remote processor has been booted by the
+ * remoteproc core, otherwise it is NULL.
+ *
+ * Based on the above, reset the virtio device section in the cached
+ * resource table only if there is one to work with.
+ */
+ if (rproc->table_ptr) {
+ rsc = (void *)rproc->table_ptr + rvring->rvdev->rsc_offset;
+ rsc->vring[idx].da = 0;
+ rsc->vring[idx].notifyid = -1;
+ }
}

static int rproc_vdev_do_start(struct rproc_subdev *subdev)
--
2.20.1

2020-06-01 17:54:31

by Mathieu Poirier

Subject: [PATCH v4 7/9] remoteproc: Refactor function rproc_trigger_auto_boot()

Refactor function rproc_trigger_auto_boot() to properly deal
with scenarios where the remoteproc core needs to attach with a
remote processor that has already been booted by an external
entity.

Signed-off-by: Mathieu Poirier <[email protected]>
---
drivers/remoteproc/remoteproc_core.c | 9 +++++++++
1 file changed, 9 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 48ddc29814be..d32ac8f0c872 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1586,6 +1586,15 @@ static int rproc_trigger_auto_boot(struct rproc *rproc)
{
int ret;

+ /*
+ * Since the remote processor is in a detached state, it has already
+ * been booted by another entity. As such there is no point in waiting
+ * for a firmware image to be loaded, we can simply initiate the process
+ * of attaching to it immediately.
+ */
+ if (rproc->state == RPROC_DETACHED)
+ return rproc_boot(rproc);
+
/*
* We're initiating an asynchronous firmware loading, so we can
* be built-in kernel code, without hanging the boot process.
--
2.20.1

2020-06-01 17:54:40

by Mathieu Poirier

Subject: [PATCH v4 5/9] remoteproc: Introducing function rproc_validate()

Add a new function to assert the general health of the remote
processor before handing it to the remoteproc core.

Signed-off-by: Mathieu Poirier <[email protected]>
---
drivers/remoteproc/remoteproc_core.c | 45 ++++++++++++++++++++++++++++
1 file changed, 45 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index c70fa0372d07..0be8343dd851 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -2060,6 +2060,47 @@ struct rproc *rproc_get_by_phandle(phandle phandle)
#endif
EXPORT_SYMBOL(rproc_get_by_phandle);

+static int rproc_validate(struct rproc *rproc)
+{
+ /*
+ * When adding a remote processor, the state of the device
+ * can be offline or detached, nothing else.
+ */
+ if (rproc->state != RPROC_OFFLINE &&
+ rproc->state != RPROC_DETACHED)
+ goto inval;
+
+ if (rproc->state == RPROC_OFFLINE) {
+ /*
+ * An offline processor without a start()
+ * function makes no sense.
+ */
+ if (!rproc->ops->start)
+ goto inval;
+ }
+
+ if (rproc->state == RPROC_DETACHED) {
+ /*
+ * A remote processor in a detached state without an
+ * attach() function makes not sense.
+ */
+ if (!rproc->ops->attach)
+ goto inval;
+ /*
+ * When attaching to a remote processor the device memory
+ * is already available and as such there is no need to have a
+ * cached table.
+ */
+ if (rproc->cached_table)
+ goto inval;
+ }
+
+ return 0;
+
+inval:
+ return -EINVAL;
+}
+
/**
* rproc_add() - register a remote processor
* @rproc: the remote processor handle to register
@@ -2089,6 +2130,10 @@ int rproc_add(struct rproc *rproc)
if (ret < 0)
return ret;

+ ret = rproc_validate(rproc);
+ if (ret < 0)
+ return ret;
+
dev_info(dev, "%s is available\n", rproc->name);

/* create debugfs entries */
--
2.20.1

2020-06-01 17:55:11

by Mathieu Poirier

Subject: [PATCH v4 3/9] remoteproc: Introducing function rproc_attach()

Introducing function rproc_attach() to enact the same actions as
rproc_start(), but without the steps related to the handling of
a firmware image. That way we can properly deal with scenarios
where the remoteproc core needs to attach with a remote processsor
that has been booted by another entity.

Signed-off-by: Mathieu Poirier <[email protected]>
---
drivers/remoteproc/remoteproc_core.c | 42 ++++++++++++++++++++++++++++
1 file changed, 42 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 9f04c30c4aaf..0b323f6b554b 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1370,6 +1370,48 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
return ret;
}

+static int __maybe_unused rproc_attach(struct rproc *rproc)
+{
+ struct device *dev = &rproc->dev;
+ int ret;
+
+ ret = rproc_prepare_subdevices(rproc);
+ if (ret) {
+ dev_err(dev, "failed to prepare subdevices for %s: %d\n",
+ rproc->name, ret);
+ goto out;
+ }
+
+ /* Attach to the remote processor */
+ ret = rproc_attach_device(rproc);
+ if (ret) {
+ dev_err(dev, "can't attach to rproc %s: %d\n",
+ rproc->name, ret);
+ goto unprepare_subdevices;
+ }
+
+ /* Start any subdevices for the remote processor */
+ ret = rproc_start_subdevices(rproc);
+ if (ret) {
+ dev_err(dev, "failed to probe subdevices for %s: %d\n",
+ rproc->name, ret);
+ goto stop_rproc;
+ }
+
+ rproc->state = RPROC_RUNNING;
+
+ dev_info(dev, "remote processor %s is now attached\n", rproc->name);
+
+ return 0;
+
+stop_rproc:
+ rproc->ops->stop(rproc);
+unprepare_subdevices:
+ rproc_unprepare_subdevices(rproc);
+out:
+ return ret;
+}
+
/*
* take a firmware and boot a remote processor with it.
*/
--
2.20.1

2020-06-01 17:55:16

by Mathieu Poirier

Subject: [PATCH v4 4/9] remoteproc: Introducing function rproc_actuate()

Introduce function rproc_actuate() that provides the same
functionatlity as rproc_fw_boot(), but without the steps that
involve interaction with the firmware image. That way we can
deal with scenarios where the remoteproc core is attaching
to a remote processor that has already been started by another
entity.

Signed-off-by: Mathieu Poirier <[email protected]>
---
drivers/remoteproc/remoteproc_core.c | 68 +++++++++++++++++++++++++++-
1 file changed, 67 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 0b323f6b554b..c70fa0372d07 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1370,7 +1370,7 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
return ret;
}

-static int __maybe_unused rproc_attach(struct rproc *rproc)
+static int rproc_attach(struct rproc *rproc)
{
struct device *dev = &rproc->dev;
int ret;
@@ -1499,6 +1499,72 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
return ret;
}

+/*
+ * Attach to remote processor - similar to rproc_fw_boot() but without
+ * the steps that deal with the firmware image.
+ */
+static int __maybe_unused rproc_actuate(struct rproc *rproc)
+{
+ struct device *dev = &rproc->dev;
+ int ret;
+
+ /* Tell the PM runtime core we need to keep this device powered */
+ ret = pm_runtime_get_sync(dev);
+ if (ret < 0) {
+ dev_err(dev, "pm_runtime_get_sync failed: %d\n", ret);
+ return ret;
+ }
+
+ /*
+ * if enabling an IOMMU isn't relevant for this rproc, this is
+ * just a nop
+ */
+ ret = rproc_enable_iommu(rproc);
+ if (ret) {
+ dev_err(dev, "can't enable iommu: %d\n", ret);
+ goto put_pm_runtime;
+ }
+
+ /* reset max_notifyid */
+ rproc->max_notifyid = -1;
+
+ /* reset handled vdev */
+ rproc->nb_vdev = 0;
+
+ /*
+ * Handle firmware resources required to attach to a remote processor.
+ * Because we are attaching rather than booting the remote processor,
+ * we expect the platform driver to properly set rproc->table_ptr.
+ */
+ ret = rproc_handle_resources(rproc, rproc_loading_handlers);
+ if (ret) {
+ dev_err(dev, "Failed to process resources: %d\n", ret);
+ goto disable_iommu;
+ }
+
+ /* Allocate carveout resources associated to rproc */
+ ret = rproc_alloc_registered_carveouts(rproc);
+ if (ret) {
+ dev_err(dev, "Failed to allocate associated carveouts: %d\n",
+ ret);
+ goto clean_up_resources;
+ }
+
+ ret = rproc_attach(rproc);
+ if (ret)
+ goto clean_up_resources;
+
+ return 0;
+
+clean_up_resources:
+ rproc_resource_cleanup(rproc);
+disable_iommu:
+ rproc_disable_iommu(rproc);
+put_pm_runtime:
+ pm_runtime_put(dev);
+ return ret;
+}
+
/*
* take a firmware and boot it up.
*
--
2.20.1

2020-06-01 17:56:07

by Mathieu Poirier

Subject: [PATCH v4 6/9] remoteproc: Refactor function rproc_boot()

Refactor function rproc_boot() to properly deal with scenarios
where the remoteproc core needs to attach with a remote
processor that has already been booted by an external entity.

Signed-off-by: Mathieu Poirier <[email protected]>
---
drivers/remoteproc/remoteproc_core.c | 28 +++++++++++++++++-----------
1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 0be8343dd851..48ddc29814be 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1503,7 +1503,7 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
* Attach to remote processor - similar to rproc_fw_boot() but without
* the steps that deal with the firmware image.
*/
-static int __maybe_unused rproc_actuate(struct rproc *rproc)
+static int rproc_actuate(struct rproc *rproc)
{
struct device *dev = &rproc->dev;
int ret;
@@ -1923,24 +1923,30 @@ int rproc_boot(struct rproc *rproc)
goto unlock_mutex;
}

- /* skip the boot process if rproc is already powered up */
+ /* skip the boot or attach process if rproc is already powered up */
if (atomic_inc_return(&rproc->power) > 1) {
ret = 0;
goto unlock_mutex;
}

- dev_info(dev, "powering up %s\n", rproc->name);
+ if (rproc->state == RPROC_DETACHED) {
+ dev_info(dev, "attaching to %s\n", rproc->name);

- /* load firmware */
- ret = request_firmware(&firmware_p, rproc->firmware, dev);
- if (ret < 0) {
- dev_err(dev, "request_firmware failed: %d\n", ret);
- goto downref_rproc;
- }
+ ret = rproc_actuate(rproc);
+ } else {
+ dev_info(dev, "powering up %s\n", rproc->name);

- ret = rproc_fw_boot(rproc, firmware_p);
+ /* load firmware */
+ ret = request_firmware(&firmware_p, rproc->firmware, dev);
+ if (ret < 0) {
+ dev_err(dev, "request_firmware failed: %d\n", ret);
+ goto downref_rproc;
+ }

- release_firmware(firmware_p);
+ ret = rproc_fw_boot(rproc, firmware_p);
+
+ release_firmware(firmware_p);
+ }

downref_rproc:
if (ret)
--
2.20.1

2020-06-01 17:57:33

by Mathieu Poirier

Subject: [PATCH v4 1/9] remoteproc: Add new RPROC_DETACHED state

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

Signed-off-by: Mathieu Poirier <[email protected]>
---
drivers/remoteproc/remoteproc_sysfs.c | 1 +
include/linux/remoteproc.h | 5 ++++-
2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
index 7f8536b73295..8b462c501465 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -71,6 +71,7 @@ static const char * const rproc_state_string[] = {
[RPROC_RUNNING] = "running",
[RPROC_CRASHED] = "crashed",
[RPROC_DELETED] = "deleted",
+ [RPROC_DETACHED] = "detached",
[RPROC_LAST] = "invalid",
};

diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index e7b7bab8b235..21182ad2d059 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -400,6 +400,8 @@ struct rproc_ops {
* @RPROC_RUNNING: device is up and running
* @RPROC_CRASHED: device has crashed; need to start recovery
* @RPROC_DELETED: device is deleted
+ * @RPROC_DETACHED: device has been booted by another entity and waiting
+ * for the core to attach to it
* @RPROC_LAST: just keep this one at the end
*
* Please note that the values of these states are used as indices
@@ -414,7 +416,8 @@ enum rproc_state {
RPROC_RUNNING = 2,
RPROC_CRASHED = 3,
RPROC_DELETED = 4,
- RPROC_LAST = 5,
+ RPROC_DETACHED = 5,
+ RPROC_LAST = 6,
};

/**
--
2.20.1

2020-06-04 14:22:42

by Arnaud POULIQUEN

Subject: Re: [PATCH v4 9/9] remoteproc: Properly handle firmware name when attaching

Hi Mathieu,

On 6/1/20 7:51 PM, Mathieu Poirier wrote:
> This patch prevents the firmware image name from being displayed when
> the remoteproc core is attaching to a remote processor. This is needed
> needed since there is no guarantee about the nature of the firmware
> image that is loaded by the external entity.
>
> Signed-off-by: Mathieu Poirier <[email protected]>
> ---
> drivers/remoteproc/remoteproc_core.c | 18 ++++++++++++++++++
> drivers/remoteproc/remoteproc_sysfs.c | 16 ++++++++++++++--
> include/linux/remoteproc.h | 2 ++
> 3 files changed, 34 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 0e23284fbd25..a8adc712e7f6 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1642,6 +1642,14 @@ 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;
> @@ -2166,6 +2174,16 @@ int rproc_add(struct rproc *rproc)
> /* create debugfs entries */
> rproc_create_debug_dir(rproc);
>
> + /*
> + * 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 8b462c501465..4ee158431f67 100644
> --- a/drivers/remoteproc/remoteproc_sysfs.c
> +++ b/drivers/remoteproc/remoteproc_sysfs.c
> @@ -14,8 +14,20 @@ static ssize_t firmware_show(struct device *dev, struct device_attribute *attr,
> char *buf)
> {
> struct rproc *rproc = to_rproc(dev);
> -
> - return sprintf(buf, "%s\n", rproc->firmware);
> + const char *firmware = rproc->firmware;
> +
> + /*
> + * 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)
> + firmware = "unknown";
> +
> + return sprintf(buf, "%s\n", firmware);
> }
>
> /* Change firmware name via sysfs */
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index bf6a310ba870..cf5e31556780 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -491,6 +491,7 @@ 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
> */
> @@ -524,6 +525,7 @@ struct rproc {
> size_t table_sz;
> bool has_iommu;
> bool auto_boot;
> + bool autonomous;

Do you need to define a new field? what about using rproc->firmware value?

In this case the platform driver could provide a name using rproc_alloc
even if in attached mode, for instance to identify a firmware version...

Regards,
Arnaud



> struct list_head dump_segments;
> int nb_vdev;
> u8 elf_class;
>

2020-06-04 14:29:40

by Arnaud POULIQUEN

Subject: Re: [PATCH v4 0/9] remoteproc: Add support for attaching with rproc

Hi Mathieu,

On 6/1/20 7:51 PM, Mathieu Poirier wrote:
> This fourth iteration implements a solution that is fairly different from
> what was proposed in V3 and earlier versions. Three aspects have been
> revisited:
>
> 1) Only the scenario where the remoteproc core is attaching to the remote
> processor is implemented. Other scenarios where actions need to be
> taken when the remote processor is stopped or crashes will be
> considered in subsequent versions.
>
> 2) The introduction of a new RPROC_DETACHED state to be set by platform
> drivers when needing to attach to an already running remote processor.
>
> 3) New functions are introduced to replicate the functionality provided by
> rproc_fw_boot() and rproc_start(), minus operations related to firmware
> management.
>
> Enhancement to the documentation has been left out intentionally until it
> is agreed to move forward with this implementation.

Look good to me, i have only a minor concerns about the code duplication
introduced by the point 3)

If you are agree with that, I plan to do a new review on the stm32 series
when you will start the documentation :-)

Regards,
Arnaud

>
> Applies cleanly on rproc-next(7dcef3988eed) and will be rebased on v5.8-rc1
> when it comes out in two weeks.
>
> Thanks,
> Mathieu
>
> Mathieu Poirier (9):
> remoteproc: Add new RPROC_DETACHED state
> remoteproc: Add new attach() remoteproc operation
> remoteproc: Introducing function rproc_attach()
> remoteproc: Introducing function rproc_actuate()
> remoteproc: Introducing function rproc_validate()
> remoteproc: Refactor function rproc_boot()
> remoteproc: Refactor function rproc_trigger_auto_boot()
> remoteproc: Refactor function rproc_free_vring()
> remoteproc: Properly handle firmware name when attaching
>
> drivers/remoteproc/remoteproc_core.c | 226 +++++++++++++++++++++--
> drivers/remoteproc/remoteproc_internal.h | 8 +
> drivers/remoteproc/remoteproc_sysfs.c | 17 +-
> include/linux/remoteproc.h | 9 +-
> 4 files changed, 243 insertions(+), 17 deletions(-)
>

2020-06-04 20:20:56

by Mathieu Poirier

Subject: Re: [PATCH v4 9/9] remoteproc: Properly handle firmware name when attaching

Good afternoon,

On Thu, 4 Jun 2020 at 08:17, Arnaud POULIQUEN <[email protected]> wrote:
>
> Hi Mathieu,
>
> On 6/1/20 7:51 PM, Mathieu Poirier wrote:
> > This patch prevents the firmware image name from being displayed when
> > the remoteproc core is attaching to a remote processor. This is needed
> > needed since there is no guarantee about the nature of the firmware
> > image that is loaded by the external entity.
> >
> > Signed-off-by: Mathieu Poirier <[email protected]>
> > ---
> > drivers/remoteproc/remoteproc_core.c | 18 ++++++++++++++++++
> > drivers/remoteproc/remoteproc_sysfs.c | 16 ++++++++++++++--
> > include/linux/remoteproc.h | 2 ++
> > 3 files changed, 34 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > index 0e23284fbd25..a8adc712e7f6 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -1642,6 +1642,14 @@ 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;
> > @@ -2166,6 +2174,16 @@ int rproc_add(struct rproc *rproc)
> > /* create debugfs entries */
> > rproc_create_debug_dir(rproc);
> >
> > + /*
> > + * 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 8b462c501465..4ee158431f67 100644
> > --- a/drivers/remoteproc/remoteproc_sysfs.c
> > +++ b/drivers/remoteproc/remoteproc_sysfs.c
> > @@ -14,8 +14,20 @@ static ssize_t firmware_show(struct device *dev, struct device_attribute *attr,
> > char *buf)
> > {
> > struct rproc *rproc = to_rproc(dev);
> > -
> > - return sprintf(buf, "%s\n", rproc->firmware);
> > + const char *firmware = rproc->firmware;
> > +
> > + /*
> > + * 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)
> > + firmware = "unknown";
> > +
> > + return sprintf(buf, "%s\n", firmware);
> > }
> >
> > /* Change firmware name via sysfs */
> > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > index bf6a310ba870..cf5e31556780 100644
> > --- a/include/linux/remoteproc.h
> > +++ b/include/linux/remoteproc.h
> > @@ -491,6 +491,7 @@ 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
> > */
> > @@ -524,6 +525,7 @@ struct rproc {
> > size_t table_sz;
> > bool has_iommu;
> > bool auto_boot;
> > + bool autonomous;
>
> Do you need to define a new field? what about using rproc->firmware value?
>
> In this case the platform driver could provide a name using rproc_alloc
> even if in attached mode, for instance to identify a firmware version...

The problem is often that what gets loaded by the external entity is
not the same as the firmware available to the kernel. As such
displaying the firmware name provided by the platform driver may not
be accurate when attaching to a remote processor. Moreover I had to
introduce a new flag because the RPROC_ATTACHED state disappears as
soon as the remoteproc core attaches to the remote processor. When
attached the state is set to RPROC_RUNNING, but there is still no
telling as to what firmware is running on the remote processor.

Thanks,
Mathieu

>
> Regards,
> Arnaud
>
>
>
> > struct list_head dump_segments;
> > int nb_vdev;
> > u8 elf_class;
> >

2020-06-04 20:30:07

by Mathieu Poirier

Subject: Re: [PATCH v4 0/9] remoteproc: Add support for attaching with rproc

On Thu, 4 Jun 2020 at 08:24, Arnaud POULIQUEN <[email protected]> wrote:
>
> Hi Mathieu,
>
> On 6/1/20 7:51 PM, Mathieu Poirier wrote:
> > This fourth iteration implements a solution that is fairly different from
> > what was proposed in V3 and earlier versions. Three aspects have been
> > revisited:
> >
> > 1) Only the scenario where the remoteproc core is attaching to the remote
> > processor is implemented. Other scenarios where actions need to be
> > taken when the remote processor is stopped or crashes will be
> > considered in subsequent versions.
> >
> > 2) The introduction of a new RPROC_DETACHED state to be set by platform
> > drivers when needing to attach to an already running remote processor.
> >
> > 3) New functions are introduced to replicate the functionality provided by
> > rproc_fw_boot() and rproc_start(), minus operations related to firmware
> > management.
> >
> > Enhancement to the documentation has been left out intentionally until it
> > is agreed to move forward with this implementation.
>
> Look good to me, i have only a minor concerns about the code duplication
> introduced by the point 3)
>

This is an idea Bjorn and I have decided to try in the hope of making
the state machine, and the feature as a whole, easier to understand.
It might be one of those rare cases where more code is better.

> If you are agree with that, I plan to do a new review on the stm32 series
> when you will start the documentation :-)

A wise decision...

Thanks for taking the time to look at this,
Mathieu

>
> Regards,
> Arnaud
>
> >
> > Applies cleanly on rproc-next(7dcef3988eed) and will be rebased on v5.8-rc1
> > when it comes out in two weeks.
> >
> > Thanks,
> > Mathieu
> >
> > Mathieu Poirier (9):
> > remoteproc: Add new RPROC_DETACHED state
> > remoteproc: Add new attach() remoteproc operation
> > remoteproc: Introducing function rproc_attach()
> > remoteproc: Introducing function rproc_actuate()
> > remoteproc: Introducing function rproc_validate()
> > remoteproc: Refactor function rproc_boot()
> > remoteproc: Refactor function rproc_trigger_auto_boot()
> > remoteproc: Refactor function rproc_free_vring()
> > remoteproc: Properly handle firmware name when attaching
> >
> > drivers/remoteproc/remoteproc_core.c | 226 +++++++++++++++++++++--
> > drivers/remoteproc/remoteproc_internal.h | 8 +
> > drivers/remoteproc/remoteproc_sysfs.c | 17 +-
> > include/linux/remoteproc.h | 9 +-
> > 4 files changed, 243 insertions(+), 17 deletions(-)
> >

2020-06-22 06:30:19

by Bjorn Andersson

Subject: Re: [PATCH v4 1/9] remoteproc: Add new RPROC_DETACHED state

On Mon 01 Jun 10:51 PDT 2020, Mathieu Poirier wrote:

> Add a new RPROC_DETACHED state to take into account scenarios
> where the remoteproc core needs to attach to a remote processor
> that is booted by another entity.
>
> Signed-off-by: Mathieu Poirier <[email protected]>

Reviewed-by: Bjorn Andersson <[email protected]>

Regards,
Bjorn

> ---
> drivers/remoteproc/remoteproc_sysfs.c | 1 +
> include/linux/remoteproc.h | 5 ++++-
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
> index 7f8536b73295..8b462c501465 100644
> --- a/drivers/remoteproc/remoteproc_sysfs.c
> +++ b/drivers/remoteproc/remoteproc_sysfs.c
> @@ -71,6 +71,7 @@ static const char * const rproc_state_string[] = {
> [RPROC_RUNNING] = "running",
> [RPROC_CRASHED] = "crashed",
> [RPROC_DELETED] = "deleted",
> + [RPROC_DETACHED] = "detached",
> [RPROC_LAST] = "invalid",
> };
>
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index e7b7bab8b235..21182ad2d059 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -400,6 +400,8 @@ struct rproc_ops {
> * @RPROC_RUNNING: device is up and running
> * @RPROC_CRASHED: device has crashed; need to start recovery
> * @RPROC_DELETED: device is deleted
> + * @RPROC_DETACHED: device has been booted by another entity and waiting
> + * for the core to attach to it
> * @RPROC_LAST: just keep this one at the end
> *
> * Please note that the values of these states are used as indices
> @@ -414,7 +416,8 @@ enum rproc_state {
> RPROC_RUNNING = 2,
> RPROC_CRASHED = 3,
> RPROC_DELETED = 4,
> - RPROC_LAST = 5,
> + RPROC_DETACHED = 5,
> + RPROC_LAST = 6,
> };
>
> /**
> --
> 2.20.1
>

2020-06-22 06:43:39

by Bjorn Andersson

Subject: Re: [PATCH v4 2/9] remoteproc: Add new attach() remoteproc operation

On Mon 01 Jun 10:51 PDT 2020, Mathieu Poirier wrote:

> Add an new attach() operation in order to properly deal with
> scenarios where the remoteproc core needs to attach to a
> remote processor that has been booted by another entity.
>
> Signed-off-by: Mathieu Poirier <[email protected]>
> ---
> drivers/remoteproc/remoteproc_internal.h | 8 ++++++++
> include/linux/remoteproc.h | 2 ++
> 2 files changed, 10 insertions(+)
>
> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
> index 4ba7cb59d3e8..fc710866f8ce 100644
> --- a/drivers/remoteproc/remoteproc_internal.h
> +++ b/drivers/remoteproc/remoteproc_internal.h
> @@ -79,6 +79,14 @@ static inline int rproc_unprepare_device(struct rproc *rproc)
> return 0;
> }
>
> +static inline int rproc_attach_device(struct rproc *rproc)
> +{
> + if (rproc->ops->attach)
> + return rproc->ops->attach(rproc);
> +
> + return 0;

Afaict we don't allow the registration of a remoteproc in DETACHED state
without an "attach" function specified and as such we shouldn't be able
to end up here.

On the other hand I think it would make sense to have a system where
"attach" simply just means bring up the communication with the remote
processor, so let's keep it as is and we can adjust it as necessary
later.

Reviewed-by: Bjorn Andersson <[email protected]>

Regards,
Bjorn

> +}
> +
> static inline
> int rproc_fw_sanity_check(struct rproc *rproc, const struct firmware *fw)
> {
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 21182ad2d059..bf6a310ba870 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -359,6 +359,7 @@ enum rsc_handling_status {
> * @unprepare: unprepare device after stop
> * @start: power on the device and boot it
> * @stop: power off the device
> + * @attach: attach to a device that his already 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)
> @@ -379,6 +380,7 @@ struct rproc_ops {
> int (*unprepare)(struct rproc *rproc);
> int (*start)(struct rproc *rproc);
> int (*stop)(struct rproc *rproc);
> + int (*attach)(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.20.1
>

2020-06-22 07:13:02

by Bjorn Andersson

Subject: Re: [PATCH v4 3/9] remoteproc: Introducing function rproc_attach()

On Mon 01 Jun 10:51 PDT 2020, Mathieu Poirier wrote:

> Introducing function rproc_attach() to enact the same actions as
> rproc_start(), but without the steps related to the handling of
> a firmware image. That way we can properly deal with scenarios
> where the remoteproc core needs to attach with a remote processsor
> that has been booted by another entity.
>
> Signed-off-by: Mathieu Poirier <[email protected]>
> ---
> drivers/remoteproc/remoteproc_core.c | 42 ++++++++++++++++++++++++++++
> 1 file changed, 42 insertions(+)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 9f04c30c4aaf..0b323f6b554b 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1370,6 +1370,48 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
> return ret;
> }
>
> +static int __maybe_unused rproc_attach(struct rproc *rproc)
> +{
> + struct device *dev = &rproc->dev;
> + int ret;
> +

For the case where we're going DETACHED -> RUNNING - > OFFLINE we
need to consider the pm_runtime (and prepare/unprepare) state of the
device as well...


Apart from that I think this looks good.

Regards,
Bjorn

> + ret = rproc_prepare_subdevices(rproc);
> + if (ret) {
> + dev_err(dev, "failed to prepare subdevices for %s: %d\n",
> + rproc->name, ret);
> + goto out;
> + }
> +
> + /* Attach to the remote processor */
> + ret = rproc_attach_device(rproc);
> + if (ret) {
> + dev_err(dev, "can't attach to rproc %s: %d\n",
> + rproc->name, ret);
> + goto unprepare_subdevices;
> + }
> +
> + /* Start any subdevices for the remote processor */
> + ret = rproc_start_subdevices(rproc);
> + if (ret) {
> + dev_err(dev, "failed to probe subdevices for %s: %d\n",
> + rproc->name, ret);
> + goto stop_rproc;
> + }
> +
> + rproc->state = RPROC_RUNNING;
> +
> + dev_info(dev, "remote processor %s is now attached\n", rproc->name);
> +
> + return 0;
> +
> +stop_rproc:
> + rproc->ops->stop(rproc);
> +unprepare_subdevices:
> + rproc_unprepare_subdevices(rproc);
> +out:
> + return ret;
> +}
> +
> /*
> * take a firmware and boot a remote processor with it.
> */
> --
> 2.20.1
>

2020-06-22 07:22:57

by Bjorn Andersson

Subject: Re: [PATCH v4 3/9] remoteproc: Introducing function rproc_attach()

On Mon 22 Jun 00:07 PDT 2020, Bjorn Andersson wrote:

> On Mon 01 Jun 10:51 PDT 2020, Mathieu Poirier wrote:
>
> > Introducing function rproc_attach() to enact the same actions as
> > rproc_start(), but without the steps related to the handling of
> > a firmware image. That way we can properly deal with scenarios
> > where the remoteproc core needs to attach with a remote processsor
> > that has been booted by another entity.
> >
> > Signed-off-by: Mathieu Poirier <[email protected]>
> > ---
> > drivers/remoteproc/remoteproc_core.c | 42 ++++++++++++++++++++++++++++
> > 1 file changed, 42 insertions(+)
> >
> > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > index 9f04c30c4aaf..0b323f6b554b 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -1370,6 +1370,48 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
> > return ret;
> > }
> >
> > +static int __maybe_unused rproc_attach(struct rproc *rproc)
> > +{
> > + struct device *dev = &rproc->dev;
> > + int ret;
> > +
>
> For the case where we're going DETACHED -> RUNNING - > OFFLINE we
> need to consider the pm_runtime (and prepare/unprepare) state of the
> device as well...
>

Missed that you do indeed pm_runtime_get() in the calling function, so I
think we're good on that part. Still need how to actually implement
that (and the prepare/unprepare), in particular if we're moving into
detaching a remoteproc.

>
> Apart from that I think this looks good.
>

Reviewed-by: Bjorn Andersson <[email protected]>

Regards,
Bjorn

> Regards,
> Bjorn
>
> > + ret = rproc_prepare_subdevices(rproc);
> > + if (ret) {
> > + dev_err(dev, "failed to prepare subdevices for %s: %d\n",
> > + rproc->name, ret);
> > + goto out;
> > + }
> > +
> > + /* Attach to the remote processor */
> > + ret = rproc_attach_device(rproc);
> > + if (ret) {
> > + dev_err(dev, "can't attach to rproc %s: %d\n",
> > + rproc->name, ret);
> > + goto unprepare_subdevices;
> > + }
> > +
> > + /* Start any subdevices for the remote processor */
> > + ret = rproc_start_subdevices(rproc);
> > + if (ret) {
> > + dev_err(dev, "failed to probe subdevices for %s: %d\n",
> > + rproc->name, ret);
> > + goto stop_rproc;
> > + }
> > +
> > + rproc->state = RPROC_RUNNING;
> > +
> > + dev_info(dev, "remote processor %s is now attached\n", rproc->name);
> > +
> > + return 0;
> > +
> > +stop_rproc:
> > + rproc->ops->stop(rproc);
> > +unprepare_subdevices:
> > + rproc_unprepare_subdevices(rproc);
> > +out:
> > + return ret;
> > +}
> > +
> > /*
> > * take a firmware and boot a remote processor with it.
> > */
> > --
> > 2.20.1
> >

2020-06-22 07:24:53

by Bjorn Andersson

Subject: Re: [PATCH v4 4/9] remoteproc: Introducing function rproc_actuate()

On Mon 01 Jun 10:51 PDT 2020, Mathieu Poirier wrote:

> Introduce function rproc_actuate() that provides the same
> functionatlity as rproc_fw_boot(), but without the steps that
> involve interaction with the firmware image. That way we can
> deal with scenarios where the remoteproc core is attaching
> to a remote processor that has already been started by another
> entity.
>
> Signed-off-by: Mathieu Poirier <[email protected]>

Reviewed-by: Bjorn Andersson <[email protected]>

Regards,
Bjorn

> ---
> drivers/remoteproc/remoteproc_core.c | 68 +++++++++++++++++++++++++++-
> 1 file changed, 67 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 0b323f6b554b..c70fa0372d07 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1370,7 +1370,7 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
> return ret;
> }
>
> -static int __maybe_unused rproc_attach(struct rproc *rproc)
> +static int rproc_attach(struct rproc *rproc)
> {
> struct device *dev = &rproc->dev;
> int ret;
> @@ -1499,6 +1499,72 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
> return ret;
> }
>
> +/*
> + * Attach to remote processor - similar to rproc_fw_boot() but without
> + * the steps that deal with the firmware image.
> + */
> +static int __maybe_unused rproc_actuate(struct rproc *rproc)
> +{
> + struct device *dev = &rproc->dev;
> + int ret;
> +
> + /* Tell the PM runtime core we need to keep this device powered */
> + ret = pm_runtime_get_sync(dev);
> + if (ret < 0) {
> + dev_err(dev, "pm_runtime_get_sync failed: %d\n", ret);
> + return ret;
> + }
> +
> + /*
> + * if enabling an IOMMU isn't relevant for this rproc, this is
> + * just a nop
> + */
> + ret = rproc_enable_iommu(rproc);
> + if (ret) {
> + dev_err(dev, "can't enable iommu: %d\n", ret);
> + goto put_pm_runtime;
> + }
> +
> + /* reset max_notifyid */
> + rproc->max_notifyid = -1;
> +
> + /* reset handled vdev */
> + rproc->nb_vdev = 0;
> +
> + /*
> + * Handle firmware resources required to attach to a remote processor.
> + * Because we are attaching rather than booting the remote processor,
> + * we expect the platform driver to properly set rproc->table_ptr.
> + */
> + ret = rproc_handle_resources(rproc, rproc_loading_handlers);
> + if (ret) {
> + dev_err(dev, "Failed to process resources: %d\n", ret);
> + goto disable_iommu;
> + }
> +
> + /* Allocate carveout resources associated to rproc */
> + ret = rproc_alloc_registered_carveouts(rproc);
> + if (ret) {
> + dev_err(dev, "Failed to allocate associated carveouts: %d\n",
> + ret);
> + goto clean_up_resources;
> + }
> +
> + ret = rproc_attach(rproc);
> + if (ret)
> + goto clean_up_resources;
> +
> + return 0;
> +
> +clean_up_resources:
> + rproc_resource_cleanup(rproc);
> +disable_iommu:
> + rproc_disable_iommu(rproc);
> +put_pm_runtime:
> + pm_runtime_put(dev);
> + return ret;
> +}
> +
> /*
> * take a firmware and boot it up.
> *
> --
> 2.20.1
>

2020-06-22 07:30:00

by Bjorn Andersson

Subject: Re: [PATCH v4 5/9] remoteproc: Introducing function rproc_validate()

On Mon 01 Jun 10:51 PDT 2020, Mathieu Poirier wrote:

> Add a new function to assert the general health of the remote
> processor before handing it to the remoteproc core.
>
> Signed-off-by: Mathieu Poirier <[email protected]>
> ---
> drivers/remoteproc/remoteproc_core.c | 45 ++++++++++++++++++++++++++++
> 1 file changed, 45 insertions(+)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index c70fa0372d07..0be8343dd851 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -2060,6 +2060,47 @@ struct rproc *rproc_get_by_phandle(phandle phandle)
> #endif
> EXPORT_SYMBOL(rproc_get_by_phandle);
>
> +static int rproc_validate(struct rproc *rproc)
> +{
> + /*
> + * When adding a remote processor, the state of the device
> + * can be offline or detached, nothing else.
> + */
> + if (rproc->state != RPROC_OFFLINE &&
> + rproc->state != RPROC_DETACHED)
> + goto inval;

I would prefer that you just return -EINVAL; directly.

Overall I think this would be better represented as a switch on
rproc->state though.


I think the logic is sound though.

Regards,
Bjorn

> +
> + if (rproc->state == RPROC_OFFLINE) {
> + /*
> + * An offline processor without a start()
> + * function makes no sense.
> + */
> + if (!rproc->ops->start)
> + goto inval;
> + }
> +
> + if (rproc->state == RPROC_DETACHED) {
> + /*
> + * A remote processor in a detached state without an
> + * attach() function makes not sense.
> + */
> + if (!rproc->ops->attach)
> + goto inval;
> + /*
> + * When attaching to a remote processor the device memory
> + * is already available and as such there is no need to have a
> + * cached table.
> + */
> + if (rproc->cached_table)
> + goto inval;
> + }
> +
> + return 0;
> +
> +inval:
> + return -EINVAL;
> +}
> +
> /**
> * rproc_add() - register a remote processor
> * @rproc: the remote processor handle to register
> @@ -2089,6 +2130,10 @@ int rproc_add(struct rproc *rproc)
> if (ret < 0)
> return ret;
>
> + ret = rproc_validate(rproc);
> + if (ret < 0)
> + return ret;
> +
> dev_info(dev, "%s is available\n", rproc->name);
>
> /* create debugfs entries */
> --
> 2.20.1
>

2020-06-22 07:30:50

by Bjorn Andersson

Subject: Re: [PATCH v4 6/9] remoteproc: Refactor function rproc_boot()

On Mon 01 Jun 10:51 PDT 2020, Mathieu Poirier wrote:

> Refactor function rproc_boot() to properly deal with scenarios
> where the remoteproc core needs to attach with a remote
> processor that has already been booted by an external entity.
>
> Signed-off-by: Mathieu Poirier <[email protected]>

Reviewed-by: Bjorn Andersson <[email protected]>

Regards,
Bjorn

> ---
> drivers/remoteproc/remoteproc_core.c | 28 +++++++++++++++++-----------
> 1 file changed, 17 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 0be8343dd851..48ddc29814be 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1503,7 +1503,7 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
> * Attach to remote processor - similar to rproc_fw_boot() but without
> * the steps that deal with the firmware image.
> */
> -static int __maybe_unused rproc_actuate(struct rproc *rproc)
> +static int rproc_actuate(struct rproc *rproc)
> {
> struct device *dev = &rproc->dev;
> int ret;
> @@ -1923,24 +1923,30 @@ int rproc_boot(struct rproc *rproc)
> goto unlock_mutex;
> }
>
> - /* skip the boot process if rproc is already powered up */
> + /* skip the boot or attach process if rproc is already powered up */
> if (atomic_inc_return(&rproc->power) > 1) {
> ret = 0;
> goto unlock_mutex;
> }
>
> - dev_info(dev, "powering up %s\n", rproc->name);
> + if (rproc->state == RPROC_DETACHED) {
> + dev_info(dev, "attaching to %s\n", rproc->name);
>
> - /* load firmware */
> - ret = request_firmware(&firmware_p, rproc->firmware, dev);
> - if (ret < 0) {
> - dev_err(dev, "request_firmware failed: %d\n", ret);
> - goto downref_rproc;
> - }
> + ret = rproc_actuate(rproc);
> + } else {
> + dev_info(dev, "powering up %s\n", rproc->name);
>
> - ret = rproc_fw_boot(rproc, firmware_p);
> + /* load firmware */
> + ret = request_firmware(&firmware_p, rproc->firmware, dev);
> + if (ret < 0) {
> + dev_err(dev, "request_firmware failed: %d\n", ret);
> + goto downref_rproc;
> + }
>
> - release_firmware(firmware_p);
> + ret = rproc_fw_boot(rproc, firmware_p);
> +
> + release_firmware(firmware_p);
> + }
>
> downref_rproc:
> if (ret)
> --
> 2.20.1
>

2020-06-22 07:32:00

by Bjorn Andersson

Subject: Re: [PATCH v4 7/9] remoteproc: Refactor function rproc_trigger_auto_boot()

On Mon 01 Jun 10:51 PDT 2020, Mathieu Poirier wrote:

> Refactor function rproc_trigger_auto_boot() to properly deal
> with scenarios where the remoteproc core needs to attach with a
> remote processor that has already been booted by an external
> entity.
>
> Signed-off-by: Mathieu Poirier <[email protected]>

Reviewed-by: Bjorn Andersson <[email protected]>

Regards,
Bjorn

> ---
> drivers/remoteproc/remoteproc_core.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 48ddc29814be..d32ac8f0c872 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1586,6 +1586,15 @@ static int rproc_trigger_auto_boot(struct rproc *rproc)
> {
> int ret;
>
> + /*
> + * Since the remote processor is in a detached state, it has already
> + * been booted by another entity. As such there is no point in waiting
> + * for a firmware image to be loaded, we can simply initiate the process
> + * of attaching to it immediately.
> + */
> + if (rproc->state == RPROC_DETACHED)
> + return rproc_boot(rproc);
> +
> /*
> * We're initiating an asynchronous firmware loading, so we can
> * be built-in kernel code, without hanging the boot process.
> --
> 2.20.1
>

2020-06-22 07:34:18

by Bjorn Andersson

Subject: Re: [PATCH v4 8/9] remoteproc: Refactor function rproc_free_vring()

On Mon 01 Jun 10:51 PDT 2020, Mathieu Poirier wrote:

> When function rproc_free_vring() clears the virtio device section
> it does so on the cached resource table rather than the one
> installed in the remote processor memory. When a remote processor
> has been booted by another entity there is no need to use a cached
> table and as such, no need to clear the virtio device section in
> it.
>
> Signed-off-by: Mathieu Poirier <[email protected]>

Reviewed-by: Bjorn Andersson <[email protected]>

Regards,
Bjorn

> ---
> drivers/remoteproc/remoteproc_core.c | 20 ++++++++++++++++----
> 1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index d32ac8f0c872..0e23284fbd25 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -411,10 +411,22 @@ void rproc_free_vring(struct rproc_vring *rvring)
>
> idr_remove(&rproc->notifyids, rvring->notifyid);
>
> - /* reset resource entry info */
> - rsc = (void *)rproc->table_ptr + rvring->rvdev->rsc_offset;
> - rsc->vring[idx].da = 0;
> - rsc->vring[idx].notifyid = -1;
> + /*
> + * At this point rproc_stop() has been called and the installed resource
> + * table in the remote processor memory may no longer be accessible. As
> + * such and as per rproc_stop(), rproc->table_ptr points to the cached
> + * resource table (rproc->cached_table). The cached resource table is
> + * only available when a remote processor has been booted by the
> + * remoteproc core, otherwise it is NULL.
> + *
> + * Based on the above, reset the virtio device section in the cached
> + * resource table only if there is one to work with.
> + */
> + if (rproc->table_ptr) {
> + rsc = (void *)rproc->table_ptr + rvring->rvdev->rsc_offset;
> + rsc->vring[idx].da = 0;
> + rsc->vring[idx].notifyid = -1;
> + }
> }
>
> static int rproc_vdev_do_start(struct rproc_subdev *subdev)
> --
> 2.20.1
>

2020-06-22 07:38:38

by Bjorn Andersson

Subject: Re: [PATCH v4 9/9] remoteproc: Properly handle firmware name when attaching

On Mon 01 Jun 10:51 PDT 2020, Mathieu Poirier wrote:

> This patch prevents the firmware image name from being displayed when
> the remoteproc core is attaching to a remote processor. This is needed
> needed since there is no guarantee about the nature of the firmware
> image that is loaded by the external entity.
>
> Signed-off-by: Mathieu Poirier <[email protected]>

How about renaming the bool "firmware_unknown"?

Apart from that, I think this looks good.

Regards,
Bjorn

> ---
> drivers/remoteproc/remoteproc_core.c | 18 ++++++++++++++++++
> drivers/remoteproc/remoteproc_sysfs.c | 16 ++++++++++++++--
> include/linux/remoteproc.h | 2 ++
> 3 files changed, 34 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 0e23284fbd25..a8adc712e7f6 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1642,6 +1642,14 @@ 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;
> @@ -2166,6 +2174,16 @@ int rproc_add(struct rproc *rproc)
> /* create debugfs entries */
> rproc_create_debug_dir(rproc);
>
> + /*
> + * 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 8b462c501465..4ee158431f67 100644
> --- a/drivers/remoteproc/remoteproc_sysfs.c
> +++ b/drivers/remoteproc/remoteproc_sysfs.c
> @@ -14,8 +14,20 @@ static ssize_t firmware_show(struct device *dev, struct device_attribute *attr,
> char *buf)
> {
> struct rproc *rproc = to_rproc(dev);
> -
> - return sprintf(buf, "%s\n", rproc->firmware);
> + const char *firmware = rproc->firmware;
> +
> + /*
> + * 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)
> + firmware = "unknown";
> +
> + return sprintf(buf, "%s\n", firmware);
> }
>
> /* Change firmware name via sysfs */
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index bf6a310ba870..cf5e31556780 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -491,6 +491,7 @@ 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
> */
> @@ -524,6 +525,7 @@ 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.20.1
>

2020-06-23 19:39:05

by Mathieu Poirier

Subject: Re: [PATCH v4 5/9] remoteproc: Introducing function rproc_validate()

On Mon, Jun 22, 2020 at 12:25:02AM -0700, Bjorn Andersson wrote:
> On Mon 01 Jun 10:51 PDT 2020, Mathieu Poirier wrote:
>
> > Add a new function to assert the general health of the remote
> > processor before handing it to the remoteproc core.
> >
> > Signed-off-by: Mathieu Poirier <[email protected]>
> > ---
> > drivers/remoteproc/remoteproc_core.c | 45 ++++++++++++++++++++++++++++
> > 1 file changed, 45 insertions(+)
> >
> > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > index c70fa0372d07..0be8343dd851 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -2060,6 +2060,47 @@ struct rproc *rproc_get_by_phandle(phandle phandle)
> > #endif
> > EXPORT_SYMBOL(rproc_get_by_phandle);
> >
> > +static int rproc_validate(struct rproc *rproc)
> > +{
> > + /*
> > + * When adding a remote processor, the state of the device
> > + * can be offline or detached, nothing else.
> > + */
> > + if (rproc->state != RPROC_OFFLINE &&
> > + rproc->state != RPROC_DETACHED)
> > + goto inval;
>
> I would prefer that you just return -EINVAL; directly.
>
> Overall I think this would be better represented as a switch on
> rproc->state though.
>

Sure thing.

>
> I think the logic is sound though.
>
> Regards,
> Bjorn
>
> > +
> > + if (rproc->state == RPROC_OFFLINE) {
> > + /*
> > + * An offline processor without a start()
> > + * function makes no sense.
> > + */
> > + if (!rproc->ops->start)
> > + goto inval;
> > + }
> > +
> > + if (rproc->state == RPROC_DETACHED) {
> > + /*
> > + * A remote processor in a detached state without an
> > + * attach() function makes not sense.
> > + */
> > + if (!rproc->ops->attach)
> > + goto inval;
> > + /*
> > + * When attaching to a remote processor the device memory
> > + * is already available and as such there is no need to have a
> > + * cached table.
> > + */
> > + if (rproc->cached_table)
> > + goto inval;
> > + }
> > +
> > + return 0;
> > +
> > +inval:
> > + return -EINVAL;
> > +}
> > +
> > /**
> > * rproc_add() - register a remote processor
> > * @rproc: the remote processor handle to register
> > @@ -2089,6 +2130,10 @@ int rproc_add(struct rproc *rproc)
> > if (ret < 0)
> > return ret;
> >
> > + ret = rproc_validate(rproc);
> > + if (ret < 0)
> > + return ret;
> > +
> > dev_info(dev, "%s is available\n", rproc->name);
> >
> > /* create debugfs entries */
> > --
> > 2.20.1
> >

2020-06-23 19:40:51

by Mathieu Poirier

Subject: Re: [PATCH v4 3/9] remoteproc: Introducing function rproc_attach()

Hi,

On Mon, Jun 22, 2020 at 12:18:04AM -0700, Bjorn Andersson wrote:
> On Mon 22 Jun 00:07 PDT 2020, Bjorn Andersson wrote:
>
> > On Mon 01 Jun 10:51 PDT 2020, Mathieu Poirier wrote:
> >
> > > Introducing function rproc_attach() to enact the same actions as
> > > rproc_start(), but without the steps related to the handling of
> > > a firmware image. That way we can properly deal with scenarios
> > > where the remoteproc core needs to attach with a remote processsor
> > > that has been booted by another entity.
> > >
> > > Signed-off-by: Mathieu Poirier <[email protected]>
> > > ---
> > > drivers/remoteproc/remoteproc_core.c | 42 ++++++++++++++++++++++++++++
> > > 1 file changed, 42 insertions(+)
> > >
> > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > > index 9f04c30c4aaf..0b323f6b554b 100644
> > > --- a/drivers/remoteproc/remoteproc_core.c
> > > +++ b/drivers/remoteproc/remoteproc_core.c
> > > @@ -1370,6 +1370,48 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
> > > return ret;
> > > }
> > >
> > > +static int __maybe_unused rproc_attach(struct rproc *rproc)
> > > +{
> > > + struct device *dev = &rproc->dev;
> > > + int ret;
> > > +
> >
> > For the case where we're going DETACHED -> RUNNING - > OFFLINE we
> > need to consider the pm_runtime (and prepare/unprepare) state of the
> > device as well...
> >
>
> Missed that you do indeed pm_runtime_get() in the calling function, so I
> think we're good on that part. Still need how to actually implement
> that (and the prepare/unprepare), in particular if we're moving into
> detaching a remoteproc.

I had planned to look at the interaction between the PM runtime and prepare/unprepare
callbacks later today. Depending on what I find I may end up modifying this
patch...

>
> >
> > Apart from that I think this looks good.
> >
>
> Reviewed-by: Bjorn Andersson <[email protected]>
>
> Regards,
> Bjorn
>
> > Regards,
> > Bjorn
> >
> > > + ret = rproc_prepare_subdevices(rproc);
> > > + if (ret) {
> > > + dev_err(dev, "failed to prepare subdevices for %s: %d\n",
> > > + rproc->name, ret);
> > > + goto out;
> > > + }
> > > +
> > > + /* Attach to the remote processor */
> > > + ret = rproc_attach_device(rproc);
> > > + if (ret) {
> > > + dev_err(dev, "can't attach to rproc %s: %d\n",
> > > + rproc->name, ret);
> > > + goto unprepare_subdevices;
> > > + }
> > > +
> > > + /* Start any subdevices for the remote processor */
> > > + ret = rproc_start_subdevices(rproc);
> > > + if (ret) {
> > > + dev_err(dev, "failed to probe subdevices for %s: %d\n",
> > > + rproc->name, ret);
> > > + goto stop_rproc;
> > > + }
> > > +
> > > + rproc->state = RPROC_RUNNING;
> > > +
> > > + dev_info(dev, "remote processor %s is now attached\n", rproc->name);
> > > +
> > > + return 0;
> > > +
> > > +stop_rproc:
> > > + rproc->ops->stop(rproc);
> > > +unprepare_subdevices:
> > > + rproc_unprepare_subdevices(rproc);
> > > +out:
> > > + return ret;
> > > +}
> > > +
> > > /*
> > > * take a firmware and boot a remote processor with it.
> > > */
> > > --
> > > 2.20.1
> > >

2020-06-23 19:49:34

by Mathieu Poirier

Subject: Re: [PATCH v4 9/9] remoteproc: Properly handle firmware name when attaching

On Mon, Jun 22, 2020 at 12:33:19AM -0700, Bjorn Andersson wrote:
> On Mon 01 Jun 10:51 PDT 2020, Mathieu Poirier wrote:
>
> > This patch prevents the firmware image name from being displayed when
> > the remoteproc core is attaching to a remote processor. This is needed
> > needed since there is no guarantee about the nature of the firmware
> > image that is loaded by the external entity.
> >
> > Signed-off-by: Mathieu Poirier <[email protected]>
>
> How about renaming the bool "firmware_unknown"?

My hope was to use the same variable, i.e "autonomous", for the RUNNING ->
DETACHED and CRASHED -> DETACHED scenarios to reduce the amount of
variables we need to keep track of when the functionality is implemented in
upcoming pachsets.

Thanks for the review,
Mathieu

>
> Apart from that, I think this looks good.
>
> Regards,
> Bjorn
>
> > ---
> > drivers/remoteproc/remoteproc_core.c | 18 ++++++++++++++++++
> > drivers/remoteproc/remoteproc_sysfs.c | 16 ++++++++++++++--
> > include/linux/remoteproc.h | 2 ++
> > 3 files changed, 34 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > index 0e23284fbd25..a8adc712e7f6 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -1642,6 +1642,14 @@ 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;
> > @@ -2166,6 +2174,16 @@ int rproc_add(struct rproc *rproc)
> > /* create debugfs entries */
> > rproc_create_debug_dir(rproc);
> >
> > + /*
> > + * 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 8b462c501465..4ee158431f67 100644
> > --- a/drivers/remoteproc/remoteproc_sysfs.c
> > +++ b/drivers/remoteproc/remoteproc_sysfs.c
> > @@ -14,8 +14,20 @@ static ssize_t firmware_show(struct device *dev, struct device_attribute *attr,
> > char *buf)
> > {
> > struct rproc *rproc = to_rproc(dev);
> > -
> > - return sprintf(buf, "%s\n", rproc->firmware);
> > + const char *firmware = rproc->firmware;
> > +
> > + /*
> > + * 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)
> > + firmware = "unknown";
> > +
> > + return sprintf(buf, "%s\n", firmware);
> > }
> >
> > /* Change firmware name via sysfs */
> > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > index bf6a310ba870..cf5e31556780 100644
> > --- a/include/linux/remoteproc.h
> > +++ b/include/linux/remoteproc.h
> > @@ -491,6 +491,7 @@ 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
> > */
> > @@ -524,6 +525,7 @@ 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.20.1
> >

2020-06-23 21:34:34

by Bjorn Andersson

Subject: Re: [PATCH v4 9/9] remoteproc: Properly handle firmware name when attaching

On Tue 23 Jun 12:48 PDT 2020, Mathieu Poirier wrote:

> On Mon, Jun 22, 2020 at 12:33:19AM -0700, Bjorn Andersson wrote:
> > On Mon 01 Jun 10:51 PDT 2020, Mathieu Poirier wrote:
> >
> > > This patch prevents the firmware image name from being displayed when
> > > the remoteproc core is attaching to a remote processor. This is needed
> > > needed since there is no guarantee about the nature of the firmware
> > > image that is loaded by the external entity.
> > >
> > > Signed-off-by: Mathieu Poirier <[email protected]>
> >
> > How about renaming the bool "firmware_unknown"?
>
> My hope was to use the same variable, i.e "autonomous", for the RUNNING ->
> DETACHED and CRASHED -> DETACHED scenarios to reduce the amount of
> variables we need to keep track of when the functionality is implemented in
> upcoming pachsets.
>

Sounds like a good goal, let's keep it as is for now!

Thanks,
Bjorn

> Thanks for the review,
> Mathieu
>
> >
> > Apart from that, I think this looks good.
> >
> > Regards,
> > Bjorn
> >
> > > ---
> > > drivers/remoteproc/remoteproc_core.c | 18 ++++++++++++++++++
> > > drivers/remoteproc/remoteproc_sysfs.c | 16 ++++++++++++++--
> > > include/linux/remoteproc.h | 2 ++
> > > 3 files changed, 34 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > > index 0e23284fbd25..a8adc712e7f6 100644
> > > --- a/drivers/remoteproc/remoteproc_core.c
> > > +++ b/drivers/remoteproc/remoteproc_core.c
> > > @@ -1642,6 +1642,14 @@ 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;
> > > @@ -2166,6 +2174,16 @@ int rproc_add(struct rproc *rproc)
> > > /* create debugfs entries */
> > > rproc_create_debug_dir(rproc);
> > >
> > > + /*
> > > + * 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 8b462c501465..4ee158431f67 100644
> > > --- a/drivers/remoteproc/remoteproc_sysfs.c
> > > +++ b/drivers/remoteproc/remoteproc_sysfs.c
> > > @@ -14,8 +14,20 @@ static ssize_t firmware_show(struct device *dev, struct device_attribute *attr,
> > > char *buf)
> > > {
> > > struct rproc *rproc = to_rproc(dev);
> > > -
> > > - return sprintf(buf, "%s\n", rproc->firmware);
> > > + const char *firmware = rproc->firmware;
> > > +
> > > + /*
> > > + * 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)
> > > + firmware = "unknown";
> > > +
> > > + return sprintf(buf, "%s\n", firmware);
> > > }
> > >
> > > /* Change firmware name via sysfs */
> > > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > > index bf6a310ba870..cf5e31556780 100644
> > > --- a/include/linux/remoteproc.h
> > > +++ b/include/linux/remoteproc.h
> > > @@ -491,6 +491,7 @@ 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
> > > */
> > > @@ -524,6 +525,7 @@ 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.20.1
> > >