[PATCH v2 00/19] remoteproc: Allow platform-specific drivers to request resources

[PATCH v2 00/19] remoteproc: Allow platform-specific drivers to request resources

[Loic Pallardy]

Once this patch-set has been applied; platform-specific remoteproc
drivers will be able to amend existing resource table entries,
provide new entries to be appended to an existing resource table
(if one already exists), start a new resource table (if one does
not already exist), and dump out resource table contents during
development/debugging.  It will also be possible to start remote
processors which do not require a resource table without being
forced to provide a fake one.

We're also taking the liberty to provide some simple fix-ups.

V2 is rebased on "auto-boot" series. It also includes following addons:
- associate an action to the resource request. This allows platform-specific
  driver to verify firmware resources, update or append resources in firmware
  resource table or create a local resource table.
- introduce spare resource to define room for resource table extension
- centralize resource table sanity checks.

Lee Jones (6):
  remoteproc: core: New API to add new resources to the resource table
  remoteproc: core: Add function to dump resource table
  remoteproc: core: Add function to amend an existing resource table
    entry
  remoteproc: core: Add function to append a new resource table entry
  remoteproc: core: Add function to over-ride current resource table
  remoteproc: core: Support empty resource tables

Loic Pallardy (13):
  remoteproc: core: Add explicit message error if cached table failed
  remoteproc: Add new resource type for resource table spare bytes
  remoteproc: core: Associate action to resource request
  remoteproc: core: Finalize dump resource table function
  remoteproc: core: Add function to verify an existing resource in rsc
    table
  remoteproc: core: Add function to get resource table spare bytes
    information
  remoteproc: core: Add vdev support and force mode to resource amending
    function
  remoteproc: core: Append resource only if spare resource present
  remoteproc: core: Add resource request action support
  remoteproc: core: Add function to verify resource table consistency
  remoteproc: core: Clean-up resource table sanity checks
  remotecore: core: Add resource table pointer argument to
    rproc_handle_resource
  remoteproc: core: Add function to create remoteproc local resource
    table

 drivers/remoteproc/remoteproc_core.c | 683 ++++++++++++++++++++++++++++++++---
 include/linux/remoteproc.h           |  59 ++-
 2 files changed, 691 insertions(+), 51 deletions(-)

-- 
1.9.1

[PATCH v2 00/19] remoteproc: Allow platform-specific drivers to request resources

[Loic Pallardy]

Once this patch-set has been applied; platform-specific remoteproc
drivers will be able to amend existing resource table entries,
provide new entries to be appended to an existing resource table
(if one already exists), start a new resource table (if one does
not already exist), and dump out resource table contents during
development/debugging.  It will also be possible to start remote
processors which do not require a resource table without being
forced to provide a fake one.

We're also taking the liberty to provide some simple fix-ups.

V2 is rebased on "auto-boot" series. It also includes following addons:
- associate an action to the resource request. This allows platform-specific
  driver to verify firmware resources, update or append resources in firmware
  resource table or create a local resource table.
- introduce spare resource to define room for resource table extension
- centralize resource table sanity checks.

Lee Jones (6):
  remoteproc: core: New API to add new resources to the resource table
  remoteproc: core: Add function to dump resource table
  remoteproc: core: Add function to amend an existing resource table
    entry
  remoteproc: core: Add function to append a new resource table entry
  remoteproc: core: Add function to over-ride current resource table
  remoteproc: core: Support empty resource tables

Loic Pallardy (13):
  remoteproc: core: Add explicit message error if cached table failed
  remoteproc: Add new resource type for resource table spare bytes
  remoteproc: core: Associate action to resource request
  remoteproc: core: Finalize dump resource table function
  remoteproc: core: Add function to verify an existing resource in rsc
    table
  remoteproc: core: Add function to get resource table spare bytes
    information
  remoteproc: core: Add vdev support and force mode to resource amending
    function
  remoteproc: core: Append resource only if spare resource present
  remoteproc: core: Add resource request action support
  remoteproc: core: Add function to verify resource table consistency
  remoteproc: core: Clean-up resource table sanity checks
  remotecore: core: Add resource table pointer argument to
    rproc_handle_resource
  remoteproc: core: Add function to create remoteproc local resource
    table

 drivers/remoteproc/remoteproc_core.c | 683 ++++++++++++++++++++++++++++++++---
 include/linux/remoteproc.h           |  59 ++-
 2 files changed, 691 insertions(+), 51 deletions(-)

-- 
1.9.1

[PATCH v2 01/19] remoteproc: core: New API to add new resources to the resource table

[Loic Pallardy]

From: Lee Jones <lee.jones@linaro.org>

In order to amend or add a new resource table entry we need a method
for a platform-specific to submit them. rproc_request_resource() is a
new public API which provides this functionality.

It is to be called between rproc_alloc() and rproc_add().

Signed-off-by: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 40 ++++++++++++++++++++++++++++++++++++
 include/linux/remoteproc.h           | 21 +++++++++++++++++++
 2 files changed, 61 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 18f4286..fb2d2a0 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -791,6 +791,45 @@ static void rproc_resource_cleanup(struct rproc *rproc)
 		rproc_remove_virtio_dev(rvdev);
 }
 
+int rproc_request_resource(struct rproc *rproc, u32 type, void *resource)
+{
+	struct device *dev = &rproc->dev;
+	struct rproc_request_resource *request;
+	int size;
+
+	request = devm_kzalloc(dev, sizeof(*request), GFP_KERNEL);
+	if (!request)
+		return -ENOMEM;
+
+	switch (type) {
+	case RSC_CARVEOUT:
+		size = sizeof(struct fw_rsc_carveout);
+		break;
+	case RSC_DEVMEM:
+		size = sizeof(struct fw_rsc_devmem);
+		break;
+	case RSC_TRACE:
+		size = sizeof(struct fw_rsc_trace);
+		break;
+	default:
+		dev_err(dev, "Unsupported resource type: %d\n", type);
+		return -EINVAL;
+	}
+
+	request->resource = devm_kzalloc(dev, size, GFP_KERNEL);
+	if (!request->resource)
+		return -ENOMEM;
+
+	memcpy(request->resource, resource, size);
+	request->type = type;
+	request->size = size;
+
+	list_add_tail(&request->node, &rproc->override_resources);
+
+	return 0;
+}
+EXPORT_SYMBOL(rproc_request_resource);
+
 /*
  * take a firmware and boot a remote processor with it.
  */
@@ -1387,6 +1426,7 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
 	INIT_LIST_HEAD(&rproc->mappings);
 	INIT_LIST_HEAD(&rproc->traces);
 	INIT_LIST_HEAD(&rproc->rvdevs);
+	INIT_LIST_HEAD(&rproc->override_resources);
 
 	INIT_WORK(&rproc->crash_handler, rproc_crash_handler_work);
 	init_completion(&rproc->crash_comp);
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index d488f9e..d0c0793 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -323,6 +323,25 @@ struct rproc_mem_entry {
 	struct list_head node;
 };
 
+/**
+ * struct rproc_requested_resources - add a resource to the resource table
+ *
+ * @resource:	pointer to a 'struct fw_rsc_*' resource
+ * @type:	'fw_resource_type' resource type
+ * @size:	size of resource
+ * @node:	list node
+ *
+ * Resources can be added by platform-specific rproc drivers calling
+ * rproc_request_resource()
+ *
+ */
+struct rproc_request_resource {
+	void *resource;
+	u32 type;
+	u32 size;
+	struct list_head node;
+};
+
 struct rproc;
 
 /**
@@ -428,6 +447,7 @@ struct rproc {
 	int num_traces;
 	struct list_head carveouts;
 	struct list_head mappings;
+	struct list_head override_resources;
 	struct completion firmware_loading_complete;
 	u32 bootaddr;
 	struct list_head rvdevs;
@@ -486,6 +506,7 @@ struct rproc_vdev {
 	u32 rsc_offset;
 };
 
+int rproc_request_resource(struct rproc *rproc, u32 type, void *res);
 struct rproc *rproc_get_by_phandle(phandle phandle);
 struct rproc *rproc_alloc(struct device *dev, const char *name,
 			  const struct rproc_ops *ops,
-- 
1.9.1

[PATCH v2 01/19] remoteproc: core: New API to add new resources to the resource table

[Loic Pallardy]

From: Lee Jones <lee.jones@linaro.org>

In order to amend or add a new resource table entry we need a method
for a platform-specific to submit them. rproc_request_resource() is a
new public API which provides this functionality.

It is to be called between rproc_alloc() and rproc_add().

Signed-off-by: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 40 ++++++++++++++++++++++++++++++++++++
 include/linux/remoteproc.h           | 21 +++++++++++++++++++
 2 files changed, 61 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 18f4286..fb2d2a0 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -791,6 +791,45 @@ static void rproc_resource_cleanup(struct rproc *rproc)
 		rproc_remove_virtio_dev(rvdev);
 }
 
+int rproc_request_resource(struct rproc *rproc, u32 type, void *resource)
+{
+	struct device *dev = &rproc->dev;
+	struct rproc_request_resource *request;
+	int size;
+
+	request = devm_kzalloc(dev, sizeof(*request), GFP_KERNEL);
+	if (!request)
+		return -ENOMEM;
+
+	switch (type) {
+	case RSC_CARVEOUT:
+		size = sizeof(struct fw_rsc_carveout);
+		break;
+	case RSC_DEVMEM:
+		size = sizeof(struct fw_rsc_devmem);
+		break;
+	case RSC_TRACE:
+		size = sizeof(struct fw_rsc_trace);
+		break;
+	default:
+		dev_err(dev, "Unsupported resource type: %d\n", type);
+		return -EINVAL;
+	}
+
+	request->resource = devm_kzalloc(dev, size, GFP_KERNEL);
+	if (!request->resource)
+		return -ENOMEM;
+
+	memcpy(request->resource, resource, size);
+	request->type = type;
+	request->size = size;
+
+	list_add_tail(&request->node, &rproc->override_resources);
+
+	return 0;
+}
+EXPORT_SYMBOL(rproc_request_resource);
+
 /*
  * take a firmware and boot a remote processor with it.
  */
@@ -1387,6 +1426,7 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
 	INIT_LIST_HEAD(&rproc->mappings);
 	INIT_LIST_HEAD(&rproc->traces);
 	INIT_LIST_HEAD(&rproc->rvdevs);
+	INIT_LIST_HEAD(&rproc->override_resources);
 
 	INIT_WORK(&rproc->crash_handler, rproc_crash_handler_work);
 	init_completion(&rproc->crash_comp);
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index d488f9e..d0c0793 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -323,6 +323,25 @@ struct rproc_mem_entry {
 	struct list_head node;
 };
 
+/**
+ * struct rproc_requested_resources - add a resource to the resource table
+ *
+ * @resource:	pointer to a 'struct fw_rsc_*' resource
+ * @type:	'fw_resource_type' resource type
+ * @size:	size of resource
+ * @node:	list node
+ *
+ * Resources can be added by platform-specific rproc drivers calling
+ * rproc_request_resource()
+ *
+ */
+struct rproc_request_resource {
+	void *resource;
+	u32 type;
+	u32 size;
+	struct list_head node;
+};
+
 struct rproc;
 
 /**
@@ -428,6 +447,7 @@ struct rproc {
 	int num_traces;
 	struct list_head carveouts;
 	struct list_head mappings;
+	struct list_head override_resources;
 	struct completion firmware_loading_complete;
 	u32 bootaddr;
 	struct list_head rvdevs;
@@ -486,6 +506,7 @@ struct rproc_vdev {
 	u32 rsc_offset;
 };
 
+int rproc_request_resource(struct rproc *rproc, u32 type, void *res);
 struct rproc *rproc_get_by_phandle(phandle phandle);
 struct rproc *rproc_alloc(struct device *dev, const char *name,
 			  const struct rproc_ops *ops,
-- 
1.9.1

[PATCH v2 02/19] remoteproc: core: Add function to dump resource table

[Loic Pallardy]

From: Lee Jones <lee.jones@linaro.org>

Firmware can be loaded with a resource table, which details
resources needed by coprocessor like carevout memory, virtual
device, trace log buffer etc.

Until now, no method exists to display resource table content.
This function adds the capability to display the different
resources associated to a firmware if DEBUG is enabled.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 85 ++++++++++++++++++++++++++++++++++++
 1 file changed, 85 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index fb2d2a0..6c48c11 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -791,6 +791,91 @@ static void rproc_resource_cleanup(struct rproc *rproc)
 		rproc_remove_virtio_dev(rvdev);
 }
 
+static void rproc_dump_resource_table(struct rproc *rproc,
+				      struct resource_table *table, int size)
+{
+	const char *types[] = {"carveout", "devmem", "trace", "vdev"};
+	struct device *dev = &rproc->dev;
+	struct fw_rsc_carveout *c;
+	struct fw_rsc_devmem *d;
+	struct fw_rsc_trace *t;
+	struct fw_rsc_vdev *v;
+	int i, j;
+
+	if (!table) {
+		dev_dbg(dev, "No resource table found\n");
+		return;
+	}
+
+	dev_dbg(dev, "Resource Table: Version %d with %d entries [size: %x]\n",
+		table->ver, table->num, size);
+
+	for (i = 0; i < table->num; i++) {
+		int offset = table->offset[i];
+		struct fw_rsc_hdr *hdr = (void *)table + offset;
+		void *rsc = (void *)hdr + sizeof(*hdr);
+
+		switch (hdr->type) {
+		case RSC_CARVEOUT:
+			c = rsc;
+			dev_dbg(dev, "Entry %d is of type %s\n", i, types[hdr->type]);
+			dev_dbg(dev, "  Device Address 0x%x\n", c->da);
+			dev_dbg(dev, "  Physical Address 0x%x\n", c->pa);
+			dev_dbg(dev, "  Length 0x%x Bytes\n", c->len);
+			dev_dbg(dev, "  Flags 0x%x\n", c->flags);
+			dev_dbg(dev, "  Reserved (should be zero) [%d]\n", c->reserved);
+			dev_dbg(dev, "  Name %s\n\n", c->name);
+			break;
+		case RSC_DEVMEM:
+			d = rsc;
+			dev_dbg(dev, "Entry %d is of type %s\n", i, types[hdr->type]);
+			dev_dbg(dev, "  Device Address 0x%x\n", d->da);
+			dev_dbg(dev, "  Physical Address 0x%x\n", d->pa);
+			dev_dbg(dev, "  Length 0x%x Bytes\n", d->len);
+			dev_dbg(dev, "  Flags 0x%x\n", d->flags);
+			dev_dbg(dev, "  Reserved (should be zero) [%d]\n", d->reserved);
+			dev_dbg(dev, "  Name %s\n\n", d->name);
+			break;
+		case RSC_TRACE:
+			t = rsc;
+			dev_dbg(dev, "Entry %d is of type %s\n", i, types[hdr->type]);
+			dev_dbg(dev, "  Device Address 0x%x\n", t->da);
+			dev_dbg(dev, "  Length 0x%x Bytes\n", t->len);
+			dev_dbg(dev, "  Reserved (should be zero) [%d]\n", t->reserved);
+			dev_dbg(dev, "  Name %s\n\n", t->name);
+			break;
+		case RSC_VDEV:
+			v = rsc;
+			dev_dbg(dev, "Entry %d is of type %s\n", i, types[hdr->type]);
+
+			dev_dbg(dev, "  ID %d\n", v->id);
+			dev_dbg(dev, "  Notify ID %d\n", v->notifyid);
+			dev_dbg(dev, "  Device features 0x%x\n", v->dfeatures);
+			dev_dbg(dev, "  Guest features 0x%x\n", v->gfeatures);
+			dev_dbg(dev, "  Config length 0x%x\n", v->config_len);
+			dev_dbg(dev, "  Status 0x%x\n", v->status);
+			dev_dbg(dev, "  Number of vrings %d\n", v->num_of_vrings);
+			dev_dbg(dev, "  Reserved (should be zero) [%d][%d]\n\n",
+				v->reserved[0], v->reserved[1]);
+
+			for (j = 0; j < v->num_of_vrings; j++) {
+				dev_dbg(dev, "  Vring %d\n", j);
+				dev_dbg(dev, "    Device Address 0x%x\n", v->vring[j].da);
+				dev_dbg(dev, "    Alignment %d\n", v->vring[j].align);
+				dev_dbg(dev, "    Number of buffers %d\n", v->vring[j].num);
+				dev_dbg(dev, "    Notify ID %d\n", v->vring[j].notifyid);
+				dev_dbg(dev, "    Reserved (should be zero) [%d]\n\n",
+					v->vring[j].reserved);
+			}
+			break;
+		default:
+			dev_dbg(dev, "Invalid resource type found: %d [hdr: %p]\n",
+				hdr->type, hdr);
+			return;
+		}
+	}
+}
+
 int rproc_request_resource(struct rproc *rproc, u32 type, void *resource)
 {
 	struct device *dev = &rproc->dev;
-- 
1.9.1

[PATCH v2 02/19] remoteproc: core: Add function to dump resource table

[Loic Pallardy]

From: Lee Jones <lee.jones@linaro.org>

Firmware can be loaded with a resource table, which details
resources needed by coprocessor like carevout memory, virtual
device, trace log buffer etc.

Until now, no method exists to display resource table content.
This function adds the capability to display the different
resources associated to a firmware if DEBUG is enabled.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 85 ++++++++++++++++++++++++++++++++++++
 1 file changed, 85 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index fb2d2a0..6c48c11 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -791,6 +791,91 @@ static void rproc_resource_cleanup(struct rproc *rproc)
 		rproc_remove_virtio_dev(rvdev);
 }
 
+static void rproc_dump_resource_table(struct rproc *rproc,
+				      struct resource_table *table, int size)
+{
+	const char *types[] = {"carveout", "devmem", "trace", "vdev"};
+	struct device *dev = &rproc->dev;
+	struct fw_rsc_carveout *c;
+	struct fw_rsc_devmem *d;
+	struct fw_rsc_trace *t;
+	struct fw_rsc_vdev *v;
+	int i, j;
+
+	if (!table) {
+		dev_dbg(dev, "No resource table found\n");
+		return;
+	}
+
+	dev_dbg(dev, "Resource Table: Version %d with %d entries [size: %x]\n",
+		table->ver, table->num, size);
+
+	for (i = 0; i < table->num; i++) {
+		int offset = table->offset[i];
+		struct fw_rsc_hdr *hdr = (void *)table + offset;
+		void *rsc = (void *)hdr + sizeof(*hdr);
+
+		switch (hdr->type) {
+		case RSC_CARVEOUT:
+			c = rsc;
+			dev_dbg(dev, "Entry %d is of type %s\n", i, types[hdr->type]);
+			dev_dbg(dev, "  Device Address 0x%x\n", c->da);
+			dev_dbg(dev, "  Physical Address 0x%x\n", c->pa);
+			dev_dbg(dev, "  Length 0x%x Bytes\n", c->len);
+			dev_dbg(dev, "  Flags 0x%x\n", c->flags);
+			dev_dbg(dev, "  Reserved (should be zero) [%d]\n", c->reserved);
+			dev_dbg(dev, "  Name %s\n\n", c->name);
+			break;
+		case RSC_DEVMEM:
+			d = rsc;
+			dev_dbg(dev, "Entry %d is of type %s\n", i, types[hdr->type]);
+			dev_dbg(dev, "  Device Address 0x%x\n", d->da);
+			dev_dbg(dev, "  Physical Address 0x%x\n", d->pa);
+			dev_dbg(dev, "  Length 0x%x Bytes\n", d->len);
+			dev_dbg(dev, "  Flags 0x%x\n", d->flags);
+			dev_dbg(dev, "  Reserved (should be zero) [%d]\n", d->reserved);
+			dev_dbg(dev, "  Name %s\n\n", d->name);
+			break;
+		case RSC_TRACE:
+			t = rsc;
+			dev_dbg(dev, "Entry %d is of type %s\n", i, types[hdr->type]);
+			dev_dbg(dev, "  Device Address 0x%x\n", t->da);
+			dev_dbg(dev, "  Length 0x%x Bytes\n", t->len);
+			dev_dbg(dev, "  Reserved (should be zero) [%d]\n", t->reserved);
+			dev_dbg(dev, "  Name %s\n\n", t->name);
+			break;
+		case RSC_VDEV:
+			v = rsc;
+			dev_dbg(dev, "Entry %d is of type %s\n", i, types[hdr->type]);
+
+			dev_dbg(dev, "  ID %d\n", v->id);
+			dev_dbg(dev, "  Notify ID %d\n", v->notifyid);
+			dev_dbg(dev, "  Device features 0x%x\n", v->dfeatures);
+			dev_dbg(dev, "  Guest features 0x%x\n", v->gfeatures);
+			dev_dbg(dev, "  Config length 0x%x\n", v->config_len);
+			dev_dbg(dev, "  Status 0x%x\n", v->status);
+			dev_dbg(dev, "  Number of vrings %d\n", v->num_of_vrings);
+			dev_dbg(dev, "  Reserved (should be zero) [%d][%d]\n\n",
+				v->reserved[0], v->reserved[1]);
+
+			for (j = 0; j < v->num_of_vrings; j++) {
+				dev_dbg(dev, "  Vring %d\n", j);
+				dev_dbg(dev, "    Device Address 0x%x\n", v->vring[j].da);
+				dev_dbg(dev, "    Alignment %d\n", v->vring[j].align);
+				dev_dbg(dev, "    Number of buffers %d\n", v->vring[j].num);
+				dev_dbg(dev, "    Notify ID %d\n", v->vring[j].notifyid);
+				dev_dbg(dev, "    Reserved (should be zero) [%d]\n\n",
+					v->vring[j].reserved);
+			}
+			break;
+		default:
+			dev_dbg(dev, "Invalid resource type found: %d [hdr: %p]\n",
+				hdr->type, hdr);
+			return;
+		}
+	}
+}
+
 int rproc_request_resource(struct rproc *rproc, u32 type, void *resource)
 {
 	struct device *dev = &rproc->dev;
-- 
1.9.1

[PATCH v2 03/19] remoteproc: core: Add function to amend an existing resource table entry

[Loic Pallardy]

From: Lee Jones <lee.jones@linaro.org>

Sometimes the firmware does not know best.

When a firmware is built, it can be loaded with a resource table, usually
detailing shared; memory, virtual device, trace log information etc.
However, some vendors require this hard-coded information to be amended
with new/improved information obtained from Device Tree for instance.

Until now, no method exists which allows the resource table to be amended.
The addition of this function changes that.  It is now possible to pull in
a resource table and amend it before it is finally shared with the remote
device.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 63 ++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 6c48c11..330ad4f 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -915,6 +915,69 @@ int rproc_request_resource(struct rproc *rproc, u32 type, void *resource)
 }
 EXPORT_SYMBOL(rproc_request_resource);
 
+static int rproc_update_resource_table_entry(struct rproc *rproc,
+				struct rproc_request_resource *request,
+				struct resource_table *table, int size)
+{
+	struct fw_rsc_carveout *tblc, *newc;
+	struct fw_rsc_devmem *tbld, *newd;
+	struct fw_rsc_trace *tblt, *newt;
+	int updated = true;
+	int i;
+
+	for (i = 0; i < table->num; i++) {
+		int offset = table->offset[i];
+		struct fw_rsc_hdr *hdr = (void *)table + offset;
+		void *rsc = (void *)hdr + sizeof(*hdr);
+
+		if (request->type != hdr->type)
+			continue;
+
+		switch (hdr->type) {
+		case RSC_CARVEOUT:
+			tblc = rsc;
+			newc = request->resource;
+
+			if (strncmp(newc->name, tblc->name,
+				    sizeof(*tblc->name)))
+				break;
+
+			memcpy(tblc, newc, request->size);
+
+			return updated;
+		case RSC_DEVMEM:
+			tbld = rsc;
+			newd = request->resource;
+
+			if (strncmp(newd->name, tbld->name,
+				    sizeof(*tbld->name)))
+				break;
+
+			memcpy(tbld, newd, request->size);
+
+			return updated;
+		case RSC_TRACE:
+			tblt = rsc;
+			newt = request->resource;
+
+			if (strncmp(newt->name, tblt->name,
+				    sizeof(*tblt->name)))
+				break;
+
+			memcpy(tblt, newt, request->size);
+
+			return updated;
+		default:
+			dev_err(&rproc->dev,
+				"Unsupported resource type: %d\n",
+				hdr->type);
+			return -EINVAL;
+		}
+	}
+
+	return !updated;
+}
+
 /*
  * take a firmware and boot a remote processor with it.
  */
-- 
1.9.1

[PATCH v2 03/19] remoteproc: core: Add function to amend an existing resource table entry

[Loic Pallardy]

From: Lee Jones <lee.jones@linaro.org>

Sometimes the firmware does not know best.

When a firmware is built, it can be loaded with a resource table, usually
detailing shared; memory, virtual device, trace log information etc.
However, some vendors require this hard-coded information to be amended
with new/improved information obtained from Device Tree for instance.

Until now, no method exists which allows the resource table to be amended.
The addition of this function changes that.  It is now possible to pull in
a resource table and amend it before it is finally shared with the remote
device.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 63 ++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 6c48c11..330ad4f 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -915,6 +915,69 @@ int rproc_request_resource(struct rproc *rproc, u32 type, void *resource)
 }
 EXPORT_SYMBOL(rproc_request_resource);
 
+static int rproc_update_resource_table_entry(struct rproc *rproc,
+				struct rproc_request_resource *request,
+				struct resource_table *table, int size)
+{
+	struct fw_rsc_carveout *tblc, *newc;
+	struct fw_rsc_devmem *tbld, *newd;
+	struct fw_rsc_trace *tblt, *newt;
+	int updated = true;
+	int i;
+
+	for (i = 0; i < table->num; i++) {
+		int offset = table->offset[i];
+		struct fw_rsc_hdr *hdr = (void *)table + offset;
+		void *rsc = (void *)hdr + sizeof(*hdr);
+
+		if (request->type != hdr->type)
+			continue;
+
+		switch (hdr->type) {
+		case RSC_CARVEOUT:
+			tblc = rsc;
+			newc = request->resource;
+
+			if (strncmp(newc->name, tblc->name,
+				    sizeof(*tblc->name)))
+				break;
+
+			memcpy(tblc, newc, request->size);
+
+			return updated;
+		case RSC_DEVMEM:
+			tbld = rsc;
+			newd = request->resource;
+
+			if (strncmp(newd->name, tbld->name,
+				    sizeof(*tbld->name)))
+				break;
+
+			memcpy(tbld, newd, request->size);
+
+			return updated;
+		case RSC_TRACE:
+			tblt = rsc;
+			newt = request->resource;
+
+			if (strncmp(newt->name, tblt->name,
+				    sizeof(*tblt->name)))
+				break;
+
+			memcpy(tblt, newt, request->size);
+
+			return updated;
+		default:
+			dev_err(&rproc->dev,
+				"Unsupported resource type: %d\n",
+				hdr->type);
+			return -EINVAL;
+		}
+	}
+
+	return !updated;
+}
+
 /*
  * take a firmware and boot a remote processor with it.
  */
-- 
1.9.1

[PATCH v2 04/19] remoteproc: core: Add function to append a new resource table entry

[Loic Pallardy]

From: Lee Jones <lee.jones@linaro.org>

A new function now exists to pull in and amend and existing resource
table entry.  But what if we wish to provide a new resource?  This
function provides functionality to append a brand new resource entry
onto the resource table.  All complexity related to shuffling parts
of the table around, providing new offsets and incriminating number
of entries in the resource table's top-level header is taken care of
here.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 55 ++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 330ad4f..c2887be 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -978,6 +978,61 @@ static int rproc_update_resource_table_entry(struct rproc *rproc,
 	return !updated;
 }
 
+static struct resource_table*
+rproc_add_resource_table_entry(struct rproc *rproc,
+			       struct rproc_request_resource *request,
+			       struct resource_table *old_table, int *tablesz)
+{
+	struct resource_table *table;
+	struct fw_rsc_hdr h;
+	void *new_rsc_loc;
+	void *fw_header_loc;
+	void *start_of_rscs;
+	int new_rsc_offset;
+	int size = *tablesz;
+	int i;
+
+	h.type = request->type;
+
+	new_rsc_offset = size;
+
+	/*
+	 * Allocate another contiguous chunk of memory, large enough to
+	 * contain the new, expanded resource table.
+	 *
+	 * The +4 is for the extra offset[] element in the top level header
+	 */
+	size += sizeof(struct fw_rsc_hdr) + request->size + 4;
+	table = devm_kmemdup(&rproc->dev, old_table, size, GFP_KERNEL);
+	if (!table)
+		return ERR_PTR(-ENOMEM);
+
+	/* Shunt table by 4 Bytes to account for the extra offset[] element */
+	start_of_rscs = (void *)table + table->offset[0];
+	memmove(start_of_rscs + 4,
+		start_of_rscs, new_rsc_offset - table->offset[0]);
+	new_rsc_offset += 4;
+
+	/* Update existing resource entry's offsets */
+	for (i = 0; i < table->num; i++)
+		table->offset[i] += 4;
+
+	/* Update the top level 'resource table' header */
+	table->offset[table->num] = new_rsc_offset;
+	table->num++;
+
+	/* Copy new firmware header into table */
+	fw_header_loc = (void *)table + new_rsc_offset;
+	memcpy(fw_header_loc, &h, sizeof(h));
+
+	/* Copy new resource entry into table */
+	new_rsc_loc = (void *)fw_header_loc + sizeof(h);
+	memcpy(new_rsc_loc, request->resource, request->size);
+
+	*tablesz = size;
+	return table;
+}
+
 /*
  * take a firmware and boot a remote processor with it.
  */
-- 
1.9.1

[PATCH v2 04/19] remoteproc: core: Add function to append a new resource table entry

[Loic Pallardy]

From: Lee Jones <lee.jones@linaro.org>

A new function now exists to pull in and amend and existing resource
table entry.  But what if we wish to provide a new resource?  This
function provides functionality to append a brand new resource entry
onto the resource table.  All complexity related to shuffling parts
of the table around, providing new offsets and incriminating number
of entries in the resource table's top-level header is taken care of
here.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 55 ++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 330ad4f..c2887be 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -978,6 +978,61 @@ static int rproc_update_resource_table_entry(struct rproc *rproc,
 	return !updated;
 }
 
+static struct resource_table*
+rproc_add_resource_table_entry(struct rproc *rproc,
+			       struct rproc_request_resource *request,
+			       struct resource_table *old_table, int *tablesz)
+{
+	struct resource_table *table;
+	struct fw_rsc_hdr h;
+	void *new_rsc_loc;
+	void *fw_header_loc;
+	void *start_of_rscs;
+	int new_rsc_offset;
+	int size = *tablesz;
+	int i;
+
+	h.type = request->type;
+
+	new_rsc_offset = size;
+
+	/*
+	 * Allocate another contiguous chunk of memory, large enough to
+	 * contain the new, expanded resource table.
+	 *
+	 * The +4 is for the extra offset[] element in the top level header
+	 */
+	size += sizeof(struct fw_rsc_hdr) + request->size + 4;
+	table = devm_kmemdup(&rproc->dev, old_table, size, GFP_KERNEL);
+	if (!table)
+		return ERR_PTR(-ENOMEM);
+
+	/* Shunt table by 4 Bytes to account for the extra offset[] element */
+	start_of_rscs = (void *)table + table->offset[0];
+	memmove(start_of_rscs + 4,
+		start_of_rscs, new_rsc_offset - table->offset[0]);
+	new_rsc_offset += 4;
+
+	/* Update existing resource entry's offsets */
+	for (i = 0; i < table->num; i++)
+		table->offset[i] += 4;
+
+	/* Update the top level 'resource table' header */
+	table->offset[table->num] = new_rsc_offset;
+	table->num++;
+
+	/* Copy new firmware header into table */
+	fw_header_loc = (void *)table + new_rsc_offset;
+	memcpy(fw_header_loc, &h, sizeof(h));
+
+	/* Copy new resource entry into table */
+	new_rsc_loc = (void *)fw_header_loc + sizeof(h);
+	memcpy(new_rsc_loc, request->resource, request->size);
+
+	*tablesz = size;
+	return table;
+}
+
 /*
  * take a firmware and boot a remote processor with it.
  */
-- 
1.9.1

[PATCH v2 05/19] remoteproc: core: Add function to over-ride current resource table

[Loic Pallardy]

From: Lee Jones <lee.jones@linaro.org>

Most of the new resource table handling function are now in place, so
it's time to put it all together.  Once new resource table information
has been requested, the structures will be held in a holding pen until
boot-time.  During boot-time rproc_apply_resource_overrides() will be
invoked which in turn will pull the new information out of the holding
pen and edit the table accordingly.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 63 ++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index c2887be..96bc8c644 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1033,6 +1033,63 @@ rproc_add_resource_table_entry(struct rproc *rproc,
 	return table;
 }
 
+static struct resource_table*
+rproc_apply_resource_overrides(struct rproc *rproc,
+			       struct resource_table **orig_table,
+			       int *tablesz)
+{
+	struct rproc_request_resource *resource;
+	struct resource_table *table = *orig_table;
+	int size = *tablesz;
+
+	if (!table && size != 0) {
+		dev_err(&rproc->dev, "No table present but table size is set\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+
+	rproc_dump_resource_table(rproc, table, size);
+
+	if (!table) {
+		size = sizeof(*table);
+		table = devm_kzalloc(&rproc->dev, size, GFP_KERNEL);
+		if (!table) {
+			table = ERR_PTR(-ENOMEM);
+			goto out;
+		}
+		table->ver = 1;
+	}
+
+	list_for_each_entry(resource, &rproc->override_resources, node) {
+		int updated = 0;
+
+		/* If we already have a table, update it with the new values. */
+		updated = rproc_update_resource_table_entry(rproc, resource,
+							    table, size);
+		if (updated < 0) {
+			table = ERR_PTR(updated);
+			goto out;
+		}
+		if (updated)
+			continue;
+
+		/* Didn't find matching resource entry -- creating a new one. */
+		table = rproc_add_resource_table_entry(rproc, resource,
+						       table, &size);
+		if (IS_ERR(table))
+			goto out;
+
+		*orig_table = table;
+	}
+
+	rproc_dump_resource_table(rproc, table, size);
+
+	*tablesz = size;
+
+ out:
+	return table;
+}
+
 /*
  * take a firmware and boot a remote processor with it.
  */
@@ -1069,6 +1126,12 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 		goto clean_up;
 	}
 
+	if (!list_empty(&rproc->override_resources)) {
+		table = rproc_apply_resource_overrides(rproc, &table, &tablesz);
+		if (IS_ERR(table))
+			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
-- 
1.9.1

[PATCH v2 05/19] remoteproc: core: Add function to over-ride current resource table

[Loic Pallardy]

From: Lee Jones <lee.jones@linaro.org>

Most of the new resource table handling function are now in place, so
it's time to put it all together.  Once new resource table information
has been requested, the structures will be held in a holding pen until
boot-time.  During boot-time rproc_apply_resource_overrides() will be
invoked which in turn will pull the new information out of the holding
pen and edit the table accordingly.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 63 ++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index c2887be..96bc8c644 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1033,6 +1033,63 @@ rproc_add_resource_table_entry(struct rproc *rproc,
 	return table;
 }
 
+static struct resource_table*
+rproc_apply_resource_overrides(struct rproc *rproc,
+			       struct resource_table **orig_table,
+			       int *tablesz)
+{
+	struct rproc_request_resource *resource;
+	struct resource_table *table = *orig_table;
+	int size = *tablesz;
+
+	if (!table && size != 0) {
+		dev_err(&rproc->dev, "No table present but table size is set\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+
+	rproc_dump_resource_table(rproc, table, size);
+
+	if (!table) {
+		size = sizeof(*table);
+		table = devm_kzalloc(&rproc->dev, size, GFP_KERNEL);
+		if (!table) {
+			table = ERR_PTR(-ENOMEM);
+			goto out;
+		}
+		table->ver = 1;
+	}
+
+	list_for_each_entry(resource, &rproc->override_resources, node) {
+		int updated = 0;
+
+		/* If we already have a table, update it with the new values. */
+		updated = rproc_update_resource_table_entry(rproc, resource,
+							    table, size);
+		if (updated < 0) {
+			table = ERR_PTR(updated);
+			goto out;
+		}
+		if (updated)
+			continue;
+
+		/* Didn't find matching resource entry -- creating a new one. */
+		table = rproc_add_resource_table_entry(rproc, resource,
+						       table, &size);
+		if (IS_ERR(table))
+			goto out;
+
+		*orig_table = table;
+	}
+
+	rproc_dump_resource_table(rproc, table, size);
+
+	*tablesz = size;
+
+ out:
+	return table;
+}
+
 /*
  * take a firmware and boot a remote processor with it.
  */
@@ -1069,6 +1126,12 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 		goto clean_up;
 	}
 
+	if (!list_empty(&rproc->override_resources)) {
+		table = rproc_apply_resource_overrides(rproc, &table, &tablesz);
+		if (IS_ERR(table))
+			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
-- 
1.9.1

[PATCH v2 06/19] remoteproc: core: Add explicit message error if cached table failed

[Loic Pallardy]

In rproc_fw_boot, add error message if kmemdump failed
as done for other errors.

Signed-off-by: Loic Pallardy <loic.pallardy@wanadoo.fr>
---
 drivers/remoteproc/remoteproc_core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 96bc8c644..3282a4e 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1139,8 +1139,10 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 	 * cached_table will be copied into device memory.
 	 */
 	rproc->cached_table = kmemdup(table, tablesz, GFP_KERNEL);
-	if (!rproc->cached_table)
+	if (!rproc->cached_table) {
+		dev_err(dev, "Failed to create cached table\n");
 		goto clean_up;
+	}
 
 	rproc->table_ptr = rproc->cached_table;
 
-- 
1.9.1

[PATCH v2 06/19] remoteproc: core: Add explicit message error if cached table failed

[Loic Pallardy]

In rproc_fw_boot, add error message if kmemdump failed
as done for other errors.

Signed-off-by: Loic Pallardy <loic.pallardy@wanadoo.fr>
---
 drivers/remoteproc/remoteproc_core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 96bc8c644..3282a4e 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1139,8 +1139,10 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 	 * cached_table will be copied into device memory.
 	 */
 	rproc->cached_table = kmemdup(table, tablesz, GFP_KERNEL);
-	if (!rproc->cached_table)
+	if (!rproc->cached_table) {
+		dev_err(dev, "Failed to create cached table\n");
 		goto clean_up;
+	}
 
 	rproc->table_ptr = rproc->cached_table;
 
-- 
1.9.1

Re: [PATCH v2 06/19] remoteproc: core: Add explicit message error if cached table failed

[Lee Jones]

On Wed, 31 Aug 2016, Loic Pallardy wrote:

> In rproc_fw_boot, add error message if kmemdump failed
> as done for other errors.
> 
> Signed-off-by: Loic Pallardy <loic.pallardy@wanadoo.fr>
> ---
>  drivers/remoteproc/remoteproc_core.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 96bc8c644..3282a4e 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1139,8 +1139,10 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>  	 * cached_table will be copied into device memory.
>  	 */
>  	rproc->cached_table = kmemdup(table, tablesz, GFP_KERNEL);
> -	if (!rproc->cached_table)
> +	if (!rproc->cached_table) {
> +		dev_err(dev, "Failed to create cached table\n");

This is an OOM error message, which are not allowed upstream.

I believe if kmemdup() fails, the OOM handler will inform the user.

>  		goto clean_up;
> +	}
>  
>  	rproc->table_ptr = rproc->cached_table;
>  

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 06/19] remoteproc: core: Add explicit message error if cached table failed

[loic pallardy]

On 09/01/2016 09:09 AM, Lee Jones wrote:
> On Wed, 31 Aug 2016, Loic Pallardy wrote:
>
>> In rproc_fw_boot, add error message if kmemdump failed
>> as done for other errors.
>>
>> Signed-off-by: Loic Pallardy <loic.pallardy@wanadoo.fr>
>> ---
>>  drivers/remoteproc/remoteproc_core.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>> index 96bc8c644..3282a4e 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -1139,8 +1139,10 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>>  	 * cached_table will be copied into device memory.
>>  	 */
>>  	rproc->cached_table = kmemdup(table, tablesz, GFP_KERNEL);
>> -	if (!rproc->cached_table)
>> +	if (!rproc->cached_table) {
>> +		dev_err(dev, "Failed to create cached table\n");
>
> This is an OOM error message, which are not allowed upstream.
>
> I believe if kmemdup() fails, the OOM handler will inform the user.

Yes true, I'll drop this patch in v2
Regards,
Loic
>
>>  		goto clean_up;
>> +	}
>>
>>  	rproc->table_ptr = rproc->cached_table;
>>
>

Re: [PATCH v2 06/19] remoteproc: core: Add explicit message error if cached table failed

[loic pallardy]

On 09/01/2016 09:09 AM, Lee Jones wrote:
> On Wed, 31 Aug 2016, Loic Pallardy wrote:
>
>> In rproc_fw_boot, add error message if kmemdump failed
>> as done for other errors.
>>
>> Signed-off-by: Loic Pallardy <loic.pallardy@wanadoo.fr>
>> ---
>>  drivers/remoteproc/remoteproc_core.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>> index 96bc8c644..3282a4e 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -1139,8 +1139,10 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>>  	 * cached_table will be copied into device memory.
>>  	 */
>>  	rproc->cached_table = kmemdup(table, tablesz, GFP_KERNEL);
>> -	if (!rproc->cached_table)
>> +	if (!rproc->cached_table) {
>> +		dev_err(dev, "Failed to create cached table\n");
>
> This is an OOM error message, which are not allowed upstream.
>
> I believe if kmemdup() fails, the OOM handler will inform the user.

Yes true, I'll drop this patch in v2
Regards,
Loic
>
>>  		goto clean_up;
>> +	}
>>
>>  	rproc->table_ptr = rproc->cached_table;
>>
>

[PATCH v2 07/19] remoteproc: Add new resource type for resource table spare bytes

[Loic Pallardy]

To allow resource appending to an existing resource table,
remoteproc framework should get information about resource
table spare area. With current resource table construction,
remoteproc is not able to identify by itself any free location.
This patch introduces a new resource type named RSC_SPARE which
allows firmware to define room for resource table extension.
Defined spare area will be used by remtoreproc to extend resource
table.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 include/linux/remoteproc.h | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index d0c0793..4e2f822 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -100,6 +100,7 @@ struct fw_rsc_hdr {
  *		    the remote processor will be writing logs.
  * @RSC_VDEV:       declare support for a virtio device, and serve as its
  *		    virtio header.
+ * @RSC_SPARE:      spare area in resource table for resource appending
  * @RSC_LAST:       just keep this one at the end
  *
  * For more details regarding a specific resource type, please see its
@@ -115,7 +116,8 @@ enum fw_resource_type {
 	RSC_DEVMEM	= 1,
 	RSC_TRACE	= 2,
 	RSC_VDEV	= 3,
-	RSC_LAST	= 4,
+	RSC_SPARE	= 4,
+	RSC_LAST	= 5,
 };
 
 #define FW_RSC_ADDR_ANY (0xFFFFFFFFFFFFFFFF)
@@ -306,6 +308,15 @@ struct fw_rsc_vdev {
 } __packed;
 
 /**
+ * struct fw_rsc_spare - resource table spare area definition
+ * @len: length of spare area in byte
+ */
+struct fw_rsc_spare {
+	u32 len;
+	u8 spare_bytes[0];
+} __packed;
+
+/**
  * struct rproc_mem_entry - memory entry descriptor
  * @va:	virtual address
  * @dma: dma address
-- 
1.9.1

[PATCH v2 07/19] remoteproc: Add new resource type for resource table spare bytes

[Loic Pallardy]

To allow resource appending to an existing resource table,
remoteproc framework should get information about resource
table spare area. With current resource table construction,
remoteproc is not able to identify by itself any free location.
This patch introduces a new resource type named RSC_SPARE which
allows firmware to define room for resource table extension.
Defined spare area will be used by remtoreproc to extend resource
table.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 include/linux/remoteproc.h | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index d0c0793..4e2f822 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -100,6 +100,7 @@ struct fw_rsc_hdr {
  *		    the remote processor will be writing logs.
  * @RSC_VDEV:       declare support for a virtio device, and serve as its
  *		    virtio header.
+ * @RSC_SPARE:      spare area in resource table for resource appending
  * @RSC_LAST:       just keep this one at the end
  *
  * For more details regarding a specific resource type, please see its
@@ -115,7 +116,8 @@ enum fw_resource_type {
 	RSC_DEVMEM	= 1,
 	RSC_TRACE	= 2,
 	RSC_VDEV	= 3,
-	RSC_LAST	= 4,
+	RSC_SPARE	= 4,
+	RSC_LAST	= 5,
 };
 
 #define FW_RSC_ADDR_ANY (0xFFFFFFFFFFFFFFFF)
@@ -306,6 +308,15 @@ struct fw_rsc_vdev {
 } __packed;
 
 /**
+ * struct fw_rsc_spare - resource table spare area definition
+ * @len: length of spare area in byte
+ */
+struct fw_rsc_spare {
+	u32 len;
+	u8 spare_bytes[0];
+} __packed;
+
+/**
  * struct rproc_mem_entry - memory entry descriptor
  * @va:	virtual address
  * @dma: dma address
-- 
1.9.1

Re: [PATCH v2 07/19] remoteproc: Add new resource type for resource table spare bytes

[Bjorn Andersson]

On Wed 31 Aug 13:50 PDT 2016, Loic Pallardy wrote:

> To allow resource appending to an existing resource table,
> remoteproc framework should get information about resource
> table spare area. With current resource table construction,
> remoteproc is not able to identify by itself any free location.
> This patch introduces a new resource type named RSC_SPARE which
> allows firmware to define room for resource table extension.
> Defined spare area will be used by remtoreproc to extend resource
> table.
> 

We don't need a dummy type for keeping track of the available room in
the resource table in the loaded firmware. All we need to do is to look
at the sh_size of the .resource_table section, which actually is what's
returned in tablesz.

So the spare size is the difference between tablesz and the end of the
last resource and if you need you can pad this when composing the
firmware.

Regards,
Bjorn

Re: [PATCH v2 07/19] remoteproc: Add new resource type for resource table spare bytes

[loic pallardy]

On 09/15/2016 07:54 PM, Bjorn Andersson wrote:
> On Wed 31 Aug 13:50 PDT 2016, Loic Pallardy wrote:
>
>> To allow resource appending to an existing resource table,
>> remoteproc framework should get information about resource
>> table spare area. With current resource table construction,
>> remoteproc is not able to identify by itself any free location.
>> This patch introduces a new resource type named RSC_SPARE which
>> allows firmware to define room for resource table extension.
>> Defined spare area will be used by remtoreproc to extend resource
>> table.
>>
>
> We don't need a dummy type for keeping track of the available room in
> the resource table in the loaded firmware. All we need to do is to look
> at the sh_size of the .resource_table section, which actually is what's
> returned in tablesz.
>
This is the size of the .resource_table section. Doesn't means that only 
resource table is stored in. Today this is the assumption and we force 
firmware to respect this.

> So the spare size is the difference between tablesz and the end of the
> last resource and if you need you can pad this when composing the
> firmware.
>
Proposal was to clearly identify the area for extension (whatever 
.resource_table section is done). But if you agree on the fact 
.resource_tabel section constains only resource table and eventualy room 
for extension, I can indeed simply room detection.

Regards,
Loic

> Regards,
> Bjorn
>

Re: [PATCH v2 07/19] remoteproc: Add new resource type for resource table spare bytes

[loic pallardy]

On 09/15/2016 07:54 PM, Bjorn Andersson wrote:
> On Wed 31 Aug 13:50 PDT 2016, Loic Pallardy wrote:
>
>> To allow resource appending to an existing resource table,
>> remoteproc framework should get information about resource
>> table spare area. With current resource table construction,
>> remoteproc is not able to identify by itself any free location.
>> This patch introduces a new resource type named RSC_SPARE which
>> allows firmware to define room for resource table extension.
>> Defined spare area will be used by remtoreproc to extend resource
>> table.
>>
>
> We don't need a dummy type for keeping track of the available room in
> the resource table in the loaded firmware. All we need to do is to look
> at the sh_size of the .resource_table section, which actually is what's
> returned in tablesz.
>
This is the size of the .resource_table section. Doesn't means that only 
resource table is stored in. Today this is the assumption and we force 
firmware to respect this.

> So the spare size is the difference between tablesz and the end of the
> last resource and if you need you can pad this when composing the
> firmware.
>
Proposal was to clearly identify the area for extension (whatever 
.resource_table section is done). But if you agree on the fact 
.resource_tabel section constains only resource table and eventualy room 
for extension, I can indeed simply room detection.

Regards,
Loic

> Regards,
> Bjorn
>

Re: [PATCH v2 07/19] remoteproc: Add new resource type for resource table spare bytes

[Bjorn Andersson]

On Fri 16 Sep 02:02 PDT 2016, loic pallardy wrote:

> 
> 
> On 09/15/2016 07:54 PM, Bjorn Andersson wrote:
> >On Wed 31 Aug 13:50 PDT 2016, Loic Pallardy wrote:
> >
> >>To allow resource appending to an existing resource table,
> >>remoteproc framework should get information about resource
> >>table spare area. With current resource table construction,
> >>remoteproc is not able to identify by itself any free location.
> >>This patch introduces a new resource type named RSC_SPARE which
> >>allows firmware to define room for resource table extension.
> >>Defined spare area will be used by remtoreproc to extend resource
> >>table.
> >>
> >
> >We don't need a dummy type for keeping track of the available room in
> >the resource table in the loaded firmware. All we need to do is to look
> >at the sh_size of the .resource_table section, which actually is what's
> >returned in tablesz.
> >
> This is the size of the .resource_table section. Doesn't means that only
> resource table is stored in.

I'm not sure I'm getting the details of what you're saying here. Do you
mean that there could be other things in the resource_table section or
just the fact that it being a section doesn't give any information about
how much space this thing will have in loaded form.

> Today this is the assumption and we force firmware to respect this.
> 

I find it unfortunate that this was put in section and that we just have
to make assumptions on how this projects onto the loaded form.

> >So the spare size is the difference between tablesz and the end of the
> >last resource and if you need you can pad this when composing the
> >firmware.
> >
> Proposal was to clearly identify the area for extension (whatever
> .resource_table section is done). But if you agree on the fact
> .resource_tabel section constains only resource table and eventualy room for
> extension, I can indeed simply room detection.
> 

Could you describe your use case for programmatically generate a
resource table for a firmware without a .resource_table? I would like to
understand the contract between the driver and the firmware when it
comes to what should go into the resource table.

Regards,
Bjorn

Re: [PATCH v2 07/19] remoteproc: Add new resource type for resource table spare bytes

[loic pallardy]

On 09/16/2016 07:12 PM, Bjorn Andersson wrote:
> On Fri 16 Sep 02:02 PDT 2016, loic pallardy wrote:
>
>>
>>
>> On 09/15/2016 07:54 PM, Bjorn Andersson wrote:
>>> On Wed 31 Aug 13:50 PDT 2016, Loic Pallardy wrote:
>>>
>>>> To allow resource appending to an existing resource table,
>>>> remoteproc framework should get information about resource
>>>> table spare area. With current resource table construction,
>>>> remoteproc is not able to identify by itself any free location.
>>>> This patch introduces a new resource type named RSC_SPARE which
>>>> allows firmware to define room for resource table extension.
>>>> Defined spare area will be used by remtoreproc to extend resource
>>>> table.
>>>>
>>>
>>> We don't need a dummy type for keeping track of the available room in
>>> the resource table in the loaded firmware. All we need to do is to look
>>> at the sh_size of the .resource_table section, which actually is what's
>>> returned in tablesz.
>>>
>> This is the size of the .resource_table section. Doesn't means that only
>> resource table is stored in.
>
> I'm not sure I'm getting the details of what you're saying here. Do you
> mean that there could be other things in the resource_table section or
> just the fact that it being a section doesn't give any information about
> how much space this thing will have in loaded form.
>
>> Today this is the assumption and we force firmware to respect this.
>>
>
> I find it unfortunate that this was put in section and that we just have
> to make assumptions on how this projects onto the loaded form.
>
>>> So the spare size is the difference between tablesz and the end of the
>>> last resource and if you need you can pad this when composing the
>>> firmware.
>>>
>> Proposal was to clearly identify the area for extension (whatever
>> .resource_table section is done). But if you agree on the fact
>> .resource_tabel section constains only resource table and eventualy room for
>> extension, I can indeed simply room detection.
>>
>
> Could you describe your use case for programmatically generate a
> resource table for a firmware without a .resource_table? I would like to
> understand the contract between the driver and the firmware when it
> comes to what should go into the resource table.

No I always consider .resource_table section.
You answer to my point just above. .resource_table section must contain 
only the resource table and nothing else. I'm fine with that.

I'll revert RSC_SPARE type.

Regards,
Loic
>
> Regards,
> Bjorn
>

Re: [PATCH v2 07/19] remoteproc: Add new resource type for resource table spare bytes

[loic pallardy]

On 09/16/2016 07:12 PM, Bjorn Andersson wrote:
> On Fri 16 Sep 02:02 PDT 2016, loic pallardy wrote:
>
>>
>>
>> On 09/15/2016 07:54 PM, Bjorn Andersson wrote:
>>> On Wed 31 Aug 13:50 PDT 2016, Loic Pallardy wrote:
>>>
>>>> To allow resource appending to an existing resource table,
>>>> remoteproc framework should get information about resource
>>>> table spare area. With current resource table construction,
>>>> remoteproc is not able to identify by itself any free location.
>>>> This patch introduces a new resource type named RSC_SPARE which
>>>> allows firmware to define room for resource table extension.
>>>> Defined spare area will be used by remtoreproc to extend resource
>>>> table.
>>>>
>>>
>>> We don't need a dummy type for keeping track of the available room in
>>> the resource table in the loaded firmware. All we need to do is to look
>>> at the sh_size of the .resource_table section, which actually is what's
>>> returned in tablesz.
>>>
>> This is the size of the .resource_table section. Doesn't means that only
>> resource table is stored in.
>
> I'm not sure I'm getting the details of what you're saying here. Do you
> mean that there could be other things in the resource_table section or
> just the fact that it being a section doesn't give any information about
> how much space this thing will have in loaded form.
>
>> Today this is the assumption and we force firmware to respect this.
>>
>
> I find it unfortunate that this was put in section and that we just have
> to make assumptions on how this projects onto the loaded form.
>
>>> So the spare size is the difference between tablesz and the end of the
>>> last resource and if you need you can pad this when composing the
>>> firmware.
>>>
>> Proposal was to clearly identify the area for extension (whatever
>> .resource_table section is done). But if you agree on the fact
>> .resource_tabel section constains only resource table and eventualy room for
>> extension, I can indeed simply room detection.
>>
>
> Could you describe your use case for programmatically generate a
> resource table for a firmware without a .resource_table? I would like to
> understand the contract between the driver and the firmware when it
> comes to what should go into the resource table.

No I always consider .resource_table section.
You answer to my point just above. .resource_table section must contain 
only the resource table and nothing else. I'm fine with that.

I'll revert RSC_SPARE type.

Regards,
Loic
>
> Regards,
> Bjorn
>

[PATCH v2 08/19] remoteproc: core: Associate action to resource request

[Loic Pallardy]

With new rproc_request_resource API, rproc driver has now the
capability to provide resources to remoteproc in order to modify
firmware resource table.
But in some cases, other operations are needed like compatibility
check between resources defined at firmware level and those handled
by rproc driver, or remoteproc local resource management when firmware
has no resource table.

This patch associates action to each resource request to:
- verify a resource
- update/amend a resource in firmware resource table
- handle locally a resource

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 13 ++++++++++++-
 include/linux/remoteproc.h           | 26 +++++++++++++++++++++++++-
 2 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 3282a4e..cd64fae 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -876,16 +876,20 @@ static void rproc_dump_resource_table(struct rproc *rproc,
 	}
 }
 
-int rproc_request_resource(struct rproc *rproc, u32 type, void *resource)
+int rproc_request_resource(struct rproc *rproc, u32 type, u32 action, void *resource)
 {
 	struct device *dev = &rproc->dev;
 	struct rproc_request_resource *request;
+	struct fw_rsc_vdev *v;
 	int size;
 
 	request = devm_kzalloc(dev, sizeof(*request), GFP_KERNEL);
 	if (!request)
 		return -ENOMEM;
 
+	if (action > RSC_ACT_LAST)
+		return -EINVAL;
+
 	switch (type) {
 	case RSC_CARVEOUT:
 		size = sizeof(struct fw_rsc_carveout);
@@ -896,6 +900,12 @@ int rproc_request_resource(struct rproc *rproc, u32 type, void *resource)
 	case RSC_TRACE:
 		size = sizeof(struct fw_rsc_trace);
 		break;
+	case RSC_VDEV:
+		v = resource;
+		size = sizeof(struct fw_rsc_vdev);
+		size += v->num_of_vrings * sizeof(struct fw_rsc_vdev_vring);
+		size += v->config_len;
+		break;
 	default:
 		dev_err(dev, "Unsupported resource type: %d\n", type);
 		return -EINVAL;
@@ -908,6 +918,7 @@ int rproc_request_resource(struct rproc *rproc, u32 type, void *resource)
 	memcpy(request->resource, resource, size);
 	request->type = type;
 	request->size = size;
+	request->action = action;
 
 	list_add_tail(&request->node, &rproc->override_resources);
 
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 4e2f822..2b0f1d7 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -335,11 +335,34 @@ struct rproc_mem_entry {
 };
 
 /**
+ * enum rproc_request_action - types of actions associated to a resource
+ * request
+ *
+ * @RSC_ACT_CHECK:	  request to verify this resource with firmware one
+ * @RSC_ACT_UPDATE:	  request to update firmware resource table with associated
+ *			  resource if possible
+ * @RSC_ACT_FORCE_UPDATE: force firmware resource table update with associated
+ *		          resource
+ * @RSC_ACT_LOCAL:        request to handle this resource localy but not to update
+ *			  firmware resource table
+ * @RSC_ACT_LAST:         just keep this one at the end
+ *
+ */
+enum rproc_request_action {
+	RSC_ACT_VERIFY		= 0,
+	RSC_ACT_UPDATE		= 1,
+	RSC_ACT_FORCE_UPDATE	= 2,
+	RSC_ACT_LOCAL		= 3,
+	RSC_ACT_LAST		= 4,
+};
+
+/**
  * struct rproc_requested_resources - add a resource to the resource table
  *
  * @resource:	pointer to a 'struct fw_rsc_*' resource
  * @type:	'fw_resource_type' resource type
  * @size:	size of resource
+ * @action:	action associated the resource
  * @node:	list node
  *
  * Resources can be added by platform-specific rproc drivers calling
@@ -350,6 +373,7 @@ struct rproc_request_resource {
 	void *resource;
 	u32 type;
 	u32 size;
+	u32 action;
 	struct list_head node;
 };
 
@@ -517,7 +541,7 @@ struct rproc_vdev {
 	u32 rsc_offset;
 };
 
-int rproc_request_resource(struct rproc *rproc, u32 type, void *res);
+int rproc_request_resource(struct rproc *rproc, u32 type, u32 action, void *res);
 struct rproc *rproc_get_by_phandle(phandle phandle);
 struct rproc *rproc_alloc(struct device *dev, const char *name,
 			  const struct rproc_ops *ops,
-- 
1.9.1

[PATCH v2 08/19] remoteproc: core: Associate action to resource request

[Loic Pallardy]

With new rproc_request_resource API, rproc driver has now the
capability to provide resources to remoteproc in order to modify
firmware resource table.
But in some cases, other operations are needed like compatibility
check between resources defined at firmware level and those handled
by rproc driver, or remoteproc local resource management when firmware
has no resource table.

This patch associates action to each resource request to:
- verify a resource
- update/amend a resource in firmware resource table
- handle locally a resource

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 13 ++++++++++++-
 include/linux/remoteproc.h           | 26 +++++++++++++++++++++++++-
 2 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 3282a4e..cd64fae 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -876,16 +876,20 @@ static void rproc_dump_resource_table(struct rproc *rproc,
 	}
 }
 
-int rproc_request_resource(struct rproc *rproc, u32 type, void *resource)
+int rproc_request_resource(struct rproc *rproc, u32 type, u32 action, void *resource)
 {
 	struct device *dev = &rproc->dev;
 	struct rproc_request_resource *request;
+	struct fw_rsc_vdev *v;
 	int size;
 
 	request = devm_kzalloc(dev, sizeof(*request), GFP_KERNEL);
 	if (!request)
 		return -ENOMEM;
 
+	if (action > RSC_ACT_LAST)
+		return -EINVAL;
+
 	switch (type) {
 	case RSC_CARVEOUT:
 		size = sizeof(struct fw_rsc_carveout);
@@ -896,6 +900,12 @@ int rproc_request_resource(struct rproc *rproc, u32 type, void *resource)
 	case RSC_TRACE:
 		size = sizeof(struct fw_rsc_trace);
 		break;
+	case RSC_VDEV:
+		v = resource;
+		size = sizeof(struct fw_rsc_vdev);
+		size += v->num_of_vrings * sizeof(struct fw_rsc_vdev_vring);
+		size += v->config_len;
+		break;
 	default:
 		dev_err(dev, "Unsupported resource type: %d\n", type);
 		return -EINVAL;
@@ -908,6 +918,7 @@ int rproc_request_resource(struct rproc *rproc, u32 type, void *resource)
 	memcpy(request->resource, resource, size);
 	request->type = type;
 	request->size = size;
+	request->action = action;
 
 	list_add_tail(&request->node, &rproc->override_resources);
 
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 4e2f822..2b0f1d7 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -335,11 +335,34 @@ struct rproc_mem_entry {
 };
 
 /**
+ * enum rproc_request_action - types of actions associated to a resource
+ * request
+ *
+ * @RSC_ACT_CHECK:	  request to verify this resource with firmware one
+ * @RSC_ACT_UPDATE:	  request to update firmware resource table with associated
+ *			  resource if possible
+ * @RSC_ACT_FORCE_UPDATE: force firmware resource table update with associated
+ *		          resource
+ * @RSC_ACT_LOCAL:        request to handle this resource localy but not to update
+ *			  firmware resource table
+ * @RSC_ACT_LAST:         just keep this one at the end
+ *
+ */
+enum rproc_request_action {
+	RSC_ACT_VERIFY		= 0,
+	RSC_ACT_UPDATE		= 1,
+	RSC_ACT_FORCE_UPDATE	= 2,
+	RSC_ACT_LOCAL		= 3,
+	RSC_ACT_LAST		= 4,
+};
+
+/**
  * struct rproc_requested_resources - add a resource to the resource table
  *
  * @resource:	pointer to a 'struct fw_rsc_*' resource
  * @type:	'fw_resource_type' resource type
  * @size:	size of resource
+ * @action:	action associated the resource
  * @node:	list node
  *
  * Resources can be added by platform-specific rproc drivers calling
@@ -350,6 +373,7 @@ struct rproc_request_resource {
 	void *resource;
 	u32 type;
 	u32 size;
+	u32 action;
 	struct list_head node;
 };
 
@@ -517,7 +541,7 @@ struct rproc_vdev {
 	u32 rsc_offset;
 };
 
-int rproc_request_resource(struct rproc *rproc, u32 type, void *res);
+int rproc_request_resource(struct rproc *rproc, u32 type, u32 action, void *res);
 struct rproc *rproc_get_by_phandle(phandle phandle);
 struct rproc *rproc_alloc(struct device *dev, const char *name,
 			  const struct rproc_ops *ops,
-- 
1.9.1

Re: [PATCH v2 08/19] remoteproc: core: Associate action to resource request

[Lee Jones]

On Wed, 31 Aug 2016, Loic Pallardy wrote:

> With new rproc_request_resource API, rproc driver has now the
> capability to provide resources to remoteproc in order to modify
> firmware resource table.
> But in some cases, other operations are needed like compatibility
> check between resources defined at firmware level and those handled
> by rproc driver, or remoteproc local resource management when firmware
> has no resource table.
> 
> This patch associates action to each resource request to:
> - verify a resource
> - update/amend a resource in firmware resource table
> - handle locally a resource
> 
> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> ---
>  drivers/remoteproc/remoteproc_core.c | 13 ++++++++++++-
>  include/linux/remoteproc.h           | 26 +++++++++++++++++++++++++-
>  2 files changed, 37 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 3282a4e..cd64fae 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -876,16 +876,20 @@ static void rproc_dump_resource_table(struct rproc *rproc,
>  	}
>  }
>  
> -int rproc_request_resource(struct rproc *rproc, u32 type, void *resource)
> +int rproc_request_resource(struct rproc *rproc, u32 type, u32 action, void *resource)
>  {
>  	struct device *dev = &rproc->dev;
>  	struct rproc_request_resource *request;
> +	struct fw_rsc_vdev *v;
>  	int size;
>
>  	request = devm_kzalloc(dev, sizeof(*request), GFP_KERNEL);
>  	if (!request)
>  		return -ENOMEM;
>  
> +	if (action > RSC_ACT_LAST)

">=".  *_LAST is not a valid request.

> +		return -EINVAL;

The switch() statement below should ha

>  	switch (type) {
>  	case RSC_CARVEOUT:
>  		size = sizeof(struct fw_rsc_carveout);
> @@ -896,6 +900,12 @@ int rproc_request_resource(struct rproc *rproc, u32 type, void *resource)
>  	case RSC_TRACE:
>  		size = sizeof(struct fw_rsc_trace);
>  		break;
> +	case RSC_VDEV:
> +		v = resource;
> +		size = sizeof(struct fw_rsc_vdev);
> +		size += v->num_of_vrings * sizeof(struct fw_rsc_vdev_vring);
> +		size += v->config_len;
> +		break;
>  	default:
>  		dev_err(dev, "Unsupported resource type: %d\n", type);
>  		return -EINVAL;
> @@ -908,6 +918,7 @@ int rproc_request_resource(struct rproc *rproc, u32 type, void *resource)
>  	memcpy(request->resource, resource, size);
>  	request->type = type;
>  	request->size = size;
> +	request->action = action;
>  
>  	list_add_tail(&request->node, &rproc->override_resources);
>  
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 4e2f822..2b0f1d7 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -335,11 +335,34 @@ struct rproc_mem_entry {
>  };
>  
>  /**
> + * enum rproc_request_action - types of actions associated to a resource
> + * request
> + *
> + * @RSC_ACT_CHECK:	  request to verify this resource with firmware one
> + * @RSC_ACT_UPDATE:	  request to update firmware resource table with associated
> + *			  resource if possible
> + * @RSC_ACT_FORCE_UPDATE: force firmware resource table update with associated
> + *		          resource
> + * @RSC_ACT_LOCAL:        request to handle this resource localy but not to update
> + *			  firmware resource table
> + * @RSC_ACT_LAST:         just keep this one at the end
> + */
> +enum rproc_request_action {
> +	RSC_ACT_VERIFY		= 0,
> +	RSC_ACT_UPDATE		= 1,
> +	RSC_ACT_FORCE_UPDATE	= 2,
> +	RSC_ACT_LOCAL		= 3,

For reviewing purposes I suggest adding these entries as you start to
support them.  Then we have the code and the suggested comment in one
patch for easy comparison.

> +	RSC_ACT_LAST		= 4,
> +};
> +
> +/**
>   * struct rproc_requested_resources - add a resource to the resource table
>   *
>   * @resource:	pointer to a 'struct fw_rsc_*' resource
>   * @type:	'fw_resource_type' resource type
>   * @size:	size of resource
> + * @action:	action associated the resource
>   * @node:	list node
>   *
>   * Resources can be added by platform-specific rproc drivers calling
> @@ -350,6 +373,7 @@ struct rproc_request_resource {
>  	void *resource;
>  	u32 type;
>  	u32 size;
> +	u32 action;
>  	struct list_head node;
>  };
>  
> @@ -517,7 +541,7 @@ struct rproc_vdev {
>  	u32 rsc_offset;
>  };
>  
> -int rproc_request_resource(struct rproc *rproc, u32 type, void *res);
> +int rproc_request_resource(struct rproc *rproc, u32 type, u32 action, void *res);
>  struct rproc *rproc_get_by_phandle(phandle phandle);
>  struct rproc *rproc_alloc(struct device *dev, const char *name,
>  			  const struct rproc_ops *ops,

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 08/19] remoteproc: core: Associate action to resource request

[loic pallardy]

On 09/01/2016 09:23 AM, Lee Jones wrote:
> On Wed, 31 Aug 2016, Loic Pallardy wrote:
>
>> With new rproc_request_resource API, rproc driver has now the
>> capability to provide resources to remoteproc in order to modify
>> firmware resource table.
>> But in some cases, other operations are needed like compatibility
>> check between resources defined at firmware level and those handled
>> by rproc driver, or remoteproc local resource management when firmware
>> has no resource table.
>>
>> This patch associates action to each resource request to:
>> - verify a resource
>> - update/amend a resource in firmware resource table
>> - handle locally a resource
>>
>> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
>> ---
>>  drivers/remoteproc/remoteproc_core.c | 13 ++++++++++++-
>>  include/linux/remoteproc.h           | 26 +++++++++++++++++++++++++-
>>  2 files changed, 37 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>> index 3282a4e..cd64fae 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -876,16 +876,20 @@ static void rproc_dump_resource_table(struct rproc *rproc,
>>  	}
>>  }
>>
>> -int rproc_request_resource(struct rproc *rproc, u32 type, void *resource)
>> +int rproc_request_resource(struct rproc *rproc, u32 type, u32 action, void *resource)
>>  {
>>  	struct device *dev = &rproc->dev;
>>  	struct rproc_request_resource *request;
>> +	struct fw_rsc_vdev *v;
>>  	int size;
>>
>>  	request = devm_kzalloc(dev, sizeof(*request), GFP_KERNEL);
>>  	if (!request)
>>  		return -ENOMEM;
>>
>> +	if (action > RSC_ACT_LAST)
>
> ">=".  *_LAST is not a valid request.
True, thanks

>
>> +		return -EINVAL;
>
> The switch() statement below should ha
>
>>  	switch (type) {
>>  	case RSC_CARVEOUT:
>>  		size = sizeof(struct fw_rsc_carveout);
>> @@ -896,6 +900,12 @@ int rproc_request_resource(struct rproc *rproc, u32 type, void *resource)
>>  	case RSC_TRACE:
>>  		size = sizeof(struct fw_rsc_trace);
>>  		break;
>> +	case RSC_VDEV:
>> +		v = resource;
>> +		size = sizeof(struct fw_rsc_vdev);
>> +		size += v->num_of_vrings * sizeof(struct fw_rsc_vdev_vring);
>> +		size += v->config_len;
>> +		break;
>>  	default:
>>  		dev_err(dev, "Unsupported resource type: %d\n", type);
>>  		return -EINVAL;
>> @@ -908,6 +918,7 @@ int rproc_request_resource(struct rproc *rproc, u32 type, void *resource)
>>  	memcpy(request->resource, resource, size);
>>  	request->type = type;
>>  	request->size = size;
>> +	request->action = action;
>>
>>  	list_add_tail(&request->node, &rproc->override_resources);
>>
>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>> index 4e2f822..2b0f1d7 100644
>> --- a/include/linux/remoteproc.h
>> +++ b/include/linux/remoteproc.h
>> @@ -335,11 +335,34 @@ struct rproc_mem_entry {
>>  };
>>
>>  /**
>> + * enum rproc_request_action - types of actions associated to a resource
>> + * request
>> + *
>> + * @RSC_ACT_CHECK:	  request to verify this resource with firmware one
>> + * @RSC_ACT_UPDATE:	  request to update firmware resource table with associated
>> + *			  resource if possible
>> + * @RSC_ACT_FORCE_UPDATE: force firmware resource table update with associated
>> + *		          resource
>> + * @RSC_ACT_LOCAL:        request to handle this resource localy but not to update
>> + *			  firmware resource table
>> + * @RSC_ACT_LAST:         just keep this one at the end
>> + */
>> +enum rproc_request_action {
>> +	RSC_ACT_VERIFY		= 0,
>> +	RSC_ACT_UPDATE		= 1,
>> +	RSC_ACT_FORCE_UPDATE	= 2,
>> +	RSC_ACT_LOCAL		= 3,
>
> For reviewing purposes I suggest adding these entries as you start to
> support them.  Then we have the code and the suggested comment in one
> patch for easy comparison.
RSC_ACT_LAST need in this patch, that's why action enum defined here.

Regards,
Loic
>
>> +	RSC_ACT_LAST		= 4,
>> +};
>> +
>> +/**
>>   * struct rproc_requested_resources - add a resource to the resource table
>>   *
>>   * @resource:	pointer to a 'struct fw_rsc_*' resource
>>   * @type:	'fw_resource_type' resource type
>>   * @size:	size of resource
>> + * @action:	action associated the resource
>>   * @node:	list node
>>   *
>>   * Resources can be added by platform-specific rproc drivers calling
>> @@ -350,6 +373,7 @@ struct rproc_request_resource {
>>  	void *resource;
>>  	u32 type;
>>  	u32 size;
>> +	u32 action;
>>  	struct list_head node;
>>  };
>>
>> @@ -517,7 +541,7 @@ struct rproc_vdev {
>>  	u32 rsc_offset;
>>  };
>>
>> -int rproc_request_resource(struct rproc *rproc, u32 type, void *res);
>> +int rproc_request_resource(struct rproc *rproc, u32 type, u32 action, void *res);
>>  struct rproc *rproc_get_by_phandle(phandle phandle);
>>  struct rproc *rproc_alloc(struct device *dev, const char *name,
>>  			  const struct rproc_ops *ops,
>

Re: [PATCH v2 08/19] remoteproc: core: Associate action to resource request

[loic pallardy]

On 09/01/2016 09:23 AM, Lee Jones wrote:
> On Wed, 31 Aug 2016, Loic Pallardy wrote:
>
>> With new rproc_request_resource API, rproc driver has now the
>> capability to provide resources to remoteproc in order to modify
>> firmware resource table.
>> But in some cases, other operations are needed like compatibility
>> check between resources defined at firmware level and those handled
>> by rproc driver, or remoteproc local resource management when firmware
>> has no resource table.
>>
>> This patch associates action to each resource request to:
>> - verify a resource
>> - update/amend a resource in firmware resource table
>> - handle locally a resource
>>
>> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
>> ---
>>  drivers/remoteproc/remoteproc_core.c | 13 ++++++++++++-
>>  include/linux/remoteproc.h           | 26 +++++++++++++++++++++++++-
>>  2 files changed, 37 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>> index 3282a4e..cd64fae 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -876,16 +876,20 @@ static void rproc_dump_resource_table(struct rproc *rproc,
>>  	}
>>  }
>>
>> -int rproc_request_resource(struct rproc *rproc, u32 type, void *resource)
>> +int rproc_request_resource(struct rproc *rproc, u32 type, u32 action, void *resource)
>>  {
>>  	struct device *dev = &rproc->dev;
>>  	struct rproc_request_resource *request;
>> +	struct fw_rsc_vdev *v;
>>  	int size;
>>
>>  	request = devm_kzalloc(dev, sizeof(*request), GFP_KERNEL);
>>  	if (!request)
>>  		return -ENOMEM;
>>
>> +	if (action > RSC_ACT_LAST)
>
> ">=".  *_LAST is not a valid request.
True, thanks

>
>> +		return -EINVAL;
>
> The switch() statement below should ha
>
>>  	switch (type) {
>>  	case RSC_CARVEOUT:
>>  		size = sizeof(struct fw_rsc_carveout);
>> @@ -896,6 +900,12 @@ int rproc_request_resource(struct rproc *rproc, u32 type, void *resource)
>>  	case RSC_TRACE:
>>  		size = sizeof(struct fw_rsc_trace);
>>  		break;
>> +	case RSC_VDEV:
>> +		v = resource;
>> +		size = sizeof(struct fw_rsc_vdev);
>> +		size += v->num_of_vrings * sizeof(struct fw_rsc_vdev_vring);
>> +		size += v->config_len;
>> +		break;
>>  	default:
>>  		dev_err(dev, "Unsupported resource type: %d\n", type);
>>  		return -EINVAL;
>> @@ -908,6 +918,7 @@ int rproc_request_resource(struct rproc *rproc, u32 type, void *resource)
>>  	memcpy(request->resource, resource, size);
>>  	request->type = type;
>>  	request->size = size;
>> +	request->action = action;
>>
>>  	list_add_tail(&request->node, &rproc->override_resources);
>>
>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>> index 4e2f822..2b0f1d7 100644
>> --- a/include/linux/remoteproc.h
>> +++ b/include/linux/remoteproc.h
>> @@ -335,11 +335,34 @@ struct rproc_mem_entry {
>>  };
>>
>>  /**
>> + * enum rproc_request_action - types of actions associated to a resource
>> + * request
>> + *
>> + * @RSC_ACT_CHECK:	  request to verify this resource with firmware one
>> + * @RSC_ACT_UPDATE:	  request to update firmware resource table with associated
>> + *			  resource if possible
>> + * @RSC_ACT_FORCE_UPDATE: force firmware resource table update with associated
>> + *		          resource
>> + * @RSC_ACT_LOCAL:        request to handle this resource localy but not to update
>> + *			  firmware resource table
>> + * @RSC_ACT_LAST:         just keep this one at the end
>> + */
>> +enum rproc_request_action {
>> +	RSC_ACT_VERIFY		= 0,
>> +	RSC_ACT_UPDATE		= 1,
>> +	RSC_ACT_FORCE_UPDATE	= 2,
>> +	RSC_ACT_LOCAL		= 3,
>
> For reviewing purposes I suggest adding these entries as you start to
> support them.  Then we have the code and the suggested comment in one
> patch for easy comparison.
RSC_ACT_LAST need in this patch, that's why action enum defined here.

Regards,
Loic
>
>> +	RSC_ACT_LAST		= 4,
>> +};
>> +
>> +/**
>>   * struct rproc_requested_resources - add a resource to the resource table
>>   *
>>   * @resource:	pointer to a 'struct fw_rsc_*' resource
>>   * @type:	'fw_resource_type' resource type
>>   * @size:	size of resource
>> + * @action:	action associated the resource
>>   * @node:	list node
>>   *
>>   * Resources can be added by platform-specific rproc drivers calling
>> @@ -350,6 +373,7 @@ struct rproc_request_resource {
>>  	void *resource;
>>  	u32 type;
>>  	u32 size;
>> +	u32 action;
>>  	struct list_head node;
>>  };
>>
>> @@ -517,7 +541,7 @@ struct rproc_vdev {
>>  	u32 rsc_offset;
>>  };
>>
>> -int rproc_request_resource(struct rproc *rproc, u32 type, void *res);
>> +int rproc_request_resource(struct rproc *rproc, u32 type, u32 action, void *res);
>>  struct rproc *rproc_get_by_phandle(phandle phandle);
>>  struct rproc *rproc_alloc(struct device *dev, const char *name,
>>  			  const struct rproc_ops *ops,
>

Re: [PATCH v2 08/19] remoteproc: core: Associate action to resource request

[Lee Jones]

On Thu, 08 Sep 2016, loic pallardy wrote:
> On 09/01/2016 09:23 AM, Lee Jones wrote:
> > On Wed, 31 Aug 2016, Loic Pallardy wrote:
> > 
> > > With new rproc_request_resource API, rproc driver has now the
> > > capability to provide resources to remoteproc in order to modify
> > > firmware resource table.
> > > But in some cases, other operations are needed like compatibility
> > > check between resources defined at firmware level and those handled
> > > by rproc driver, or remoteproc local resource management when firmware
> > > has no resource table.
> > > 
> > > This patch associates action to each resource request to:
> > > - verify a resource
> > > - update/amend a resource in firmware resource table
> > > - handle locally a resource
> > > 
> > > Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> > > ---
> > >  drivers/remoteproc/remoteproc_core.c | 13 ++++++++++++-
> > >  include/linux/remoteproc.h           | 26 +++++++++++++++++++++++++-
> > >  2 files changed, 37 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > > index 3282a4e..cd64fae 100644
> > > --- a/drivers/remoteproc/remoteproc_core.c
> > > +++ b/drivers/remoteproc/remoteproc_core.c
> > > @@ -876,16 +876,20 @@ static void rproc_dump_resource_table(struct rproc *rproc,

[...]

> > >  /**
> > > + * enum rproc_request_action - types of actions associated to a resource
> > > + * request
> > > + *
> > > + * @RSC_ACT_CHECK:	  request to verify this resource with firmware one
> > > + * @RSC_ACT_UPDATE:	  request to update firmware resource table with associated
> > > + *			  resource if possible
> > > + * @RSC_ACT_FORCE_UPDATE: force firmware resource table update with associated
> > > + *		          resource
> > > + * @RSC_ACT_LOCAL:        request to handle this resource localy but not to update
> > > + *			  firmware resource table
> > > + * @RSC_ACT_LAST:         just keep this one at the end
> > > + */
> > > +enum rproc_request_action {
> > > +	RSC_ACT_VERIFY		= 0,
> > > +	RSC_ACT_UPDATE		= 1,
> > > +	RSC_ACT_FORCE_UPDATE	= 2,
> > > +	RSC_ACT_LOCAL		= 3,
> > 
> > For reviewing purposes I suggest adding these entries as you start to
> > support them.  Then we have the code and the suggested comment in one
> > patch for easy comparison.
> RSC_ACT_LAST need in this patch, that's why action enum defined here.

Okay, good point.

> > > +	RSC_ACT_LAST		= 4,
> > > +};
> > > +
> > > +/**
> > >   * struct rproc_requested_resources - add a resource to the resource table
> > >   *
> > >   * @resource:	pointer to a 'struct fw_rsc_*' resource
> > >   * @type:	'fw_resource_type' resource type
> > >   * @size:	size of resource
> > > + * @action:	action associated the resource
> > >   * @node:	list node
> > >   *
> > >   * Resources can be added by platform-specific rproc drivers calling
> > > @@ -350,6 +373,7 @@ struct rproc_request_resource {
> > >  	void *resource;
> > >  	u32 type;
> > >  	u32 size;
> > > +	u32 action;
> > >  	struct list_head node;
> > >  };
> > > 
> > > @@ -517,7 +541,7 @@ struct rproc_vdev {
> > >  	u32 rsc_offset;
> > >  };
> > > 
> > > -int rproc_request_resource(struct rproc *rproc, u32 type, void *res);
> > > +int rproc_request_resource(struct rproc *rproc, u32 type, u32 action, void *res);
> > >  struct rproc *rproc_get_by_phandle(phandle phandle);
> > >  struct rproc *rproc_alloc(struct device *dev, const char *name,
> > >  			  const struct rproc_ops *ops,
> > 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v2 09/19] remoteproc: core: Finalize dump resource table function

[Loic Pallardy]

Diverse updates:
- add cfg field display of vdev struct
- add support of spare resource
- put rproc_dump_resource_table under DEBUG compilation flag

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index cd64fae..345bdfb 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -791,15 +791,17 @@ static void rproc_resource_cleanup(struct rproc *rproc)
 		rproc_remove_virtio_dev(rvdev);
 }
 
+#if defined(DEBUG)
 static void rproc_dump_resource_table(struct rproc *rproc,
 				      struct resource_table *table, int size)
 {
-	const char *types[] = {"carveout", "devmem", "trace", "vdev"};
+	static const char *types[] = {"carveout", "devmem", "trace", "vdev", "spare"};
 	struct device *dev = &rproc->dev;
 	struct fw_rsc_carveout *c;
 	struct fw_rsc_devmem *d;
 	struct fw_rsc_trace *t;
 	struct fw_rsc_vdev *v;
+	struct fw_rsc_spare *s;
 	int i, j;
 
 	if (!table) {
@@ -814,6 +816,8 @@ static void rproc_dump_resource_table(struct rproc *rproc,
 		int offset = table->offset[i];
 		struct fw_rsc_hdr *hdr = (void *)table + offset;
 		void *rsc = (void *)hdr + sizeof(*hdr);
+		unsigned char *cfg;
+		int len;
 
 		switch (hdr->type) {
 		case RSC_CARVEOUT:
@@ -867,14 +871,35 @@ static void rproc_dump_resource_table(struct rproc *rproc,
 				dev_dbg(dev, "    Reserved (should be zero) [%d]\n\n",
 					v->vring[j].reserved);
 			}
+
+			dev_dbg(dev, "  Config table\n");
+			cfg = (unsigned char *)(&v->vring[v->num_of_vrings]);
+			len = 0;
+			do {
+				j = min(16, v->config_len - len);
+				dev_dbg(dev, "    Config[%2d-%2d] = %*phC\n",
+					len, len + j - 1, j, cfg + len);
+				len += j;
+			} while (len < v->config_len);
+
+			break;
+		case RSC_SPARE:
+			s = rsc;
+			dev_dbg(dev, "Entry %d is of type %s\n", i, types[hdr->type]);
+			dev_dbg(dev, "  Spare size: 0x%x bytes\n\n", s->len);
 			break;
 		default:
-			dev_dbg(dev, "Invalid resource type found: %d [hdr: %p]\n",
-				hdr->type, hdr);
+			dev_dbg(dev, "Entry %d: Invalid resource type found: %d [hdr: %p]\n",
+				i, hdr->type, hdr);
 			return;
 		}
 	}
 }
+#else
+static inline void rproc_dump_resource_table(struct rproc *rproc,
+				      struct resource_table *table, int size)
+{}
+#endif
 
 int rproc_request_resource(struct rproc *rproc, u32 type, u32 action, void *resource)
 {
-- 
1.9.1

[PATCH v2 09/19] remoteproc: core: Finalize dump resource table function

[Loic Pallardy]

Diverse updates:
- add cfg field display of vdev struct
- add support of spare resource
- put rproc_dump_resource_table under DEBUG compilation flag

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index cd64fae..345bdfb 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -791,15 +791,17 @@ static void rproc_resource_cleanup(struct rproc *rproc)
 		rproc_remove_virtio_dev(rvdev);
 }
 
+#if defined(DEBUG)
 static void rproc_dump_resource_table(struct rproc *rproc,
 				      struct resource_table *table, int size)
 {
-	const char *types[] = {"carveout", "devmem", "trace", "vdev"};
+	static const char *types[] = {"carveout", "devmem", "trace", "vdev", "spare"};
 	struct device *dev = &rproc->dev;
 	struct fw_rsc_carveout *c;
 	struct fw_rsc_devmem *d;
 	struct fw_rsc_trace *t;
 	struct fw_rsc_vdev *v;
+	struct fw_rsc_spare *s;
 	int i, j;
 
 	if (!table) {
@@ -814,6 +816,8 @@ static void rproc_dump_resource_table(struct rproc *rproc,
 		int offset = table->offset[i];
 		struct fw_rsc_hdr *hdr = (void *)table + offset;
 		void *rsc = (void *)hdr + sizeof(*hdr);
+		unsigned char *cfg;
+		int len;
 
 		switch (hdr->type) {
 		case RSC_CARVEOUT:
@@ -867,14 +871,35 @@ static void rproc_dump_resource_table(struct rproc *rproc,
 				dev_dbg(dev, "    Reserved (should be zero) [%d]\n\n",
 					v->vring[j].reserved);
 			}
+
+			dev_dbg(dev, "  Config table\n");
+			cfg = (unsigned char *)(&v->vring[v->num_of_vrings]);
+			len = 0;
+			do {
+				j = min(16, v->config_len - len);
+				dev_dbg(dev, "    Config[%2d-%2d] = %*phC\n",
+					len, len + j - 1, j, cfg + len);
+				len += j;
+			} while (len < v->config_len);
+
+			break;
+		case RSC_SPARE:
+			s = rsc;
+			dev_dbg(dev, "Entry %d is of type %s\n", i, types[hdr->type]);
+			dev_dbg(dev, "  Spare size: 0x%x bytes\n\n", s->len);
 			break;
 		default:
-			dev_dbg(dev, "Invalid resource type found: %d [hdr: %p]\n",
-				hdr->type, hdr);
+			dev_dbg(dev, "Entry %d: Invalid resource type found: %d [hdr: %p]\n",
+				i, hdr->type, hdr);
 			return;
 		}
 	}
 }
+#else
+static inline void rproc_dump_resource_table(struct rproc *rproc,
+				      struct resource_table *table, int size)
+{}
+#endif
 
 int rproc_request_resource(struct rproc *rproc, u32 type, u32 action, void *resource)
 {
-- 
1.9.1

Re: [PATCH v2 09/19] remoteproc: core: Finalize dump resource table function

[Lee Jones]

On Wed, 31 Aug 2016, Loic Pallardy wrote:

> Diverse updates:
> - add cfg field display of vdev struct
> - add support of spare resource
> - put rproc_dump_resource_table under DEBUG compilation flag
> 
> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> ---
>  drivers/remoteproc/remoteproc_core.c | 31 ++++++++++++++++++++++++++++---
>  1 file changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index cd64fae..345bdfb 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -791,15 +791,17 @@ static void rproc_resource_cleanup(struct rproc *rproc)
>  		rproc_remove_virtio_dev(rvdev);
>  }
>  
> +#if defined(DEBUG)

Yuk!  I hate #iferey in *.c files if it can be helped.

Instead, just use if (IS_ENABLED(CONFIG_DEBUG)) at the call-site and
let the compiler optimise it out.

>  static void rproc_dump_resource_table(struct rproc *rproc,
>  				      struct resource_table *table, int size)
>  {
> -	const char *types[] = {"carveout", "devmem", "trace", "vdev"};
> +	static const char *types[] = {"carveout", "devmem", "trace", "vdev", "spare"};
>  	struct device *dev = &rproc->dev;
>  	struct fw_rsc_carveout *c;
>  	struct fw_rsc_devmem *d;
>  	struct fw_rsc_trace *t;
>  	struct fw_rsc_vdev *v;
> +	struct fw_rsc_spare *s;
>  	int i, j;
>  
>  	if (!table) {
> @@ -814,6 +816,8 @@ static void rproc_dump_resource_table(struct rproc *rproc,
>  		int offset = table->offset[i];
>  		struct fw_rsc_hdr *hdr = (void *)table + offset;
>  		void *rsc = (void *)hdr + sizeof(*hdr);
> +		unsigned char *cfg;
> +		int len;
>  
>  		switch (hdr->type) {
>  		case RSC_CARVEOUT:
> @@ -867,14 +871,35 @@ static void rproc_dump_resource_table(struct rproc *rproc,
>  				dev_dbg(dev, "    Reserved (should be zero) [%d]\n\n",
>  					v->vring[j].reserved);
>  			}
> +
> +			dev_dbg(dev, "  Config table\n");
> +			cfg = (unsigned char *)(&v->vring[v->num_of_vrings]);
> +			len = 0;
> +			do {
> +				j = min(16, v->config_len - len);
> +				dev_dbg(dev, "    Config[%2d-%2d] = %*phC\n",
> +					len, len + j - 1, j, cfg + len);
> +				len += j;
> +			} while (len < v->config_len);
> +
> +			break;
> +		case RSC_SPARE:
> +			s = rsc;
> +			dev_dbg(dev, "Entry %d is of type %s\n", i, types[hdr->type]);
> +			dev_dbg(dev, "  Spare size: 0x%x bytes\n\n", s->len);
>  			break;
>  		default:
> -			dev_dbg(dev, "Invalid resource type found: %d [hdr: %p]\n",
> -				hdr->type, hdr);
> +			dev_dbg(dev, "Entry %d: Invalid resource type found: %d [hdr: %p]\n",
> +				i, hdr->type, hdr);

You're doing a lot of stuff in the patch.  If I were maintainer, I'd
be asking you to separate the functionality into separate patches.

>  			return;
>  		}
>  	}
>  }
> +#else
> +static inline void rproc_dump_resource_table(struct rproc *rproc,
> +				      struct resource_table *table, int size)
> +{}
> +#endif
>  
>  int rproc_request_resource(struct rproc *rproc, u32 type, u32 action, void *resource)
>  {

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 09/19] remoteproc: core: Finalize dump resource table function

[loic pallardy]

On 09/08/2016 10:26 AM, Lee Jones wrote:
> On Wed, 31 Aug 2016, Loic Pallardy wrote:
>
>> Diverse updates:
>> - add cfg field display of vdev struct
>> - add support of spare resource
>> - put rproc_dump_resource_table under DEBUG compilation flag
>>
>> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
>> ---
>>  drivers/remoteproc/remoteproc_core.c | 31 ++++++++++++++++++++++++++++---
>>  1 file changed, 28 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>> index cd64fae..345bdfb 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -791,15 +791,17 @@ static void rproc_resource_cleanup(struct rproc *rproc)
>>  		rproc_remove_virtio_dev(rvdev);
>>  }
>>
>> +#if defined(DEBUG)
>
> Yuk!  I hate #iferey in *.c files if it can be helped.
>
> Instead, just use if (IS_ENABLED(CONFIG_DEBUG)) at the call-site and
> let the compiler optimise it out.
Indeed looks better. I'll update in V3
>
>>  static void rproc_dump_resource_table(struct rproc *rproc,
>>  				      struct resource_table *table, int size)
>>  {
>> -	const char *types[] = {"carveout", "devmem", "trace", "vdev"};
>> +	static const char *types[] = {"carveout", "devmem", "trace", "vdev", "spare"};
>>  	struct device *dev = &rproc->dev;
>>  	struct fw_rsc_carveout *c;
>>  	struct fw_rsc_devmem *d;
>>  	struct fw_rsc_trace *t;
>>  	struct fw_rsc_vdev *v;
>> +	struct fw_rsc_spare *s;
>>  	int i, j;
>>
>>  	if (!table) {
>> @@ -814,6 +816,8 @@ static void rproc_dump_resource_table(struct rproc *rproc,
>>  		int offset = table->offset[i];
>>  		struct fw_rsc_hdr *hdr = (void *)table + offset;
>>  		void *rsc = (void *)hdr + sizeof(*hdr);
>> +		unsigned char *cfg;
>> +		int len;
>>
>>  		switch (hdr->type) {
>>  		case RSC_CARVEOUT:
>> @@ -867,14 +871,35 @@ static void rproc_dump_resource_table(struct rproc *rproc,
>>  				dev_dbg(dev, "    Reserved (should be zero) [%d]\n\n",
>>  					v->vring[j].reserved);
>>  			}
>> +
>> +			dev_dbg(dev, "  Config table\n");
>> +			cfg = (unsigned char *)(&v->vring[v->num_of_vrings]);
>> +			len = 0;
>> +			do {
>> +				j = min(16, v->config_len - len);
>> +				dev_dbg(dev, "    Config[%2d-%2d] = %*phC\n",
>> +					len, len + j - 1, j, cfg + len);
>> +				len += j;
>> +			} while (len < v->config_len);
>> +
>> +			break;
>> +		case RSC_SPARE:
>> +			s = rsc;
>> +			dev_dbg(dev, "Entry %d is of type %s\n", i, types[hdr->type]);
>> +			dev_dbg(dev, "  Spare size: 0x%x bytes\n\n", s->len);
>>  			break;
>>  		default:
>> -			dev_dbg(dev, "Invalid resource type found: %d [hdr: %p]\n",
>> -				hdr->type, hdr);
>> +			dev_dbg(dev, "Entry %d: Invalid resource type found: %d [hdr: %p]\n",
>> +				i, hdr->type, hdr);
>
> You're doing a lot of stuff in the patch.  If I were maintainer, I'd
> be asking you to separate the functionality into separate patches.
No problem to split in smaller patches
/Loic
>
>>  			return;
>>  		}
>>  	}
>>  }
>> +#else
>> +static inline void rproc_dump_resource_table(struct rproc *rproc,
>> +				      struct resource_table *table, int size)
>> +{}
>> +#endif
>>
>>  int rproc_request_resource(struct rproc *rproc, u32 type, u32 action, void *resource)
>>  {
>

Re: [PATCH v2 09/19] remoteproc: core: Finalize dump resource table function

[loic pallardy]

On 09/08/2016 10:26 AM, Lee Jones wrote:
> On Wed, 31 Aug 2016, Loic Pallardy wrote:
>
>> Diverse updates:
>> - add cfg field display of vdev struct
>> - add support of spare resource
>> - put rproc_dump_resource_table under DEBUG compilation flag
>>
>> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
>> ---
>>  drivers/remoteproc/remoteproc_core.c | 31 ++++++++++++++++++++++++++++---
>>  1 file changed, 28 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>> index cd64fae..345bdfb 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -791,15 +791,17 @@ static void rproc_resource_cleanup(struct rproc *rproc)
>>  		rproc_remove_virtio_dev(rvdev);
>>  }
>>
>> +#if defined(DEBUG)
>
> Yuk!  I hate #iferey in *.c files if it can be helped.
>
> Instead, just use if (IS_ENABLED(CONFIG_DEBUG)) at the call-site and
> let the compiler optimise it out.
Indeed looks better. I'll update in V3
>
>>  static void rproc_dump_resource_table(struct rproc *rproc,
>>  				      struct resource_table *table, int size)
>>  {
>> -	const char *types[] = {"carveout", "devmem", "trace", "vdev"};
>> +	static const char *types[] = {"carveout", "devmem", "trace", "vdev", "spare"};
>>  	struct device *dev = &rproc->dev;
>>  	struct fw_rsc_carveout *c;
>>  	struct fw_rsc_devmem *d;
>>  	struct fw_rsc_trace *t;
>>  	struct fw_rsc_vdev *v;
>> +	struct fw_rsc_spare *s;
>>  	int i, j;
>>
>>  	if (!table) {
>> @@ -814,6 +816,8 @@ static void rproc_dump_resource_table(struct rproc *rproc,
>>  		int offset = table->offset[i];
>>  		struct fw_rsc_hdr *hdr = (void *)table + offset;
>>  		void *rsc = (void *)hdr + sizeof(*hdr);
>> +		unsigned char *cfg;
>> +		int len;
>>
>>  		switch (hdr->type) {
>>  		case RSC_CARVEOUT:
>> @@ -867,14 +871,35 @@ static void rproc_dump_resource_table(struct rproc *rproc,
>>  				dev_dbg(dev, "    Reserved (should be zero) [%d]\n\n",
>>  					v->vring[j].reserved);
>>  			}
>> +
>> +			dev_dbg(dev, "  Config table\n");
>> +			cfg = (unsigned char *)(&v->vring[v->num_of_vrings]);
>> +			len = 0;
>> +			do {
>> +				j = min(16, v->config_len - len);
>> +				dev_dbg(dev, "    Config[%2d-%2d] = %*phC\n",
>> +					len, len + j - 1, j, cfg + len);
>> +				len += j;
>> +			} while (len < v->config_len);
>> +
>> +			break;
>> +		case RSC_SPARE:
>> +			s = rsc;
>> +			dev_dbg(dev, "Entry %d is of type %s\n", i, types[hdr->type]);
>> +			dev_dbg(dev, "  Spare size: 0x%x bytes\n\n", s->len);
>>  			break;
>>  		default:
>> -			dev_dbg(dev, "Invalid resource type found: %d [hdr: %p]\n",
>> -				hdr->type, hdr);
>> +			dev_dbg(dev, "Entry %d: Invalid resource type found: %d [hdr: %p]\n",
>> +				i, hdr->type, hdr);
>
> You're doing a lot of stuff in the patch.  If I were maintainer, I'd
> be asking you to separate the functionality into separate patches.
No problem to split in smaller patches
/Loic
>
>>  			return;
>>  		}
>>  	}
>>  }
>> +#else
>> +static inline void rproc_dump_resource_table(struct rproc *rproc,
>> +				      struct resource_table *table, int size)
>> +{}
>> +#endif
>>
>>  int rproc_request_resource(struct rproc *rproc, u32 type, u32 action, void *resource)
>>  {
>

[PATCH v2 10/19] remoteproc: core: Add function to verify an existing resource in rsc table

[Loic Pallardy]

When a firmware is build, it can be loaded with a resource
usually detailing shared; memory, virtual device, trace log
information etc.
However, some SoCs or platforms require some hard-coded information.
Information may be fixed on both sides: firmware and rproc.
Until now, no method exists which allows to compare resource table
fixed resources and rproc driver configuration.
This new function changes that performing comparison between resources.
Only carveout resource supported for the moment.

It is rproc driver responsibility to provide list of resources to
verify during boot.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 47 ++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 345bdfb..ffb56c0 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -951,6 +951,53 @@ int rproc_request_resource(struct rproc *rproc, u32 type, u32 action, void *reso
 }
 EXPORT_SYMBOL(rproc_request_resource);
 
+static int __verify_rsc_tbl_entry(struct rproc *rproc,
+				struct rproc_request_resource *request,
+				struct resource_table *table, int size)
+{
+	struct fw_rsc_carveout *tblc, *newc;
+	int i;
+
+	for (i = 0; i < table->num; i++) {
+		int offset = table->offset[i];
+		struct fw_rsc_hdr *hdr = (void *)table + offset;
+		void *rsc = (void *)hdr + sizeof(*hdr);
+
+		if (request->type != hdr->type)
+			continue;
+
+		switch (hdr->type) {
+		case RSC_CARVEOUT:
+			tblc = rsc;
+			newc = request->resource;
+
+			if (strncmp(newc->name, tblc->name,
+				    sizeof(*tblc->name)))
+				break;
+
+			/* verify firmware resource is part of rproc one */
+			if (tblc->pa != FW_RSC_ADDR_ANY) {
+				int pa_offset = tblc->pa - newc->pa;
+
+				/* is carevout too small ? */
+				if (pa_offset < 0)
+					return -EINVAL;
+
+				if (pa_offset + tblc->len > newc->len)
+					return -EINVAL;
+			}
+			return 0;
+		default:
+			dev_err(&rproc->dev,
+				"Unsupported resource type: %d\n",
+				hdr->type);
+			return -EINVAL;
+		}
+	}
+
+	return -EINVAL;
+}
+
 static int rproc_update_resource_table_entry(struct rproc *rproc,
 				struct rproc_request_resource *request,
 				struct resource_table *table, int size)
-- 
1.9.1

[PATCH v2 10/19] remoteproc: core: Add function to verify an existing resource in rsc table

[Loic Pallardy]

When a firmware is build, it can be loaded with a resource
usually detailing shared; memory, virtual device, trace log
information etc.
However, some SoCs or platforms require some hard-coded information.
Information may be fixed on both sides: firmware and rproc.
Until now, no method exists which allows to compare resource table
fixed resources and rproc driver configuration.
This new function changes that performing comparison between resources.
Only carveout resource supported for the moment.

It is rproc driver responsibility to provide list of resources to
verify during boot.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 47 ++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 345bdfb..ffb56c0 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -951,6 +951,53 @@ int rproc_request_resource(struct rproc *rproc, u32 type, u32 action, void *reso
 }
 EXPORT_SYMBOL(rproc_request_resource);
 
+static int __verify_rsc_tbl_entry(struct rproc *rproc,
+				struct rproc_request_resource *request,
+				struct resource_table *table, int size)
+{
+	struct fw_rsc_carveout *tblc, *newc;
+	int i;
+
+	for (i = 0; i < table->num; i++) {
+		int offset = table->offset[i];
+		struct fw_rsc_hdr *hdr = (void *)table + offset;
+		void *rsc = (void *)hdr + sizeof(*hdr);
+
+		if (request->type != hdr->type)
+			continue;
+
+		switch (hdr->type) {
+		case RSC_CARVEOUT:
+			tblc = rsc;
+			newc = request->resource;
+
+			if (strncmp(newc->name, tblc->name,
+				    sizeof(*tblc->name)))
+				break;
+
+			/* verify firmware resource is part of rproc one */
+			if (tblc->pa != FW_RSC_ADDR_ANY) {
+				int pa_offset = tblc->pa - newc->pa;
+
+				/* is carevout too small ? */
+				if (pa_offset < 0)
+					return -EINVAL;
+
+				if (pa_offset + tblc->len > newc->len)
+					return -EINVAL;
+			}
+			return 0;
+		default:
+			dev_err(&rproc->dev,
+				"Unsupported resource type: %d\n",
+				hdr->type);
+			return -EINVAL;
+		}
+	}
+
+	return -EINVAL;
+}
+
 static int rproc_update_resource_table_entry(struct rproc *rproc,
 				struct rproc_request_resource *request,
 				struct resource_table *table, int size)
-- 
1.9.1

[PATCH v2 11/19] remoteproc: core: Add function to get resource table spare bytes information

[Loic Pallardy]

Remoteproc needs to know if a resource table has spare resource
allowing resoure table extension.
This function parse resource table to detect spare resource.
If any, it returns available spare bytes and index of spare resource.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index ffb56c0..30e9c70 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -951,6 +951,35 @@ int rproc_request_resource(struct rproc *rproc, u32 type, u32 action, void *reso
 }
 EXPORT_SYMBOL(rproc_request_resource);
 
+static int __get_rsc_tbl_spare_size(struct rproc *rproc,
+					  struct resource_table *table_ptr, int len,
+					  int *spare_index)
+{
+	struct device *dev = &rproc->dev;
+	int i;
+
+	for (i = 0; i < table_ptr->num; i++) {
+		int offset = table_ptr->offset[i];
+		struct fw_rsc_hdr *hdr = (void *)table_ptr + offset;
+		struct fw_rsc_spare *spare = (void *)hdr + sizeof(*hdr);
+
+		dev_dbg(dev, "rsc: type %d\n", hdr->type);
+
+		if (hdr->type >= RSC_LAST) {
+			dev_warn(dev, "unsupported resource %d\n", hdr->type);
+			continue;
+		}
+
+		if (hdr->type != RSC_SPARE)
+			continue;
+
+		*spare_index = i;
+		return spare->len;
+	}
+
+	return 0;
+}
+
 static int __verify_rsc_tbl_entry(struct rproc *rproc,
 				struct rproc_request_resource *request,
 				struct resource_table *table, int size)
-- 
1.9.1

[PATCH v2 11/19] remoteproc: core: Add function to get resource table spare bytes information

[Loic Pallardy]

Remoteproc needs to know if a resource table has spare resource
allowing resoure table extension.
This function parse resource table to detect spare resource.
If any, it returns available spare bytes and index of spare resource.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index ffb56c0..30e9c70 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -951,6 +951,35 @@ int rproc_request_resource(struct rproc *rproc, u32 type, u32 action, void *reso
 }
 EXPORT_SYMBOL(rproc_request_resource);
 
+static int __get_rsc_tbl_spare_size(struct rproc *rproc,
+					  struct resource_table *table_ptr, int len,
+					  int *spare_index)
+{
+	struct device *dev = &rproc->dev;
+	int i;
+
+	for (i = 0; i < table_ptr->num; i++) {
+		int offset = table_ptr->offset[i];
+		struct fw_rsc_hdr *hdr = (void *)table_ptr + offset;
+		struct fw_rsc_spare *spare = (void *)hdr + sizeof(*hdr);
+
+		dev_dbg(dev, "rsc: type %d\n", hdr->type);
+
+		if (hdr->type >= RSC_LAST) {
+			dev_warn(dev, "unsupported resource %d\n", hdr->type);
+			continue;
+		}
+
+		if (hdr->type != RSC_SPARE)
+			continue;
+
+		*spare_index = i;
+		return spare->len;
+	}
+
+	return 0;
+}
+
 static int __verify_rsc_tbl_entry(struct rproc *rproc,
 				struct rproc_request_resource *request,
 				struct resource_table *table, int size)
-- 
1.9.1

Re: [PATCH v2 11/19] remoteproc: core: Add function to get resource table spare bytes information

[Lee Jones]

On Wed, 31 Aug 2016, Loic Pallardy wrote:

> Remoteproc needs to know if a resource table has spare resource
> allowing resoure table extension.
> This function parse resource table to detect spare resource.
> If any, it returns available spare bytes and index of spare resource.
> 
> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> ---
>  drivers/remoteproc/remoteproc_core.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index ffb56c0..30e9c70 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -951,6 +951,35 @@ int rproc_request_resource(struct rproc *rproc, u32 type, u32 action, void *reso
>  }
>  EXPORT_SYMBOL(rproc_request_resource);
>  
> +static int __get_rsc_tbl_spare_size(struct rproc *rproc,

size_t?

> +					  struct resource_table *table_ptr, int len,
> +					  int *spare_index)
> +{
> +	struct device *dev = &rproc->dev;
> +	int i;
> +
> +	for (i = 0; i < table_ptr->num; i++) {
> +		int offset = table_ptr->offset[i];
> +		struct fw_rsc_hdr *hdr = (void *)table_ptr + offset;
> +		struct fw_rsc_spare *spare = (void *)hdr + sizeof(*hdr);
> +
> +		dev_dbg(dev, "rsc: type %d\n", hdr->type);

This doesn't need to be in upstream code.

> +		if (hdr->type >= RSC_LAST) {
> +			dev_warn(dev, "unsupported resource %d\n", hdr->type);
> +			continue;
> +		}

If we're only interested in RSC_SPARE, I think we can omit this check.

No need to police the resource table everywhere.

> +		if (hdr->type != RSC_SPARE)
> +			continue;
> +
> +		*spare_index = i;
> +		return spare->len;
> +	}
> +
> +	return 0;
> +}
> +
>  static int __verify_rsc_tbl_entry(struct rproc *rproc,
>  				struct rproc_request_resource *request,
>  				struct resource_table *table, int size)

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 11/19] remoteproc: core: Add function to get resource table spare bytes information

[loic pallardy]

On 09/08/2016 10:32 AM, Lee Jones wrote:
> On Wed, 31 Aug 2016, Loic Pallardy wrote:
>
>> Remoteproc needs to know if a resource table has spare resource
>> allowing resoure table extension.
>> This function parse resource table to detect spare resource.
>> If any, it returns available spare bytes and index of spare resource.
>>
>> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
>> ---
>>  drivers/remoteproc/remoteproc_core.c | 29 +++++++++++++++++++++++++++++
>>  1 file changed, 29 insertions(+)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>> index ffb56c0..30e9c70 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -951,6 +951,35 @@ int rproc_request_resource(struct rproc *rproc, u32 type, u32 action, void *reso
>>  }
>>  EXPORT_SYMBOL(rproc_request_resource);
>>
>> +static int __get_rsc_tbl_spare_size(struct rproc *rproc,
>
> size_t?
ok
>
>> +					  struct resource_table *table_ptr, int len,
>> +					  int *spare_index)
>> +{
>> +	struct device *dev = &rproc->dev;
>> +	int i;
>> +
>> +	for (i = 0; i < table_ptr->num; i++) {
>> +		int offset = table_ptr->offset[i];
>> +		struct fw_rsc_hdr *hdr = (void *)table_ptr + offset;
>> +		struct fw_rsc_spare *spare = (void *)hdr + sizeof(*hdr);
>> +
>> +		dev_dbg(dev, "rsc: type %d\n", hdr->type);
>
> This doesn't need to be in upstream code.
Sure
>
>> +		if (hdr->type >= RSC_LAST) {
>> +			dev_warn(dev, "unsupported resource %d\n", hdr->type);
>> +			continue;
>> +		}
>
> If we're only interested in RSC_SPARE, I think we can omit this check.
>
> No need to police the resource table everywhere.
True and complete table sanity check done before.
I'll remove
Thanks,
Loic

>
>> +		if (hdr->type != RSC_SPARE)
>> +			continue;
>> +
>> +		*spare_index = i;
>> +		return spare->len;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  static int __verify_rsc_tbl_entry(struct rproc *rproc,
>>  				struct rproc_request_resource *request,
>>  				struct resource_table *table, int size)
>

Re: [PATCH v2 11/19] remoteproc: core: Add function to get resource table spare bytes information

[loic pallardy]

On 09/08/2016 10:32 AM, Lee Jones wrote:
> On Wed, 31 Aug 2016, Loic Pallardy wrote:
>
>> Remoteproc needs to know if a resource table has spare resource
>> allowing resoure table extension.
>> This function parse resource table to detect spare resource.
>> If any, it returns available spare bytes and index of spare resource.
>>
>> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
>> ---
>>  drivers/remoteproc/remoteproc_core.c | 29 +++++++++++++++++++++++++++++
>>  1 file changed, 29 insertions(+)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>> index ffb56c0..30e9c70 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -951,6 +951,35 @@ int rproc_request_resource(struct rproc *rproc, u32 type, u32 action, void *reso
>>  }
>>  EXPORT_SYMBOL(rproc_request_resource);
>>
>> +static int __get_rsc_tbl_spare_size(struct rproc *rproc,
>
> size_t?
ok
>
>> +					  struct resource_table *table_ptr, int len,
>> +					  int *spare_index)
>> +{
>> +	struct device *dev = &rproc->dev;
>> +	int i;
>> +
>> +	for (i = 0; i < table_ptr->num; i++) {
>> +		int offset = table_ptr->offset[i];
>> +		struct fw_rsc_hdr *hdr = (void *)table_ptr + offset;
>> +		struct fw_rsc_spare *spare = (void *)hdr + sizeof(*hdr);
>> +
>> +		dev_dbg(dev, "rsc: type %d\n", hdr->type);
>
> This doesn't need to be in upstream code.
Sure
>
>> +		if (hdr->type >= RSC_LAST) {
>> +			dev_warn(dev, "unsupported resource %d\n", hdr->type);
>> +			continue;
>> +		}
>
> If we're only interested in RSC_SPARE, I think we can omit this check.
>
> No need to police the resource table everywhere.
True and complete table sanity check done before.
I'll remove
Thanks,
Loic

>
>> +		if (hdr->type != RSC_SPARE)
>> +			continue;
>> +
>> +		*spare_index = i;
>> +		return spare->len;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  static int __verify_rsc_tbl_entry(struct rproc *rproc,
>>  				struct rproc_request_resource *request,
>>  				struct resource_table *table, int size)
>

[PATCH v2 12/19] remoteproc: core: Add vdev support and force mode to resource amending function

[Loic Pallardy]

This patch proposes diverse updates to rproc_update_resource_table_entry
function:
- rename rproc_update_resource_table_entry to __update_rsc_tbl_entry to
  have shorter function name.
- add RSC_VDEV support
- add force mode resource even if resource already fixed on firmware side.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 30e9c70..aff1a00 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1027,13 +1027,15 @@ static int __verify_rsc_tbl_entry(struct rproc *rproc,
 	return -EINVAL;
 }
 
-static int rproc_update_resource_table_entry(struct rproc *rproc,
+static int __update_rsc_tbl_entry(struct rproc *rproc,
 				struct rproc_request_resource *request,
-				struct resource_table *table, int size)
+				struct resource_table *table, int size,
+				bool force)
 {
 	struct fw_rsc_carveout *tblc, *newc;
 	struct fw_rsc_devmem *tbld, *newd;
 	struct fw_rsc_trace *tblt, *newt;
+	struct fw_rsc_vdev *tblv, *newv;
 	int updated = true;
 	int i;
 
@@ -1054,7 +1056,8 @@ static int rproc_update_resource_table_entry(struct rproc *rproc,
 				    sizeof(*tblc->name)))
 				break;
 
-			memcpy(tblc, newc, request->size);
+			if (tblc->pa == FW_RSC_ADDR_ANY || force)
+				memcpy(tblc, newc, request->size);
 
 			return updated;
 		case RSC_DEVMEM:
@@ -1079,6 +1082,20 @@ static int rproc_update_resource_table_entry(struct rproc *rproc,
 			memcpy(tblt, newt, request->size);
 
 			return updated;
+		case RSC_VDEV:
+			tblv = rsc;
+			newv = request->resource;
+			if (newv->id != tblv->id)
+				break;
+
+			if (request->size > (sizeof(*tblv) +
+				    tblv->num_of_vrings * sizeof(struct fw_rsc_vdev_vring) +
+				    tblv->config_len))
+				return -ENOSPC;
+
+			memcpy(tblv, newv, request->size);
+
+			return updated;
 		default:
 			dev_err(&rproc->dev,
 				"Unsupported resource type: %d\n",
@@ -1176,8 +1193,8 @@ rproc_apply_resource_overrides(struct rproc *rproc,
 		int updated = 0;
 
 		/* If we already have a table, update it with the new values. */
-		updated = rproc_update_resource_table_entry(rproc, resource,
-							    table, size);
+		updated = __update_rsc_tbl_entry(rproc, resource, table, size,
+						 false);
 		if (updated < 0) {
 			table = ERR_PTR(updated);
 			goto out;
-- 
1.9.1

[PATCH v2 12/19] remoteproc: core: Add vdev support and force mode to resource amending function

[Loic Pallardy]

This patch proposes diverse updates to rproc_update_resource_table_entry
function:
- rename rproc_update_resource_table_entry to __update_rsc_tbl_entry to
  have shorter function name.
- add RSC_VDEV support
- add force mode resource even if resource already fixed on firmware side.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 30e9c70..aff1a00 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1027,13 +1027,15 @@ static int __verify_rsc_tbl_entry(struct rproc *rproc,
 	return -EINVAL;
 }
 
-static int rproc_update_resource_table_entry(struct rproc *rproc,
+static int __update_rsc_tbl_entry(struct rproc *rproc,
 				struct rproc_request_resource *request,
-				struct resource_table *table, int size)
+				struct resource_table *table, int size,
+				bool force)
 {
 	struct fw_rsc_carveout *tblc, *newc;
 	struct fw_rsc_devmem *tbld, *newd;
 	struct fw_rsc_trace *tblt, *newt;
+	struct fw_rsc_vdev *tblv, *newv;
 	int updated = true;
 	int i;
 
@@ -1054,7 +1056,8 @@ static int rproc_update_resource_table_entry(struct rproc *rproc,
 				    sizeof(*tblc->name)))
 				break;
 
-			memcpy(tblc, newc, request->size);
+			if (tblc->pa == FW_RSC_ADDR_ANY || force)
+				memcpy(tblc, newc, request->size);
 
 			return updated;
 		case RSC_DEVMEM:
@@ -1079,6 +1082,20 @@ static int rproc_update_resource_table_entry(struct rproc *rproc,
 			memcpy(tblt, newt, request->size);
 
 			return updated;
+		case RSC_VDEV:
+			tblv = rsc;
+			newv = request->resource;
+			if (newv->id != tblv->id)
+				break;
+
+			if (request->size > (sizeof(*tblv) +
+				    tblv->num_of_vrings * sizeof(struct fw_rsc_vdev_vring) +
+				    tblv->config_len))
+				return -ENOSPC;
+
+			memcpy(tblv, newv, request->size);
+
+			return updated;
 		default:
 			dev_err(&rproc->dev,
 				"Unsupported resource type: %d\n",
@@ -1176,8 +1193,8 @@ rproc_apply_resource_overrides(struct rproc *rproc,
 		int updated = 0;
 
 		/* If we already have a table, update it with the new values. */
-		updated = rproc_update_resource_table_entry(rproc, resource,
-							    table, size);
+		updated = __update_rsc_tbl_entry(rproc, resource, table, size,
+						 false);
 		if (updated < 0) {
 			table = ERR_PTR(updated);
 			goto out;
-- 
1.9.1

Re: [PATCH v2 12/19] remoteproc: core: Add vdev support and force mode to resource amending function

[Lee Jones]

On Wed, 31 Aug 2016, Loic Pallardy wrote:

> This patch proposes diverse updates to rproc_update_resource_table_entry
> function:
> - rename rproc_update_resource_table_entry to __update_rsc_tbl_entry to
>   have shorter function name.
> - add RSC_VDEV support
> - add force mode resource even if resource already fixed on firmware side.
> 
> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> ---
>  drivers/remoteproc/remoteproc_core.c | 27 ++++++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 30e9c70..aff1a00 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1027,13 +1027,15 @@ static int __verify_rsc_tbl_entry(struct rproc *rproc,
>  	return -EINVAL;
>  }
>  
> -static int rproc_update_resource_table_entry(struct rproc *rproc,
> +static int __update_rsc_tbl_entry(struct rproc *rproc,

Unless the name is unruly, (which I don't think it is, you're still
having to line wrap at the call site), I tend to go for clarity over
brevity.

>  				struct rproc_request_resource *request,
> -				struct resource_table *table, int size)
> +				struct resource_table *table, int size,
> +				bool force)
>  {
>  	struct fw_rsc_carveout *tblc, *newc;
>  	struct fw_rsc_devmem *tbld, *newd;
>  	struct fw_rsc_trace *tblt, *newt;
> +	struct fw_rsc_vdev *tblv, *newv;
>  	int updated = true;
>  	int i;
>  
> @@ -1054,7 +1056,8 @@ static int rproc_update_resource_table_entry(struct rproc *rproc,
>  				    sizeof(*tblc->name)))
>  				break;
>  
> -			memcpy(tblc, newc, request->size);
> +			if (tblc->pa == FW_RSC_ADDR_ANY || force)
> +				memcpy(tblc, newc, request->size);
>  
>  			return updated;
>  		case RSC_DEVMEM:
> @@ -1079,6 +1082,20 @@ static int rproc_update_resource_table_entry(struct rproc *rproc,
>  			memcpy(tblt, newt, request->size);
>  
>  			return updated;
> +		case RSC_VDEV:
> +			tblv = rsc;
> +			newv = request->resource;
> +			if (newv->id != tblv->id)
> +				break;
> +
> +			if (request->size > (sizeof(*tblv) +
> +				    tblv->num_of_vrings * sizeof(struct fw_rsc_vdev_vring) +
> +				    tblv->config_len))
> +				return -ENOSPC;
> +
> +			memcpy(tblv, newv, request->size);
> +
> +			return updated;

Again, there is more than one functional change in this patch.  You're
(unnecessarily IMO) renaming things, adding a force argument and
supplying support for a new type of device, all in one patch.

If any one of those functional changes has to be reverted, the
Maintainer will have no choice but to either revert the whole thing,
or someone will have to physically write an anti-patch, which is more
time consuming.

>  		default:
>  			dev_err(&rproc->dev,
>  				"Unsupported resource type: %d\n",
> @@ -1176,8 +1193,8 @@ rproc_apply_resource_overrides(struct rproc *rproc,
>  		int updated = 0;
>  
>  		/* If we already have a table, update it with the new values. */
> -		updated = rproc_update_resource_table_entry(rproc, resource,
> -							    table, size);
> +		updated = __update_rsc_tbl_entry(rproc, resource, table, size,
> +						 false);
>  		if (updated < 0) {
>  			table = ERR_PTR(updated);
>  			goto out;

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 12/19] remoteproc: core: Add vdev support and force mode to resource amending function

[loic pallardy]

On 09/08/2016 10:48 AM, Lee Jones wrote:
> On Wed, 31 Aug 2016, Loic Pallardy wrote:
>
>> This patch proposes diverse updates to rproc_update_resource_table_entry
>> function:
>> - rename rproc_update_resource_table_entry to __update_rsc_tbl_entry to
>>   have shorter function name.
>> - add RSC_VDEV support
>> - add force mode resource even if resource already fixed on firmware side.
>>
>> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
>> ---
>>  drivers/remoteproc/remoteproc_core.c | 27 ++++++++++++++++++++++-----
>>  1 file changed, 22 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>> index 30e9c70..aff1a00 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -1027,13 +1027,15 @@ static int __verify_rsc_tbl_entry(struct rproc *rproc,
>>  	return -EINVAL;
>>  }
>>
>> -static int rproc_update_resource_table_entry(struct rproc *rproc,
>> +static int __update_rsc_tbl_entry(struct rproc *rproc,
>
> Unless the name is unruly, (which I don't think it is, you're still
> having to line wrap at the call site), I tend to go for clarity over
> brevity.
It was only to have reasonable line length. I can keept original name 
and see impact on rest of the code.

>
>>  				struct rproc_request_resource *request,
>> -				struct resource_table *table, int size)
>> +				struct resource_table *table, int size,
>> +				bool force)
>>  {
>>  	struct fw_rsc_carveout *tblc, *newc;
>>  	struct fw_rsc_devmem *tbld, *newd;
>>  	struct fw_rsc_trace *tblt, *newt;
>> +	struct fw_rsc_vdev *tblv, *newv;
>>  	int updated = true;
>>  	int i;
>>
>> @@ -1054,7 +1056,8 @@ static int rproc_update_resource_table_entry(struct rproc *rproc,
>>  				    sizeof(*tblc->name)))
>>  				break;
>>
>> -			memcpy(tblc, newc, request->size);
>> +			if (tblc->pa == FW_RSC_ADDR_ANY || force)
>> +				memcpy(tblc, newc, request->size);
>>
>>  			return updated;
>>  		case RSC_DEVMEM:
>> @@ -1079,6 +1082,20 @@ static int rproc_update_resource_table_entry(struct rproc *rproc,
>>  			memcpy(tblt, newt, request->size);
>>
>>  			return updated;
>> +		case RSC_VDEV:
>> +			tblv = rsc;
>> +			newv = request->resource;
>> +			if (newv->id != tblv->id)
>> +				break;
>> +
>> +			if (request->size > (sizeof(*tblv) +
>> +				    tblv->num_of_vrings * sizeof(struct fw_rsc_vdev_vring) +
>> +				    tblv->config_len))
>> +				return -ENOSPC;
>> +
>> +			memcpy(tblv, newv, request->size);
>> +
>> +			return updated;
>
> Again, there is more than one functional change in this patch.  You're
> (unnecessarily IMO) renaming things, adding a force argument and
> supplying support for a new type of device, all in one patch.
>
> If any one of those functional changes has to be reverted, the
> Maintainer will have no choice but to either revert the whole thing,
> or someone will have to physically write an anti-patch, which is more
> time consuming.

Ok I'll split feature by feature

Thanks,
Loic
>
>>  		default:
>>  			dev_err(&rproc->dev,
>>  				"Unsupported resource type: %d\n",
>> @@ -1176,8 +1193,8 @@ rproc_apply_resource_overrides(struct rproc *rproc,
>>  		int updated = 0;
>>
>>  		/* If we already have a table, update it with the new values. */
>> -		updated = rproc_update_resource_table_entry(rproc, resource,
>> -							    table, size);
>> +		updated = __update_rsc_tbl_entry(rproc, resource, table, size,
>> +						 false);
>>  		if (updated < 0) {
>>  			table = ERR_PTR(updated);
>>  			goto out;
>

Re: [PATCH v2 12/19] remoteproc: core: Add vdev support and force mode to resource amending function

[loic pallardy]

On 09/08/2016 10:48 AM, Lee Jones wrote:
> On Wed, 31 Aug 2016, Loic Pallardy wrote:
>
>> This patch proposes diverse updates to rproc_update_resource_table_entry
>> function:
>> - rename rproc_update_resource_table_entry to __update_rsc_tbl_entry to
>>   have shorter function name.
>> - add RSC_VDEV support
>> - add force mode resource even if resource already fixed on firmware side.
>>
>> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
>> ---
>>  drivers/remoteproc/remoteproc_core.c | 27 ++++++++++++++++++++++-----
>>  1 file changed, 22 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>> index 30e9c70..aff1a00 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -1027,13 +1027,15 @@ static int __verify_rsc_tbl_entry(struct rproc *rproc,
>>  	return -EINVAL;
>>  }
>>
>> -static int rproc_update_resource_table_entry(struct rproc *rproc,
>> +static int __update_rsc_tbl_entry(struct rproc *rproc,
>
> Unless the name is unruly, (which I don't think it is, you're still
> having to line wrap at the call site), I tend to go for clarity over
> brevity.
It was only to have reasonable line length. I can keept original name 
and see impact on rest of the code.

>
>>  				struct rproc_request_resource *request,
>> -				struct resource_table *table, int size)
>> +				struct resource_table *table, int size,
>> +				bool force)
>>  {
>>  	struct fw_rsc_carveout *tblc, *newc;
>>  	struct fw_rsc_devmem *tbld, *newd;
>>  	struct fw_rsc_trace *tblt, *newt;
>> +	struct fw_rsc_vdev *tblv, *newv;
>>  	int updated = true;
>>  	int i;
>>
>> @@ -1054,7 +1056,8 @@ static int rproc_update_resource_table_entry(struct rproc *rproc,
>>  				    sizeof(*tblc->name)))
>>  				break;
>>
>> -			memcpy(tblc, newc, request->size);
>> +			if (tblc->pa == FW_RSC_ADDR_ANY || force)
>> +				memcpy(tblc, newc, request->size);
>>
>>  			return updated;
>>  		case RSC_DEVMEM:
>> @@ -1079,6 +1082,20 @@ static int rproc_update_resource_table_entry(struct rproc *rproc,
>>  			memcpy(tblt, newt, request->size);
>>
>>  			return updated;
>> +		case RSC_VDEV:
>> +			tblv = rsc;
>> +			newv = request->resource;
>> +			if (newv->id != tblv->id)
>> +				break;
>> +
>> +			if (request->size > (sizeof(*tblv) +
>> +				    tblv->num_of_vrings * sizeof(struct fw_rsc_vdev_vring) +
>> +				    tblv->config_len))
>> +				return -ENOSPC;
>> +
>> +			memcpy(tblv, newv, request->size);
>> +
>> +			return updated;
>
> Again, there is more than one functional change in this patch.  You're
> (unnecessarily IMO) renaming things, adding a force argument and
> supplying support for a new type of device, all in one patch.
>
> If any one of those functional changes has to be reverted, the
> Maintainer will have no choice but to either revert the whole thing,
> or someone will have to physically write an anti-patch, which is more
> time consuming.

Ok I'll split feature by feature

Thanks,
Loic
>
>>  		default:
>>  			dev_err(&rproc->dev,
>>  				"Unsupported resource type: %d\n",
>> @@ -1176,8 +1193,8 @@ rproc_apply_resource_overrides(struct rproc *rproc,
>>  		int updated = 0;
>>
>>  		/* If we already have a table, update it with the new values. */
>> -		updated = rproc_update_resource_table_entry(rproc, resource,
>> -							    table, size);
>> +		updated = __update_rsc_tbl_entry(rproc, resource, table, size,
>> +						 false);
>>  		if (updated < 0) {
>>  			table = ERR_PTR(updated);
>>  			goto out;
>

Re: [PATCH v2 12/19] remoteproc: core: Add vdev support and force mode to resource amending function

[Lee Jones]

On Thu, 08 Sep 2016, loic pallardy wrote:

> 
> 
> On 09/08/2016 10:48 AM, Lee Jones wrote:
> > On Wed, 31 Aug 2016, Loic Pallardy wrote:
> > 
> > > This patch proposes diverse updates to rproc_update_resource_table_entry
> > > function:
> > > - rename rproc_update_resource_table_entry to __update_rsc_tbl_entry to
> > >   have shorter function name.
> > > - add RSC_VDEV support
> > > - add force mode resource even if resource already fixed on firmware side.
> > > 
> > > Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> > > ---
> > >  drivers/remoteproc/remoteproc_core.c | 27 ++++++++++++++++++++++-----
> > >  1 file changed, 22 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > > index 30e9c70..aff1a00 100644
> > > --- a/drivers/remoteproc/remoteproc_core.c
> > > +++ b/drivers/remoteproc/remoteproc_core.c
> > > @@ -1027,13 +1027,15 @@ static int __verify_rsc_tbl_entry(struct rproc *rproc,
> > >  	return -EINVAL;
> > >  }
> > > 
> > > -static int rproc_update_resource_table_entry(struct rproc *rproc,
> > > +static int __update_rsc_tbl_entry(struct rproc *rproc,
> > 
> > Unless the name is unruly, (which I don't think it is, you're still
> > having to line wrap at the call site), I tend to go for clarity over
> > brevity.
> It was only to have reasonable line length. I can keept original name and
> see impact on rest of the code.

Reasonable line length is 80.  This line was 65.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 12/19] remoteproc: core: Add vdev support and force mode to resource amending function

[loic pallardy]

On 09/08/2016 01:02 PM, Lee Jones wrote:
> On Thu, 08 Sep 2016, loic pallardy wrote:
>
>>
>>
>> On 09/08/2016 10:48 AM, Lee Jones wrote:
>>> On Wed, 31 Aug 2016, Loic Pallardy wrote:
>>>
>>>> This patch proposes diverse updates to rproc_update_resource_table_entry
>>>> function:
>>>> - rename rproc_update_resource_table_entry to __update_rsc_tbl_entry to
>>>>   have shorter function name.
>>>> - add RSC_VDEV support
>>>> - add force mode resource even if resource already fixed on firmware side.
>>>>
>>>> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
>>>> ---
>>>>  drivers/remoteproc/remoteproc_core.c | 27 ++++++++++++++++++++++-----
>>>>  1 file changed, 22 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>>>> index 30e9c70..aff1a00 100644
>>>> --- a/drivers/remoteproc/remoteproc_core.c
>>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>>> @@ -1027,13 +1027,15 @@ static int __verify_rsc_tbl_entry(struct rproc *rproc,
>>>>  	return -EINVAL;
>>>>  }
>>>>
>>>> -static int rproc_update_resource_table_entry(struct rproc *rproc,
>>>> +static int __update_rsc_tbl_entry(struct rproc *rproc,
>>>
>>> Unless the name is unruly, (which I don't think it is, you're still
>>> having to line wrap at the call site), I tend to go for clarity over
>>> brevity.
>> It was only to have reasonable line length. I can keept original name and
>> see impact on rest of the code.
>
> Reasonable line length is 80.  This line was 65.
This line yes. Name change proposal is to reduce line where this 
function is called (in rproc_apply_resource_overrides for exemple)
But OK to keep standard rproc_xxx naming
>

Re: [PATCH v2 12/19] remoteproc: core: Add vdev support and force mode to resource amending function

[loic pallardy]

On 09/08/2016 01:02 PM, Lee Jones wrote:
> On Thu, 08 Sep 2016, loic pallardy wrote:
>
>>
>>
>> On 09/08/2016 10:48 AM, Lee Jones wrote:
>>> On Wed, 31 Aug 2016, Loic Pallardy wrote:
>>>
>>>> This patch proposes diverse updates to rproc_update_resource_table_entry
>>>> function:
>>>> - rename rproc_update_resource_table_entry to __update_rsc_tbl_entry to
>>>>   have shorter function name.
>>>> - add RSC_VDEV support
>>>> - add force mode resource even if resource already fixed on firmware side.
>>>>
>>>> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
>>>> ---
>>>>  drivers/remoteproc/remoteproc_core.c | 27 ++++++++++++++++++++++-----
>>>>  1 file changed, 22 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>>>> index 30e9c70..aff1a00 100644
>>>> --- a/drivers/remoteproc/remoteproc_core.c
>>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>>> @@ -1027,13 +1027,15 @@ static int __verify_rsc_tbl_entry(struct rproc *rproc,
>>>>  	return -EINVAL;
>>>>  }
>>>>
>>>> -static int rproc_update_resource_table_entry(struct rproc *rproc,
>>>> +static int __update_rsc_tbl_entry(struct rproc *rproc,
>>>
>>> Unless the name is unruly, (which I don't think it is, you're still
>>> having to line wrap at the call site), I tend to go for clarity over
>>> brevity.
>> It was only to have reasonable line length. I can keept original name and
>> see impact on rest of the code.
>
> Reasonable line length is 80.  This line was 65.
This line yes. Name change proposal is to reduce line where this 
function is called (in rproc_apply_resource_overrides for exemple)
But OK to keep standard rproc_xxx naming
>

[PATCH v2 13/19] remoteproc: core: Append resource only if spare resource present

[Loic Pallardy]

This patch renames rproc_add_resource_table_entry in __add_rsc_tbl_entry
to have shorter function name and adds spare resource support.
To guarantee remoteproc won't overwrite firmware data when copying
back modified resource table, __add_rsc_tbl_entry verifies first that
resource table owns a spare resource and uses spare bytes to create
a new resource entry. Spare resource is updated according to changes.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 54 ++++++++++++++++++++----------------
 1 file changed, 30 insertions(+), 24 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index aff1a00..25a429b 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1107,39 +1107,34 @@ static int __update_rsc_tbl_entry(struct rproc *rproc,
 	return !updated;
 }
 
-static struct resource_table*
-rproc_add_resource_table_entry(struct rproc *rproc,
+static int __add_rsc_tbl_entry(struct rproc *rproc,
 			       struct rproc_request_resource *request,
-			       struct resource_table *old_table, int *tablesz)
+			       struct resource_table *table, int tablesz)
 {
-	struct resource_table *table;
 	struct fw_rsc_hdr h;
+	struct fw_rsc_spare spare;
 	void *new_rsc_loc;
 	void *fw_header_loc;
 	void *start_of_rscs;
 	int new_rsc_offset;
-	int size = *tablesz;
-	int i;
+	int new_spare_offset;
+	int i, spare_index = 0;
 
 	h.type = request->type;
 
-	new_rsc_offset = size;
+	/* check available spare size */
+	spare.len = __get_rsc_tbl_spare_size(rproc, table, tablesz, &spare_index);
+	if (spare.len < (sizeof(h) + request->size + 4)) /* new offset entry */
+		return -EPERM;
 
-	/*
-	 * Allocate another contiguous chunk of memory, large enough to
-	 * contain the new, expanded resource table.
-	 *
-	 * The +4 is for the extra offset[] element in the top level header
-	 */
-	size += sizeof(struct fw_rsc_hdr) + request->size + 4;
-	table = devm_kmemdup(&rproc->dev, old_table, size, GFP_KERNEL);
-	if (!table)
-		return ERR_PTR(-ENOMEM);
+	new_rsc_offset = table->offset[spare_index];
 
 	/* Shunt table by 4 Bytes to account for the extra offset[] element */
 	start_of_rscs = (void *)table + table->offset[0];
 	memmove(start_of_rscs + 4,
 		start_of_rscs, new_rsc_offset - table->offset[0]);
+
+	spare.len -= 4;
 	new_rsc_offset += 4;
 
 	/* Update existing resource entry's offsets */
@@ -1153,13 +1148,27 @@ rproc_add_resource_table_entry(struct rproc *rproc,
 	/* Copy new firmware header into table */
 	fw_header_loc = (void *)table + new_rsc_offset;
 	memcpy(fw_header_loc, &h, sizeof(h));
+	spare.len -= sizeof(h);
 
 	/* Copy new resource entry into table */
 	new_rsc_loc = (void *)fw_header_loc + sizeof(h);
 	memcpy(new_rsc_loc, request->resource, request->size);
+	spare.len -= request->size;
 
-	*tablesz = size;
-	return table;
+	/* create new rsc spare resource at the end of remaining spare */
+	new_spare_offset = new_rsc_offset + sizeof(h) + request->size;
+	h.type = RSC_SPARE;
+
+	fw_header_loc = (void *)table + new_spare_offset;
+	memcpy(fw_header_loc, &h, sizeof(h));
+
+	new_rsc_loc = (void *)fw_header_loc + sizeof(h);
+	memcpy(new_rsc_loc, &spare, sizeof(spare));
+
+	/* update spare offset */
+	table->offset[spare_index] = new_spare_offset;
+
+	return 0;
 }
 
 static struct resource_table*
@@ -1203,12 +1212,9 @@ rproc_apply_resource_overrides(struct rproc *rproc,
 			continue;
 
 		/* Didn't find matching resource entry -- creating a new one. */
-		table = rproc_add_resource_table_entry(rproc, resource,
-						       table, &size);
-		if (IS_ERR(table))
+		updated = __add_rsc_tbl_entry(rproc, resource, table, size);
+		if (updated)
 			goto out;
-
-		*orig_table = table;
 	}
 
 	rproc_dump_resource_table(rproc, table, size);
-- 
1.9.1

[PATCH v2 13/19] remoteproc: core: Append resource only if spare resource present

[Loic Pallardy]

This patch renames rproc_add_resource_table_entry in __add_rsc_tbl_entry
to have shorter function name and adds spare resource support.
To guarantee remoteproc won't overwrite firmware data when copying
back modified resource table, __add_rsc_tbl_entry verifies first that
resource table owns a spare resource and uses spare bytes to create
a new resource entry. Spare resource is updated according to changes.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 54 ++++++++++++++++++++----------------
 1 file changed, 30 insertions(+), 24 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index aff1a00..25a429b 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1107,39 +1107,34 @@ static int __update_rsc_tbl_entry(struct rproc *rproc,
 	return !updated;
 }
 
-static struct resource_table*
-rproc_add_resource_table_entry(struct rproc *rproc,
+static int __add_rsc_tbl_entry(struct rproc *rproc,
 			       struct rproc_request_resource *request,
-			       struct resource_table *old_table, int *tablesz)
+			       struct resource_table *table, int tablesz)
 {
-	struct resource_table *table;
 	struct fw_rsc_hdr h;
+	struct fw_rsc_spare spare;
 	void *new_rsc_loc;
 	void *fw_header_loc;
 	void *start_of_rscs;
 	int new_rsc_offset;
-	int size = *tablesz;
-	int i;
+	int new_spare_offset;
+	int i, spare_index = 0;
 
 	h.type = request->type;
 
-	new_rsc_offset = size;
+	/* check available spare size */
+	spare.len = __get_rsc_tbl_spare_size(rproc, table, tablesz, &spare_index);
+	if (spare.len < (sizeof(h) + request->size + 4)) /* new offset entry */
+		return -EPERM;
 
-	/*
-	 * Allocate another contiguous chunk of memory, large enough to
-	 * contain the new, expanded resource table.
-	 *
-	 * The +4 is for the extra offset[] element in the top level header
-	 */
-	size += sizeof(struct fw_rsc_hdr) + request->size + 4;
-	table = devm_kmemdup(&rproc->dev, old_table, size, GFP_KERNEL);
-	if (!table)
-		return ERR_PTR(-ENOMEM);
+	new_rsc_offset = table->offset[spare_index];
 
 	/* Shunt table by 4 Bytes to account for the extra offset[] element */
 	start_of_rscs = (void *)table + table->offset[0];
 	memmove(start_of_rscs + 4,
 		start_of_rscs, new_rsc_offset - table->offset[0]);
+
+	spare.len -= 4;
 	new_rsc_offset += 4;
 
 	/* Update existing resource entry's offsets */
@@ -1153,13 +1148,27 @@ rproc_add_resource_table_entry(struct rproc *rproc,
 	/* Copy new firmware header into table */
 	fw_header_loc = (void *)table + new_rsc_offset;
 	memcpy(fw_header_loc, &h, sizeof(h));
+	spare.len -= sizeof(h);
 
 	/* Copy new resource entry into table */
 	new_rsc_loc = (void *)fw_header_loc + sizeof(h);
 	memcpy(new_rsc_loc, request->resource, request->size);
+	spare.len -= request->size;
 
-	*tablesz = size;
-	return table;
+	/* create new rsc spare resource at the end of remaining spare */
+	new_spare_offset = new_rsc_offset + sizeof(h) + request->size;
+	h.type = RSC_SPARE;
+
+	fw_header_loc = (void *)table + new_spare_offset;
+	memcpy(fw_header_loc, &h, sizeof(h));
+
+	new_rsc_loc = (void *)fw_header_loc + sizeof(h);
+	memcpy(new_rsc_loc, &spare, sizeof(spare));
+
+	/* update spare offset */
+	table->offset[spare_index] = new_spare_offset;
+
+	return 0;
 }
 
 static struct resource_table*
@@ -1203,12 +1212,9 @@ rproc_apply_resource_overrides(struct rproc *rproc,
 			continue;
 
 		/* Didn't find matching resource entry -- creating a new one. */
-		table = rproc_add_resource_table_entry(rproc, resource,
-						       table, &size);
-		if (IS_ERR(table))
+		updated = __add_rsc_tbl_entry(rproc, resource, table, size);
+		if (updated)
 			goto out;
-
-		*orig_table = table;
 	}
 
 	rproc_dump_resource_table(rproc, table, size);
-- 
1.9.1

Re: [PATCH v2 13/19] remoteproc: core: Append resource only if spare resource present

[Lee Jones]

On Wed, 31 Aug 2016, Loic Pallardy wrote:

> This patch renames rproc_add_resource_table_entry in __add_rsc_tbl_entry
> to have shorter function name and adds spare resource support.
> To guarantee remoteproc won't overwrite firmware data when copying
> back modified resource table, __add_rsc_tbl_entry verifies first that
> resource table owns a spare resource and uses spare bytes to create
> a new resource entry. Spare resource is updated according to changes.
> 
> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> ---
>  drivers/remoteproc/remoteproc_core.c | 54 ++++++++++++++++++++----------------
>  1 file changed, 30 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index aff1a00..25a429b 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1107,39 +1107,34 @@ static int __update_rsc_tbl_entry(struct rproc *rproc,
>  	return !updated;
>  }
>  
> -static struct resource_table*
> -rproc_add_resource_table_entry(struct rproc *rproc,
> +static int __add_rsc_tbl_entry(struct rproc *rproc,

Once again, I prefer plain English over cryptic abbreviations.  Makes
things much more difficult for developers who are new to, or are just
dipping into RemoteProc code.

>  			       struct rproc_request_resource *request,
> -			       struct resource_table *old_table, int *tablesz)
> +			       struct resource_table *table, int tablesz)
>  {
> -	struct resource_table *table;
>  	struct fw_rsc_hdr h;
> +	struct fw_rsc_spare spare;
>  	void *new_rsc_loc;
>  	void *fw_header_loc;
>  	void *start_of_rscs;
>  	int new_rsc_offset;
> -	int size = *tablesz;
> -	int i;
> +	int new_spare_offset;
> +	int i, spare_index = 0;
>  
>  	h.type = request->type;
>  
> -	new_rsc_offset = size;
> +	/* check available spare size */

In keeping with the existing comments, please use correct grammar.

Capital letters to start and for names etc.

Much more professional IMO.

> +	spare.len = __get_rsc_tbl_spare_size(rproc, table, tablesz, &spare_index);
> +	if (spare.len < (sizeof(h) + request->size + 4)) /* new offset entry */

Not sure that comment makes the code any clearer?

All you're doing here is checking if we have enough space, right?

I think the 4 is a 'magic' number.  I'd either provide a comment (like
I did below), or define it.

> +		return -EPERM;

What does this have to do with permissions?

> -	/*
> -	 * Allocate another contiguous chunk of memory, large enough to
> -	 * contain the new, expanded resource table.
> -	 *
> -	 * The +4 is for the extra offset[] element in the top level header
> -	 */
> -	size += sizeof(struct fw_rsc_hdr) + request->size + 4;
> -	table = devm_kmemdup(&rproc->dev, old_table, size, GFP_KERNEL);
> -	if (!table)
> -		return ERR_PTR(-ENOMEM);
> +	new_rsc_offset = table->offset[spare_index];
>  
>  	/* Shunt table by 4 Bytes to account for the extra offset[] element */
>  	start_of_rscs = (void *)table + table->offset[0];
>  	memmove(start_of_rscs + 4,
>  		start_of_rscs, new_rsc_offset - table->offset[0]);
> +
> +	spare.len -= 4;

This probably deserves a comment too.

/*
 * The spare area is finite.  Since we are increasing the size of the
 * header and shunting the tables, we need to reduce the size of the
 * available 'spare' area by the shunt size.
 */

>  	new_rsc_offset += 4;
>  
>  	/* Update existing resource entry's offsets */
> @@ -1153,13 +1148,27 @@ rproc_add_resource_table_entry(struct rproc *rproc,
>  	/* Copy new firmware header into table */
>  	fw_header_loc = (void *)table + new_rsc_offset;
>  	memcpy(fw_header_loc, &h, sizeof(h));
> +	spare.len -= sizeof(h);
>  
>  	/* Copy new resource entry into table */
>  	new_rsc_loc = (void *)fw_header_loc + sizeof(h);
>  	memcpy(new_rsc_loc, request->resource, request->size);
> +	spare.len -= request->size;
>  
> -	*tablesz = size;
> -	return table;
> +	/* create new rsc spare resource at the end of remaining spare */

Same comment about using nice grammar in comments.

> +	new_spare_offset = new_rsc_offset + sizeof(h) + request->size;
> +	h.type = RSC_SPARE;
> +
> +	fw_header_loc = (void *)table + new_spare_offset;
> +	memcpy(fw_header_loc, &h, sizeof(h));
> +
> +	new_rsc_loc = (void *)fw_header_loc + sizeof(h);
> +	memcpy(new_rsc_loc, &spare, sizeof(spare));
> +
> +	/* update spare offset */
> +	table->offset[spare_index] = new_spare_offset;
> +
> +	return 0;
>  }
>  
>  static struct resource_table*
> @@ -1203,12 +1212,9 @@ rproc_apply_resource_overrides(struct rproc *rproc,
>  			continue;
>  
>  		/* Didn't find matching resource entry -- creating a new one. */
> -		table = rproc_add_resource_table_entry(rproc, resource,
> -						       table, &size);
> -		if (IS_ERR(table))
> +		updated = __add_rsc_tbl_entry(rproc, resource, table, size);
> +		if (updated)
>  			goto out;
> -
> -		*orig_table = table;
>  	}
>  
>  	rproc_dump_resource_table(rproc, table, size);

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 13/19] remoteproc: core: Append resource only if spare resource present

[loic pallardy]

On 09/08/2016 11:33 AM, Lee Jones wrote:
> On Wed, 31 Aug 2016, Loic Pallardy wrote:
>
>> This patch renames rproc_add_resource_table_entry in __add_rsc_tbl_entry
>> to have shorter function name and adds spare resource support.
>> To guarantee remoteproc won't overwrite firmware data when copying
>> back modified resource table, __add_rsc_tbl_entry verifies first that
>> resource table owns a spare resource and uses spare bytes to create
>> a new resource entry. Spare resource is updated according to changes.
>>
>> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
>> ---
>>  drivers/remoteproc/remoteproc_core.c | 54 ++++++++++++++++++++----------------
>>  1 file changed, 30 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>> index aff1a00..25a429b 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -1107,39 +1107,34 @@ static int __update_rsc_tbl_entry(struct rproc *rproc,
>>  	return !updated;
>>  }
>>
>> -static struct resource_table*
>> -rproc_add_resource_table_entry(struct rproc *rproc,
>> +static int __add_rsc_tbl_entry(struct rproc *rproc,
>
> Once again, I prefer plain English over cryptic abbreviations.  Makes
> things much more difficult for developers who are new to, or are just
> dipping into RemoteProc code.

OK
>
>>  			       struct rproc_request_resource *request,
>> -			       struct resource_table *old_table, int *tablesz)
>> +			       struct resource_table *table, int tablesz)
>>  {
>> -	struct resource_table *table;
>>  	struct fw_rsc_hdr h;
>> +	struct fw_rsc_spare spare;
>>  	void *new_rsc_loc;
>>  	void *fw_header_loc;
>>  	void *start_of_rscs;
>>  	int new_rsc_offset;
>> -	int size = *tablesz;
>> -	int i;
>> +	int new_spare_offset;
>> +	int i, spare_index = 0;
>>
>>  	h.type = request->type;
>>
>> -	new_rsc_offset = size;
>> +	/* check available spare size */
>
> In keeping with the existing comments, please use correct grammar.
>
> Capital letters to start and for names etc.
>
> Much more professional IMO.

I'll correct
>
>> +	spare.len = __get_rsc_tbl_spare_size(rproc, table, tablesz, &spare_index);
>> +	if (spare.len < (sizeof(h) + request->size + 4)) /* new offset entry */
>
> Not sure that comment makes the code any clearer?
>
> All you're doing here is checking if we have enough space, right?
>
> I think the 4 is a 'magic' number.  I'd either provide a comment (like
> I did below), or define it.
I'll add a comment as you did to explain the "4".

>
>> +		return -EPERM;
>
> What does this have to do with permissions?
Yes sure, I'll replaced by -EINVAL as resource too large compare to 
spare area capability.

>
>> -	/*
>> -	 * Allocate another contiguous chunk of memory, large enough to
>> -	 * contain the new, expanded resource table.
>> -	 *
>> -	 * The +4 is for the extra offset[] element in the top level header
>> -	 */
>> -	size += sizeof(struct fw_rsc_hdr) + request->size + 4;
>> -	table = devm_kmemdup(&rproc->dev, old_table, size, GFP_KERNEL);
>> -	if (!table)
>> -		return ERR_PTR(-ENOMEM);
>> +	new_rsc_offset = table->offset[spare_index];
>>
>>  	/* Shunt table by 4 Bytes to account for the extra offset[] element */
>>  	start_of_rscs = (void *)table + table->offset[0];
>>  	memmove(start_of_rscs + 4,
>>  		start_of_rscs, new_rsc_offset - table->offset[0]);
>> +
>> +	spare.len -= 4;
>
> This probably deserves a comment too.
I'll add a comment too
>
> /*
>  * The spare area is finite.  Since we are increasing the size of the
>  * header and shunting the tables, we need to reduce the size of the
>  * available 'spare' area by the shunt size.
>  */
>
>>  	new_rsc_offset += 4;
>>
>>  	/* Update existing resource entry's offsets */
>> @@ -1153,13 +1148,27 @@ rproc_add_resource_table_entry(struct rproc *rproc,
>>  	/* Copy new firmware header into table */
>>  	fw_header_loc = (void *)table + new_rsc_offset;
>>  	memcpy(fw_header_loc, &h, sizeof(h));
>> +	spare.len -= sizeof(h);
>>
>>  	/* Copy new resource entry into table */
>>  	new_rsc_loc = (void *)fw_header_loc + sizeof(h);
>>  	memcpy(new_rsc_loc, request->resource, request->size);
>> +	spare.len -= request->size;
>>
>> -	*tablesz = size;
>> -	return table;
>> +	/* create new rsc spare resource at the end of remaining spare */
>
> Same comment about using nice grammar in comments.
Ok
Thanks,
Loic
>
>> +	new_spare_offset = new_rsc_offset + sizeof(h) + request->size;
>> +	h.type = RSC_SPARE;
>> +
>> +	fw_header_loc = (void *)table + new_spare_offset;
>> +	memcpy(fw_header_loc, &h, sizeof(h));
>> +
>> +	new_rsc_loc = (void *)fw_header_loc + sizeof(h);
>> +	memcpy(new_rsc_loc, &spare, sizeof(spare));
>> +
>> +	/* update spare offset */
>> +	table->offset[spare_index] = new_spare_offset;
>> +
>> +	return 0;
>>  }
>>
>>  static struct resource_table*
>> @@ -1203,12 +1212,9 @@ rproc_apply_resource_overrides(struct rproc *rproc,
>>  			continue;
>>
>>  		/* Didn't find matching resource entry -- creating a new one. */
>> -		table = rproc_add_resource_table_entry(rproc, resource,
>> -						       table, &size);
>> -		if (IS_ERR(table))
>> +		updated = __add_rsc_tbl_entry(rproc, resource, table, size);
>> +		if (updated)
>>  			goto out;
>> -
>> -		*orig_table = table;
>>  	}
>>
>>  	rproc_dump_resource_table(rproc, table, size);
>

Re: [PATCH v2 13/19] remoteproc: core: Append resource only if spare resource present

[loic pallardy]

On 09/08/2016 11:33 AM, Lee Jones wrote:
> On Wed, 31 Aug 2016, Loic Pallardy wrote:
>
>> This patch renames rproc_add_resource_table_entry in __add_rsc_tbl_entry
>> to have shorter function name and adds spare resource support.
>> To guarantee remoteproc won't overwrite firmware data when copying
>> back modified resource table, __add_rsc_tbl_entry verifies first that
>> resource table owns a spare resource and uses spare bytes to create
>> a new resource entry. Spare resource is updated according to changes.
>>
>> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
>> ---
>>  drivers/remoteproc/remoteproc_core.c | 54 ++++++++++++++++++++----------------
>>  1 file changed, 30 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>> index aff1a00..25a429b 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -1107,39 +1107,34 @@ static int __update_rsc_tbl_entry(struct rproc *rproc,
>>  	return !updated;
>>  }
>>
>> -static struct resource_table*
>> -rproc_add_resource_table_entry(struct rproc *rproc,
>> +static int __add_rsc_tbl_entry(struct rproc *rproc,
>
> Once again, I prefer plain English over cryptic abbreviations.  Makes
> things much more difficult for developers who are new to, or are just
> dipping into RemoteProc code.

OK
>
>>  			       struct rproc_request_resource *request,
>> -			       struct resource_table *old_table, int *tablesz)
>> +			       struct resource_table *table, int tablesz)
>>  {
>> -	struct resource_table *table;
>>  	struct fw_rsc_hdr h;
>> +	struct fw_rsc_spare spare;
>>  	void *new_rsc_loc;
>>  	void *fw_header_loc;
>>  	void *start_of_rscs;
>>  	int new_rsc_offset;
>> -	int size = *tablesz;
>> -	int i;
>> +	int new_spare_offset;
>> +	int i, spare_index = 0;
>>
>>  	h.type = request->type;
>>
>> -	new_rsc_offset = size;
>> +	/* check available spare size */
>
> In keeping with the existing comments, please use correct grammar.
>
> Capital letters to start and for names etc.
>
> Much more professional IMO.

I'll correct
>
>> +	spare.len = __get_rsc_tbl_spare_size(rproc, table, tablesz, &spare_index);
>> +	if (spare.len < (sizeof(h) + request->size + 4)) /* new offset entry */
>
> Not sure that comment makes the code any clearer?
>
> All you're doing here is checking if we have enough space, right?
>
> I think the 4 is a 'magic' number.  I'd either provide a comment (like
> I did below), or define it.
I'll add a comment as you did to explain the "4".

>
>> +		return -EPERM;
>
> What does this have to do with permissions?
Yes sure, I'll replaced by -EINVAL as resource too large compare to 
spare area capability.

>
>> -	/*
>> -	 * Allocate another contiguous chunk of memory, large enough to
>> -	 * contain the new, expanded resource table.
>> -	 *
>> -	 * The +4 is for the extra offset[] element in the top level header
>> -	 */
>> -	size += sizeof(struct fw_rsc_hdr) + request->size + 4;
>> -	table = devm_kmemdup(&rproc->dev, old_table, size, GFP_KERNEL);
>> -	if (!table)
>> -		return ERR_PTR(-ENOMEM);
>> +	new_rsc_offset = table->offset[spare_index];
>>
>>  	/* Shunt table by 4 Bytes to account for the extra offset[] element */
>>  	start_of_rscs = (void *)table + table->offset[0];
>>  	memmove(start_of_rscs + 4,
>>  		start_of_rscs, new_rsc_offset - table->offset[0]);
>> +
>> +	spare.len -= 4;
>
> This probably deserves a comment too.
I'll add a comment too
>
> /*
>  * The spare area is finite.  Since we are increasing the size of the
>  * header and shunting the tables, we need to reduce the size of the
>  * available 'spare' area by the shunt size.
>  */
>
>>  	new_rsc_offset += 4;
>>
>>  	/* Update existing resource entry's offsets */
>> @@ -1153,13 +1148,27 @@ rproc_add_resource_table_entry(struct rproc *rproc,
>>  	/* Copy new firmware header into table */
>>  	fw_header_loc = (void *)table + new_rsc_offset;
>>  	memcpy(fw_header_loc, &h, sizeof(h));
>> +	spare.len -= sizeof(h);
>>
>>  	/* Copy new resource entry into table */
>>  	new_rsc_loc = (void *)fw_header_loc + sizeof(h);
>>  	memcpy(new_rsc_loc, request->resource, request->size);
>> +	spare.len -= request->size;
>>
>> -	*tablesz = size;
>> -	return table;
>> +	/* create new rsc spare resource at the end of remaining spare */
>
> Same comment about using nice grammar in comments.
Ok
Thanks,
Loic
>
>> +	new_spare_offset = new_rsc_offset + sizeof(h) + request->size;
>> +	h.type = RSC_SPARE;
>> +
>> +	fw_header_loc = (void *)table + new_spare_offset;
>> +	memcpy(fw_header_loc, &h, sizeof(h));
>> +
>> +	new_rsc_loc = (void *)fw_header_loc + sizeof(h);
>> +	memcpy(new_rsc_loc, &spare, sizeof(spare));
>> +
>> +	/* update spare offset */
>> +	table->offset[spare_index] = new_spare_offset;
>> +
>> +	return 0;
>>  }
>>
>>  static struct resource_table*
>> @@ -1203,12 +1212,9 @@ rproc_apply_resource_overrides(struct rproc *rproc,
>>  			continue;
>>
>>  		/* Didn't find matching resource entry -- creating a new one. */
>> -		table = rproc_add_resource_table_entry(rproc, resource,
>> -						       table, &size);
>> -		if (IS_ERR(table))
>> +		updated = __add_rsc_tbl_entry(rproc, resource, table, size);
>> +		if (updated)
>>  			goto out;
>> -
>> -		*orig_table = table;
>>  	}
>>
>>  	rproc_dump_resource_table(rproc, table, size);
>

Re: [PATCH v2 13/19] remoteproc: core: Append resource only if spare resource present

[Lee Jones]

On Thu, 08 Sep 2016, loic pallardy wrote:

> 
> 
> On 09/08/2016 11:33 AM, Lee Jones wrote:
> > On Wed, 31 Aug 2016, Loic Pallardy wrote:
> > 
> > > This patch renames rproc_add_resource_table_entry in __add_rsc_tbl_entry
> > > to have shorter function name and adds spare resource support.
> > > To guarantee remoteproc won't overwrite firmware data when copying
> > > back modified resource table, __add_rsc_tbl_entry verifies first that
> > > resource table owns a spare resource and uses spare bytes to create
> > > a new resource entry. Spare resource is updated according to changes.
> > > 
> > > Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> > > ---
> > >  drivers/remoteproc/remoteproc_core.c | 54 ++++++++++++++++++++----------------
> > >  1 file changed, 30 insertions(+), 24 deletions(-)
> > > 
> > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > > index aff1a00..25a429b 100644
> > > --- a/drivers/remoteproc/remoteproc_core.c
> > > +++ b/drivers/remoteproc/remoteproc_core.c
> > > @@ -1107,39 +1107,34 @@ static int __update_rsc_tbl_entry(struct rproc *rproc,
> > >  	return !updated;
> > >  }

[...]

> > > +	spare.len = __get_rsc_tbl_spare_size(rproc, table, tablesz, &spare_index);
> > > +	if (spare.len < (sizeof(h) + request->size + 4)) /* new offset entry */
> > > +		return -EPERM;
> > 
> > What does this have to do with permissions?
> Yes sure, I'll replaced by -EINVAL as resource too large compare to spare
> area capability.

Yes, or perhaps -ENOSPC?

[...]

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v2 14/19] remoteproc: core: Add resource request action support

[Loic Pallardy]

Handle resource requests according to associated action.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 96 ++++++++++++++++++++----------------
 1 file changed, 53 insertions(+), 43 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 25a429b..67b83d0 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1171,58 +1171,64 @@ static int __add_rsc_tbl_entry(struct rproc *rproc,
 	return 0;
 }
 
-static struct resource_table*
-rproc_apply_resource_overrides(struct rproc *rproc,
-			       struct resource_table **orig_table,
-			       int *tablesz)
+static int rproc_apply_resource_overrides(struct rproc *rproc,
+			       struct resource_table *table,
+			       int tablesz)
 {
 	struct rproc_request_resource *resource;
-	struct resource_table *table = *orig_table;
-	int size = *tablesz;
+	int ret = 0, size = tablesz;
 
 	if (!table && size != 0) {
 		dev_err(&rproc->dev, "No table present but table size is set\n");
-		return ERR_PTR(-EINVAL);
+		ret = -EINVAL;
+		goto out;
 	}
 
-
-	rproc_dump_resource_table(rproc, table, size);
-
-	if (!table) {
-		size = sizeof(*table);
-		table = devm_kzalloc(&rproc->dev, size, GFP_KERNEL);
-		if (!table) {
-			table = ERR_PTR(-ENOMEM);
-			goto out;
-		}
-		table->ver = 1;
-	}
+	rproc_dump_resource_table(rproc, table, tablesz);
 
 	list_for_each_entry(resource, &rproc->override_resources, node) {
-		int updated = 0;
+		switch (resource->action) {
+		case RSC_ACT_VERIFY:
+			if (__verify_rsc_tbl_entry(rproc, resource, table, size)) {
+				ret = -EINVAL;
+				goto out;
+			}
+			break;
+		case RSC_ACT_UPDATE:
+			/* If we already have a table, update it with the new values. */
+			ret = __update_rsc_tbl_entry(rproc, resource, table, size, false);
+			if (ret < 0)
+				goto out;
+			break;
+		case RSC_ACT_FORCE_UPDATE:
+			/* If we already have a table, update it with the new values. */
+			ret = __update_rsc_tbl_entry(rproc, resource, table, size, true);
+			if (ret < 0)
+				goto out;
 
-		/* If we already have a table, update it with the new values. */
-		updated = __update_rsc_tbl_entry(rproc, resource, table, size,
-						 false);
-		if (updated < 0) {
-			table = ERR_PTR(updated);
-			goto out;
-		}
-		if (updated)
-			continue;
+			if (ret)
+				break;
 
-		/* Didn't find matching resource entry -- creating a new one. */
-		updated = __add_rsc_tbl_entry(rproc, resource, table, size);
-		if (updated)
+			/* Didn't find matching resource entry -- creating a new one. */
+			ret = __add_rsc_tbl_entry(rproc, resource, table, size);
+			if (ret)
+				goto out;
+
+			break;
+		case RSC_ACT_LOCAL:
+			/* nothing to do */
+			break;
+		default:
+			dev_err(&rproc->dev, "Unsupported resource action: %d\n",
+				resource->action);
+			ret = -EINVAL;
 			goto out;
+		};
 	}
 
-	rproc_dump_resource_table(rproc, table, size);
-
-	*tablesz = size;
-
  out:
-	return table;
+	rproc_dump_resource_table(rproc, table, tablesz);
+	return ret;
 }
 
 /*
@@ -1261,12 +1267,6 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 		goto clean_up;
 	}
 
-	if (!list_empty(&rproc->override_resources)) {
-		table = rproc_apply_resource_overrides(rproc, &table, &tablesz);
-		if (IS_ERR(table))
-			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
@@ -1281,6 +1281,16 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 
 	rproc->table_ptr = rproc->cached_table;
 
+	if (!list_empty(&rproc->override_resources)) {
+		ret = rproc_apply_resource_overrides(rproc, rproc->cached_table,
+						     tablesz);
+		if (ret) {
+			dev_err(dev, "Failed to apply overrides resources\n");
+			goto clean_up;
+		}
+
+	}
+
 	/* reset max_notifyid */
 	rproc->max_notifyid = -1;
 
-- 
1.9.1

[PATCH v2 14/19] remoteproc: core: Add resource request action support

[Loic Pallardy]

Handle resource requests according to associated action.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 96 ++++++++++++++++++++----------------
 1 file changed, 53 insertions(+), 43 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 25a429b..67b83d0 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1171,58 +1171,64 @@ static int __add_rsc_tbl_entry(struct rproc *rproc,
 	return 0;
 }
 
-static struct resource_table*
-rproc_apply_resource_overrides(struct rproc *rproc,
-			       struct resource_table **orig_table,
-			       int *tablesz)
+static int rproc_apply_resource_overrides(struct rproc *rproc,
+			       struct resource_table *table,
+			       int tablesz)
 {
 	struct rproc_request_resource *resource;
-	struct resource_table *table = *orig_table;
-	int size = *tablesz;
+	int ret = 0, size = tablesz;
 
 	if (!table && size != 0) {
 		dev_err(&rproc->dev, "No table present but table size is set\n");
-		return ERR_PTR(-EINVAL);
+		ret = -EINVAL;
+		goto out;
 	}
 
-
-	rproc_dump_resource_table(rproc, table, size);
-
-	if (!table) {
-		size = sizeof(*table);
-		table = devm_kzalloc(&rproc->dev, size, GFP_KERNEL);
-		if (!table) {
-			table = ERR_PTR(-ENOMEM);
-			goto out;
-		}
-		table->ver = 1;
-	}
+	rproc_dump_resource_table(rproc, table, tablesz);
 
 	list_for_each_entry(resource, &rproc->override_resources, node) {
-		int updated = 0;
+		switch (resource->action) {
+		case RSC_ACT_VERIFY:
+			if (__verify_rsc_tbl_entry(rproc, resource, table, size)) {
+				ret = -EINVAL;
+				goto out;
+			}
+			break;
+		case RSC_ACT_UPDATE:
+			/* If we already have a table, update it with the new values. */
+			ret = __update_rsc_tbl_entry(rproc, resource, table, size, false);
+			if (ret < 0)
+				goto out;
+			break;
+		case RSC_ACT_FORCE_UPDATE:
+			/* If we already have a table, update it with the new values. */
+			ret = __update_rsc_tbl_entry(rproc, resource, table, size, true);
+			if (ret < 0)
+				goto out;
 
-		/* If we already have a table, update it with the new values. */
-		updated = __update_rsc_tbl_entry(rproc, resource, table, size,
-						 false);
-		if (updated < 0) {
-			table = ERR_PTR(updated);
-			goto out;
-		}
-		if (updated)
-			continue;
+			if (ret)
+				break;
 
-		/* Didn't find matching resource entry -- creating a new one. */
-		updated = __add_rsc_tbl_entry(rproc, resource, table, size);
-		if (updated)
+			/* Didn't find matching resource entry -- creating a new one. */
+			ret = __add_rsc_tbl_entry(rproc, resource, table, size);
+			if (ret)
+				goto out;
+
+			break;
+		case RSC_ACT_LOCAL:
+			/* nothing to do */
+			break;
+		default:
+			dev_err(&rproc->dev, "Unsupported resource action: %d\n",
+				resource->action);
+			ret = -EINVAL;
 			goto out;
+		};
 	}
 
-	rproc_dump_resource_table(rproc, table, size);
-
-	*tablesz = size;
-
  out:
-	return table;
+	rproc_dump_resource_table(rproc, table, tablesz);
+	return ret;
 }
 
 /*
@@ -1261,12 +1267,6 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 		goto clean_up;
 	}
 
-	if (!list_empty(&rproc->override_resources)) {
-		table = rproc_apply_resource_overrides(rproc, &table, &tablesz);
-		if (IS_ERR(table))
-			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
@@ -1281,6 +1281,16 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 
 	rproc->table_ptr = rproc->cached_table;
 
+	if (!list_empty(&rproc->override_resources)) {
+		ret = rproc_apply_resource_overrides(rproc, rproc->cached_table,
+						     tablesz);
+		if (ret) {
+			dev_err(dev, "Failed to apply overrides resources\n");
+			goto clean_up;
+		}
+
+	}
+
 	/* reset max_notifyid */
 	rproc->max_notifyid = -1;
 
-- 
1.9.1

[PATCH v2 15/19] remoteproc: core: Add function to verify resource table consistency

[Loic Pallardy]

As resource table could be parsed several times, at different times,
to avoid sanity check duplication, let's introduce a new function
to verify the complete integrity of one resource table.

This new function is called before copying it in cache and
accessing it.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 79 ++++++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 67b83d0..b43553f 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -791,6 +791,78 @@ static void rproc_resource_cleanup(struct rproc *rproc)
 		rproc_remove_virtio_dev(rvdev);
 }
 
+static int rproc_rsc_tbl_sanity_check(struct rproc *rproc,
+		struct resource_table *table_ptr, int len)
+{
+	struct device *dev = &rproc->dev;
+	int i;
+
+	if (len < sizeof(*table_ptr))
+		goto out;
+
+	for (i = 0; i < table_ptr->num; i++) {
+		int offset = table_ptr->offset[i];
+		struct fw_rsc_hdr *hdr = (void *)table_ptr + offset;
+		int avail = len - offset - sizeof(*hdr);
+		void *rsc = (void *)hdr + sizeof(*hdr);
+		struct fw_rsc_vdev *v;
+		struct fw_rsc_spare *s;
+
+		/* make sure table isn't truncated */
+		if (avail < 0)
+			goto out;
+
+		if (offset == FW_RSC_ADDR_ANY || offset == 0) {
+			dev_err(dev, "Entry %d: bad offset value %x\n", i, offset);
+			return -EINVAL;
+		}
+
+		dev_dbg(dev, "rsc: type %d\n", hdr->type);
+
+		switch (hdr->type) {
+		case RSC_CARVEOUT:
+			avail -= sizeof(struct fw_rsc_carveout);
+			break;
+		case RSC_DEVMEM:
+			avail -= sizeof(struct fw_rsc_devmem);
+			break;
+		case RSC_TRACE:
+			avail -= sizeof(struct fw_rsc_trace);
+			break;
+		case RSC_VDEV:
+			v = rsc;
+			avail -= sizeof(struct fw_rsc_vdev);
+			if (avail < 0)
+				goto out;
+
+			avail -= v->num_of_vrings * sizeof(struct fw_rsc_vdev_vring)
+				 + v->config_len;
+			break;
+		case RSC_SPARE:
+			avail -= sizeof(struct fw_rsc_spare);
+			if (avail < 0)
+				goto out;
+
+			s = rsc;
+			avail -= s->len;
+			break;
+		default:
+			dev_err(&rproc->dev, "Unsupported resource type: %d\n",
+				hdr->type);
+			return -EINVAL;
+		}
+		if (avail < 0)
+			goto out;
+	}
+
+	return 0;
+
+out:
+	dev_err(dev, "Invalid resource table format\n");
+	dump_stack();
+	return -EINVAL;
+}
+
 #if defined(DEBUG)
 static void rproc_dump_resource_table(struct rproc *rproc,
 				      struct resource_table *table, int size)
@@ -1267,6 +1339,13 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 		goto clean_up;
 	}
 
+	/*  verify resource table consistency */
+	ret = rproc_rsc_tbl_sanity_check(rproc, table, tablesz);
+	if (ret) {
+		dev_err(dev, "Failed to get valid resource table,%d\n", ret);
+		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
-- 
1.9.1

[PATCH v2 15/19] remoteproc: core: Add function to verify resource table consistency

[Loic Pallardy]

As resource table could be parsed several times, at different times,
to avoid sanity check duplication, let's introduce a new function
to verify the complete integrity of one resource table.

This new function is called before copying it in cache and
accessing it.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 79 ++++++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 67b83d0..b43553f 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -791,6 +791,78 @@ static void rproc_resource_cleanup(struct rproc *rproc)
 		rproc_remove_virtio_dev(rvdev);
 }
 
+static int rproc_rsc_tbl_sanity_check(struct rproc *rproc,
+		struct resource_table *table_ptr, int len)
+{
+	struct device *dev = &rproc->dev;
+	int i;
+
+	if (len < sizeof(*table_ptr))
+		goto out;
+
+	for (i = 0; i < table_ptr->num; i++) {
+		int offset = table_ptr->offset[i];
+		struct fw_rsc_hdr *hdr = (void *)table_ptr + offset;
+		int avail = len - offset - sizeof(*hdr);
+		void *rsc = (void *)hdr + sizeof(*hdr);
+		struct fw_rsc_vdev *v;
+		struct fw_rsc_spare *s;
+
+		/* make sure table isn't truncated */
+		if (avail < 0)
+			goto out;
+
+		if (offset == FW_RSC_ADDR_ANY || offset == 0) {
+			dev_err(dev, "Entry %d: bad offset value %x\n", i, offset);
+			return -EINVAL;
+		}
+
+		dev_dbg(dev, "rsc: type %d\n", hdr->type);
+
+		switch (hdr->type) {
+		case RSC_CARVEOUT:
+			avail -= sizeof(struct fw_rsc_carveout);
+			break;
+		case RSC_DEVMEM:
+			avail -= sizeof(struct fw_rsc_devmem);
+			break;
+		case RSC_TRACE:
+			avail -= sizeof(struct fw_rsc_trace);
+			break;
+		case RSC_VDEV:
+			v = rsc;
+			avail -= sizeof(struct fw_rsc_vdev);
+			if (avail < 0)
+				goto out;
+
+			avail -= v->num_of_vrings * sizeof(struct fw_rsc_vdev_vring)
+				 + v->config_len;
+			break;
+		case RSC_SPARE:
+			avail -= sizeof(struct fw_rsc_spare);
+			if (avail < 0)
+				goto out;
+
+			s = rsc;
+			avail -= s->len;
+			break;
+		default:
+			dev_err(&rproc->dev, "Unsupported resource type: %d\n",
+				hdr->type);
+			return -EINVAL;
+		}
+		if (avail < 0)
+			goto out;
+	}
+
+	return 0;
+
+out:
+	dev_err(dev, "Invalid resource table format\n");
+	dump_stack();
+	return -EINVAL;
+}
+
 #if defined(DEBUG)
 static void rproc_dump_resource_table(struct rproc *rproc,
 				      struct resource_table *table, int size)
@@ -1267,6 +1339,13 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 		goto clean_up;
 	}
 
+	/*  verify resource table consistency */
+	ret = rproc_rsc_tbl_sanity_check(rproc, table, tablesz);
+	if (ret) {
+		dev_err(dev, "Failed to get valid resource table,%d\n", ret);
+		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
-- 
1.9.1

[PATCH v2 16/19] remoteproc: core: Clean-up resource table sanity checks

[Loic Pallardy]

As resource table has been verified after just loading,
the different sanity checks done when accessing resources
are no more needed.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 47 ++++++------------------------------
 1 file changed, 7 insertions(+), 40 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index b43553f..462cf79 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -50,7 +50,7 @@ static LIST_HEAD(rproc_list);
 typedef int (*rproc_handle_resources_t)(struct rproc *rproc,
 				struct resource_table *table, int len);
 typedef int (*rproc_handle_resource_t)(struct rproc *rproc,
-				 void *, int offset, int avail);
+				 void *, int offset);
 
 /* Unique indices for remoteproc devices */
 static DEFINE_IDA(rproc_dev_index);
@@ -306,7 +306,6 @@ void rproc_free_vring(struct rproc_vring *rvring)
  * rproc_handle_vdev() - handle a vdev fw resource
  * @rproc: the remote processor
  * @rsc: the vring resource descriptor
- * @avail: size of available data (for sanity checking the image)
  *
  * This resource entry requests the host to statically register a virtio
  * device (vdev), and setup everything needed to support it. It contains
@@ -330,19 +329,12 @@ void rproc_free_vring(struct rproc_vring *rvring)
  * Returns 0 on success, or an appropriate error code otherwise
  */
 static int rproc_handle_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc,
-			     int offset, int avail)
+			     int offset)
 {
 	struct device *dev = &rproc->dev;
 	struct rproc_vdev *rvdev;
 	int i, ret;
 
-	/* make sure resource isn't truncated */
-	if (sizeof(*rsc) + rsc->num_of_vrings * sizeof(struct fw_rsc_vdev_vring)
-			+ rsc->config_len > avail) {
-		dev_err(dev, "vdev rsc is truncated\n");
-		return -EINVAL;
-	}
-
 	/* make sure reserved bytes are zeroes */
 	if (rsc->reserved[0] || rsc->reserved[1]) {
 		dev_err(dev, "vdev rsc has non zero reserved bytes\n");
@@ -394,7 +386,6 @@ free_rvdev:
  * rproc_handle_trace() - handle a shared trace buffer resource
  * @rproc: the remote processor
  * @rsc: the trace resource descriptor
- * @avail: size of available data (for sanity checking the image)
  *
  * In case the remote processor dumps trace logs into memory,
  * export it via debugfs.
@@ -407,18 +398,13 @@ free_rvdev:
  * Returns 0 on success, or an appropriate error code otherwise
  */
 static int rproc_handle_trace(struct rproc *rproc, struct fw_rsc_trace *rsc,
-			      int offset, int avail)
+			      int offset)
 {
 	struct rproc_mem_entry *trace;
 	struct device *dev = &rproc->dev;
 	void *ptr;
 	char name[15];
 
-	if (sizeof(*rsc) > avail) {
-		dev_err(dev, "trace rsc is truncated\n");
-		return -EINVAL;
-	}
-
 	/* make sure reserved bytes are zeroes */
 	if (rsc->reserved) {
 		dev_err(dev, "trace rsc has non zero reserved bytes\n");
@@ -465,7 +451,6 @@ static int rproc_handle_trace(struct rproc *rproc, struct fw_rsc_trace *rsc,
  * rproc_handle_devmem() - handle devmem resource entry
  * @rproc: remote processor handle
  * @rsc: the devmem resource entry
- * @avail: size of available data (for sanity checking the image)
  *
  * Remote processors commonly need to access certain on-chip peripherals.
  *
@@ -487,7 +472,7 @@ static int rproc_handle_trace(struct rproc *rproc, struct fw_rsc_trace *rsc,
  * are outside those ranges.
  */
 static int rproc_handle_devmem(struct rproc *rproc, struct fw_rsc_devmem *rsc,
-			       int offset, int avail)
+			       int offset)
 {
 	struct rproc_mem_entry *mapping;
 	struct device *dev = &rproc->dev;
@@ -497,11 +482,6 @@ static int rproc_handle_devmem(struct rproc *rproc, struct fw_rsc_devmem *rsc,
 	if (!rproc->domain)
 		return -EINVAL;
 
-	if (sizeof(*rsc) > avail) {
-		dev_err(dev, "devmem rsc is truncated\n");
-		return -EINVAL;
-	}
-
 	/* make sure reserved bytes are zeroes */
 	if (rsc->reserved) {
 		dev_err(dev, "devmem rsc has non zero reserved bytes\n");
@@ -543,7 +523,6 @@ out:
  * rproc_handle_carveout() - handle phys contig memory allocation requests
  * @rproc: rproc handle
  * @rsc: the resource entry
- * @avail: size of available data (for image validation)
  *
  * This function will handle firmware requests for allocation of physically
  * contiguous memory regions.
@@ -559,7 +538,7 @@ out:
  */
 static int rproc_handle_carveout(struct rproc *rproc,
 				 struct fw_rsc_carveout *rsc,
-				 int offset, int avail)
+				 int offset)
 {
 	struct rproc_mem_entry *carveout, *mapping;
 	struct device *dev = &rproc->dev;
@@ -567,11 +546,6 @@ static int rproc_handle_carveout(struct rproc *rproc,
 	void *va;
 	int ret;
 
-	if (sizeof(*rsc) > avail) {
-		dev_err(dev, "carveout rsc is truncated\n");
-		return -EINVAL;
-	}
-
 	/* make sure reserved bytes are zeroes */
 	if (rsc->reserved) {
 		dev_err(dev, "carveout rsc has non zero reserved bytes\n");
@@ -680,7 +654,7 @@ free_carv:
 }
 
 static int rproc_count_vrings(struct rproc *rproc, struct fw_rsc_vdev *rsc,
-			      int offset, int avail)
+			      int offset)
 {
 	/* Summarize the number of notification IDs */
 	rproc->max_notifyid += rsc->num_of_vrings;
@@ -714,15 +688,8 @@ 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);
 		void *rsc = (void *)hdr + sizeof(*hdr);
 
-		/* make sure table isn't truncated */
-		if (avail < 0) {
-			dev_err(dev, "rsc table is truncated\n");
-			return -EINVAL;
-		}
-
 		dev_dbg(dev, "rsc: type %d\n", hdr->type);
 
 		if (hdr->type >= RSC_LAST) {
@@ -734,7 +701,7 @@ static int rproc_handle_resources(struct rproc *rproc, int len,
 		if (!handler)
 			continue;
 
-		ret = handler(rproc, rsc, offset + sizeof(*hdr), avail);
+		ret = handler(rproc, rsc, offset + sizeof(*hdr));
 		if (ret)
 			break;
 	}
-- 
1.9.1

[PATCH v2 16/19] remoteproc: core: Clean-up resource table sanity checks

[Loic Pallardy]

As resource table has been verified after just loading,
the different sanity checks done when accessing resources
are no more needed.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 47 ++++++------------------------------
 1 file changed, 7 insertions(+), 40 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index b43553f..462cf79 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -50,7 +50,7 @@ static LIST_HEAD(rproc_list);
 typedef int (*rproc_handle_resources_t)(struct rproc *rproc,
 				struct resource_table *table, int len);
 typedef int (*rproc_handle_resource_t)(struct rproc *rproc,
-				 void *, int offset, int avail);
+				 void *, int offset);
 
 /* Unique indices for remoteproc devices */
 static DEFINE_IDA(rproc_dev_index);
@@ -306,7 +306,6 @@ void rproc_free_vring(struct rproc_vring *rvring)
  * rproc_handle_vdev() - handle a vdev fw resource
  * @rproc: the remote processor
  * @rsc: the vring resource descriptor
- * @avail: size of available data (for sanity checking the image)
  *
  * This resource entry requests the host to statically register a virtio
  * device (vdev), and setup everything needed to support it. It contains
@@ -330,19 +329,12 @@ void rproc_free_vring(struct rproc_vring *rvring)
  * Returns 0 on success, or an appropriate error code otherwise
  */
 static int rproc_handle_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc,
-			     int offset, int avail)
+			     int offset)
 {
 	struct device *dev = &rproc->dev;
 	struct rproc_vdev *rvdev;
 	int i, ret;
 
-	/* make sure resource isn't truncated */
-	if (sizeof(*rsc) + rsc->num_of_vrings * sizeof(struct fw_rsc_vdev_vring)
-			+ rsc->config_len > avail) {
-		dev_err(dev, "vdev rsc is truncated\n");
-		return -EINVAL;
-	}
-
 	/* make sure reserved bytes are zeroes */
 	if (rsc->reserved[0] || rsc->reserved[1]) {
 		dev_err(dev, "vdev rsc has non zero reserved bytes\n");
@@ -394,7 +386,6 @@ free_rvdev:
  * rproc_handle_trace() - handle a shared trace buffer resource
  * @rproc: the remote processor
  * @rsc: the trace resource descriptor
- * @avail: size of available data (for sanity checking the image)
  *
  * In case the remote processor dumps trace logs into memory,
  * export it via debugfs.
@@ -407,18 +398,13 @@ free_rvdev:
  * Returns 0 on success, or an appropriate error code otherwise
  */
 static int rproc_handle_trace(struct rproc *rproc, struct fw_rsc_trace *rsc,
-			      int offset, int avail)
+			      int offset)
 {
 	struct rproc_mem_entry *trace;
 	struct device *dev = &rproc->dev;
 	void *ptr;
 	char name[15];
 
-	if (sizeof(*rsc) > avail) {
-		dev_err(dev, "trace rsc is truncated\n");
-		return -EINVAL;
-	}
-
 	/* make sure reserved bytes are zeroes */
 	if (rsc->reserved) {
 		dev_err(dev, "trace rsc has non zero reserved bytes\n");
@@ -465,7 +451,6 @@ static int rproc_handle_trace(struct rproc *rproc, struct fw_rsc_trace *rsc,
  * rproc_handle_devmem() - handle devmem resource entry
  * @rproc: remote processor handle
  * @rsc: the devmem resource entry
- * @avail: size of available data (for sanity checking the image)
  *
  * Remote processors commonly need to access certain on-chip peripherals.
  *
@@ -487,7 +472,7 @@ static int rproc_handle_trace(struct rproc *rproc, struct fw_rsc_trace *rsc,
  * are outside those ranges.
  */
 static int rproc_handle_devmem(struct rproc *rproc, struct fw_rsc_devmem *rsc,
-			       int offset, int avail)
+			       int offset)
 {
 	struct rproc_mem_entry *mapping;
 	struct device *dev = &rproc->dev;
@@ -497,11 +482,6 @@ static int rproc_handle_devmem(struct rproc *rproc, struct fw_rsc_devmem *rsc,
 	if (!rproc->domain)
 		return -EINVAL;
 
-	if (sizeof(*rsc) > avail) {
-		dev_err(dev, "devmem rsc is truncated\n");
-		return -EINVAL;
-	}
-
 	/* make sure reserved bytes are zeroes */
 	if (rsc->reserved) {
 		dev_err(dev, "devmem rsc has non zero reserved bytes\n");
@@ -543,7 +523,6 @@ out:
  * rproc_handle_carveout() - handle phys contig memory allocation requests
  * @rproc: rproc handle
  * @rsc: the resource entry
- * @avail: size of available data (for image validation)
  *
  * This function will handle firmware requests for allocation of physically
  * contiguous memory regions.
@@ -559,7 +538,7 @@ out:
  */
 static int rproc_handle_carveout(struct rproc *rproc,
 				 struct fw_rsc_carveout *rsc,
-				 int offset, int avail)
+				 int offset)
 {
 	struct rproc_mem_entry *carveout, *mapping;
 	struct device *dev = &rproc->dev;
@@ -567,11 +546,6 @@ static int rproc_handle_carveout(struct rproc *rproc,
 	void *va;
 	int ret;
 
-	if (sizeof(*rsc) > avail) {
-		dev_err(dev, "carveout rsc is truncated\n");
-		return -EINVAL;
-	}
-
 	/* make sure reserved bytes are zeroes */
 	if (rsc->reserved) {
 		dev_err(dev, "carveout rsc has non zero reserved bytes\n");
@@ -680,7 +654,7 @@ free_carv:
 }
 
 static int rproc_count_vrings(struct rproc *rproc, struct fw_rsc_vdev *rsc,
-			      int offset, int avail)
+			      int offset)
 {
 	/* Summarize the number of notification IDs */
 	rproc->max_notifyid += rsc->num_of_vrings;
@@ -714,15 +688,8 @@ 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);
 		void *rsc = (void *)hdr + sizeof(*hdr);
 
-		/* make sure table isn't truncated */
-		if (avail < 0) {
-			dev_err(dev, "rsc table is truncated\n");
-			return -EINVAL;
-		}
-
 		dev_dbg(dev, "rsc: type %d\n", hdr->type);
 
 		if (hdr->type >= RSC_LAST) {
@@ -734,7 +701,7 @@ static int rproc_handle_resources(struct rproc *rproc, int len,
 		if (!handler)
 			continue;
 
-		ret = handler(rproc, rsc, offset + sizeof(*hdr), avail);
+		ret = handler(rproc, rsc, offset + sizeof(*hdr));
 		if (ret)
 			break;
 	}
-- 
1.9.1

[PATCH v2 17/19] remotecore: core: Add resource table pointer argument to rproc_handle_resource

[Loic Pallardy]

In current version, rproc_handle_resource use rproc->table_ptr
as default resource table, fixing table to handle.
This patch adds resource table as function argument to have more
flexibility and be able to handle resources from any table.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 462cf79..cbfbdf8 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -678,16 +678,16 @@ static rproc_handle_resource_t rproc_vdev_handler[RSC_LAST] = {
 };
 
 /* handle firmware resource entries before booting the remote processor */
-static int rproc_handle_resources(struct rproc *rproc, int len,
-				  rproc_handle_resource_t handlers[RSC_LAST])
+static int rproc_handle_resources(struct rproc *rproc, struct resource_table *table_ptr,
+				  int len, rproc_handle_resource_t handlers[RSC_LAST])
 {
 	struct device *dev = &rproc->dev;
 	rproc_handle_resource_t handler;
 	int ret = 0, i;
 
-	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;
+	for (i = 0; i < table_ptr->num; i++) {
+		int offset = table_ptr->offset[i];
+		struct fw_rsc_hdr *hdr = (void *)table_ptr + offset;
 		void *rsc = (void *)hdr + sizeof(*hdr);
 
 		dev_dbg(dev, "rsc: type %d\n", hdr->type);
@@ -1341,14 +1341,16 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 	rproc->max_notifyid = -1;
 
 	/* look for virtio devices and register them */
-	ret = rproc_handle_resources(rproc, tablesz, rproc_vdev_handler);
+	ret = rproc_handle_resources(rproc, rproc->cached_table, tablesz,
+				     rproc_vdev_handler);
 	if (ret) {
 		dev_err(dev, "Failed to handle vdev resources: %d\n", ret);
 		goto clean_up;
 	}
 
 	/* handle fw resources which are required to boot rproc */
-	ret = rproc_handle_resources(rproc, tablesz, rproc_loading_handlers);
+	ret = rproc_handle_resources(rproc, rproc->cached_table, tablesz,
+				     rproc_loading_handlers);
 	if (ret) {
 		dev_err(dev, "Failed to process resources: %d\n", ret);
 		goto clean_up;
-- 
1.9.1

[PATCH v2 17/19] remotecore: core: Add resource table pointer argument to rproc_handle_resource

[Loic Pallardy]

In current version, rproc_handle_resource use rproc->table_ptr
as default resource table, fixing table to handle.
This patch adds resource table as function argument to have more
flexibility and be able to handle resources from any table.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 462cf79..cbfbdf8 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -678,16 +678,16 @@ static rproc_handle_resource_t rproc_vdev_handler[RSC_LAST] = {
 };
 
 /* handle firmware resource entries before booting the remote processor */
-static int rproc_handle_resources(struct rproc *rproc, int len,
-				  rproc_handle_resource_t handlers[RSC_LAST])
+static int rproc_handle_resources(struct rproc *rproc, struct resource_table *table_ptr,
+				  int len, rproc_handle_resource_t handlers[RSC_LAST])
 {
 	struct device *dev = &rproc->dev;
 	rproc_handle_resource_t handler;
 	int ret = 0, i;
 
-	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;
+	for (i = 0; i < table_ptr->num; i++) {
+		int offset = table_ptr->offset[i];
+		struct fw_rsc_hdr *hdr = (void *)table_ptr + offset;
 		void *rsc = (void *)hdr + sizeof(*hdr);
 
 		dev_dbg(dev, "rsc: type %d\n", hdr->type);
@@ -1341,14 +1341,16 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 	rproc->max_notifyid = -1;
 
 	/* look for virtio devices and register them */
-	ret = rproc_handle_resources(rproc, tablesz, rproc_vdev_handler);
+	ret = rproc_handle_resources(rproc, rproc->cached_table, tablesz,
+				     rproc_vdev_handler);
 	if (ret) {
 		dev_err(dev, "Failed to handle vdev resources: %d\n", ret);
 		goto clean_up;
 	}
 
 	/* handle fw resources which are required to boot rproc */
-	ret = rproc_handle_resources(rproc, tablesz, rproc_loading_handlers);
+	ret = rproc_handle_resources(rproc, rproc->cached_table, tablesz,
+				     rproc_loading_handlers);
 	if (ret) {
 		dev_err(dev, "Failed to process resources: %d\n", ret);
 		goto clean_up;
-- 
1.9.1

[PATCH v2 18/19] remoteproc: core: Add function to create remoteproc local resource table

[Loic Pallardy]

Rproc driver has now the capability to add resources dynamically
thanks to rproc_request_resource API.
Depending on associated action, resource request could impact
firmware resource table or define new local resource.

In order to preserve current remoteproc resource handling
mechanism, all local resources are gathered in a local resource
table which won't be shared with firmware and proceed by
remoteproc core as firmware one.

It is rproc driver responsibility to provide the right resource
information using rproc_request_resource API.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 80 +++++++++++++++++++++++++++++++++++-
 include/linux/remoteproc.h           |  1 +
 2 files changed, 80 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index cbfbdf8..73b460a 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1270,6 +1270,65 @@ static int rproc_apply_resource_overrides(struct rproc *rproc,
 	return ret;
 }
 
+static struct resource_table*
+rproc_local_resource_create(struct rproc *rproc, int *tablesz)
+{
+	struct fw_rsc_hdr *hdr;
+	struct fw_rsc_spare *spare_rsc;
+	struct rproc_request_resource *resource;
+	struct resource_table *table = NULL;
+	int size = 0, ret;
+
+	/* compute total request size */
+	list_for_each_entry(resource, &rproc->override_resources, node) {
+		if (resource->action == RSC_ACT_LOCAL)
+			size += resource->size + sizeof(hdr) + 4; /* entry offset */
+	}
+
+	/* any extra resource ? */
+	if (!size)
+		goto out;
+
+	/* add table header and spare resource */
+	size += sizeof(*table);
+	size += sizeof(*hdr) + sizeof(*spare_rsc) + 4;
+
+	/* create new rsc tbl with only a spare resource */
+	table = devm_kzalloc(&rproc->dev, size, GFP_KERNEL);
+	if (!table) {
+		table = ERR_PTR(-ENOMEM);
+		goto out;
+	}
+	table->ver = 1;
+	table->num = 1;
+	table->offset[0] = sizeof(*table) + 4;
+
+	hdr = (void *)table + table->offset[0];
+	hdr->type = RSC_SPARE;
+
+	spare_rsc = (void *)hdr + sizeof(*hdr);
+	spare_rsc->len = size - table->offset[0] - sizeof(*hdr) - sizeof(*spare_rsc);
+
+	/* add new resource one by one */
+	list_for_each_entry(resource, &rproc->override_resources, node) {
+		if (resource->action == RSC_ACT_LOCAL) {
+			/* Create a new enty */
+			ret = __add_rsc_tbl_entry(rproc, resource,
+					table, size);
+			if (ret) {
+				table = ERR_PTR(ret);
+				goto out;
+			}
+		}
+	}
+
+	*tablesz = size;
+	rproc_dump_resource_table(rproc, table, *tablesz);
+out:
+	return table;
+}
+
+
 /*
  * take a firmware and boot a remote processor with it.
  */
@@ -1278,7 +1337,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, *loaded_table;
-	int ret, tablesz;
+	int ret, tablesz, local_tablesz;
 
 	ret = rproc_fw_sanity_check(rproc, fw);
 	if (ret)
@@ -1335,6 +1394,11 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 			goto clean_up;
 		}
 
+		rproc->local_table = rproc_local_resource_create(rproc, &local_tablesz);
+		if (IS_ERR(rproc->local_table)) {
+			dev_err(dev, "Failed to create local resource table\n");
+			goto clean_up;
+		}
 	}
 
 	/* reset max_notifyid */
@@ -1348,6 +1412,13 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 		goto clean_up;
 	}
 
+	ret = rproc_handle_resources(rproc, rproc->local_table,
+				     local_tablesz, rproc_vdev_handler);
+	if (ret) {
+		dev_err(dev, "Failed to handle vdev resources: %d\n", ret);
+		goto clean_up;
+	}
+
 	/* handle fw resources which are required to boot rproc */
 	ret = rproc_handle_resources(rproc, rproc->cached_table, tablesz,
 				     rproc_loading_handlers);
@@ -1356,6 +1427,13 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 		goto clean_up;
 	}
 
+	ret = rproc_handle_resources(rproc, rproc->local_table,
+				     local_tablesz, rproc_loading_handlers);
+	if (ret) {
+		dev_err(dev, "Failed to handle vdev resources: %d\n", ret);
+		goto clean_up;
+	}
+
 	/* load the ELF segments to memory */
 	ret = rproc_load_segments(rproc, fw);
 	if (ret) {
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 2b0f1d7..653e6f3 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -495,6 +495,7 @@ struct rproc {
 	int max_notifyid;
 	struct resource_table *table_ptr;
 	struct resource_table *cached_table;
+	struct resource_table *local_table;
 	bool has_iommu;
 	bool auto_boot;
 };
-- 
1.9.1

[PATCH v2 18/19] remoteproc: core: Add function to create remoteproc local resource table

[Loic Pallardy]

Rproc driver has now the capability to add resources dynamically
thanks to rproc_request_resource API.
Depending on associated action, resource request could impact
firmware resource table or define new local resource.

In order to preserve current remoteproc resource handling
mechanism, all local resources are gathered in a local resource
table which won't be shared with firmware and proceed by
remoteproc core as firmware one.

It is rproc driver responsibility to provide the right resource
information using rproc_request_resource API.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 80 +++++++++++++++++++++++++++++++++++-
 include/linux/remoteproc.h           |  1 +
 2 files changed, 80 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index cbfbdf8..73b460a 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1270,6 +1270,65 @@ static int rproc_apply_resource_overrides(struct rproc *rproc,
 	return ret;
 }
 
+static struct resource_table*
+rproc_local_resource_create(struct rproc *rproc, int *tablesz)
+{
+	struct fw_rsc_hdr *hdr;
+	struct fw_rsc_spare *spare_rsc;
+	struct rproc_request_resource *resource;
+	struct resource_table *table = NULL;
+	int size = 0, ret;
+
+	/* compute total request size */
+	list_for_each_entry(resource, &rproc->override_resources, node) {
+		if (resource->action == RSC_ACT_LOCAL)
+			size += resource->size + sizeof(hdr) + 4; /* entry offset */
+	}
+
+	/* any extra resource ? */
+	if (!size)
+		goto out;
+
+	/* add table header and spare resource */
+	size += sizeof(*table);
+	size += sizeof(*hdr) + sizeof(*spare_rsc) + 4;
+
+	/* create new rsc tbl with only a spare resource */
+	table = devm_kzalloc(&rproc->dev, size, GFP_KERNEL);
+	if (!table) {
+		table = ERR_PTR(-ENOMEM);
+		goto out;
+	}
+	table->ver = 1;
+	table->num = 1;
+	table->offset[0] = sizeof(*table) + 4;
+
+	hdr = (void *)table + table->offset[0];
+	hdr->type = RSC_SPARE;
+
+	spare_rsc = (void *)hdr + sizeof(*hdr);
+	spare_rsc->len = size - table->offset[0] - sizeof(*hdr) - sizeof(*spare_rsc);
+
+	/* add new resource one by one */
+	list_for_each_entry(resource, &rproc->override_resources, node) {
+		if (resource->action == RSC_ACT_LOCAL) {
+			/* Create a new enty */
+			ret = __add_rsc_tbl_entry(rproc, resource,
+					table, size);
+			if (ret) {
+				table = ERR_PTR(ret);
+				goto out;
+			}
+		}
+	}
+
+	*tablesz = size;
+	rproc_dump_resource_table(rproc, table, *tablesz);
+out:
+	return table;
+}
+
+
 /*
  * take a firmware and boot a remote processor with it.
  */
@@ -1278,7 +1337,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, *loaded_table;
-	int ret, tablesz;
+	int ret, tablesz, local_tablesz;
 
 	ret = rproc_fw_sanity_check(rproc, fw);
 	if (ret)
@@ -1335,6 +1394,11 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 			goto clean_up;
 		}
 
+		rproc->local_table = rproc_local_resource_create(rproc, &local_tablesz);
+		if (IS_ERR(rproc->local_table)) {
+			dev_err(dev, "Failed to create local resource table\n");
+			goto clean_up;
+		}
 	}
 
 	/* reset max_notifyid */
@@ -1348,6 +1412,13 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 		goto clean_up;
 	}
 
+	ret = rproc_handle_resources(rproc, rproc->local_table,
+				     local_tablesz, rproc_vdev_handler);
+	if (ret) {
+		dev_err(dev, "Failed to handle vdev resources: %d\n", ret);
+		goto clean_up;
+	}
+
 	/* handle fw resources which are required to boot rproc */
 	ret = rproc_handle_resources(rproc, rproc->cached_table, tablesz,
 				     rproc_loading_handlers);
@@ -1356,6 +1427,13 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 		goto clean_up;
 	}
 
+	ret = rproc_handle_resources(rproc, rproc->local_table,
+				     local_tablesz, rproc_loading_handlers);
+	if (ret) {
+		dev_err(dev, "Failed to handle vdev resources: %d\n", ret);
+		goto clean_up;
+	}
+
 	/* load the ELF segments to memory */
 	ret = rproc_load_segments(rproc, fw);
 	if (ret) {
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 2b0f1d7..653e6f3 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -495,6 +495,7 @@ struct rproc {
 	int max_notifyid;
 	struct resource_table *table_ptr;
 	struct resource_table *cached_table;
+	struct resource_table *local_table;
 	bool has_iommu;
 	bool auto_boot;
 };
-- 
1.9.1

Re: [PATCH v2 18/19] remoteproc: core: Add function to create remoteproc local resource table

[Lee Jones]

On Wed, 31 Aug 2016, Loic Pallardy wrote:

> Rproc driver has now the capability to add resources dynamically
> thanks to rproc_request_resource API.
> Depending on associated action, resource request could impact
> firmware resource table or define new local resource.
> 
> In order to preserve current remoteproc resource handling
> mechanism, all local resources are gathered in a local resource
> table which won't be shared with firmware and proceed by
> remoteproc core as firmware one.
> 
> It is rproc driver responsibility to provide the right resource
> information using rproc_request_resource API.
> 
> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> ---
>  drivers/remoteproc/remoteproc_core.c | 80 +++++++++++++++++++++++++++++++++++-
>  include/linux/remoteproc.h           |  1 +
>  2 files changed, 80 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index cbfbdf8..73b460a 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1270,6 +1270,65 @@ static int rproc_apply_resource_overrides(struct rproc *rproc,
>  	return ret;
>  }
>  
> +static struct resource_table*
> +rproc_local_resource_create(struct rproc *rproc, int *tablesz)

Oh, you're happy to use "resource" (instead of rsc) in function names
that *you* introduce! ;)

> +{
> +	struct fw_rsc_hdr *hdr;
> +	struct fw_rsc_spare *spare_rsc;
> +	struct rproc_request_resource *resource;
> +	struct resource_table *table = NULL;
> +	int size = 0, ret;
> +
> +	/* compute total request size */

Grammar.

> +	list_for_each_entry(resource, &rproc->override_resources, node) {
> +		if (resource->action == RSC_ACT_LOCAL)
> +			size += resource->size + sizeof(hdr) + 4; /* entry offset */
> +	}

The {} are superfluous.

Still non sure if that comment helps at all.

> +	/* any extra resource ? */

/* If there isn't any resource remaining, don't ... XXX */

> +	if (!size)
> +		goto out;
> +
> +	/* add table header and spare resource */
> +	size += sizeof(*table);
> +	size += sizeof(*hdr) + sizeof(*spare_rsc) + 4;
> +
> +	/* create new rsc tbl with only a spare resource */

I would be as forthcoming as possible in comments.  Use
full/descriptive names for things.

> +	table = devm_kzalloc(&rproc->dev, size, GFP_KERNEL);
> +	if (!table) {
> +		table = ERR_PTR(-ENOMEM);
> +		goto out;
> +	}

'\n'

> +	table->ver = 1;
> +	table->num = 1;
> +	table->offset[0] = sizeof(*table) + 4;
> +
> +	hdr = (void *)table + table->offset[0];
> +	hdr->type = RSC_SPARE;
> +
> +	spare_rsc = (void *)hdr + sizeof(*hdr);
> +	spare_rsc->len = size - table->offset[0] - sizeof(*hdr) - sizeof(*spare_rsc);
> +
> +	/* add new resource one by one */

"resources"

> +	list_for_each_entry(resource, &rproc->override_resources, node) {
> +		if (resource->action == RSC_ACT_LOCAL) {
> +			/* Create a new enty */

This comment doesn't add any more information than the function name.

> +			ret = __add_rsc_tbl_entry(rproc, resource,
> +					table, size);
> +			if (ret) {
> +				table = ERR_PTR(ret);
> +				goto out;
> +			}
> +		}
> +	}
> +
> +	*tablesz = size;
> +	rproc_dump_resource_table(rproc, table, *tablesz);

This is going to add up to a lot of dumps of the resource table?

> +out:
> +	return table;
> +}
> +
> +

Superfluous '\n'.

>  /*
>   * take a firmware and boot a remote processor with it.
>   */
> @@ -1278,7 +1337,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, *loaded_table;
> -	int ret, tablesz;
> +	int ret, tablesz, local_tablesz;
>  
>  	ret = rproc_fw_sanity_check(rproc, fw);
>  	if (ret)
> @@ -1335,6 +1394,11 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>  			goto clean_up;
>  		}
>  
> +		rproc->local_table = rproc_local_resource_create(rproc, &local_tablesz);
> +		if (IS_ERR(rproc->local_table)) {
> +			dev_err(dev, "Failed to create local resource table\n");
> +			goto clean_up;
> +		}
>  	}
>  
>  	/* reset max_notifyid */
> @@ -1348,6 +1412,13 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>  		goto clean_up;
>  	}
>  
> +	ret = rproc_handle_resources(rproc, rproc->local_table,
> +				     local_tablesz, rproc_vdev_handler);
> +	if (ret) {
> +		dev_err(dev, "Failed to handle vdev resources: %d\n", ret);
> +		goto clean_up;
> +	}
> +
>  	/* handle fw resources which are required to boot rproc */
>  	ret = rproc_handle_resources(rproc, rproc->cached_table, tablesz,
>  				     rproc_loading_handlers);
> @@ -1356,6 +1427,13 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>  		goto clean_up;
>  	}
>  
> +	ret = rproc_handle_resources(rproc, rproc->local_table,
> +				     local_tablesz, rproc_loading_handlers);
> +	if (ret) {
> +		dev_err(dev, "Failed to handle vdev resources: %d\n", ret);
> +		goto clean_up;
> +	}
> +
>  	/* load the ELF segments to memory */
>  	ret = rproc_load_segments(rproc, fw);
>  	if (ret) {
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 2b0f1d7..653e6f3 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -495,6 +495,7 @@ struct rproc {
>  	int max_notifyid;
>  	struct resource_table *table_ptr;
>  	struct resource_table *cached_table;
> +	struct resource_table *local_table;
>  	bool has_iommu;
>  	bool auto_boot;
>  };

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 18/19] remoteproc: core: Add function to create remoteproc local resource table

[loic pallardy]

On 09/08/2016 12:20 PM, Lee Jones wrote:
> On Wed, 31 Aug 2016, Loic Pallardy wrote:
>
>> Rproc driver has now the capability to add resources dynamically
>> thanks to rproc_request_resource API.
>> Depending on associated action, resource request could impact
>> firmware resource table or define new local resource.
>>
>> In order to preserve current remoteproc resource handling
>> mechanism, all local resources are gathered in a local resource
>> table which won't be shared with firmware and proceed by
>> remoteproc core as firmware one.
>>
>> It is rproc driver responsibility to provide the right resource
>> information using rproc_request_resource API.
>>
>> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
>> ---
>>  drivers/remoteproc/remoteproc_core.c | 80 +++++++++++++++++++++++++++++++++++-
>>  include/linux/remoteproc.h           |  1 +
>>  2 files changed, 80 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>> index cbfbdf8..73b460a 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -1270,6 +1270,65 @@ static int rproc_apply_resource_overrides(struct rproc *rproc,
>>  	return ret;
>>  }
>>
>> +static struct resource_table*
>> +rproc_local_resource_create(struct rproc *rproc, int *tablesz)
>
> Oh, you're happy to use "resource" (instead of rsc) in function names
> that *you* introduce! ;)
In fact I changed only rproc_apply_resource_overrides sub functions, 
don't touch to other. But as mentioned previously, I'll revert name 
changing and come back to original naming in v3
>
>> +{
>> +	struct fw_rsc_hdr *hdr;
>> +	struct fw_rsc_spare *spare_rsc;
>> +	struct rproc_request_resource *resource;
>> +	struct resource_table *table = NULL;
>> +	int size = 0, ret;
>> +
>> +	/* compute total request size */
>
> Grammar.
ok
>
>> +	list_for_each_entry(resource, &rproc->override_resources, node) {
>> +		if (resource->action == RSC_ACT_LOCAL)
>> +			size += resource->size + sizeof(hdr) + 4; /* entry offset */
>> +	}
>
> The {} are superfluous.
>
> Still non sure if that comment helps at all.
>
>> +	/* any extra resource ? */
>
> /* If there isn't any resource remaining, don't ... XXX */
>
>> +	if (!size)
>> +		goto out;
>> +
>> +	/* add table header and spare resource */
>> +	size += sizeof(*table);
>> +	size += sizeof(*hdr) + sizeof(*spare_rsc) + 4;
>> +
>> +	/* create new rsc tbl with only a spare resource */
>
> I would be as forthcoming as possible in comments.  Use
> full/descriptive names for things.
ok
>
>> +	table = devm_kzalloc(&rproc->dev, size, GFP_KERNEL);
>> +	if (!table) {
>> +		table = ERR_PTR(-ENOMEM);
>> +		goto out;
>> +	}
>
> '\n'
ok
>
>> +	table->ver = 1;
>> +	table->num = 1;
>> +	table->offset[0] = sizeof(*table) + 4;
>> +
>> +	hdr = (void *)table + table->offset[0];
>> +	hdr->type = RSC_SPARE;
>> +
>> +	spare_rsc = (void *)hdr + sizeof(*hdr);
>> +	spare_rsc->len = size - table->offset[0] - sizeof(*hdr) - sizeof(*spare_rsc);
>> +
>> +	/* add new resource one by one */
>
> "resources"
thanks
>
>> +	list_for_each_entry(resource, &rproc->override_resources, node) {
>> +		if (resource->action == RSC_ACT_LOCAL) {
>> +			/* Create a new enty */
>
> This comment doesn't add any more information than the function name.
I'll remove
>
>> +			ret = __add_rsc_tbl_entry(rproc, resource,
>> +					table, size);
>> +			if (ret) {
>> +				table = ERR_PTR(ret);
>> +				goto out;
>> +			}
>> +		}
>> +	}
>> +
>> +	*tablesz = size;
>> +	rproc_dump_resource_table(rproc, table, *tablesz);
>
> This is going to add up to a lot of dumps of the resource table?
No only once when the complete table is populated
>
>> +out:
>> +	return table;
>> +}
>> +
>> +
>
> Superfluous '\n'.
ok

Thanks for your review Lee, I'll prepare a V3 including your remarks

Regards,
Loic
>
>>  /*
>>   * take a firmware and boot a remote processor with it.
>>   */
>> @@ -1278,7 +1337,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, *loaded_table;
>> -	int ret, tablesz;
>> +	int ret, tablesz, local_tablesz;
>>
>>  	ret = rproc_fw_sanity_check(rproc, fw);
>>  	if (ret)
>> @@ -1335,6 +1394,11 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>>  			goto clean_up;
>>  		}
>>
>> +		rproc->local_table = rproc_local_resource_create(rproc, &local_tablesz);
>> +		if (IS_ERR(rproc->local_table)) {
>> +			dev_err(dev, "Failed to create local resource table\n");
>> +			goto clean_up;
>> +		}
>>  	}
>>
>>  	/* reset max_notifyid */
>> @@ -1348,6 +1412,13 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>>  		goto clean_up;
>>  	}
>>
>> +	ret = rproc_handle_resources(rproc, rproc->local_table,
>> +				     local_tablesz, rproc_vdev_handler);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to handle vdev resources: %d\n", ret);
>> +		goto clean_up;
>> +	}
>> +
>>  	/* handle fw resources which are required to boot rproc */
>>  	ret = rproc_handle_resources(rproc, rproc->cached_table, tablesz,
>>  				     rproc_loading_handlers);
>> @@ -1356,6 +1427,13 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>>  		goto clean_up;
>>  	}
>>
>> +	ret = rproc_handle_resources(rproc, rproc->local_table,
>> +				     local_tablesz, rproc_loading_handlers);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to handle vdev resources: %d\n", ret);
>> +		goto clean_up;
>> +	}
>> +
>>  	/* load the ELF segments to memory */
>>  	ret = rproc_load_segments(rproc, fw);
>>  	if (ret) {
>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>> index 2b0f1d7..653e6f3 100644
>> --- a/include/linux/remoteproc.h
>> +++ b/include/linux/remoteproc.h
>> @@ -495,6 +495,7 @@ struct rproc {
>>  	int max_notifyid;
>>  	struct resource_table *table_ptr;
>>  	struct resource_table *cached_table;
>> +	struct resource_table *local_table;
>>  	bool has_iommu;
>>  	bool auto_boot;
>>  };
>

Re: [PATCH v2 18/19] remoteproc: core: Add function to create remoteproc local resource table

[loic pallardy]

On 09/08/2016 12:20 PM, Lee Jones wrote:
> On Wed, 31 Aug 2016, Loic Pallardy wrote:
>
>> Rproc driver has now the capability to add resources dynamically
>> thanks to rproc_request_resource API.
>> Depending on associated action, resource request could impact
>> firmware resource table or define new local resource.
>>
>> In order to preserve current remoteproc resource handling
>> mechanism, all local resources are gathered in a local resource
>> table which won't be shared with firmware and proceed by
>> remoteproc core as firmware one.
>>
>> It is rproc driver responsibility to provide the right resource
>> information using rproc_request_resource API.
>>
>> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
>> ---
>>  drivers/remoteproc/remoteproc_core.c | 80 +++++++++++++++++++++++++++++++++++-
>>  include/linux/remoteproc.h           |  1 +
>>  2 files changed, 80 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>> index cbfbdf8..73b460a 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -1270,6 +1270,65 @@ static int rproc_apply_resource_overrides(struct rproc *rproc,
>>  	return ret;
>>  }
>>
>> +static struct resource_table*
>> +rproc_local_resource_create(struct rproc *rproc, int *tablesz)
>
> Oh, you're happy to use "resource" (instead of rsc) in function names
> that *you* introduce! ;)
In fact I changed only rproc_apply_resource_overrides sub functions, 
don't touch to other. But as mentioned previously, I'll revert name 
changing and come back to original naming in v3
>
>> +{
>> +	struct fw_rsc_hdr *hdr;
>> +	struct fw_rsc_spare *spare_rsc;
>> +	struct rproc_request_resource *resource;
>> +	struct resource_table *table = NULL;
>> +	int size = 0, ret;
>> +
>> +	/* compute total request size */
>
> Grammar.
ok
>
>> +	list_for_each_entry(resource, &rproc->override_resources, node) {
>> +		if (resource->action == RSC_ACT_LOCAL)
>> +			size += resource->size + sizeof(hdr) + 4; /* entry offset */
>> +	}
>
> The {} are superfluous.
>
> Still non sure if that comment helps at all.
>
>> +	/* any extra resource ? */
>
> /* If there isn't any resource remaining, don't ... XXX */
>
>> +	if (!size)
>> +		goto out;
>> +
>> +	/* add table header and spare resource */
>> +	size += sizeof(*table);
>> +	size += sizeof(*hdr) + sizeof(*spare_rsc) + 4;
>> +
>> +	/* create new rsc tbl with only a spare resource */
>
> I would be as forthcoming as possible in comments.  Use
> full/descriptive names for things.
ok
>
>> +	table = devm_kzalloc(&rproc->dev, size, GFP_KERNEL);
>> +	if (!table) {
>> +		table = ERR_PTR(-ENOMEM);
>> +		goto out;
>> +	}
>
> '\n'
ok
>
>> +	table->ver = 1;
>> +	table->num = 1;
>> +	table->offset[0] = sizeof(*table) + 4;
>> +
>> +	hdr = (void *)table + table->offset[0];
>> +	hdr->type = RSC_SPARE;
>> +
>> +	spare_rsc = (void *)hdr + sizeof(*hdr);
>> +	spare_rsc->len = size - table->offset[0] - sizeof(*hdr) - sizeof(*spare_rsc);
>> +
>> +	/* add new resource one by one */
>
> "resources"
thanks
>
>> +	list_for_each_entry(resource, &rproc->override_resources, node) {
>> +		if (resource->action == RSC_ACT_LOCAL) {
>> +			/* Create a new enty */
>
> This comment doesn't add any more information than the function name.
I'll remove
>
>> +			ret = __add_rsc_tbl_entry(rproc, resource,
>> +					table, size);
>> +			if (ret) {
>> +				table = ERR_PTR(ret);
>> +				goto out;
>> +			}
>> +		}
>> +	}
>> +
>> +	*tablesz = size;
>> +	rproc_dump_resource_table(rproc, table, *tablesz);
>
> This is going to add up to a lot of dumps of the resource table?
No only once when the complete table is populated
>
>> +out:
>> +	return table;
>> +}
>> +
>> +
>
> Superfluous '\n'.
ok

Thanks for your review Lee, I'll prepare a V3 including your remarks

Regards,
Loic
>
>>  /*
>>   * take a firmware and boot a remote processor with it.
>>   */
>> @@ -1278,7 +1337,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, *loaded_table;
>> -	int ret, tablesz;
>> +	int ret, tablesz, local_tablesz;
>>
>>  	ret = rproc_fw_sanity_check(rproc, fw);
>>  	if (ret)
>> @@ -1335,6 +1394,11 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>>  			goto clean_up;
>>  		}
>>
>> +		rproc->local_table = rproc_local_resource_create(rproc, &local_tablesz);
>> +		if (IS_ERR(rproc->local_table)) {
>> +			dev_err(dev, "Failed to create local resource table\n");
>> +			goto clean_up;
>> +		}
>>  	}
>>
>>  	/* reset max_notifyid */
>> @@ -1348,6 +1412,13 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>>  		goto clean_up;
>>  	}
>>
>> +	ret = rproc_handle_resources(rproc, rproc->local_table,
>> +				     local_tablesz, rproc_vdev_handler);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to handle vdev resources: %d\n", ret);
>> +		goto clean_up;
>> +	}
>> +
>>  	/* handle fw resources which are required to boot rproc */
>>  	ret = rproc_handle_resources(rproc, rproc->cached_table, tablesz,
>>  				     rproc_loading_handlers);
>> @@ -1356,6 +1427,13 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>>  		goto clean_up;
>>  	}
>>
>> +	ret = rproc_handle_resources(rproc, rproc->local_table,
>> +				     local_tablesz, rproc_loading_handlers);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to handle vdev resources: %d\n", ret);
>> +		goto clean_up;
>> +	}
>> +
>>  	/* load the ELF segments to memory */
>>  	ret = rproc_load_segments(rproc, fw);
>>  	if (ret) {
>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>> index 2b0f1d7..653e6f3 100644
>> --- a/include/linux/remoteproc.h
>> +++ b/include/linux/remoteproc.h
>> @@ -495,6 +495,7 @@ struct rproc {
>>  	int max_notifyid;
>>  	struct resource_table *table_ptr;
>>  	struct resource_table *cached_table;
>> +	struct resource_table *local_table;
>>  	bool has_iommu;
>>  	bool auto_boot;
>>  };
>

Re: [PATCH v2 18/19] remoteproc: core: Add function to create remoteproc local resource table

[Bjorn Andersson]

On Wed 31 Aug 13:50 PDT 2016, Loic Pallardy wrote:

> Rproc driver has now the capability to add resources dynamically
> thanks to rproc_request_resource API.
> Depending on associated action, resource request could impact
> firmware resource table or define new local resource.
> 
> In order to preserve current remoteproc resource handling
> mechanism, all local resources are gathered in a local resource
> table which won't be shared with firmware and proceed by
> remoteproc core as firmware one.
> 
> It is rproc driver responsibility to provide the right resource
> information using rproc_request_resource API.
> 
> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> ---
>  drivers/remoteproc/remoteproc_core.c | 80 +++++++++++++++++++++++++++++++++++-
>  include/linux/remoteproc.h           |  1 +
>  2 files changed, 80 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index cbfbdf8..73b460a 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1270,6 +1270,65 @@ static int rproc_apply_resource_overrides(struct rproc *rproc,
>  	return ret;
>  }
>  
> +static struct resource_table*
> +rproc_local_resource_create(struct rproc *rproc, int *tablesz)

The resource table is a persistent data structure and as such it's
awkward and inconvenient to operate on. This does, unfortunately, show
throughout this series.

But to here traverse a list of resources, generate a new table of the
left-over ones just so that we can allocate them and put them back on
another list is a clear sign to me that we're doing it wrong way.

Further more, we will have to support 64 bit addresses in the firmware
so I do not want to lock in the core implementation in version 1 of the
resource table format.



Rather than using the resource tables directly I think we should parse
the resource table into lists of objects that are convenient to operate
on. While doing this we can squash "duplicates" and validate that they
are compatible.

This allows the driver to (optionally) register static resources
and the resource table parser to (optionally!) register firmware
resources. We would then traverse these lists and if resources are
referenced update the resource table entries - i.e. like vrings are
handled already.


We can do this two ways:
1) As we register entries in these lists we check for previous
allocations and fall back to allocate the resources directly. Allowing
the drivers to fill in the lists and override resources from the table
parser. A drawback with this approach is that driver-registered
resources must stay allocated throughout the driver's life cycle.

2) We defer allocations to after we've built up the resource lists,
allowing the static/driver resources to be allocated as we boot the
rproc. Grouping the allocations would, likely, make it easier to reason
about error paths etc.

Regards,
Bjorn

Re: [PATCH v2 18/19] remoteproc: core: Add function to create remoteproc local resource table

[loic pallardy]

On 09/15/2016 08:58 PM, Bjorn Andersson wrote:
> On Wed 31 Aug 13:50 PDT 2016, Loic Pallardy wrote:
>
>> Rproc driver has now the capability to add resources dynamically
>> thanks to rproc_request_resource API.
>> Depending on associated action, resource request could impact
>> firmware resource table or define new local resource.
>>
>> In order to preserve current remoteproc resource handling
>> mechanism, all local resources are gathered in a local resource
>> table which won't be shared with firmware and proceed by
>> remoteproc core as firmware one.
>>
>> It is rproc driver responsibility to provide the right resource
>> information using rproc_request_resource API.
>>
>> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
>> ---
>>  drivers/remoteproc/remoteproc_core.c | 80 +++++++++++++++++++++++++++++++++++-
>>  include/linux/remoteproc.h           |  1 +
>>  2 files changed, 80 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>> index cbfbdf8..73b460a 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -1270,6 +1270,65 @@ static int rproc_apply_resource_overrides(struct rproc *rproc,
>>  	return ret;
>>  }
>>
>> +static struct resource_table*
>> +rproc_local_resource_create(struct rproc *rproc, int *tablesz)
>
> The resource table is a persistent data structure and as such it's
> awkward and inconvenient to operate on. This does, unfortunately, show
> throughout this series.
>
> But to here traverse a list of resources, generate a new table of the
> left-over ones just so that we can allocate them and put them back on
> another list is a clear sign to me that we're doing it wrong way.

Ok goal was to keep current resource allocation mechanism without 
changing current rproc_handle_xxx functions.
The idea was to avoid fake resource table creation at rproc driver level...
>
> Further more, we will have to support 64 bit addresses in the firmware
> so I do not want to lock in the core implementation in version 1 of the
> resource table format.
>
>
>
> Rather than using the resource tables directly I think we should parse
> the resource table into lists of objects that are convenient to operate
> on. While doing this we can squash "duplicates" and validate that they
> are compatible.

Yes possible to keep only internal list of request.
>
> This allows the driver to (optionally) register static resources
> and the resource table parser to (optionally!) register firmware
> resources. We would then traverse these lists and if resources are
> referenced update the resource table entries - i.e. like vrings are
> handled already.
>
>
> We can do this two ways:
> 1) As we register entries in these lists we check for previous
> allocations and fall back to allocate the resources directly. Allowing
> the drivers to fill in the lists and override resources from the table
> parser. A drawback with this approach is that driver-registered
> resources must stay allocated throughout the driver's life cycle.
>
> 2) We defer allocations to after we've built up the resource lists,
> allowing the static/driver resources to be allocated as we boot the
> rproc. Grouping the allocations would, likely, make it easier to reason
> about error paths etc.
>
As we are discussing specific rproc allocation in another thread and 
that local resource management is linked to, I propose to drop local 
resource concept from this series and come back later with a solution 
alignment with dedicated rproc memory chunk management.

Regards,
Loic

> Regards,
> Bjorn
>

Re: [PATCH v2 18/19] remoteproc: core: Add function to create remoteproc local resource table

[loic pallardy]

On 09/15/2016 08:58 PM, Bjorn Andersson wrote:
> On Wed 31 Aug 13:50 PDT 2016, Loic Pallardy wrote:
>
>> Rproc driver has now the capability to add resources dynamically
>> thanks to rproc_request_resource API.
>> Depending on associated action, resource request could impact
>> firmware resource table or define new local resource.
>>
>> In order to preserve current remoteproc resource handling
>> mechanism, all local resources are gathered in a local resource
>> table which won't be shared with firmware and proceed by
>> remoteproc core as firmware one.
>>
>> It is rproc driver responsibility to provide the right resource
>> information using rproc_request_resource API.
>>
>> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
>> ---
>>  drivers/remoteproc/remoteproc_core.c | 80 +++++++++++++++++++++++++++++++++++-
>>  include/linux/remoteproc.h           |  1 +
>>  2 files changed, 80 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>> index cbfbdf8..73b460a 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -1270,6 +1270,65 @@ static int rproc_apply_resource_overrides(struct rproc *rproc,
>>  	return ret;
>>  }
>>
>> +static struct resource_table*
>> +rproc_local_resource_create(struct rproc *rproc, int *tablesz)
>
> The resource table is a persistent data structure and as such it's
> awkward and inconvenient to operate on. This does, unfortunately, show
> throughout this series.
>
> But to here traverse a list of resources, generate a new table of the
> left-over ones just so that we can allocate them and put them back on
> another list is a clear sign to me that we're doing it wrong way.

Ok goal was to keep current resource allocation mechanism without 
changing current rproc_handle_xxx functions.
The idea was to avoid fake resource table creation at rproc driver level...
>
> Further more, we will have to support 64 bit addresses in the firmware
> so I do not want to lock in the core implementation in version 1 of the
> resource table format.
>
>
>
> Rather than using the resource tables directly I think we should parse
> the resource table into lists of objects that are convenient to operate
> on. While doing this we can squash "duplicates" and validate that they
> are compatible.

Yes possible to keep only internal list of request.
>
> This allows the driver to (optionally) register static resources
> and the resource table parser to (optionally!) register firmware
> resources. We would then traverse these lists and if resources are
> referenced update the resource table entries - i.e. like vrings are
> handled already.
>
>
> We can do this two ways:
> 1) As we register entries in these lists we check for previous
> allocations and fall back to allocate the resources directly. Allowing
> the drivers to fill in the lists and override resources from the table
> parser. A drawback with this approach is that driver-registered
> resources must stay allocated throughout the driver's life cycle.
>
> 2) We defer allocations to after we've built up the resource lists,
> allowing the static/driver resources to be allocated as we boot the
> rproc. Grouping the allocations would, likely, make it easier to reason
> about error paths etc.
>
As we are discussing specific rproc allocation in another thread and 
that local resource management is linked to, I propose to drop local 
resource concept from this series and come back later with a solution 
alignment with dedicated rproc memory chunk management.

Regards,
Loic

> Regards,
> Bjorn
>

[PATCH v2 19/19] remoteproc: core: Support empty resource tables

[Loic Pallardy]

From: Lee Jones <lee.jones@linaro.org>

Currently, when a remote processor does not require resources, the
platform-specific remoteproc driver has to create a fake resource
table in order to by-pass the strict checking.  But there is no hard
requirement for a remote processor so require or support shared
resources.  This patch removes the strict checking and skips
resource table related operations if none is provided.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 73b460a..235ca2a 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -685,6 +685,9 @@ static int rproc_handle_resources(struct rproc *rproc, struct resource_table *ta
 	rproc_handle_resource_t handler;
 	int ret = 0, i;
 
+	if (!table_ptr)
+		return ret;
+
 	for (i = 0; i < table_ptr->num; i++) {
 		int offset = table_ptr->offset[i];
 		struct fw_rsc_hdr *hdr = (void *)table_ptr + offset;
@@ -1337,7 +1340,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, *loaded_table;
-	int ret, tablesz, local_tablesz;
+	int ret, tablesz = 0, local_tablesz;
 
 	ret = rproc_fw_sanity_check(rproc, fw);
 	if (ret)
@@ -1361,8 +1364,8 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 	/* 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;
+		rproc->cached_table = table;
+		goto skip_resources;
 	}
 
 	/*  verify resource table consistency */
@@ -1384,6 +1387,7 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 		goto clean_up;
 	}
 
+skip_resources:
 	rproc->table_ptr = rproc->cached_table;
 
 	if (!list_empty(&rproc->override_resources)) {
-- 
1.9.1

[PATCH v2 19/19] remoteproc: core: Support empty resource tables

[Loic Pallardy]

From: Lee Jones <lee.jones@linaro.org>

Currently, when a remote processor does not require resources, the
platform-specific remoteproc driver has to create a fake resource
table in order to by-pass the strict checking.  But there is no hard
requirement for a remote processor so require or support shared
resources.  This patch removes the strict checking and skips
resource table related operations if none is provided.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 73b460a..235ca2a 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -685,6 +685,9 @@ static int rproc_handle_resources(struct rproc *rproc, struct resource_table *ta
 	rproc_handle_resource_t handler;
 	int ret = 0, i;
 
+	if (!table_ptr)
+		return ret;
+
 	for (i = 0; i < table_ptr->num; i++) {
 		int offset = table_ptr->offset[i];
 		struct fw_rsc_hdr *hdr = (void *)table_ptr + offset;
@@ -1337,7 +1340,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, *loaded_table;
-	int ret, tablesz, local_tablesz;
+	int ret, tablesz = 0, local_tablesz;
 
 	ret = rproc_fw_sanity_check(rproc, fw);
 	if (ret)
@@ -1361,8 +1364,8 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 	/* 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;
+		rproc->cached_table = table;
+		goto skip_resources;
 	}
 
 	/*  verify resource table consistency */
@@ -1384,6 +1387,7 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 		goto clean_up;
 	}
 
+skip_resources:
 	rproc->table_ptr = rproc->cached_table;
 
 	if (!list_empty(&rproc->override_resources)) {
-- 
1.9.1