Re: [PATCH v5 07/19] remoteproc: Add new get_loaded_rsc_table() to rproc_ops

On Mon, Feb 15, 2021 at 02:10:10PM +0100, Arnaud POULIQUEN wrote:

Hi Mathieu,

On 2/12/21 12:46 AM, Mathieu Poirier wrote:

quoted

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 <mathieu.poirier@linaro.org>
---
New for V5:
- Added function rproc_set_loaded_rsc_table() to keep rproc_attach() clean.
- Setting ->cached_table, ->table_ptr and ->table_sz in the remoteproc core
  rather than the platform drivers.
---
 drivers/remoteproc/remoteproc_core.c     | 35 ++++++++++++++++++++++++
 drivers/remoteproc/remoteproc_internal.h | 10 +++++++
 include/linux/remoteproc.h               |  6 +++-
 3 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index e6606d10a4c8..741bc20de437 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c

@@ -1537,6 +1537,35 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 	return ret;
 }
 
+static int rproc_set_loaded_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 (IS_ERR_OR_NULL(table_ptr)) {
+		if (!table_ptr)
+			ret = -EINVAL;

I did few tests on this showing that this approach does not cover all use cases.

The first one is a firmware without resource table. In this case table_ptr
should be null, or we have to consider the -ENOENT error as a non error usecase.

Right, I'll provision for those cases.
 

The second one, more tricky, is a firmware started by the remoteproc framework.
In this case the resource table address is retrieved from the ELF file by the
core part.

Correct.

So if we detach and reattach rproc_get_loaded_rsc_table cannot return the
address. Look to me that we should have also an alocation of the clean_table in
rproc_start and then to keep the memory allocated until a shutdown.

I assumed the address of the resource table found in the ELF image was the same
as the one known by the platform driver.  In hindsight I realise the platform
driver may not know that address.

That said regarding the complexity to re-attach, I wonder if it would not be
better to focus first on a simple detach, and address the reattachment in a
separate series, to move forward in stages.

I agree that OFFLINE -> RUNNING -> DETACHED -> ATTACHED is introducing some
complexity related to the management of the resource table that where not
expected.  We could concentrate on a simple detach and see where that takes us.
It would also mean to get rid of the "autonomous-on-core-shutdown" DT binding. 

Thanks,
Mathieu

Regards,
Arnaud

quoted

+		else
+			ret = PTR_ERR(table_ptr);
+
+		dev_err(dev, "can't load resource table: %d\n", ret);
+		return ret;
+	}
+
+	/*
+	 * The resource table is already loaded in device memory, no need
+	 * to work with a cached table.
+	 */
+	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 +1585,12 @@ static int rproc_attach(struct rproc *rproc)
 		return ret;
 	}
 
+	ret = rproc_set_loaded_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);

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel