[PATCH 0/8] Remoteproc cleanups

[PATCH 0/8] Remoteproc cleanups

[Bjorn Andersson]

The first patch removes code that became unnecessary when the recovery flow was
redesigned.

The following patches moves the assignment of cached_table to the resource
table loader, rather than core code, which allows this to made optional and
finally drops the various dummy resource tables provided by drivers.

Then finally the last patch ensures that table_ptr isn't left pointing into
memory of a stopped remoteproc.

Bjorn Andersson (8):
  remoteproc: Remove depricated crash completion
  remoteproc: Cache resource table size
  remoteproc: Clone rproc_ops in rproc_alloc()
  remoteproc: Merge rproc_ops and rproc_fw_ops
  remoteproc: Don't handle empty resource table
  remoteproc: Move resource table load logic to find
  remoteproc: Drop dangling find_rsc_table dummies
  remoteproc: Reset table_ptr on stop

 drivers/remoteproc/qcom_adsp_pil.c         |  8 +--
 drivers/remoteproc/qcom_common.c           | 19 -------
 drivers/remoteproc/qcom_common.h           |  4 --
 drivers/remoteproc/qcom_q6v5_pil.c         | 18 +------
 drivers/remoteproc/qcom_wcnss.c            |  8 +--
 drivers/remoteproc/remoteproc_core.c       | 83 ++++++++++++------------------
 drivers/remoteproc/remoteproc_elf_loader.c | 59 ++++++++++-----------
 drivers/remoteproc/remoteproc_internal.h   | 57 +++++++-------------
 drivers/remoteproc/st_slim_rproc.c         | 29 -----------
 include/linux/remoteproc.h                 | 20 +++++--
 10 files changed, 102 insertions(+), 203 deletions(-)

-- 
2.15.0

[PATCH 1/8] remoteproc: Remove depricated crash completion

[Bjorn Andersson]

The crash handling now happens in a single execution context, so there's
no longer a need for a completion to synchronize this.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/remoteproc/remoteproc_core.c | 10 ----------
 include/linux/remoteproc.h           |  2 --
 2 files changed, 12 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index b2f8afee2b67..15e2c4530a0a 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1028,10 +1028,6 @@ static int rproc_stop(struct rproc *rproc, bool graceful)
 		return ret;
 	}
 
-	/* if in crash state, unlock crash handler */
-	if (rproc->state == RPROC_CRASHED)
-		complete_all(&rproc->crash_comp);
-
 	rproc->state = RPROC_OFFLINE;
 
 	dev_info(dev, "stopped remote processor %s\n", rproc->name);
@@ -1057,8 +1053,6 @@ int rproc_trigger_recovery(struct rproc *rproc)
 
 	dev_err(dev, "recovering %s\n", rproc->name);
 
-	init_completion(&rproc->crash_comp);
-
 	ret = mutex_lock_interruptible(&rproc->lock);
 	if (ret)
 		return ret;
@@ -1067,9 +1061,6 @@ int rproc_trigger_recovery(struct rproc *rproc)
 	if (ret)
 		goto unlock_mutex;
 
-	/* wait until there is no more rproc users */
-	wait_for_completion(&rproc->crash_comp);
-
 	/* load firmware */
 	ret = request_firmware(&firmware_p, rproc->firmware, dev);
 	if (ret < 0) {
@@ -1459,7 +1450,6 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
 	INIT_LIST_HEAD(&rproc->subdevs);
 
 	INIT_WORK(&rproc->crash_handler, rproc_crash_handler_work);
-	init_completion(&rproc->crash_comp);
 
 	rproc->state = RPROC_OFFLINE;
 
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 20a9467744ea..2d1058111e57 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -406,7 +406,6 @@ enum rproc_crash_type {
  * @index: index of this rproc device
  * @crash_handler: workqueue for handling a crash
  * @crash_cnt: crash counter
- * @crash_comp: completion used to sync crash handler and the rproc reload
  * @recovery_disabled: flag that state if recovery was disabled
  * @max_notifyid: largest allocated notify id.
  * @table_ptr: pointer to the resource table in effect
@@ -437,7 +436,6 @@ struct rproc {
 	int index;
 	struct work_struct crash_handler;
 	unsigned int crash_cnt;
-	struct completion crash_comp;
 	bool recovery_disabled;
 	int max_notifyid;
 	struct resource_table *table_ptr;
-- 
2.15.0

[PATCH 2/8] remoteproc: Cache resource table size

[Bjorn Andersson]

We don't re-read the resource table during a recovery, so it is possible
in the recovery path that the resource table has a different size than
cached_table. Store the original size of cached_table to avoid these
getting out of sync.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/remoteproc/remoteproc_core.c | 20 +++++++-------------
 include/linux/remoteproc.h           |  2 ++
 2 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 15e2c4530a0a..761268096fe0 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -732,7 +732,7 @@ static rproc_handle_resource_t rproc_loading_handlers[RSC_LAST] = {
 };
 
 /* handle firmware resource entries before booting the remote processor */
-static int rproc_handle_resources(struct rproc *rproc, int len,
+static int rproc_handle_resources(struct rproc *rproc,
 				  rproc_handle_resource_t handlers[RSC_LAST])
 {
 	struct device *dev = &rproc->dev;
@@ -742,7 +742,7 @@ static int rproc_handle_resources(struct rproc *rproc, int len,
 	for (i = 0; i < rproc->table_ptr->num; i++) {
 		int offset = rproc->table_ptr->offset[i];
 		struct fw_rsc_hdr *hdr = (void *)rproc->table_ptr + offset;
-		int avail = len - offset - sizeof(*hdr);
+		int avail = rproc->table_sz - offset - sizeof(*hdr);
 		void *rsc = (void *)hdr + sizeof(*hdr);
 
 		/* make sure table isn't truncated */
@@ -849,16 +849,9 @@ static void rproc_resource_cleanup(struct rproc *rproc)
 
 static int rproc_start(struct rproc *rproc, const struct firmware *fw)
 {
-	struct resource_table *table, *loaded_table;
+	struct resource_table *loaded_table;
 	struct device *dev = &rproc->dev;
-	int ret, tablesz;
-
-	/* look for the resource table */
-	table = rproc_find_rsc_table(rproc, fw, &tablesz);
-	if (!table) {
-		dev_err(dev, "Resource table look up failed\n");
-		return -EINVAL;
-	}
+	int ret;
 
 	/* load the ELF segments to memory */
 	ret = rproc_load_segments(rproc, fw);
@@ -877,7 +870,7 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
 	 */
 	loaded_table = rproc_find_loaded_rsc_table(rproc, fw);
 	if (loaded_table) {
-		memcpy(loaded_table, rproc->cached_table, tablesz);
+		memcpy(loaded_table, rproc->cached_table, rproc->table_sz);
 		rproc->table_ptr = loaded_table;
 	}
 
@@ -951,12 +944,13 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 		goto clean_up;
 
 	rproc->table_ptr = rproc->cached_table;
+	rproc->table_sz = tablesz;
 
 	/* reset max_notifyid */
 	rproc->max_notifyid = -1;
 
 	/* handle fw resources which are required to boot rproc */
-	ret = rproc_handle_resources(rproc, tablesz, rproc_loading_handlers);
+	ret = rproc_handle_resources(rproc, rproc_loading_handlers);
 	if (ret) {
 		dev_err(dev, "Failed to process resources: %d\n", ret);
 		goto clean_up_resources;
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 2d1058111e57..b72be13d773d 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -410,6 +410,7 @@ enum rproc_crash_type {
  * @max_notifyid: largest allocated notify id.
  * @table_ptr: pointer to the resource table in effect
  * @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
  */
 struct rproc {
@@ -440,6 +441,7 @@ struct rproc {
 	int max_notifyid;
 	struct resource_table *table_ptr;
 	struct resource_table *cached_table;
+	size_t table_sz;
 	bool has_iommu;
 	bool auto_boot;
 };
-- 
2.15.0

[PATCH 3/8] remoteproc: Clone rproc_ops in rproc_alloc()

[Bjorn Andersson]

In order to allow rproc_alloc() to, in a future patch, update entries in
the "ops" struct we need to make a local copy of it.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/remoteproc/remoteproc_core.c | 9 ++++++++-
 include/linux/remoteproc.h           | 2 +-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 761268096fe0..9bc9ee8f4513 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1342,6 +1342,7 @@ static void rproc_type_release(struct device *dev)
 		ida_simple_remove(&rproc_dev_index, rproc->index);
 
 	kfree(rproc->firmware);
+	kfree(rproc->ops);
 	kfree(rproc);
 }
 
@@ -1406,9 +1407,15 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
 		return NULL;
 	}
 
+	rproc->ops = kmemdup(ops, sizeof(*ops), GFP_KERNEL);
+	if (!rproc->ops) {
+		kfree(p);
+		kfree(rproc);
+		return NULL;
+	}
+
 	rproc->firmware = p;
 	rproc->name = name;
-	rproc->ops = ops;
 	rproc->priv = &rproc[1];
 	rproc->auto_boot = true;
 
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index b72be13d773d..6cbc0379a926 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -419,7 +419,7 @@ struct rproc {
 	const char *name;
 	char *firmware;
 	void *priv;
-	const struct rproc_ops *ops;
+	struct rproc_ops *ops;
 	struct device dev;
 	const struct rproc_fw_ops *fw_ops;
 	atomic_t power;
-- 
2.15.0

[PATCH 4/8] remoteproc: Merge rproc_ops and rproc_fw_ops

[Bjorn Andersson]

There are currently a few different schemes used for overriding fw_ops
or parts of fw_ops. Merge fw_ops into rproc_ops and expose the default
ELF-loader symbols so that they can be assigned by the drivers.

To keep backwards compatibility with the "default" case, a driver not
specifying the "load" operation is assumed to want the full ELF-loader
suit of functions.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/remoteproc/qcom_adsp_pil.c         |  9 ++----
 drivers/remoteproc/qcom_q6v5_pil.c         |  9 ++----
 drivers/remoteproc/qcom_wcnss.c            |  9 ++----
 drivers/remoteproc/remoteproc_core.c       | 10 ++++--
 drivers/remoteproc/remoteproc_elf_loader.c | 30 +++++++-----------
 drivers/remoteproc/remoteproc_internal.h   | 51 +++++++++++-------------------
 drivers/remoteproc/st_slim_rproc.c         | 19 +++--------
 include/linux/remoteproc.h                 | 17 ++++++++--
 8 files changed, 64 insertions(+), 90 deletions(-)

diff --git a/drivers/remoteproc/qcom_adsp_pil.c b/drivers/remoteproc/qcom_adsp_pil.c
index 45e7e66604d4..7b9d810b23f1 100644
--- a/drivers/remoteproc/qcom_adsp_pil.c
+++ b/drivers/remoteproc/qcom_adsp_pil.c
@@ -89,11 +89,6 @@ static int adsp_load(struct rproc *rproc, const struct firmware *fw)
 			     adsp->mem_region, adsp->mem_phys, adsp->mem_size);
 }
 
-static const struct rproc_fw_ops adsp_fw_ops = {
-	.find_rsc_table = qcom_mdt_find_rsc_table,
-	.load = adsp_load,
-};
-
 static int adsp_start(struct rproc *rproc)
 {
 	struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
@@ -186,6 +181,8 @@ static const struct rproc_ops adsp_ops = {
 	.start = adsp_start,
 	.stop = adsp_stop,
 	.da_to_va = adsp_da_to_va,
+	.find_rsc_table = qcom_mdt_find_rsc_table,
+	.load = adsp_load,
 };
 
 static irqreturn_t adsp_wdog_interrupt(int irq, void *dev)
@@ -348,8 +345,6 @@ static int adsp_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
-	rproc->fw_ops = &adsp_fw_ops;
-
 	adsp = (struct qcom_adsp *)rproc->priv;
 	adsp->dev = &pdev->dev;
 	adsp->rproc = rproc;
diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index 342d5eb5d6ed..fbff5d842581 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -343,11 +343,6 @@ static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
 	return 0;
 }
 
-static const struct rproc_fw_ops q6v5_fw_ops = {
-	.find_rsc_table = q6v5_find_rsc_table,
-	.load = q6v5_load,
-};
-
 static int q6v5_rmb_pbl_wait(struct q6v5 *qproc, int ms)
 {
 	unsigned long timeout;
@@ -932,6 +927,8 @@ static const struct rproc_ops q6v5_ops = {
 	.start = q6v5_start,
 	.stop = q6v5_stop,
 	.da_to_va = q6v5_da_to_va,
+	.find_rsc_table = q6v5_find_rsc_table,
+	.load = q6v5_load,
 };
 
 static irqreturn_t q6v5_wdog_interrupt(int irq, void *dev)
@@ -1151,8 +1148,6 @@ static int q6v5_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
-	rproc->fw_ops = &q6v5_fw_ops;
-
 	qproc = (struct q6v5 *)rproc->priv;
 	qproc->dev = &pdev->dev;
 	qproc->rproc = rproc;
diff --git a/drivers/remoteproc/qcom_wcnss.c b/drivers/remoteproc/qcom_wcnss.c
index dc79239c43f9..cc44ec598522 100644
--- a/drivers/remoteproc/qcom_wcnss.c
+++ b/drivers/remoteproc/qcom_wcnss.c
@@ -158,11 +158,6 @@ static int wcnss_load(struct rproc *rproc, const struct firmware *fw)
 			     wcnss->mem_region, wcnss->mem_phys, wcnss->mem_size);
 }
 
-static const struct rproc_fw_ops wcnss_fw_ops = {
-	.find_rsc_table = qcom_mdt_find_rsc_table,
-	.load = wcnss_load,
-};
-
 static void wcnss_indicate_nv_download(struct qcom_wcnss *wcnss)
 {
 	u32 val;
@@ -315,6 +310,8 @@ static const struct rproc_ops wcnss_ops = {
 	.start = wcnss_start,
 	.stop = wcnss_stop,
 	.da_to_va = wcnss_da_to_va,
+	.find_rsc_table = qcom_mdt_find_rsc_table,
+	.load = wcnss_load,
 };
 
 static irqreturn_t wcnss_wdog_interrupt(int irq, void *dev)
@@ -494,8 +491,6 @@ static int wcnss_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
-	rproc->fw_ops = &wcnss_fw_ops;
-
 	wcnss = (struct qcom_wcnss *)rproc->priv;
 	wcnss->dev = &pdev->dev;
 	wcnss->rproc = rproc;
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 9bc9ee8f4513..f2b6aa5d204c 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1437,8 +1437,14 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
 
 	atomic_set(&rproc->power, 0);
 
-	/* Set ELF as the default fw_ops handler */
-	rproc->fw_ops = &rproc_elf_fw_ops;
+	/* Default to ELF loader if no load function is specified */
+	if (!rproc->ops->load) {
+		rproc->ops->load = rproc_elf_load_segments;
+		rproc->ops->find_rsc_table = rproc_elf_find_rsc_table;
+		rproc->ops->find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table;
+		rproc->ops->sanity_check = rproc_elf_sanity_check;
+		rproc->ops->get_boot_addr = rproc_elf_get_boot_addr;
+	}
 
 	mutex_init(&rproc->lock);
 
diff --git a/drivers/remoteproc/remoteproc_elf_loader.c b/drivers/remoteproc/remoteproc_elf_loader.c
index c523983a4aec..822fa1bf893f 100644
--- a/drivers/remoteproc/remoteproc_elf_loader.c
+++ b/drivers/remoteproc/remoteproc_elf_loader.c
@@ -39,8 +39,7 @@
  *
  * Make sure this fw image is sane.
  */
-static int
-rproc_elf_sanity_check(struct rproc *rproc, const struct firmware *fw)
+int rproc_elf_sanity_check(struct rproc *rproc, const struct firmware *fw)
 {
 	const char *name = rproc->firmware;
 	struct device *dev = &rproc->dev;
@@ -98,6 +97,7 @@ rproc_elf_sanity_check(struct rproc *rproc, const struct firmware *fw)
 
 	return 0;
 }
+EXPORT_SYMBOL(rproc_elf_sanity_check);
 
 /**
  * rproc_elf_get_boot_addr() - Get rproc's boot address.
@@ -110,13 +110,13 @@ rproc_elf_sanity_check(struct rproc *rproc, const struct firmware *fw)
  * Note that the boot address is not a configurable property of all remote
  * processors. Some will always boot at a specific hard-coded address.
  */
-static
 u32 rproc_elf_get_boot_addr(struct rproc *rproc, const struct firmware *fw)
 {
 	struct elf32_hdr *ehdr  = (struct elf32_hdr *)fw->data;
 
 	return ehdr->e_entry;
 }
+EXPORT_SYMBOL(rproc_elf_get_boot_addr);
 
 /**
  * rproc_elf_load_segments() - load firmware segments to memory
@@ -142,8 +142,7 @@ u32 rproc_elf_get_boot_addr(struct rproc *rproc, const struct firmware *fw)
  * directly allocate memory for every segment/resource. This is not yet
  * supported, though.
  */
-static int
-rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
+int rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
 {
 	struct device *dev = &rproc->dev;
 	struct elf32_hdr *ehdr;
@@ -207,6 +206,7 @@ rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
 
 	return ret;
 }
+EXPORT_SYMBOL(rproc_elf_load_segments);
 
 static struct elf32_shdr *
 find_table(struct device *dev, struct elf32_hdr *ehdr, size_t fw_size)
@@ -282,9 +282,9 @@ find_table(struct device *dev, struct elf32_hdr *ehdr, size_t fw_size)
  * size into @tablesz. If a valid table isn't found, NULL is returned
  * (and @tablesz isn't set).
  */
-static struct resource_table *
-rproc_elf_find_rsc_table(struct rproc *rproc, const struct firmware *fw,
-			 int *tablesz)
+struct resource_table *rproc_elf_find_rsc_table(struct rproc *rproc,
+						const struct firmware *fw,
+						int *tablesz)
 {
 	struct elf32_hdr *ehdr;
 	struct elf32_shdr *shdr;
@@ -303,6 +303,7 @@ rproc_elf_find_rsc_table(struct rproc *rproc, const struct firmware *fw,
 
 	return table;
 }
+EXPORT_SYMBOL(rproc_elf_find_rsc_table);
 
 /**
  * rproc_elf_find_loaded_rsc_table() - find the loaded resource table
@@ -315,8 +316,8 @@ rproc_elf_find_rsc_table(struct rproc *rproc, const struct firmware *fw,
  * Returns the pointer to the resource table if it is found or NULL otherwise.
  * If the table wasn't loaded yet the result is unspecified.
  */
-static struct resource_table *
-rproc_elf_find_loaded_rsc_table(struct rproc *rproc, const struct firmware *fw)
+struct resource_table *rproc_elf_find_loaded_rsc_table(struct rproc *rproc,
+						       const struct firmware *fw)
 {
 	struct elf32_hdr *ehdr = (struct elf32_hdr *)fw->data;
 	struct elf32_shdr *shdr;
@@ -327,11 +328,4 @@ rproc_elf_find_loaded_rsc_table(struct rproc *rproc, const struct firmware *fw)
 
 	return rproc_da_to_va(rproc, shdr->sh_addr, shdr->sh_size);
 }
-
-const struct rproc_fw_ops rproc_elf_fw_ops = {
-	.load = rproc_elf_load_segments,
-	.find_rsc_table = rproc_elf_find_rsc_table,
-	.find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table,
-	.sanity_check = rproc_elf_sanity_check,
-	.get_boot_addr = rproc_elf_get_boot_addr
-};
+EXPORT_SYMBOL(rproc_elf_find_loaded_rsc_table);
diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
index c1077bec5d0b..a42690c514e2 100644
--- a/drivers/remoteproc/remoteproc_internal.h
+++ b/drivers/remoteproc/remoteproc_internal.h
@@ -25,26 +25,6 @@
 
 struct rproc;
 
-/**
- * struct rproc_fw_ops - firmware format specific operations.
- * @find_rsc_table:	find the resource table inside the firmware image
- * @find_loaded_rsc_table: find the loaded resouce table
- * @load:		load firmeware to memory, where the remote processor
- *			expects to find it
- * @sanity_check:	sanity check the fw image
- * @get_boot_addr:	get boot address to entry point specified in firmware
- */
-struct rproc_fw_ops {
-	struct resource_table *(*find_rsc_table)(struct rproc *rproc,
-						 const struct firmware *fw,
-						 int *tablesz);
-	struct resource_table *(*find_loaded_rsc_table)(
-				struct rproc *rproc, const struct firmware *fw);
-	int (*load)(struct rproc *rproc, const struct firmware *fw);
-	int (*sanity_check)(struct rproc *rproc, const struct firmware *fw);
-	u32 (*get_boot_addr)(struct rproc *rproc, const struct firmware *fw);
-};
-
 /* from remoteproc_core.c */
 void rproc_release(struct kref *kref);
 irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id);
@@ -74,11 +54,20 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);
 void *rproc_da_to_va(struct rproc *rproc, u64 da, int len);
 int rproc_trigger_recovery(struct rproc *rproc);
 
+int rproc_elf_sanity_check(struct rproc *rproc, const struct firmware *fw);
+u32 rproc_elf_get_boot_addr(struct rproc *rproc, const struct firmware *fw);
+int rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw);
+struct resource_table *rproc_elf_find_rsc_table(struct rproc *rproc,
+						const struct firmware *fw,
+						int *tablesz);
+struct resource_table *rproc_elf_find_loaded_rsc_table(struct rproc *rproc,
+						       const struct firmware *fw);
+
 static inline
 int rproc_fw_sanity_check(struct rproc *rproc, const struct firmware *fw)
 {
-	if (rproc->fw_ops->sanity_check)
-		return rproc->fw_ops->sanity_check(rproc, fw);
+	if (rproc->ops->sanity_check)
+		return rproc->ops->sanity_check(rproc, fw);
 
 	return 0;
 }
@@ -86,8 +75,8 @@ int rproc_fw_sanity_check(struct rproc *rproc, const struct firmware *fw)
 static inline
 u32 rproc_get_boot_addr(struct rproc *rproc, const struct firmware *fw)
 {
-	if (rproc->fw_ops->get_boot_addr)
-		return rproc->fw_ops->get_boot_addr(rproc, fw);
+	if (rproc->ops->get_boot_addr)
+		return rproc->ops->get_boot_addr(rproc, fw);
 
 	return 0;
 }
@@ -95,8 +84,8 @@ u32 rproc_get_boot_addr(struct rproc *rproc, const struct firmware *fw)
 static inline
 int rproc_load_segments(struct rproc *rproc, const struct firmware *fw)
 {
-	if (rproc->fw_ops->load)
-		return rproc->fw_ops->load(rproc, fw);
+	if (rproc->ops->load)
+		return rproc->ops->load(rproc, fw);
 
 	return -EINVAL;
 }
@@ -106,8 +95,8 @@ struct resource_table *rproc_find_rsc_table(struct rproc *rproc,
 					    const struct firmware *fw,
 					    int *tablesz)
 {
-	if (rproc->fw_ops->find_rsc_table)
-		return rproc->fw_ops->find_rsc_table(rproc, fw, tablesz);
+	if (rproc->ops->find_rsc_table)
+		return rproc->ops->find_rsc_table(rproc, fw, tablesz);
 
 	return NULL;
 }
@@ -116,12 +105,10 @@ static inline
 struct resource_table *rproc_find_loaded_rsc_table(struct rproc *rproc,
 						   const struct firmware *fw)
 {
-	if (rproc->fw_ops->find_loaded_rsc_table)
-		return rproc->fw_ops->find_loaded_rsc_table(rproc, fw);
+	if (rproc->ops->find_loaded_rsc_table)
+		return rproc->ops->find_loaded_rsc_table(rproc, fw);
 
 	return NULL;
 }
 
-extern const struct rproc_fw_ops rproc_elf_fw_ops;
-
 #endif /* REMOTEPROC_INTERNAL_H */
diff --git a/drivers/remoteproc/st_slim_rproc.c b/drivers/remoteproc/st_slim_rproc.c
index 6cfd862f945b..1538ea915c49 100644
--- a/drivers/remoteproc/st_slim_rproc.c
+++ b/drivers/remoteproc/st_slim_rproc.c
@@ -200,12 +200,6 @@ static void *slim_rproc_da_to_va(struct rproc *rproc, u64 da, int len)
 	return va;
 }
 
-static const struct rproc_ops slim_rproc_ops = {
-	.start		= slim_rproc_start,
-	.stop		= slim_rproc_stop,
-	.da_to_va       = slim_rproc_da_to_va,
-};
-
 /*
  * Firmware handler operations: sanity, boot address, load ...
  */
@@ -223,7 +217,10 @@ static struct resource_table *slim_rproc_find_rsc_table(struct rproc *rproc,
 	return &empty_rsc_tbl;
 }
 
-static struct rproc_fw_ops slim_rproc_fw_ops = {
+static const struct rproc_ops slim_rproc_ops = {
+	.start		= slim_rproc_start,
+	.stop		= slim_rproc_stop,
+	.da_to_va       = slim_rproc_da_to_va,
 	.find_rsc_table = slim_rproc_find_rsc_table,
 };
 
@@ -249,7 +246,6 @@ struct st_slim_rproc *st_slim_rproc_alloc(struct platform_device *pdev,
 	struct rproc *rproc;
 	struct resource *res;
 	int err, i;
-	const struct rproc_fw_ops *elf_ops;
 
 	if (!fw_name)
 		return ERR_PTR(-EINVAL);
@@ -267,13 +263,6 @@ struct st_slim_rproc *st_slim_rproc_alloc(struct platform_device *pdev,
 	slim_rproc = rproc->priv;
 	slim_rproc->rproc = rproc;
 
-	elf_ops = rproc->fw_ops;
-	/* Use some generic elf ops */
-	slim_rproc_fw_ops.load = elf_ops->load;
-	slim_rproc_fw_ops.sanity_check = elf_ops->sanity_check;
-
-	rproc->fw_ops = &slim_rproc_fw_ops;
-
 	/* get imem and dmem */
 	for (i = 0; i < ARRAY_SIZE(mem_names); i++) {
 		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 6cbc0379a926..e21de55e19d1 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -324,6 +324,7 @@ struct rproc_mem_entry {
 };
 
 struct rproc;
+struct firmware;
 
 /**
  * struct rproc_ops - platform-specific device handlers
@@ -331,12 +332,26 @@ struct rproc;
  * @stop:	power off the device
  * @kick:	kick a virtqueue (virtqueue id given as a parameter)
  * @da_to_va:	optional platform hook to perform address translations
+ * @find_rsc_table:	find the resource table inside the firmware image
+ * @find_loaded_rsc_table: find the loaded resouce table
+ * @load:		load firmeware to memory, where the remote processor
+ *			expects to find it
+ * @sanity_check:	sanity check the fw image
+ * @get_boot_addr:	get boot address to entry point specified in firmware
  */
 struct rproc_ops {
 	int (*start)(struct rproc *rproc);
 	int (*stop)(struct rproc *rproc);
 	void (*kick)(struct rproc *rproc, int vqid);
 	void * (*da_to_va)(struct rproc *rproc, u64 da, int len);
+	struct resource_table *(*find_rsc_table)(struct rproc *rproc,
+						 const struct firmware *fw,
+						 int *tablesz);
+	struct resource_table *(*find_loaded_rsc_table)(
+				struct rproc *rproc, const struct firmware *fw);
+	int (*load)(struct rproc *rproc, const struct firmware *fw);
+	int (*sanity_check)(struct rproc *rproc, const struct firmware *fw);
+	u32 (*get_boot_addr)(struct rproc *rproc, const struct firmware *fw);
 };
 
 /**
@@ -390,7 +405,6 @@ enum rproc_crash_type {
  * @priv: private data which belongs to the platform-specific rproc module
  * @ops: platform-specific start/stop rproc handlers
  * @dev: virtual device for refcounting and common remoteproc behavior
- * @fw_ops: firmware-specific handlers
  * @power: refcount of users who need this rproc powered up
  * @state: state of the device
  * @lock: lock which protects concurrent manipulations of the rproc
@@ -421,7 +435,6 @@ struct rproc {
 	void *priv;
 	struct rproc_ops *ops;
 	struct device dev;
-	const struct rproc_fw_ops *fw_ops;
 	atomic_t power;
 	unsigned int state;
 	struct mutex lock;
-- 
2.15.0

[PATCH 5/8] remoteproc: Don't handle empty resource table

[Bjorn Andersson]

Allow a NULL table_ptr to have the same meaning as a table with 0
entries, allowing a subsequent patch to skip the assignment step.

A few other places in the implementation does dereference table_ptr, but
they are currently all coming from rproc_handle_resources().

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/remoteproc/remoteproc_core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index f2b6aa5d204c..63d88d1d206e 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -739,6 +739,9 @@ static int rproc_handle_resources(struct rproc *rproc,
 	rproc_handle_resource_t handler;
 	int ret = 0, i;
 
+	if (!rproc->table_ptr)
+		return 0;
+
 	for (i = 0; i < rproc->table_ptr->num; i++) {
 		int offset = rproc->table_ptr->offset[i];
 		struct fw_rsc_hdr *hdr = (void *)rproc->table_ptr + offset;
-- 
2.15.0

[PATCH 6/8] remoteproc: Move resource table load logic to find

[Bjorn Andersson]

Extend the previous operation of finding the resource table in the ELF
with the extra step of populating the rproc struct with a copy and the
size. This allows drivers to override the mechanism used for acquiring
the resource table, or omit it for firmware that is known not to have a
resource table.

This leaves the custom, dummy, find_rsc_table implementations found in
some drivers dangling.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/remoteproc/remoteproc_core.c       | 32 ++++++--------------------
 drivers/remoteproc/remoteproc_elf_loader.c | 37 ++++++++++++++++++------------
 drivers/remoteproc/remoteproc_internal.h   | 16 +++++--------
 include/linux/remoteproc.h                 |  1 +
 4 files changed, 36 insertions(+), 50 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 63d88d1d206e..cbd12382b219 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -907,8 +907,7 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 {
 	struct device *dev = &rproc->dev;
 	const char *name = rproc->firmware;
-	struct resource_table *table;
-	int ret, tablesz;
+	int ret;
 
 	ret = rproc_fw_sanity_check(rproc, fw);
 	if (ret)
@@ -927,27 +926,11 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 	}
 
 	rproc->bootaddr = rproc_get_boot_addr(rproc, fw);
-	ret = -EINVAL;
-
-	/* look for the resource table */
-	table = rproc_find_rsc_table(rproc, fw, &tablesz);
-	if (!table) {
-		dev_err(dev, "Failed to find resource table\n");
-		goto clean_up;
-	}
-
-	/*
-	 * Create a copy of the resource table. When a virtio device starts
-	 * and calls vring_new_virtqueue() the address of the allocated vring
-	 * will be stored in the cached_table. Before the device is started,
-	 * cached_table will be copied into device memory.
-	 */
-	rproc->cached_table = kmemdup(table, tablesz, GFP_KERNEL);
-	if (!rproc->cached_table)
-		goto clean_up;
 
-	rproc->table_ptr = rproc->cached_table;
-	rproc->table_sz = tablesz;
+	/* load resource table */
+	ret = rproc_load_rsc_table(rproc, fw);
+	if (ret)
+		goto disable_iommu;
 
 	/* reset max_notifyid */
 	rproc->max_notifyid = -1;
@@ -967,11 +950,10 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 
 clean_up_resources:
 	rproc_resource_cleanup(rproc);
-clean_up:
 	kfree(rproc->cached_table);
 	rproc->cached_table = NULL;
 	rproc->table_ptr = NULL;
-
+disable_iommu:
 	rproc_disable_iommu(rproc);
 	return ret;
 }
@@ -1443,7 +1425,7 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
 	/* Default to ELF loader if no load function is specified */
 	if (!rproc->ops->load) {
 		rproc->ops->load = rproc_elf_load_segments;
-		rproc->ops->find_rsc_table = rproc_elf_find_rsc_table;
+		rproc->ops->load_rsc_table = rproc_elf_load_rsc_table;
 		rproc->ops->find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table;
 		rproc->ops->sanity_check = rproc_elf_sanity_check;
 		rproc->ops->get_boot_addr = rproc_elf_get_boot_addr;
diff --git a/drivers/remoteproc/remoteproc_elf_loader.c b/drivers/remoteproc/remoteproc_elf_loader.c
index 822fa1bf893f..b17d72ec8603 100644
--- a/drivers/remoteproc/remoteproc_elf_loader.c
+++ b/drivers/remoteproc/remoteproc_elf_loader.c
@@ -268,42 +268,49 @@ find_table(struct device *dev, struct elf32_hdr *ehdr, size_t fw_size)
 }
 
 /**
- * rproc_elf_find_rsc_table() - find the resource table
+ * rproc_elf_load_rsc_table() - load the resource table
  * @rproc: the rproc handle
  * @fw: the ELF firmware image
- * @tablesz: place holder for providing back the table size
  *
  * This function finds the resource table inside the remote processor's
- * firmware. It is used both upon the registration of @rproc (in order
- * to look for and register the supported virito devices), and when the
- * @rproc is booted.
+ * firmware, load it into the @cached_table and update @table_ptr.
  *
- * Returns the pointer to the resource table if it is found, and write its
- * size into @tablesz. If a valid table isn't found, NULL is returned
- * (and @tablesz isn't set).
+ * Return: 0 on success, negative errno on failure.
  */
-struct resource_table *rproc_elf_find_rsc_table(struct rproc *rproc,
-						const struct firmware *fw,
-						int *tablesz)
+int rproc_elf_load_rsc_table(struct rproc *rproc, const struct firmware *fw)
 {
 	struct elf32_hdr *ehdr;
 	struct elf32_shdr *shdr;
 	struct device *dev = &rproc->dev;
 	struct resource_table *table = NULL;
 	const u8 *elf_data = fw->data;
+	size_t tablesz;
 
 	ehdr = (struct elf32_hdr *)elf_data;
 
 	shdr = find_table(dev, ehdr, fw->size);
 	if (!shdr)
-		return NULL;
+		return -EINVAL;
 
 	table = (struct resource_table *)(elf_data + shdr->sh_offset);
-	*tablesz = shdr->sh_size;
+	tablesz = shdr->sh_size;
+
+	/*
+	 * Create a copy of the resource table. When a virtio device starts
+	 * and calls vring_new_virtqueue() the address of the allocated vring
+	 * will be stored in the cached_table. Before the device is started,
+	 * cached_table will be copied into device memory.
+	 */
+	rproc->cached_table = kmemdup(table, tablesz, GFP_KERNEL);
+	if (!rproc->cached_table)
+		return -ENOMEM;
 
-	return table;
+	rproc->table_ptr = rproc->cached_table;
+	rproc->table_sz = tablesz;
+
+	return 0;
 }
-EXPORT_SYMBOL(rproc_elf_find_rsc_table);
+EXPORT_SYMBOL(rproc_elf_load_rsc_table);
 
 /**
  * rproc_elf_find_loaded_rsc_table() - find the loaded resource table
diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
index a42690c514e2..55a2950c5cb7 100644
--- a/drivers/remoteproc/remoteproc_internal.h
+++ b/drivers/remoteproc/remoteproc_internal.h
@@ -57,9 +57,7 @@ int rproc_trigger_recovery(struct rproc *rproc);
 int rproc_elf_sanity_check(struct rproc *rproc, const struct firmware *fw);
 u32 rproc_elf_get_boot_addr(struct rproc *rproc, const struct firmware *fw);
 int rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw);
-struct resource_table *rproc_elf_find_rsc_table(struct rproc *rproc,
-						const struct firmware *fw,
-						int *tablesz);
+int rproc_elf_load_rsc_table(struct rproc *rproc, const struct firmware *fw);
 struct resource_table *rproc_elf_find_loaded_rsc_table(struct rproc *rproc,
 						       const struct firmware *fw);
 
@@ -90,15 +88,13 @@ int rproc_load_segments(struct rproc *rproc, const struct firmware *fw)
 	return -EINVAL;
 }
 
-static inline
-struct resource_table *rproc_find_rsc_table(struct rproc *rproc,
-					    const struct firmware *fw,
-					    int *tablesz)
+static inline int rproc_load_rsc_table(struct rproc *rproc,
+				       const struct firmware *fw)
 {
-	if (rproc->ops->find_rsc_table)
-		return rproc->ops->find_rsc_table(rproc, fw, tablesz);
+	if (rproc->ops->load_rsc_table)
+		return rproc->ops->load_rsc_table(rproc, fw);
 
-	return NULL;
+	return 0;
 }
 
 static inline
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index e21de55e19d1..ec1ada7cc6d7 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -344,6 +344,7 @@ struct rproc_ops {
 	int (*stop)(struct rproc *rproc);
 	void (*kick)(struct rproc *rproc, int vqid);
 	void * (*da_to_va)(struct rproc *rproc, u64 da, int len);
+	int (*load_rsc_table)(struct rproc *rproc, const struct firmware *fw);
 	struct resource_table *(*find_rsc_table)(struct rproc *rproc,
 						 const struct firmware *fw,
 						 int *tablesz);
-- 
2.15.0

RE: [PATCH 6/8] remoteproc: Move resource table load logic to find

[Loic PALLARDY]

> -----Original Message-----
> From: linux-remoteproc-owner@vger.kernel.org [mailto:linux-remoteproc-
> owner@vger.kernel.org] On Behalf Of Bjorn Andersson
> Sent: Wednesday, December 13, 2017 11:41 PM
> To: Ohad Ben-Cohen <ohad@wizery.com>; Bjorn Andersson
> <bjorn.andersson@linaro.org>
> Cc: linux-remoteproc@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [PATCH 6/8] remoteproc: Move resource table load logic to find
> 
> Extend the previous operation of finding the resource table in the ELF
> with the extra step of populating the rproc struct with a copy and the
> size. This allows drivers to override the mechanism used for acquiring
> the resource table, or omit it for firmware that is known not to have a
> resource table.
> 
> This leaves the custom, dummy, find_rsc_table implementations found in
> some drivers dangling.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  drivers/remoteproc/remoteproc_core.c       | 32 ++++++--------------------
>  drivers/remoteproc/remoteproc_elf_loader.c | 37 ++++++++++++++++++--
> ----------
>  drivers/remoteproc/remoteproc_internal.h   | 16 +++++--------
>  include/linux/remoteproc.h                 |  1 +
>  4 files changed, 36 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c
> b/drivers/remoteproc/remoteproc_core.c
> index 63d88d1d206e..cbd12382b219 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -907,8 +907,7 @@ static int rproc_fw_boot(struct rproc *rproc, const
> struct firmware *fw)
>  {
>  	struct device *dev = &rproc->dev;
>  	const char *name = rproc->firmware;
> -	struct resource_table *table;
> -	int ret, tablesz;
> +	int ret;
> 
>  	ret = rproc_fw_sanity_check(rproc, fw);
>  	if (ret)
> @@ -927,27 +926,11 @@ static int rproc_fw_boot(struct rproc *rproc, const
> struct firmware *fw)
>  	}
> 
>  	rproc->bootaddr = rproc_get_boot_addr(rproc, fw);
> -	ret = -EINVAL;
> -
> -	/* look for the resource table */
> -	table = rproc_find_rsc_table(rproc, fw, &tablesz);
> -	if (!table) {
> -		dev_err(dev, "Failed to find resource table\n");
> -		goto clean_up;
> -	}
> -
> -	/*
> -	 * Create a copy of the resource table. When a virtio device starts
> -	 * and calls vring_new_virtqueue() the address of the allocated vring
> -	 * will be stored in the cached_table. Before the device is started,
> -	 * cached_table will be copied into device memory.
> -	 */
> -	rproc->cached_table = kmemdup(table, tablesz, GFP_KERNEL);
> -	if (!rproc->cached_table)
> -		goto clean_up;
> 
> -	rproc->table_ptr = rproc->cached_table;
> -	rproc->table_sz = tablesz;
> +	/* load resource table */
> +	ret = rproc_load_rsc_table(rproc, fw);
> +	if (ret)
> +		goto disable_iommu;
> 
>  	/* reset max_notifyid */
>  	rproc->max_notifyid = -1;
> @@ -967,11 +950,10 @@ static int rproc_fw_boot(struct rproc *rproc, const
> struct firmware *fw)
> 
>  clean_up_resources:
>  	rproc_resource_cleanup(rproc);
> -clean_up:
>  	kfree(rproc->cached_table);
>  	rproc->cached_table = NULL;
>  	rproc->table_ptr = NULL;
> -
> +disable_iommu:
>  	rproc_disable_iommu(rproc);
>  	return ret;
>  }
> @@ -1443,7 +1425,7 @@ struct rproc *rproc_alloc(struct device *dev, const
> char *name,
>  	/* Default to ELF loader if no load function is specified */
>  	if (!rproc->ops->load) {
>  		rproc->ops->load = rproc_elf_load_segments;
> -		rproc->ops->find_rsc_table = rproc_elf_find_rsc_table;
> +		rproc->ops->load_rsc_table = rproc_elf_load_rsc_table;
>  		rproc->ops->find_loaded_rsc_table =
> rproc_elf_find_loaded_rsc_table;
>  		rproc->ops->sanity_check = rproc_elf_sanity_check;
>  		rproc->ops->get_boot_addr = rproc_elf_get_boot_addr;
> diff --git a/drivers/remoteproc/remoteproc_elf_loader.c
> b/drivers/remoteproc/remoteproc_elf_loader.c
> index 822fa1bf893f..b17d72ec8603 100644
> --- a/drivers/remoteproc/remoteproc_elf_loader.c
> +++ b/drivers/remoteproc/remoteproc_elf_loader.c
> @@ -268,42 +268,49 @@ find_table(struct device *dev, struct elf32_hdr
> *ehdr, size_t fw_size)
>  }
> 
>  /**
> - * rproc_elf_find_rsc_table() - find the resource table
> + * rproc_elf_load_rsc_table() - load the resource table
>   * @rproc: the rproc handle
>   * @fw: the ELF firmware image
> - * @tablesz: place holder for providing back the table size
>   *
>   * This function finds the resource table inside the remote processor's
> - * firmware. It is used both upon the registration of @rproc (in order
> - * to look for and register the supported virito devices), and when the
> - * @rproc is booted.
> + * firmware, load it into the @cached_table and update @table_ptr.
>   *
> - * Returns the pointer to the resource table if it is found, and write its
> - * size into @tablesz. If a valid table isn't found, NULL is returned
> - * (and @tablesz isn't set).
> + * Return: 0 on success, negative errno on failure.
>   */
> -struct resource_table *rproc_elf_find_rsc_table(struct rproc *rproc,
> -						const struct firmware *fw,
> -						int *tablesz)
> +int rproc_elf_load_rsc_table(struct rproc *rproc, const struct firmware *fw)
>  {
>  	struct elf32_hdr *ehdr;
>  	struct elf32_shdr *shdr;
>  	struct device *dev = &rproc->dev;
>  	struct resource_table *table = NULL;
>  	const u8 *elf_data = fw->data;
> +	size_t tablesz;
> 
>  	ehdr = (struct elf32_hdr *)elf_data;
> 
>  	shdr = find_table(dev, ehdr, fw->size);
>  	if (!shdr)
> -		return NULL;
> +		return -EINVAL;
> 
>  	table = (struct resource_table *)(elf_data + shdr->sh_offset);
> -	*tablesz = shdr->sh_size;
> +	tablesz = shdr->sh_size;
> +
> +	/*
> +	 * Create a copy of the resource table. When a virtio device starts
> +	 * and calls vring_new_virtqueue() the address of the allocated vring
> +	 * will be stored in the cached_table. Before the device is started,
> +	 * cached_table will be copied into device memory.
> +	 */
> +	rproc->cached_table = kmemdup(table, tablesz, GFP_KERNEL);
> +	if (!rproc->cached_table)
> +		return -ENOMEM;
> 
> -	return table;
> +	rproc->table_ptr = rproc->cached_table;
> +	rproc->table_sz = tablesz;
> +
> +	return 0;
>  }
> -EXPORT_SYMBOL(rproc_elf_find_rsc_table);
> +EXPORT_SYMBOL(rproc_elf_load_rsc_table);
[LPA] Hi Bjorn,

I understand the simplification of the  code, but I would like to keep rproc_elf_find_rsc_table function as helper one.
Indeed if a driver wants to override the default rproc_elf_load_rsc_table, it may have to parse first the elf file to look up for resource table using rproc_elf_find_rsc_table as helper.

Regards,
Loic

> 
>  /**
>   * rproc_elf_find_loaded_rsc_table() - find the loaded resource table
> diff --git a/drivers/remoteproc/remoteproc_internal.h
> b/drivers/remoteproc/remoteproc_internal.h
> index a42690c514e2..55a2950c5cb7 100644
> --- a/drivers/remoteproc/remoteproc_internal.h
> +++ b/drivers/remoteproc/remoteproc_internal.h
> @@ -57,9 +57,7 @@ int rproc_trigger_recovery(struct rproc *rproc);
>  int rproc_elf_sanity_check(struct rproc *rproc, const struct firmware *fw);
>  u32 rproc_elf_get_boot_addr(struct rproc *rproc, const struct firmware
> *fw);
>  int rproc_elf_load_segments(struct rproc *rproc, const struct firmware
> *fw);
> -struct resource_table *rproc_elf_find_rsc_table(struct rproc *rproc,
> -						const struct firmware *fw,
> -						int *tablesz);
> +int rproc_elf_load_rsc_table(struct rproc *rproc, const struct firmware
> *fw);
>  struct resource_table *rproc_elf_find_loaded_rsc_table(struct rproc *rproc,
>  						       const struct firmware
> *fw);
> 
> @@ -90,15 +88,13 @@ int rproc_load_segments(struct rproc *rproc, const
> struct firmware *fw)
>  	return -EINVAL;
>  }
> 
> -static inline
> -struct resource_table *rproc_find_rsc_table(struct rproc *rproc,
> -					    const struct firmware *fw,
> -					    int *tablesz)
> +static inline int rproc_load_rsc_table(struct rproc *rproc,
> +				       const struct firmware *fw)
>  {
> -	if (rproc->ops->find_rsc_table)
> -		return rproc->ops->find_rsc_table(rproc, fw, tablesz);
> +	if (rproc->ops->load_rsc_table)
> +		return rproc->ops->load_rsc_table(rproc, fw);
> 
> -	return NULL;
> +	return 0;
>  }
> 
>  static inline
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index e21de55e19d1..ec1ada7cc6d7 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -344,6 +344,7 @@ struct rproc_ops {
>  	int (*stop)(struct rproc *rproc);
>  	void (*kick)(struct rproc *rproc, int vqid);
>  	void * (*da_to_va)(struct rproc *rproc, u64 da, int len);
> +	int (*load_rsc_table)(struct rproc *rproc, const struct firmware *fw);
>  	struct resource_table *(*find_rsc_table)(struct rproc *rproc,
>  						 const struct firmware *fw,
>  						 int *tablesz);
> --
> 2.15.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/8] remoteproc: Move resource table load logic to find

[Bjorn Andersson]

On Thu 14 Dec 03:25 PST 2017, Loic PALLARDY wrote:
> > -----Original Message-----
> > From: linux-remoteproc-owner@vger.kernel.org [mailto:linux-remoteproc-
> > owner@vger.kernel.org] On Behalf Of Bjorn Andersson
> > Sent: Wednesday, December 13, 2017 11:41 PM
[..]
> > -struct resource_table *rproc_elf_find_rsc_table(struct rproc *rproc,
> > -						const struct firmware *fw,
> > -						int *tablesz)
> > +int rproc_elf_load_rsc_table(struct rproc *rproc, const struct firmware *fw)
> >  {
> >  	struct elf32_hdr *ehdr;
> >  	struct elf32_shdr *shdr;
> >  	struct device *dev = &rproc->dev;
> >  	struct resource_table *table = NULL;
> >  	const u8 *elf_data = fw->data;
> > +	size_t tablesz;
> > 
> >  	ehdr = (struct elf32_hdr *)elf_data;
> > 
> >  	shdr = find_table(dev, ehdr, fw->size);
> >  	if (!shdr)
> > -		return NULL;
> > +		return -EINVAL;
> > 
> >  	table = (struct resource_table *)(elf_data + shdr->sh_offset);
> > -	*tablesz = shdr->sh_size;
> > +	tablesz = shdr->sh_size;
> > +
> > +	/*
> > +	 * Create a copy of the resource table. When a virtio device starts
> > +	 * and calls vring_new_virtqueue() the address of the allocated vring
> > +	 * will be stored in the cached_table. Before the device is started,
> > +	 * cached_table will be copied into device memory.
> > +	 */
> > +	rproc->cached_table = kmemdup(table, tablesz, GFP_KERNEL);
> > +	if (!rproc->cached_table)
> > +		return -ENOMEM;
> > 
> > -	return table;
> > +	rproc->table_ptr = rproc->cached_table;
> > +	rproc->table_sz = tablesz;
> > +
> > +	return 0;
> >  }
> > -EXPORT_SYMBOL(rproc_elf_find_rsc_table);
> > +EXPORT_SYMBOL(rproc_elf_load_rsc_table);
> [LPA] Hi Bjorn,
> 
> I understand the simplification of the  code, but I would like to keep
> rproc_elf_find_rsc_table function as helper one.  Indeed if a driver
> wants to override the default rproc_elf_load_rsc_table, it may have to
> parse first the elf file to look up for resource table using
> rproc_elf_find_rsc_table as helper.

find_table() finds the .resource_table segment and verifies that this
contains a valid, version 1, resource table.

So AFAICT there are two scenarios here:
1) A driver wants to support custom entries in the resource table, for
which Suman's proposal of adding a way for drivers to implement custom
properties seems like a good solution.

2) A driver wants to support something other than a v1 resource table,
in which case they would need to reimplement find_table() anyways.


It's possible that the part of find_table() that looks up the segment by
name could be exposed for drivers to use this, but half of find_table()
deals with verifying that the content is a proper resource table.

Please let me know if you have a use case beyond these.

Regards,
Bjorn

RE: [PATCH 6/8] remoteproc: Move resource table load logic to find

[Loic PALLARDY]

> -----Original Message-----
> From: linux-remoteproc-owner@vger.kernel.org [mailto:linux-remoteproc-
> owner@vger.kernel.org] On Behalf Of Bjorn Andersson
> Sent: Wednesday, December 13, 2017 11:41 PM
> To: Ohad Ben-Cohen <ohad@wizery.com>; Bjorn Andersson
> <bjorn.andersson@linaro.org>
> Cc: linux-remoteproc@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [PATCH 6/8] remoteproc: Move resource table load logic to find
> 
> Extend the previous operation of finding the resource table in the ELF
> with the extra step of populating the rproc struct with a copy and the
> size. This allows drivers to override the mechanism used for acquiring
> the resource table, or omit it for firmware that is known not to have a
> resource table.
> 
> This leaves the custom, dummy, find_rsc_table implementations found in
> some drivers dangling.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  drivers/remoteproc/remoteproc_core.c       | 32 ++++++--------------------
>  drivers/remoteproc/remoteproc_elf_loader.c | 37 ++++++++++++++++++--
> ----------
>  drivers/remoteproc/remoteproc_internal.h   | 16 +++++--------
>  include/linux/remoteproc.h                 |  1 +
>  4 files changed, 36 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c
> b/drivers/remoteproc/remoteproc_core.c
> index 63d88d1d206e..cbd12382b219 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -907,8 +907,7 @@ static int rproc_fw_boot(struct rproc *rproc, const
> struct firmware *fw)
>  {
>  	struct device *dev = &rproc->dev;
>  	const char *name = rproc->firmware;
> -	struct resource_table *table;
> -	int ret, tablesz;
> +	int ret;
> 
>  	ret = rproc_fw_sanity_check(rproc, fw);
>  	if (ret)
> @@ -927,27 +926,11 @@ static int rproc_fw_boot(struct rproc *rproc, const
> struct firmware *fw)
>  	}
> 
>  	rproc->bootaddr = rproc_get_boot_addr(rproc, fw);
> -	ret = -EINVAL;
> -
> -	/* look for the resource table */
> -	table = rproc_find_rsc_table(rproc, fw, &tablesz);
> -	if (!table) {
> -		dev_err(dev, "Failed to find resource table\n");
> -		goto clean_up;
> -	}
> -
> -	/*
> -	 * Create a copy of the resource table. When a virtio device starts
> -	 * and calls vring_new_virtqueue() the address of the allocated vring
> -	 * will be stored in the cached_table. Before the device is started,
> -	 * cached_table will be copied into device memory.
> -	 */
> -	rproc->cached_table = kmemdup(table, tablesz, GFP_KERNEL);
> -	if (!rproc->cached_table)
> -		goto clean_up;
> 
> -	rproc->table_ptr = rproc->cached_table;
> -	rproc->table_sz = tablesz;
> +	/* load resource table */
> +	ret = rproc_load_rsc_table(rproc, fw);
> +	if (ret)
> +		goto disable_iommu;
> 
>  	/* reset max_notifyid */
>  	rproc->max_notifyid = -1;
> @@ -967,11 +950,10 @@ static int rproc_fw_boot(struct rproc *rproc, const
> struct firmware *fw)
> 
>  clean_up_resources:
>  	rproc_resource_cleanup(rproc);
> -clean_up:
>  	kfree(rproc->cached_table);
>  	rproc->cached_table = NULL;
>  	rproc->table_ptr = NULL;
> -
> +disable_iommu:
>  	rproc_disable_iommu(rproc);
>  	return ret;
>  }
> @@ -1443,7 +1425,7 @@ struct rproc *rproc_alloc(struct device *dev, const
> char *name,
>  	/* Default to ELF loader if no load function is specified */
>  	if (!rproc->ops->load) {
>  		rproc->ops->load = rproc_elf_load_segments;
> -		rproc->ops->find_rsc_table = rproc_elf_find_rsc_table;
> +		rproc->ops->load_rsc_table = rproc_elf_load_rsc_table;
>  		rproc->ops->find_loaded_rsc_table =
> rproc_elf_find_loaded_rsc_table;
>  		rproc->ops->sanity_check = rproc_elf_sanity_check;
>  		rproc->ops->get_boot_addr = rproc_elf_get_boot_addr;
> diff --git a/drivers/remoteproc/remoteproc_elf_loader.c
> b/drivers/remoteproc/remoteproc_elf_loader.c
> index 822fa1bf893f..b17d72ec8603 100644
> --- a/drivers/remoteproc/remoteproc_elf_loader.c
> +++ b/drivers/remoteproc/remoteproc_elf_loader.c
> @@ -268,42 +268,49 @@ find_table(struct device *dev, struct elf32_hdr
> *ehdr, size_t fw_size)
>  }
> 
>  /**
> - * rproc_elf_find_rsc_table() - find the resource table
> + * rproc_elf_load_rsc_table() - load the resource table
>   * @rproc: the rproc handle
>   * @fw: the ELF firmware image
> - * @tablesz: place holder for providing back the table size
>   *
>   * This function finds the resource table inside the remote processor's
> - * firmware. It is used both upon the registration of @rproc (in order
> - * to look for and register the supported virito devices), and when the
> - * @rproc is booted.
> + * firmware, load it into the @cached_table and update @table_ptr.
>   *
> - * Returns the pointer to the resource table if it is found, and write its
> - * size into @tablesz. If a valid table isn't found, NULL is returned
> - * (and @tablesz isn't set).
> + * Return: 0 on success, negative errno on failure.
>   */
> -struct resource_table *rproc_elf_find_rsc_table(struct rproc *rproc,
> -						const struct firmware *fw,
> -						int *tablesz)
> +int rproc_elf_load_rsc_table(struct rproc *rproc, const struct firmware *fw)
>  {
>  	struct elf32_hdr *ehdr;
>  	struct elf32_shdr *shdr;
>  	struct device *dev = &rproc->dev;
>  	struct resource_table *table = NULL;
>  	const u8 *elf_data = fw->data;
> +	size_t tablesz;
> 
>  	ehdr = (struct elf32_hdr *)elf_data;
> 
>  	shdr = find_table(dev, ehdr, fw->size);
>  	if (!shdr)
> -		return NULL;
> +		return -EINVAL;
Reading patch 7, I don't catch how firmware without resource table are supported as if no table found, an error is returned here.
In rproc_fw_boot, you have:
	ret = rproc_load_rsc_table(rproc, fw);
	if (ret)
		goto disable_iommu;
So boot process is interrupted...

Regards,
Loic

> 
>  	table = (struct resource_table *)(elf_data + shdr->sh_offset);
> -	*tablesz = shdr->sh_size;
> +	tablesz = shdr->sh_size;
> +
> +	/*
> +	 * Create a copy of the resource table. When a virtio device starts
> +	 * and calls vring_new_virtqueue() the address of the allocated vring
> +	 * will be stored in the cached_table. Before the device is started,
> +	 * cached_table will be copied into device memory.
> +	 */
> +	rproc->cached_table = kmemdup(table, tablesz, GFP_KERNEL);
> +	if (!rproc->cached_table)
> +		return -ENOMEM;
> 
> -	return table;
> +	rproc->table_ptr = rproc->cached_table;
> +	rproc->table_sz = tablesz;
> +
> +	return 0;
>  }
> -EXPORT_SYMBOL(rproc_elf_find_rsc_table);
> +EXPORT_SYMBOL(rproc_elf_load_rsc_table);
> 
>  /**
>   * rproc_elf_find_loaded_rsc_table() - find the loaded resource table
> diff --git a/drivers/remoteproc/remoteproc_internal.h
> b/drivers/remoteproc/remoteproc_internal.h
> index a42690c514e2..55a2950c5cb7 100644
> --- a/drivers/remoteproc/remoteproc_internal.h
> +++ b/drivers/remoteproc/remoteproc_internal.h
> @@ -57,9 +57,7 @@ int rproc_trigger_recovery(struct rproc *rproc);
>  int rproc_elf_sanity_check(struct rproc *rproc, const struct firmware *fw);
>  u32 rproc_elf_get_boot_addr(struct rproc *rproc, const struct firmware
> *fw);
>  int rproc_elf_load_segments(struct rproc *rproc, const struct firmware
> *fw);
> -struct resource_table *rproc_elf_find_rsc_table(struct rproc *rproc,
> -						const struct firmware *fw,
> -						int *tablesz);
> +int rproc_elf_load_rsc_table(struct rproc *rproc, const struct firmware
> *fw);
>  struct resource_table *rproc_elf_find_loaded_rsc_table(struct rproc *rproc,
>  						       const struct firmware
> *fw);
> 
> @@ -90,15 +88,13 @@ int rproc_load_segments(struct rproc *rproc, const
> struct firmware *fw)
>  	return -EINVAL;
>  }
> 
> -static inline
> -struct resource_table *rproc_find_rsc_table(struct rproc *rproc,
> -					    const struct firmware *fw,
> -					    int *tablesz)
> +static inline int rproc_load_rsc_table(struct rproc *rproc,
> +				       const struct firmware *fw)
>  {
> -	if (rproc->ops->find_rsc_table)
> -		return rproc->ops->find_rsc_table(rproc, fw, tablesz);
> +	if (rproc->ops->load_rsc_table)
> +		return rproc->ops->load_rsc_table(rproc, fw);
> 
> -	return NULL;
> +	return 0;
>  }
> 
>  static inline
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index e21de55e19d1..ec1ada7cc6d7 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -344,6 +344,7 @@ struct rproc_ops {
>  	int (*stop)(struct rproc *rproc);
>  	void (*kick)(struct rproc *rproc, int vqid);
>  	void * (*da_to_va)(struct rproc *rproc, u64 da, int len);
> +	int (*load_rsc_table)(struct rproc *rproc, const struct firmware *fw);
>  	struct resource_table *(*find_rsc_table)(struct rproc *rproc,
>  						 const struct firmware *fw,
>  						 int *tablesz);
> --
> 2.15.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/8] remoteproc: Move resource table load logic to find

[Bjorn Andersson]

On Thu 14 Dec 04:00 PST 2017, Loic PALLARDY wrote:
> > -----Original Message-----
> > From: linux-remoteproc-owner@vger.kernel.org [mailto:linux-remoteproc-
> > owner@vger.kernel.org] On Behalf Of Bjorn Andersson
[..]
> > +int rproc_elf_load_rsc_table(struct rproc *rproc, const struct firmware *fw)
> >  {
> >  	struct elf32_hdr *ehdr;
> >  	struct elf32_shdr *shdr;
> >  	struct device *dev = &rproc->dev;
> >  	struct resource_table *table = NULL;
> >  	const u8 *elf_data = fw->data;
> > +	size_t tablesz;
> > 
> >  	ehdr = (struct elf32_hdr *)elf_data;
> > 
> >  	shdr = find_table(dev, ehdr, fw->size);
> >  	if (!shdr)
> > -		return NULL;
> > +		return -EINVAL;
> Reading patch 7, I don't catch how firmware without resource table are
> supported as if no table found, an error is returned here.
> In rproc_fw_boot, you have:
> 	ret = rproc_load_rsc_table(rproc, fw);
> 	if (ret)
> 		goto disable_iommu;
> So boot process is interrupted...
> 

When a driver registers a remoteproc with load_rsc_table specified that
implies that the resource table is not optional as such it's an error to
not find one.

A driver that expects to load firmware without a resource table, such as
st_slim_rproc with it's dummy resource table would set load_rsc_table to
NULL.


So the purpose is to maintain the current requirements, with the
addition of removing the dummy implementations of find_rsc_table.

Regards,
Bjorn

RE: [PATCH 6/8] remoteproc: Move resource table load logic to find

[Loic PALLARDY]

> -----Original Message-----
> From: linux-remoteproc-owner@vger.kernel.org [mailto:linux-remoteproc-
> owner@vger.kernel.org] On Behalf Of Bjorn Andersson
> Sent: Thursday, December 14, 2017 8:48 PM
> To: Loic PALLARDY <loic.pallardy@st.com>
> Cc: Ohad Ben-Cohen <ohad@wizery.com>; linux-
> remoteproc@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 6/8] remoteproc: Move resource table load logic to find
> 
> On Thu 14 Dec 04:00 PST 2017, Loic PALLARDY wrote:
> > > -----Original Message-----
> > > From: linux-remoteproc-owner@vger.kernel.org [mailto:linux-
> remoteproc-
> > > owner@vger.kernel.org] On Behalf Of Bjorn Andersson
> [..]
> > > +int rproc_elf_load_rsc_table(struct rproc *rproc, const struct firmware
> *fw)
> > >  {
> > >  	struct elf32_hdr *ehdr;
> > >  	struct elf32_shdr *shdr;
> > >  	struct device *dev = &rproc->dev;
> > >  	struct resource_table *table = NULL;
> > >  	const u8 *elf_data = fw->data;
> > > +	size_t tablesz;
> > >
> > >  	ehdr = (struct elf32_hdr *)elf_data;
> > >
> > >  	shdr = find_table(dev, ehdr, fw->size);
> > >  	if (!shdr)
> > > -		return NULL;
> > > +		return -EINVAL;
> > Reading patch 7, I don't catch how firmware without resource table are
> > supported as if no table found, an error is returned here.
> > In rproc_fw_boot, you have:
> > 	ret = rproc_load_rsc_table(rproc, fw);
> > 	if (ret)
> > 		goto disable_iommu;
> > So boot process is interrupted...
> >
> 
> When a driver registers a remoteproc with load_rsc_table specified that
> implies that the resource table is not optional as such it's an error to
> not find one.
> 
> A driver that expects to load firmware without a resource table, such as
> st_slim_rproc with it's dummy resource table would set load_rsc_table to
> NULL.
Yes I saw that if no load_rsc_table, 0 is returned. Thanks for the explanations, I'll apply the series and review final code.
Regards,
Loic
> 
> 
> So the purpose is to maintain the current requirements, with the
> addition of removing the dummy implementations of find_rsc_table.
> 
> Regards,
> Bjorn
> --
> To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 7/8] remoteproc: Drop dangling find_rsc_table dummies

[Bjorn Andersson]

As the core now deals with the lack of a resource table, remove the
dangling custom dummy implementations of find_rsc_table from drivers.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/remoteproc/qcom_adsp_pil.c |  1 -
 drivers/remoteproc/qcom_common.c   | 19 -------------------
 drivers/remoteproc/qcom_common.h   |  4 ----
 drivers/remoteproc/qcom_q6v5_pil.c | 11 -----------
 drivers/remoteproc/qcom_wcnss.c    |  1 -
 drivers/remoteproc/st_slim_rproc.c | 18 ------------------
 include/linux/remoteproc.h         |  4 ----
 7 files changed, 58 deletions(-)

diff --git a/drivers/remoteproc/qcom_adsp_pil.c b/drivers/remoteproc/qcom_adsp_pil.c
index 7b9d810b23f1..b0b0d5ca1ca0 100644
--- a/drivers/remoteproc/qcom_adsp_pil.c
+++ b/drivers/remoteproc/qcom_adsp_pil.c
@@ -181,7 +181,6 @@ static const struct rproc_ops adsp_ops = {
 	.start = adsp_start,
 	.stop = adsp_stop,
 	.da_to_va = adsp_da_to_va,
-	.find_rsc_table = qcom_mdt_find_rsc_table,
 	.load = adsp_load,
 };
 
diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
index 818ee3657043..ce2dcc4f7de7 100644
--- a/drivers/remoteproc/qcom_common.c
+++ b/drivers/remoteproc/qcom_common.c
@@ -32,25 +32,6 @@
 
 static BLOCKING_NOTIFIER_HEAD(ssr_notifiers);
 
-/**
- * qcom_mdt_find_rsc_table() - provide dummy resource table for remoteproc
- * @rproc:	remoteproc handle
- * @fw:		firmware header
- * @tablesz:	outgoing size of the table
- *
- * Returns a dummy table.
- */
-struct resource_table *qcom_mdt_find_rsc_table(struct rproc *rproc,
-					       const struct firmware *fw,
-					       int *tablesz)
-{
-	static struct resource_table table = { .ver = 1, };
-
-	*tablesz = sizeof(table);
-	return &table;
-}
-EXPORT_SYMBOL_GPL(qcom_mdt_find_rsc_table);
-
 static int glink_subdev_probe(struct rproc_subdev *subdev)
 {
 	struct qcom_rproc_glink *glink = to_glink_subdev(subdev);
diff --git a/drivers/remoteproc/qcom_common.h b/drivers/remoteproc/qcom_common.h
index 541586e528b3..73efed969bfd 100644
--- a/drivers/remoteproc/qcom_common.h
+++ b/drivers/remoteproc/qcom_common.h
@@ -30,10 +30,6 @@ struct qcom_rproc_ssr {
 	const char *name;
 };
 
-struct resource_table *qcom_mdt_find_rsc_table(struct rproc *rproc,
-					       const struct firmware *fw,
-					       int *tablesz);
-
 void qcom_add_glink_subdev(struct rproc *rproc, struct qcom_rproc_glink *glink);
 void qcom_remove_glink_subdev(struct rproc *rproc, struct qcom_rproc_glink *glink);
 
diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index fbff5d842581..6f6ea0414366 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -304,16 +304,6 @@ static void q6v5_clk_disable(struct device *dev,
 		clk_disable_unprepare(clks[i]);
 }
 
-static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc,
-						  const struct firmware *fw,
-						  int *tablesz)
-{
-	static struct resource_table table = { .ver = 1, };
-
-	*tablesz = sizeof(table);
-	return &table;
-}
-
 static int q6v5_xfer_mem_ownership(struct q6v5 *qproc, int *current_perm,
 				   bool remote_owner, phys_addr_t addr,
 				   size_t size)
@@ -927,7 +917,6 @@ static const struct rproc_ops q6v5_ops = {
 	.start = q6v5_start,
 	.stop = q6v5_stop,
 	.da_to_va = q6v5_da_to_va,
-	.find_rsc_table = q6v5_find_rsc_table,
 	.load = q6v5_load,
 };
 
diff --git a/drivers/remoteproc/qcom_wcnss.c b/drivers/remoteproc/qcom_wcnss.c
index cc44ec598522..1fa5253020dd 100644
--- a/drivers/remoteproc/qcom_wcnss.c
+++ b/drivers/remoteproc/qcom_wcnss.c
@@ -310,7 +310,6 @@ static const struct rproc_ops wcnss_ops = {
 	.start = wcnss_start,
 	.stop = wcnss_stop,
 	.da_to_va = wcnss_da_to_va,
-	.find_rsc_table = qcom_mdt_find_rsc_table,
 	.load = wcnss_load,
 };
 
diff --git a/drivers/remoteproc/st_slim_rproc.c b/drivers/remoteproc/st_slim_rproc.c
index 1538ea915c49..c6a2a8b68c7a 100644
--- a/drivers/remoteproc/st_slim_rproc.c
+++ b/drivers/remoteproc/st_slim_rproc.c
@@ -200,28 +200,10 @@ static void *slim_rproc_da_to_va(struct rproc *rproc, u64 da, int len)
 	return va;
 }
 
-/*
- * Firmware handler operations: sanity, boot address, load ...
- */
-
-static struct resource_table empty_rsc_tbl = {
-	.ver = 1,
-	.num = 0,
-};
-
-static struct resource_table *slim_rproc_find_rsc_table(struct rproc *rproc,
-					       const struct firmware *fw,
-					       int *tablesz)
-{
-	*tablesz = sizeof(empty_rsc_tbl);
-	return &empty_rsc_tbl;
-}
-
 static const struct rproc_ops slim_rproc_ops = {
 	.start		= slim_rproc_start,
 	.stop		= slim_rproc_stop,
 	.da_to_va       = slim_rproc_da_to_va,
-	.find_rsc_table = slim_rproc_find_rsc_table,
 };
 
 /**
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index ec1ada7cc6d7..51ddde7535b9 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -332,7 +332,6 @@ struct firmware;
  * @stop:	power off the device
  * @kick:	kick a virtqueue (virtqueue id given as a parameter)
  * @da_to_va:	optional platform hook to perform address translations
- * @find_rsc_table:	find the resource table inside the firmware image
  * @find_loaded_rsc_table: find the loaded resouce table
  * @load:		load firmeware to memory, where the remote processor
  *			expects to find it
@@ -345,9 +344,6 @@ struct rproc_ops {
 	void (*kick)(struct rproc *rproc, int vqid);
 	void * (*da_to_va)(struct rproc *rproc, u64 da, int len);
 	int (*load_rsc_table)(struct rproc *rproc, const struct firmware *fw);
-	struct resource_table *(*find_rsc_table)(struct rproc *rproc,
-						 const struct firmware *fw,
-						 int *tablesz);
 	struct resource_table *(*find_loaded_rsc_table)(
 				struct rproc *rproc, const struct firmware *fw);
 	int (*load)(struct rproc *rproc, const struct firmware *fw);
-- 
2.15.0

RE: [PATCH 7/8] remoteproc: Drop dangling find_rsc_table dummies

[Loic PALLARDY]

> -----Original Message-----
> From: linux-remoteproc-owner@vger.kernel.org [mailto:linux-remoteproc-
> owner@vger.kernel.org] On Behalf Of Bjorn Andersson
> Sent: Wednesday, December 13, 2017 11:41 PM
> To: Ohad Ben-Cohen <ohad@wizery.com>; Bjorn Andersson
> <bjorn.andersson@linaro.org>
> Cc: linux-remoteproc@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [PATCH 7/8] remoteproc: Drop dangling find_rsc_table dummies
> 
> As the core now deals with the lack of a resource table, remove the
> dangling custom dummy implementations of find_rsc_table from drivers.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  drivers/remoteproc/qcom_adsp_pil.c |  1 -
>  drivers/remoteproc/qcom_common.c   | 19 -------------------
>  drivers/remoteproc/qcom_common.h   |  4 ----
>  drivers/remoteproc/qcom_q6v5_pil.c | 11 -----------
>  drivers/remoteproc/qcom_wcnss.c    |  1 -
>  drivers/remoteproc/st_slim_rproc.c | 18 ------------------
>  include/linux/remoteproc.h         |  4 ----
>  7 files changed, 58 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_adsp_pil.c
> b/drivers/remoteproc/qcom_adsp_pil.c
> index 7b9d810b23f1..b0b0d5ca1ca0 100644
> --- a/drivers/remoteproc/qcom_adsp_pil.c
> +++ b/drivers/remoteproc/qcom_adsp_pil.c
> @@ -181,7 +181,6 @@ static const struct rproc_ops adsp_ops = {
>  	.start = adsp_start,
>  	.stop = adsp_stop,
>  	.da_to_va = adsp_da_to_va,
> -	.find_rsc_table = qcom_mdt_find_rsc_table,
>  	.load = adsp_load,
>  };
> 
> diff --git a/drivers/remoteproc/qcom_common.c
> b/drivers/remoteproc/qcom_common.c
> index 818ee3657043..ce2dcc4f7de7 100644
> --- a/drivers/remoteproc/qcom_common.c
> +++ b/drivers/remoteproc/qcom_common.c
> @@ -32,25 +32,6 @@
> 
>  static BLOCKING_NOTIFIER_HEAD(ssr_notifiers);
> 
> -/**
> - * qcom_mdt_find_rsc_table() - provide dummy resource table for
> remoteproc
> - * @rproc:	remoteproc handle
> - * @fw:		firmware header
> - * @tablesz:	outgoing size of the table
> - *
> - * Returns a dummy table.
> - */
> -struct resource_table *qcom_mdt_find_rsc_table(struct rproc *rproc,
> -					       const struct firmware *fw,
> -					       int *tablesz)
> -{
> -	static struct resource_table table = { .ver = 1, };
> -
> -	*tablesz = sizeof(table);
> -	return &table;
> -}
> -EXPORT_SYMBOL_GPL(qcom_mdt_find_rsc_table);
> -
>  static int glink_subdev_probe(struct rproc_subdev *subdev)
>  {
>  	struct qcom_rproc_glink *glink = to_glink_subdev(subdev);
> diff --git a/drivers/remoteproc/qcom_common.h
> b/drivers/remoteproc/qcom_common.h
> index 541586e528b3..73efed969bfd 100644
> --- a/drivers/remoteproc/qcom_common.h
> +++ b/drivers/remoteproc/qcom_common.h
> @@ -30,10 +30,6 @@ struct qcom_rproc_ssr {
>  	const char *name;
>  };
> 
> -struct resource_table *qcom_mdt_find_rsc_table(struct rproc *rproc,
> -					       const struct firmware *fw,
> -					       int *tablesz);
> -
>  void qcom_add_glink_subdev(struct rproc *rproc, struct qcom_rproc_glink
> *glink);
>  void qcom_remove_glink_subdev(struct rproc *rproc, struct
> qcom_rproc_glink *glink);
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c
> b/drivers/remoteproc/qcom_q6v5_pil.c
> index fbff5d842581..6f6ea0414366 100644
> --- a/drivers/remoteproc/qcom_q6v5_pil.c
> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
> @@ -304,16 +304,6 @@ static void q6v5_clk_disable(struct device *dev,
>  		clk_disable_unprepare(clks[i]);
>  }
> 
> -static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc,
> -						  const struct firmware *fw,
> -						  int *tablesz)
> -{
> -	static struct resource_table table = { .ver = 1, };
> -
> -	*tablesz = sizeof(table);
> -	return &table;
> -}
> -
>  static int q6v5_xfer_mem_ownership(struct q6v5 *qproc, int
> *current_perm,
>  				   bool remote_owner, phys_addr_t addr,
>  				   size_t size)
> @@ -927,7 +917,6 @@ static const struct rproc_ops q6v5_ops = {
>  	.start = q6v5_start,
>  	.stop = q6v5_stop,
>  	.da_to_va = q6v5_da_to_va,
> -	.find_rsc_table = q6v5_find_rsc_table,
>  	.load = q6v5_load,
>  };
> 
> diff --git a/drivers/remoteproc/qcom_wcnss.c
> b/drivers/remoteproc/qcom_wcnss.c
> index cc44ec598522..1fa5253020dd 100644
> --- a/drivers/remoteproc/qcom_wcnss.c
> +++ b/drivers/remoteproc/qcom_wcnss.c
> @@ -310,7 +310,6 @@ static const struct rproc_ops wcnss_ops = {
>  	.start = wcnss_start,
>  	.stop = wcnss_stop,
>  	.da_to_va = wcnss_da_to_va,
> -	.find_rsc_table = qcom_mdt_find_rsc_table,
>  	.load = wcnss_load,
>  };
> 
> diff --git a/drivers/remoteproc/st_slim_rproc.c
> b/drivers/remoteproc/st_slim_rproc.c
> index 1538ea915c49..c6a2a8b68c7a 100644
> --- a/drivers/remoteproc/st_slim_rproc.c
> +++ b/drivers/remoteproc/st_slim_rproc.c
> @@ -200,28 +200,10 @@ static void *slim_rproc_da_to_va(struct rproc
> *rproc, u64 da, int len)
>  	return va;
>  }
> 
> -/*
> - * Firmware handler operations: sanity, boot address, load ...
> - */
> -
> -static struct resource_table empty_rsc_tbl = {
> -	.ver = 1,
> -	.num = 0,
> -};
> -
> -static struct resource_table *slim_rproc_find_rsc_table(struct rproc *rproc,
> -					       const struct firmware *fw,
> -					       int *tablesz)
> -{
> -	*tablesz = sizeof(empty_rsc_tbl);
> -	return &empty_rsc_tbl;
> -}
> -
>  static const struct rproc_ops slim_rproc_ops = {
>  	.start		= slim_rproc_start,
>  	.stop		= slim_rproc_stop,
>  	.da_to_va       = slim_rproc_da_to_va,
> -	.find_rsc_table = slim_rproc_find_rsc_table,
Hi Bjorn, 
Your patch is not complete for st_slim_rproc and so not working.
In your patch 6/8, .load_rsc_table is define to default rproc_elf_load_rsc_table if no load ops defined.

	/* Default to ELF loader if no load function is specified */
 	if (!rproc->ops->load) {
 		rproc->ops->load = rproc_elf_load_segments;
-		rproc->ops->find_rsc_table = rproc_elf_find_rsc_table;
+		rproc->ops->load_rsc_table = rproc_elf_load_rsc_table;

As st_slim_rproc has no load ops, it will inherit from all default ops including rproc_elf_load_rsc_table.
As no resource table present in firmware, an error will be returned and st_slim_rproc will failed.
In case of Qualcom, load ops is defined...
See below B2260 log (with additional error message in rproc_load_rsc_table function.

[   10.201079] remoteproc remoteproc2: st231-delta is available                                               
[   10.258121] remoteproc remoteproc2: powering up st231-delta                                                
[   10.258143] remoteproc remoteproc2: Booting fw image rproc-st231-delta-fw, size 44416                      
[   10.258151] rproc_load_rsc_table: error -22  

Moreover with your proposal, as the choice to support or not a resource table is based on the fact rproc->ops->load_rsc_table is set or not, it is not possible with one unique driver to support firmware with resource table and firmware without resource table.
Will be better from my pov to consider that no resource table  found in rproc_elf_load_rsc_table function as a normal case and not an error.

Regards,
Loic
>  };
> 
>  /**
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index ec1ada7cc6d7..51ddde7535b9 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -332,7 +332,6 @@ struct firmware;
>   * @stop:	power off the device
>   * @kick:	kick a virtqueue (virtqueue id given as a parameter)
>   * @da_to_va:	optional platform hook to perform address translations
> - * @find_rsc_table:	find the resource table inside the firmware image
>   * @find_loaded_rsc_table: find the loaded resouce table
>   * @load:		load firmeware to memory, where the remote
> processor
>   *			expects to find it
> @@ -345,9 +344,6 @@ struct rproc_ops {
>  	void (*kick)(struct rproc *rproc, int vqid);
>  	void * (*da_to_va)(struct rproc *rproc, u64 da, int len);
>  	int (*load_rsc_table)(struct rproc *rproc, const struct firmware *fw);
> -	struct resource_table *(*find_rsc_table)(struct rproc *rproc,
> -						 const struct firmware *fw,
> -						 int *tablesz);
>  	struct resource_table *(*find_loaded_rsc_table)(
>  				struct rproc *rproc, const struct firmware
> *fw);
>  	int (*load)(struct rproc *rproc, const struct firmware *fw);
> --
> 2.15.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 7/8] remoteproc: Drop dangling find_rsc_table dummies

[Bjorn Andersson]

On Fri 05 Jan 08:53 PST 2018, Loic PALLARDY wrote:

> 
> 
> > -----Original Message-----
> > From: linux-remoteproc-owner@vger.kernel.org [mailto:linux-remoteproc-
> > owner@vger.kernel.org] On Behalf Of Bjorn Andersson
> > Sent: Wednesday, December 13, 2017 11:41 PM
> > To: Ohad Ben-Cohen <ohad@wizery.com>; Bjorn Andersson
> > <bjorn.andersson@linaro.org>
> > Cc: linux-remoteproc@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: [PATCH 7/8] remoteproc: Drop dangling find_rsc_table dummies
> > 
> > As the core now deals with the lack of a resource table, remove the
> > dangling custom dummy implementations of find_rsc_table from drivers.
> > 
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> >  drivers/remoteproc/qcom_adsp_pil.c |  1 -
> >  drivers/remoteproc/qcom_common.c   | 19 -------------------
> >  drivers/remoteproc/qcom_common.h   |  4 ----
> >  drivers/remoteproc/qcom_q6v5_pil.c | 11 -----------
> >  drivers/remoteproc/qcom_wcnss.c    |  1 -
> >  drivers/remoteproc/st_slim_rproc.c | 18 ------------------
> >  include/linux/remoteproc.h         |  4 ----
> >  7 files changed, 58 deletions(-)
> > 
> > diff --git a/drivers/remoteproc/qcom_adsp_pil.c
> > b/drivers/remoteproc/qcom_adsp_pil.c
> > index 7b9d810b23f1..b0b0d5ca1ca0 100644
> > --- a/drivers/remoteproc/qcom_adsp_pil.c
> > +++ b/drivers/remoteproc/qcom_adsp_pil.c
> > @@ -181,7 +181,6 @@ static const struct rproc_ops adsp_ops = {
> >  	.start = adsp_start,
> >  	.stop = adsp_stop,
> >  	.da_to_va = adsp_da_to_va,
> > -	.find_rsc_table = qcom_mdt_find_rsc_table,
> >  	.load = adsp_load,
> >  };
> > 
> > diff --git a/drivers/remoteproc/qcom_common.c
> > b/drivers/remoteproc/qcom_common.c
> > index 818ee3657043..ce2dcc4f7de7 100644
> > --- a/drivers/remoteproc/qcom_common.c
> > +++ b/drivers/remoteproc/qcom_common.c
> > @@ -32,25 +32,6 @@
> > 
> >  static BLOCKING_NOTIFIER_HEAD(ssr_notifiers);
> > 
> > -/**
> > - * qcom_mdt_find_rsc_table() - provide dummy resource table for
> > remoteproc
> > - * @rproc:	remoteproc handle
> > - * @fw:		firmware header
> > - * @tablesz:	outgoing size of the table
> > - *
> > - * Returns a dummy table.
> > - */
> > -struct resource_table *qcom_mdt_find_rsc_table(struct rproc *rproc,
> > -					       const struct firmware *fw,
> > -					       int *tablesz)
> > -{
> > -	static struct resource_table table = { .ver = 1, };
> > -
> > -	*tablesz = sizeof(table);
> > -	return &table;
> > -}
> > -EXPORT_SYMBOL_GPL(qcom_mdt_find_rsc_table);
> > -
> >  static int glink_subdev_probe(struct rproc_subdev *subdev)
> >  {
> >  	struct qcom_rproc_glink *glink = to_glink_subdev(subdev);
> > diff --git a/drivers/remoteproc/qcom_common.h
> > b/drivers/remoteproc/qcom_common.h
> > index 541586e528b3..73efed969bfd 100644
> > --- a/drivers/remoteproc/qcom_common.h
> > +++ b/drivers/remoteproc/qcom_common.h
> > @@ -30,10 +30,6 @@ struct qcom_rproc_ssr {
> >  	const char *name;
> >  };
> > 
> > -struct resource_table *qcom_mdt_find_rsc_table(struct rproc *rproc,
> > -					       const struct firmware *fw,
> > -					       int *tablesz);
> > -
> >  void qcom_add_glink_subdev(struct rproc *rproc, struct qcom_rproc_glink
> > *glink);
> >  void qcom_remove_glink_subdev(struct rproc *rproc, struct
> > qcom_rproc_glink *glink);
> > 
> > diff --git a/drivers/remoteproc/qcom_q6v5_pil.c
> > b/drivers/remoteproc/qcom_q6v5_pil.c
> > index fbff5d842581..6f6ea0414366 100644
> > --- a/drivers/remoteproc/qcom_q6v5_pil.c
> > +++ b/drivers/remoteproc/qcom_q6v5_pil.c
> > @@ -304,16 +304,6 @@ static void q6v5_clk_disable(struct device *dev,
> >  		clk_disable_unprepare(clks[i]);
> >  }
> > 
> > -static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc,
> > -						  const struct firmware *fw,
> > -						  int *tablesz)
> > -{
> > -	static struct resource_table table = { .ver = 1, };
> > -
> > -	*tablesz = sizeof(table);
> > -	return &table;
> > -}
> > -
> >  static int q6v5_xfer_mem_ownership(struct q6v5 *qproc, int
> > *current_perm,
> >  				   bool remote_owner, phys_addr_t addr,
> >  				   size_t size)
> > @@ -927,7 +917,6 @@ static const struct rproc_ops q6v5_ops = {
> >  	.start = q6v5_start,
> >  	.stop = q6v5_stop,
> >  	.da_to_va = q6v5_da_to_va,
> > -	.find_rsc_table = q6v5_find_rsc_table,
> >  	.load = q6v5_load,
> >  };
> > 
> > diff --git a/drivers/remoteproc/qcom_wcnss.c
> > b/drivers/remoteproc/qcom_wcnss.c
> > index cc44ec598522..1fa5253020dd 100644
> > --- a/drivers/remoteproc/qcom_wcnss.c
> > +++ b/drivers/remoteproc/qcom_wcnss.c
> > @@ -310,7 +310,6 @@ static const struct rproc_ops wcnss_ops = {
> >  	.start = wcnss_start,
> >  	.stop = wcnss_stop,
> >  	.da_to_va = wcnss_da_to_va,
> > -	.find_rsc_table = qcom_mdt_find_rsc_table,
> >  	.load = wcnss_load,
> >  };
> > 
> > diff --git a/drivers/remoteproc/st_slim_rproc.c
> > b/drivers/remoteproc/st_slim_rproc.c
> > index 1538ea915c49..c6a2a8b68c7a 100644
> > --- a/drivers/remoteproc/st_slim_rproc.c
> > +++ b/drivers/remoteproc/st_slim_rproc.c
> > @@ -200,28 +200,10 @@ static void *slim_rproc_da_to_va(struct rproc
> > *rproc, u64 da, int len)
> >  	return va;
> >  }
> > 
> > -/*
> > - * Firmware handler operations: sanity, boot address, load ...
> > - */
> > -
> > -static struct resource_table empty_rsc_tbl = {
> > -	.ver = 1,
> > -	.num = 0,
> > -};
> > -
> > -static struct resource_table *slim_rproc_find_rsc_table(struct rproc *rproc,
> > -					       const struct firmware *fw,
> > -					       int *tablesz)
> > -{
> > -	*tablesz = sizeof(empty_rsc_tbl);
> > -	return &empty_rsc_tbl;
> > -}
> > -
> >  static const struct rproc_ops slim_rproc_ops = {
> >  	.start		= slim_rproc_start,
> >  	.stop		= slim_rproc_stop,
> >  	.da_to_va       = slim_rproc_da_to_va,
> > -	.find_rsc_table = slim_rproc_find_rsc_table,
> Hi Bjorn, 
> Your patch is not complete for st_slim_rproc and so not working.
> In your patch 6/8, .load_rsc_table is define to default rproc_elf_load_rsc_table if no load ops defined.
> 
> 	/* Default to ELF loader if no load function is specified */
>  	if (!rproc->ops->load) {
>  		rproc->ops->load = rproc_elf_load_segments;
> -		rproc->ops->find_rsc_table = rproc_elf_find_rsc_table;
> +		rproc->ops->load_rsc_table = rproc_elf_load_rsc_table;
> 
> As st_slim_rproc has no load ops, it will inherit from all default ops including rproc_elf_load_rsc_table.
> As no resource table present in firmware, an error will be returned and st_slim_rproc will failed.

Thanks for catching my mistake, Loic! The expected outcome would be that
the slim_rproc_ops does point the "load" to the now exported ELF loader
symbol and by that won't get a find_rsc_table entry from the default
set..

> In case of Qualcom, load ops is defined...

Right, in the end the st_slim driver should have been defined just as
the Qualcomm one - but referencing rproc_elf_load_segments().

> See below B2260 log (with additional error message in rproc_load_rsc_table function.
> 
> [   10.201079] remoteproc remoteproc2: st231-delta is available
> [   10.258121] remoteproc remoteproc2: powering up st231-delta
> [   10.258143] remoteproc remoteproc2: Booting fw image rproc-st231-delta-fw, size 44416
> [   10.258151] rproc_load_rsc_table: error -22  
> 
> Moreover with your proposal, as the choice to support or not a
> resource table is based on the fact rproc->ops->load_rsc_table is set
> or not, it is not possible with one unique driver to support firmware
> with resource table and firmware without resource table.

This is retaining the previous behaviour of failing to load/start a
remoteproc if no resource table is found, when the driver expects one.
The new scheme would allow you to specify a custom load_rsc_table that
calls rproc_elf_load_rsc_table() and ignores the return value to support
your use case.

> Will be better from my pov to consider that no resource table  found
> in rproc_elf_load_rsc_table function as a normal case and not an
> error.

This would be a change in behavior and I can see how this could be
annoying to people (by not catching their mistakes during development).

If you think this is the preferred implementation then please submit a
separate patch for this so we can get some feedback from other users. 

Regards,
Bjorn

RE: [PATCH 7/8] remoteproc: Drop dangling find_rsc_table dummies

[Loic PALLARDY]

> -----Original Message-----
> From: Bjorn Andersson [mailto:bjorn.andersson@linaro.org]
> Sent: Friday, January 05, 2018 7:51 PM
> To: Loic PALLARDY <loic.pallardy@st.com>
> Cc: Ohad Ben-Cohen <ohad@wizery.com>; linux-
> remoteproc@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 7/8] remoteproc: Drop dangling find_rsc_table dummies
> 
> On Fri 05 Jan 08:53 PST 2018, Loic PALLARDY wrote:
> 
> >
> >
> > > -----Original Message-----
> > > From: linux-remoteproc-owner@vger.kernel.org [mailto:linux-
> remoteproc-
> > > owner@vger.kernel.org] On Behalf Of Bjorn Andersson
> > > Sent: Wednesday, December 13, 2017 11:41 PM
> > > To: Ohad Ben-Cohen <ohad@wizery.com>; Bjorn Andersson
> > > <bjorn.andersson@linaro.org>
> > > Cc: linux-remoteproc@vger.kernel.org; linux-kernel@vger.kernel.org
> > > Subject: [PATCH 7/8] remoteproc: Drop dangling find_rsc_table dummies
> > >
> > > As the core now deals with the lack of a resource table, remove the
> > > dangling custom dummy implementations of find_rsc_table from drivers.
> > >
> > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > ---
> > >  drivers/remoteproc/qcom_adsp_pil.c |  1 -
> > >  drivers/remoteproc/qcom_common.c   | 19 -------------------
> > >  drivers/remoteproc/qcom_common.h   |  4 ----
> > >  drivers/remoteproc/qcom_q6v5_pil.c | 11 -----------
> > >  drivers/remoteproc/qcom_wcnss.c    |  1 -
> > >  drivers/remoteproc/st_slim_rproc.c | 18 ------------------
> > >  include/linux/remoteproc.h         |  4 ----
> > >  7 files changed, 58 deletions(-)
> > >
> > > diff --git a/drivers/remoteproc/qcom_adsp_pil.c
> > > b/drivers/remoteproc/qcom_adsp_pil.c
> > > index 7b9d810b23f1..b0b0d5ca1ca0 100644
> > > --- a/drivers/remoteproc/qcom_adsp_pil.c
> > > +++ b/drivers/remoteproc/qcom_adsp_pil.c
> > > @@ -181,7 +181,6 @@ static const struct rproc_ops adsp_ops = {
> > >  	.start = adsp_start,
> > >  	.stop = adsp_stop,
> > >  	.da_to_va = adsp_da_to_va,
> > > -	.find_rsc_table = qcom_mdt_find_rsc_table,
> > >  	.load = adsp_load,
> > >  };
> > >
> > > diff --git a/drivers/remoteproc/qcom_common.c
> > > b/drivers/remoteproc/qcom_common.c
> > > index 818ee3657043..ce2dcc4f7de7 100644
> > > --- a/drivers/remoteproc/qcom_common.c
> > > +++ b/drivers/remoteproc/qcom_common.c
> > > @@ -32,25 +32,6 @@
> > >
> > >  static BLOCKING_NOTIFIER_HEAD(ssr_notifiers);
> > >
> > > -/**
> > > - * qcom_mdt_find_rsc_table() - provide dummy resource table for
> > > remoteproc
> > > - * @rproc:	remoteproc handle
> > > - * @fw:		firmware header
> > > - * @tablesz:	outgoing size of the table
> > > - *
> > > - * Returns a dummy table.
> > > - */
> > > -struct resource_table *qcom_mdt_find_rsc_table(struct rproc *rproc,
> > > -					       const struct firmware *fw,
> > > -					       int *tablesz)
> > > -{
> > > -	static struct resource_table table = { .ver = 1, };
> > > -
> > > -	*tablesz = sizeof(table);
> > > -	return &table;
> > > -}
> > > -EXPORT_SYMBOL_GPL(qcom_mdt_find_rsc_table);
> > > -
> > >  static int glink_subdev_probe(struct rproc_subdev *subdev)
> > >  {
> > >  	struct qcom_rproc_glink *glink = to_glink_subdev(subdev);
> > > diff --git a/drivers/remoteproc/qcom_common.h
> > > b/drivers/remoteproc/qcom_common.h
> > > index 541586e528b3..73efed969bfd 100644
> > > --- a/drivers/remoteproc/qcom_common.h
> > > +++ b/drivers/remoteproc/qcom_common.h
> > > @@ -30,10 +30,6 @@ struct qcom_rproc_ssr {
> > >  	const char *name;
> > >  };
> > >
> > > -struct resource_table *qcom_mdt_find_rsc_table(struct rproc *rproc,
> > > -					       const struct firmware *fw,
> > > -					       int *tablesz);
> > > -
> > >  void qcom_add_glink_subdev(struct rproc *rproc, struct
> qcom_rproc_glink
> > > *glink);
> > >  void qcom_remove_glink_subdev(struct rproc *rproc, struct
> > > qcom_rproc_glink *glink);
> > >
> > > diff --git a/drivers/remoteproc/qcom_q6v5_pil.c
> > > b/drivers/remoteproc/qcom_q6v5_pil.c
> > > index fbff5d842581..6f6ea0414366 100644
> > > --- a/drivers/remoteproc/qcom_q6v5_pil.c
> > > +++ b/drivers/remoteproc/qcom_q6v5_pil.c
> > > @@ -304,16 +304,6 @@ static void q6v5_clk_disable(struct device *dev,
> > >  		clk_disable_unprepare(clks[i]);
> > >  }
> > >
> > > -static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc,
> > > -						  const struct firmware *fw,
> > > -						  int *tablesz)
> > > -{
> > > -	static struct resource_table table = { .ver = 1, };
> > > -
> > > -	*tablesz = sizeof(table);
> > > -	return &table;
> > > -}
> > > -
> > >  static int q6v5_xfer_mem_ownership(struct q6v5 *qproc, int
> > > *current_perm,
> > >  				   bool remote_owner, phys_addr_t addr,
> > >  				   size_t size)
> > > @@ -927,7 +917,6 @@ static const struct rproc_ops q6v5_ops = {
> > >  	.start = q6v5_start,
> > >  	.stop = q6v5_stop,
> > >  	.da_to_va = q6v5_da_to_va,
> > > -	.find_rsc_table = q6v5_find_rsc_table,
> > >  	.load = q6v5_load,
> > >  };
> > >
> > > diff --git a/drivers/remoteproc/qcom_wcnss.c
> > > b/drivers/remoteproc/qcom_wcnss.c
> > > index cc44ec598522..1fa5253020dd 100644
> > > --- a/drivers/remoteproc/qcom_wcnss.c
> > > +++ b/drivers/remoteproc/qcom_wcnss.c
> > > @@ -310,7 +310,6 @@ static const struct rproc_ops wcnss_ops = {
> > >  	.start = wcnss_start,
> > >  	.stop = wcnss_stop,
> > >  	.da_to_va = wcnss_da_to_va,
> > > -	.find_rsc_table = qcom_mdt_find_rsc_table,
> > >  	.load = wcnss_load,
> > >  };
> > >
> > > diff --git a/drivers/remoteproc/st_slim_rproc.c
> > > b/drivers/remoteproc/st_slim_rproc.c
> > > index 1538ea915c49..c6a2a8b68c7a 100644
> > > --- a/drivers/remoteproc/st_slim_rproc.c
> > > +++ b/drivers/remoteproc/st_slim_rproc.c
> > > @@ -200,28 +200,10 @@ static void *slim_rproc_da_to_va(struct rproc
> > > *rproc, u64 da, int len)
> > >  	return va;
> > >  }
> > >
> > > -/*
> > > - * Firmware handler operations: sanity, boot address, load ...
> > > - */
> > > -
> > > -static struct resource_table empty_rsc_tbl = {
> > > -	.ver = 1,
> > > -	.num = 0,
> > > -};
> > > -
> > > -static struct resource_table *slim_rproc_find_rsc_table(struct rproc
> *rproc,
> > > -					       const struct firmware *fw,
> > > -					       int *tablesz)
> > > -{
> > > -	*tablesz = sizeof(empty_rsc_tbl);
> > > -	return &empty_rsc_tbl;
> > > -}
> > > -
> > >  static const struct rproc_ops slim_rproc_ops = {
> > >  	.start		= slim_rproc_start,
> > >  	.stop		= slim_rproc_stop,
> > >  	.da_to_va       = slim_rproc_da_to_va,
> > > -	.find_rsc_table = slim_rproc_find_rsc_table,
> > Hi Bjorn,
> > Your patch is not complete for st_slim_rproc and so not working.
> > In your patch 6/8, .load_rsc_table is define to default
> rproc_elf_load_rsc_table if no load ops defined.
> >
> > 	/* Default to ELF loader if no load function is specified */
> >  	if (!rproc->ops->load) {
> >  		rproc->ops->load = rproc_elf_load_segments;
> > -		rproc->ops->find_rsc_table = rproc_elf_find_rsc_table;
> > +		rproc->ops->load_rsc_table = rproc_elf_load_rsc_table;
> >
> > As st_slim_rproc has no load ops, it will inherit from all default ops including
> rproc_elf_load_rsc_table.
> > As no resource table present in firmware, an error will be returned and
> st_slim_rproc will failed.
> 
> Thanks for catching my mistake, Loic! The expected outcome would be that
> the slim_rproc_ops does point the "load" to the now exported ELF loader
> symbol and by that won't get a find_rsc_table entry from the default
> set..
> 
> > In case of Qualcom, load ops is defined...
> 
> Right, in the end the st_slim driver should have been defined just as
> the Qualcomm one - but referencing rproc_elf_load_segments().
> 
> > See below B2260 log (with additional error message in
> rproc_load_rsc_table function.
> >
> > [   10.201079] remoteproc remoteproc2: st231-delta is available
> > [   10.258121] remoteproc remoteproc2: powering up st231-delta
> > [   10.258143] remoteproc remoteproc2: Booting fw image rproc-st231-
> delta-fw, size 44416
> > [   10.258151] rproc_load_rsc_table: error -22
> >
> > Moreover with your proposal, as the choice to support or not a
> > resource table is based on the fact rproc->ops->load_rsc_table is set
> > or not, it is not possible with one unique driver to support firmware
> > with resource table and firmware without resource table.
> 
> This is retaining the previous behaviour of failing to load/start a
> remoteproc if no resource table is found, when the driver expects one.
> The new scheme would allow you to specify a custom load_rsc_table that
> calls rproc_elf_load_rsc_table() and ignores the return value to support
> your use case.
OK would be nice to document somewhere what's framework responsibilities and what's driver ones.

> 
> > Will be better from my pov to consider that no resource table  found
> > in rproc_elf_load_rsc_table function as a normal case and not an
> > error.
> 
> This would be a change in behavior and I can see how this could be
> annoying to people (by not catching their mistakes during development).
You're right

> 
> If you think this is the preferred implementation then please submit a
> separate patch for this so we can get some feedback from other users.
No it is OK. I'll review v3.

Regards,
Loic
> 
> Regards,
> Bjorn

[PATCH 8/8] remoteproc: Reset table_ptr on stop

[Bjorn Andersson]

The installed resource table is no longer accessible after stopping the
remote, so update table_ptr to point to the local copy.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/remoteproc/remoteproc_core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index cbd12382b219..98c088d80b3d 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1000,6 +1000,9 @@ static int rproc_stop(struct rproc *rproc, bool graceful)
 	/* remove any subdevices for the remote processor */
 	rproc_remove_subdevices(rproc, graceful);
 
+	/* the installed resource table is no longer accessible */
+	rproc->table_ptr = rproc->cached_table;
+
 	/* power off the remote processor */
 	ret = rproc->ops->stop(rproc);
 	if (ret) {
-- 
2.15.0