[PATCH 0/9] remoteproc: Support bi-directional vdev config space

[PATCH 0/9] remoteproc: Support bi-directional vdev config space

[sjur.brandeland]

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

This patch-set adds support for shared resource table between
Linux kernel and remote devices.
- dynamically-allocated address of the vrings can be communicated
- vdev statuses can be communicated
- virtio config space becomes bi-directional
- virtio feature negotiation is two-way
- max_nofiyid is reported correctly

No device firmware changes are required for this patch-set.

Cheers,
Sjur

Dmitry Tarnyagin (1):
  remoteproc: Bugfix: Deallocate firmware image on shutdown

Sjur Brændeland (8):
  remoteproc: Refactor function rproc_elf_find_rsc_table
  remoteproc: Parse ELF file to find resource table address
  remoteproc: Parse STE-firmware and find resource table address
  remoteproc: Set vring addresses in resource table
  remoteproc: Support virtio config space.
  remoteproc: Code cleanup of resource parsing
  remoteproc: Calculate max_notifyid by counting vrings
  remoteproc: Always perserve resource table data

 drivers/remoteproc/remoteproc_core.c       |  146 ++++++++++++++++------------
 drivers/remoteproc/remoteproc_elf_loader.c |   96 +++++++++++++------
 drivers/remoteproc/remoteproc_internal.h   |   13 +++
 drivers/remoteproc/remoteproc_virtio.c     |   38 +++++++-
 drivers/remoteproc/ste_modem_rproc.c       |   50 +++++++---
 include/linux/remoteproc.h                 |    7 +-
 6 files changed, 240 insertions(+), 110 deletions(-)

-- 
1.7.5.4

[PATCH 1/9] remoteproc: Bugfix: Deallocate firmware image on shutdown

[sjur.brandeland]

From: Dmitry Tarnyagin <dmitry.tarnyagin@stericsson.com>

Fixes coherent memory leakage, caused by non-deallocated
firmware image chunk.

Signed-off-by: Dmitry Tarnyagin <dmitry.tarnyagin@stericsson.com>
---
 drivers/remoteproc/ste_modem_rproc.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/drivers/remoteproc/ste_modem_rproc.c b/drivers/remoteproc/ste_modem_rproc.c
index a7743c0..fb95c42 100644
--- a/drivers/remoteproc/ste_modem_rproc.c
+++ b/drivers/remoteproc/ste_modem_rproc.c
@@ -240,6 +240,8 @@ static int sproc_drv_remove(struct platform_device *pdev)
 
 	/* Unregister as remoteproc device */
 	rproc_del(sproc->rproc);
+	dma_free_coherent(sproc->rproc->dev.parent, SPROC_FW_SIZE,
+			  sproc->fw_addr, sproc->fw_dma_addr);
 	rproc_put(sproc->rproc);
 
 	mdev->drv_data = NULL;
@@ -297,10 +299,13 @@ static int sproc_probe(struct platform_device *pdev)
 	/* Register as a remoteproc device */
 	err = rproc_add(rproc);
 	if (err)
-		goto free_rproc;
+		goto free_mem;
 
 	return 0;
 
+free_mem:
+	dma_free_coherent(rproc->dev.parent, SPROC_FW_SIZE,
+			  sproc->fw_addr, sproc->fw_dma_addr);
 free_rproc:
 	/* Reset device data upon error */
 	mdev->drv_data = NULL;
-- 
1.7.5.4

[PATCH 2/9] remoteproc: Refactor function rproc_elf_find_rsc_table

[sjur.brandeland]

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

Refactor rproc_elf_find_rsc_table and split out the scanning
for the section header named resource table. This is done to
prepare for loading firmware once.

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 drivers/remoteproc/remoteproc_elf_loader.c |   79 ++++++++++++++++++----------
 1 files changed, 52 insertions(+), 27 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_elf_loader.c b/drivers/remoteproc/remoteproc_elf_loader.c
index 0d36f94..a958950 100644
--- a/drivers/remoteproc/remoteproc_elf_loader.c
+++ b/drivers/remoteproc/remoteproc_elf_loader.c
@@ -208,38 +208,19 @@ rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
 	return ret;
 }
 
-/**
- * rproc_elf_find_rsc_table() - find the resource table
- * @rproc: the rproc handle
- * @fw: the ELF firmware image
- * @tablesz: place holder for providing back the table size
- *
- * This function finds the resource table inside the remote processor's
- * firmware. It is used both upon the registration of @rproc (in order
- * to look for and register the supported virito devices), and when the
- * @rproc is booted.
- *
- * Returns the pointer to the resource table if it is found, and write its
- * size into @tablesz. If a valid table isn't found, NULL is returned
- * (and @tablesz isn't set).
- */
-static struct resource_table *
-rproc_elf_find_rsc_table(struct rproc *rproc, const struct firmware *fw,
-							int *tablesz)
+static struct elf32_shdr *
+find_rsc_shdr(struct device *dev, struct elf32_hdr *ehdr, size_t fw_size)
 {
-	struct elf32_hdr *ehdr;
 	struct elf32_shdr *shdr;
+	int i;
 	const char *name_table;
-	struct device *dev = &rproc->dev;
 	struct resource_table *table = NULL;
-	int i;
-	const u8 *elf_data = fw->data;
+	const u8 *elf_data = (void *)ehdr;
 
-	ehdr = (struct elf32_hdr *)elf_data;
+	/* look for the resource table and handle it */
 	shdr = (struct elf32_shdr *)(elf_data + ehdr->e_shoff);
 	name_table = elf_data + shdr[ehdr->e_shstrndx].sh_offset;
 
-	/* look for the resource table and handle it */
 	for (i = 0; i < ehdr->e_shnum; i++, shdr++) {
 		int size = shdr->sh_size;
 		int offset = shdr->sh_offset;
@@ -250,7 +231,7 @@ rproc_elf_find_rsc_table(struct rproc *rproc, const struct firmware *fw,
 		table = (struct resource_table *)(elf_data + offset);
 
 		/* make sure we have the entire table */
-		if (offset + size > fw->size) {
+		if (offset + size > fw_size) {
 			dev_err(dev, "resource table truncated\n");
 			return NULL;
 		}
@@ -280,10 +261,54 @@ rproc_elf_find_rsc_table(struct rproc *rproc, const struct firmware *fw,
 			return NULL;
 		}
 
-		*tablesz = shdr->sh_size;
-		break;
+		return shdr;
 	}
 
+	return NULL;
+}
+
+/**
+ * rproc_elf_find_rsc_table() - find the resource table
+ * @rproc: the rproc handle
+ * @fw: the ELF firmware image
+ * @tablesz: place holder for providing back the table size
+ *
+ * This function finds the resource table inside the remote processor's
+ * firmware. It is used both upon the registration of @rproc (in order
+ * to look for and register the supported virito devices), and when the
+ * @rproc is booted.
+ *
+ * Returns the pointer to the resource table if it is found, and write its
+ * size into @tablesz. If a valid table isn't found, NULL is returned
+ * (and @tablesz isn't set).
+ */
+static struct resource_table *
+rproc_elf_find_rsc_table(struct rproc *rproc, const struct firmware *fw,
+							int *tablesz)
+{
+	struct elf32_hdr *ehdr;
+	struct elf32_shdr *shdr;
+
+	struct device *dev = &rproc->dev;
+	struct resource_table *table = NULL;
+
+	const u8 *elf_data = fw->data;
+
+	ehdr = (struct elf32_hdr *)elf_data;
+
+	shdr = find_rsc_shdr(dev, ehdr, fw->size);
+	if (!shdr)
+		return NULL;
+
+	/* make sure we have the entire table */
+	if (shdr->sh_offset + shdr->sh_size > fw->size) {
+		dev_err(dev, "resource table truncated\n");
+		return NULL;
+	}
+
+	table = (struct resource_table *)(elf_data + shdr->sh_offset);
+	*tablesz = shdr->sh_size;
+
 	return table;
 }
 
-- 
1.7.5.4

[PATCH 3/9] remoteproc: Parse ELF file to find resource table address

[sjur.brandeland]

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

Add function find_rsc_table_va to firmware ops. This function
returns the location of the resource table in shared memory
after loading.

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 drivers/remoteproc/remoteproc_elf_loader.c |   17 ++++++++++++++++-
 drivers/remoteproc/remoteproc_internal.h   |   13 +++++++++++++
 2 files changed, 29 insertions(+), 1 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_elf_loader.c b/drivers/remoteproc/remoteproc_elf_loader.c
index a958950..3137fba 100644
--- a/drivers/remoteproc/remoteproc_elf_loader.c
+++ b/drivers/remoteproc/remoteproc_elf_loader.c
@@ -312,9 +312,24 @@ rproc_elf_find_rsc_table(struct rproc *rproc, const struct firmware *fw,
 	return table;
 }
 
+struct resource_table *rproc_elf_get_rsctab_addr(struct rproc *rproc,
+						 const struct firmware *fw)
+{
+	struct elf32_shdr *shdr;
+
+	shdr = find_rsc_shdr(&rproc->dev, (struct elf32_hdr *)fw->data,
+				fw->size);
+	if (!shdr)
+		return NULL;
+
+	/* Find resource table in loaded segments */
+	return rproc_da_to_va(rproc, shdr->sh_addr, shdr->sh_size);
+}
+
 const struct rproc_fw_ops rproc_elf_fw_ops = {
 	.load = rproc_elf_load_segments,
 	.find_rsc_table = rproc_elf_find_rsc_table,
 	.sanity_check = rproc_elf_sanity_check,
-	.get_boot_addr = rproc_elf_get_boot_addr
+	.get_boot_addr = rproc_elf_get_boot_addr,
+	.get_rsctab_addr = rproc_elf_get_rsctab_addr
 };
diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
index 7bb6648..3a5cb7d 100644
--- a/drivers/remoteproc/remoteproc_internal.h
+++ b/drivers/remoteproc/remoteproc_internal.h
@@ -32,6 +32,7 @@ struct rproc;
  *			expects to find it
  * @sanity_check:	sanity check the fw image
  * @get_boot_addr:	get boot address to entry point specified in firmware
+ * @get_rsctab_addr:	get resouce table address as specified in firmware
  */
 struct rproc_fw_ops {
 	struct resource_table *(*find_rsc_table) (struct rproc *rproc,
@@ -40,6 +41,8 @@ struct rproc_fw_ops {
 	int (*load)(struct rproc *rproc, const struct firmware *fw);
 	int (*sanity_check)(struct rproc *rproc, const struct firmware *fw);
 	u32 (*get_boot_addr)(struct rproc *rproc, const struct firmware *fw);
+	struct resource_table *(*get_rsctab_addr)(struct rproc *rproc,
+						const struct firmware *fw);
 };
 
 /* from remoteproc_core.c */
@@ -102,6 +105,16 @@ struct resource_table *rproc_find_rsc_table(struct rproc *rproc,
 	return NULL;
 }
 
+static inline
+struct resource_table *rproc_get_rsctab_addr(struct rproc *rproc,
+				const struct firmware *fw)
+{
+	if (rproc->fw_ops->get_rsctab_addr)
+		return rproc->fw_ops->get_rsctab_addr(rproc, fw);
+
+	return NULL;
+}
+
 extern const struct rproc_fw_ops rproc_elf_fw_ops;
 
 #endif /* REMOTEPROC_INTERNAL_H */
-- 
1.7.5.4

[PATCH 4/9] remoteproc: Parse STE-firmware and find resource table address

[sjur.brandeland]

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

Parse the STE firmware and scan the TOC-table to find the address
of the resource table.

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 drivers/remoteproc/ste_modem_rproc.c |   43 +++++++++++++++++++++++-----------
 1 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/drivers/remoteproc/ste_modem_rproc.c b/drivers/remoteproc/ste_modem_rproc.c
index fb95c42..932e97d 100644
--- a/drivers/remoteproc/ste_modem_rproc.c
+++ b/drivers/remoteproc/ste_modem_rproc.c
@@ -64,26 +64,18 @@ static int sproc_load_segments(struct rproc *rproc, const struct firmware *fw)
 }
 
 /* Find the entry for resource table in the Table of Content */
-static struct ste_toc_entry *sproc_find_rsc_entry(const struct firmware *fw)
+static const struct ste_toc_entry *sproc_find_rsc_entry(const void *data)
 {
 	int i;
-	struct ste_toc *toc;
-
-	if (!fw)
-		return NULL;
-
-	toc = (void *)fw->data;
+	const struct ste_toc *toc;
+	toc = data;
 
 	/* Search the table for the resource table */
 	for (i = 0; i < SPROC_MAX_TOC_ENTRIES &&
 		    toc->table[i].start != 0xffffffff; i++) {
-
 		if (!strncmp(toc->table[i].name, SPROC_RESOURCE_NAME,
-			     sizeof(toc->table[i].name))) {
-			if (toc->table[i].start > fw->size)
-				return NULL;
+			     sizeof(toc->table[i].name)))
 			return &toc->table[i];
-		}
 	}
 
 	return NULL;
@@ -96,9 +88,12 @@ sproc_find_rsc_table(struct rproc *rproc, const struct firmware *fw,
 {
 	struct sproc *sproc = rproc->priv;
 	struct resource_table *table;
-	struct ste_toc_entry *entry;
+	const struct ste_toc_entry *entry;
 
-	entry = sproc_find_rsc_entry(fw);
+	if (!fw)
+		return NULL;
+
+	entry = sproc_find_rsc_entry(fw->data);
 	if (!entry) {
 		sproc_err(sproc, "resource table not found in fw\n");
 		return NULL;
@@ -149,10 +144,30 @@ sproc_find_rsc_table(struct rproc *rproc, const struct firmware *fw,
 	return table;
 }
 
+/* Find the resource table inside the remote processor's firmware. */
+static struct resource_table *
+sproc_get_rsctab_addr(struct rproc *rproc, const struct firmware *fw)
+{
+	struct sproc *sproc = rproc->priv;
+	const struct ste_toc_entry *entry;
+
+	if (!fw || !sproc->fw_addr)
+		return NULL;
+
+	entry = sproc_find_rsc_entry(sproc->fw_addr);
+	if (!entry) {
+		sproc_err(sproc, "resource table not found in fw\n");
+		return NULL;
+	}
+
+	return sproc->fw_addr + entry->start;
+}
+
 /* STE modem firmware handler operations */
 const struct rproc_fw_ops sproc_fw_ops = {
 	.load = sproc_load_segments,
 	.find_rsc_table = sproc_find_rsc_table,
+	.get_rsctab_addr = sproc_get_rsctab_addr,
 };
 
 /* Kick the modem with specified notification id */
-- 
1.7.5.4

[PATCH 5/9] remoteproc: Set vring addresses in resource table

[sjur.brandeland]

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

Set the vring addresses in the resource table so that
the remote device can read the actual addresses used.

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 drivers/remoteproc/remoteproc_core.c |   13 +++++++++++--
 include/linux/remoteproc.h           |    2 ++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index dd3bfaf..c12a385 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -207,7 +207,6 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
 	/*
 	 * Allocate non-cacheable memory for the vring. In the future
 	 * this call will also configure the IOMMU for us
-	 * TODO: let the rproc know the da of this vring
 	 */
 	va = dma_alloc_coherent(dev->parent, size, &dma, GFP_KERNEL);
 	if (!va) {
@@ -218,7 +217,6 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
 	/*
 	 * Assign an rproc-wide unique index for this vring
 	 * TODO: assign a notifyid for rvdev updates as well
-	 * TODO: let the rproc know the notifyid of this vring
 	 * TODO: support predefined notifyids (via resource table)
 	 */
 	ret = idr_get_new(&rproc->notifyids, rvring, &notifyid);
@@ -238,6 +236,14 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
 	rvring->dma = dma;
 	rvring->notifyid = notifyid;
 
+	/*
+	 * Let the rproc know the notifyid and da of this vring.
+	 * Not all platforms use dma_alloc_coherent to automatically
+	 * set up the iommu. In this case the device address (da) will
+	 * hold the physical address and not the device address.
+	 */
+	rvdev->rsc->vring[i].da = dma;
+	rvdev->rsc->vring[i].notifyid = notifyid;
 	return 0;
 }
 
@@ -365,6 +371,9 @@ static int rproc_handle_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc,
 	/* remember the device features */
 	rvdev->dfeatures = rsc->dfeatures;
 
+	/* remember the resource entry */
+	rvdev->rsc = rsc;
+
 	list_add_tail(&rvdev->node, &rproc->rvdevs);
 
 	/* it is now safe to add the virtio device */
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index faf3332..cdacd66 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -464,6 +464,7 @@ struct rproc_vring {
  * @vring: the vrings for this vdev
  * @dfeatures: virtio device features
  * @gfeatures: virtio guest features
+ * @rsc: vdev resource entry
  */
 struct rproc_vdev {
 	struct list_head node;
@@ -472,6 +473,7 @@ struct rproc_vdev {
 	struct rproc_vring vring[RVDEV_NUM_VRINGS];
 	unsigned long dfeatures;
 	unsigned long gfeatures;
+	struct fw_rsc_vdev *rsc;
 };
 
 struct rproc *rproc_alloc(struct device *dev, const char *name,
-- 
1.7.5.4

[PATCH 6/9] remoteproc: Support virtio config space.

[sjur.brandeland]

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

Support virtio configuration space and device status and
feature negotiation with remote device. This virtio device
can now access the resource table in shared memory.

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 drivers/remoteproc/remoteproc_core.c   |    3 --
 drivers/remoteproc/remoteproc_virtio.c |   38 +++++++++++++++++++++++++++++--
 include/linux/remoteproc.h             |    2 -
 3 files changed, 35 insertions(+), 8 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index c12a385..1bf410d 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -368,9 +368,6 @@ static int rproc_handle_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc,
 			goto free_rvdev;
 	}
 
-	/* remember the device features */
-	rvdev->dfeatures = rsc->dfeatures;
-
 	/* remember the resource entry */
 	rvdev->rsc = rsc;
 
diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index 9e198e5..d98cdd8 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -182,16 +182,21 @@ error:
  */
 static u8 rproc_virtio_get_status(struct virtio_device *vdev)
 {
-	return 0;
+	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
+	return rvdev->rsc->status;
 }
 
 static void rproc_virtio_set_status(struct virtio_device *vdev, u8 status)
 {
+	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
+	rvdev->rsc->status = status;
 	dev_dbg(&vdev->dev, "status: %d\n", status);
 }
 
 static void rproc_virtio_reset(struct virtio_device *vdev)
 {
+	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
+	rvdev->rsc->status = 0;
 	dev_dbg(&vdev->dev, "reset !\n");
 }
 
@@ -200,7 +205,7 @@ static u32 rproc_virtio_get_features(struct virtio_device *vdev)
 {
 	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
 
-	return rvdev->dfeatures;
+	return rvdev->rsc->dfeatures;
 }
 
 static void rproc_virtio_finalize_features(struct virtio_device *vdev)
@@ -219,7 +224,31 @@ static void rproc_virtio_finalize_features(struct virtio_device *vdev)
 	 * fixed as part of a small resource table overhaul and then an
 	 * extension of the virtio resource entries.
 	 */
-	rvdev->gfeatures = vdev->features[0];
+	rvdev->rsc->gfeatures = vdev->features[0];
+}
+
+void rproc_virtio_get(struct virtio_device *vdev, unsigned offset,
+							void *buf, unsigned len)
+{
+	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
+	void *cfg = &rvdev->rsc->vring[rvdev->rsc->num_of_vrings];
+	if (offset + len > rvdev->rsc->config_len)
+		dev_err(&vdev->dev,
+			"rproc_virtio_get: access out of bounds\n");
+	else
+		memcpy(buf, cfg + offset, len);
+}
+
+void rproc_virtio_set(struct virtio_device *vdev, unsigned offset,
+		      const void *buf, unsigned len)
+{
+	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
+	void *cfg = &rvdev->rsc->vring[rvdev->rsc->num_of_vrings];
+	if (offset + len > rvdev->rsc->config_len)
+		dev_err(&vdev->dev,
+			"rproc_virtio_set: access out of bounds\n");
+	else
+		memcpy(cfg + offset, buf, len);
 }
 
 static struct virtio_config_ops rproc_virtio_config_ops = {
@@ -230,6 +259,9 @@ static struct virtio_config_ops rproc_virtio_config_ops = {
 	.reset		= rproc_virtio_reset,
 	.set_status	= rproc_virtio_set_status,
 	.get_status	= rproc_virtio_get_status,
+	.get		= rproc_virtio_get,
+	.set		= rproc_virtio_set,
+
 };
 
 /*
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index cdacd66..c0c363c 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -471,8 +471,6 @@ struct rproc_vdev {
 	struct rproc *rproc;
 	struct virtio_device vdev;
 	struct rproc_vring vring[RVDEV_NUM_VRINGS];
-	unsigned long dfeatures;
-	unsigned long gfeatures;
 	struct fw_rsc_vdev *rsc;
 };
 
-- 
1.7.5.4

[PATCH 7/9] remoteproc: Code cleanup of resource parsing

[sjur.brandeland]

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

Combine the almost identical functions rproc_handle_virtio_rsc
and rproc_handle_boot_rsc.

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 drivers/remoteproc/remoteproc_core.c |   51 ++++++++--------------------------
 1 files changed, 12 insertions(+), 39 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 1bf410d..ec9f81e 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -683,16 +683,22 @@ free_carv:
  * A lookup table for resource handlers. The indices are defined in
  * enum fw_resource_type.
  */
-static rproc_handle_resource_t rproc_handle_rsc[] = {
+static rproc_handle_resource_t rproc_handle_rsc[RSC_LAST] = {
 	[RSC_CARVEOUT] = (rproc_handle_resource_t)rproc_handle_carveout,
 	[RSC_DEVMEM] = (rproc_handle_resource_t)rproc_handle_devmem,
 	[RSC_TRACE] = (rproc_handle_resource_t)rproc_handle_trace,
 	[RSC_VDEV] = NULL, /* VDEVs were handled upon registrarion */
 };
 
+static rproc_handle_resource_t rproc_handle_vdev_rsc[RSC_LAST] = {
+	[RSC_VDEV] = (rproc_handle_resource_t)rproc_handle_vdev,
+};
+
 /* handle firmware resource entries before booting the remote processor */
 static int
-rproc_handle_boot_rsc(struct rproc *rproc, struct resource_table *table, int len)
+rproc_handle_resource(struct rproc *rproc, struct resource_table *table,
+				int len,
+				rproc_handle_resource_t handlers[RSC_LAST])
 {
 	struct device *dev = &rproc->dev;
 	rproc_handle_resource_t handler;
@@ -717,7 +723,7 @@ rproc_handle_boot_rsc(struct rproc *rproc, struct resource_table *table, int len
 			continue;
 		}
 
-		handler = rproc_handle_rsc[hdr->type];
+		handler = handlers[hdr->type];
 		if (!handler)
 			continue;
 
@@ -729,40 +735,6 @@ rproc_handle_boot_rsc(struct rproc *rproc, struct resource_table *table, int len
 	return ret;
 }
 
-/* handle firmware resource entries while registering the remote processor */
-static int
-rproc_handle_virtio_rsc(struct rproc *rproc, struct resource_table *table, int len)
-{
-	struct device *dev = &rproc->dev;
-	int ret = 0, i;
-
-	for (i = 0; i < table->num; i++) {
-		int offset = table->offset[i];
-		struct fw_rsc_hdr *hdr = (void *)table + offset;
-		int avail = len - offset - sizeof(*hdr);
-		struct fw_rsc_vdev *vrsc;
-
-		/* make sure table isn't truncated */
-		if (avail < 0) {
-			dev_err(dev, "rsc table is truncated\n");
-			return -EINVAL;
-		}
-
-		dev_dbg(dev, "%s: rsc type %d\n", __func__, hdr->type);
-
-		if (hdr->type != RSC_VDEV)
-			continue;
-
-		vrsc = (struct fw_rsc_vdev *)hdr->data;
-
-		ret = rproc_handle_vdev(rproc, vrsc, avail);
-		if (ret)
-			break;
-	}
-
-	return ret;
-}
-
 /**
  * rproc_resource_cleanup() - clean up and free all acquired resources
  * @rproc: rproc handle
@@ -842,7 +814,7 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 	}
 
 	/* handle fw resources which are required to boot rproc */
-	ret = rproc_handle_boot_rsc(rproc, table, tablesz);
+	ret = rproc_handle_resource(rproc, table, tablesz, rproc_handle_rsc);
 	if (ret) {
 		dev_err(dev, "Failed to process resources: %d\n", ret);
 		goto clean_up;
@@ -897,7 +869,8 @@ static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
 		goto out;
 
 	/* look for virtio devices and register them */
-	ret = rproc_handle_virtio_rsc(rproc, table, tablesz);
+	ret = rproc_handle_resource(rproc, table, tablesz,
+						rproc_handle_vdev_rsc);
 	if (ret)
 		goto out;
 
-- 
1.7.5.4

[PATCH 8/9] remoteproc: Calculate max_notifyid by counting vrings

[sjur.brandeland]

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

Simplify handling of max_notifyid by simply counting the
number of vrings.

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 drivers/remoteproc/remoteproc_core.c |   34 +++++++++++++++++++---------------
 1 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index ec9f81e..14f40eb 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -226,9 +226,6 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
 		return ret;
 	}
 
-	/* Store largest notifyid */
-	rproc->max_notifyid = max(rproc->max_notifyid, notifyid);
-
 	dev_dbg(dev, "vring%d: va %p dma %llx size %x idr %d\n", i, va,
 				(unsigned long long)dma, size, notifyid);
 
@@ -278,25 +275,13 @@ rproc_parse_vring(struct rproc_vdev *rvdev, struct fw_rsc_vdev *rsc, int i)
 	return 0;
 }
 
-static int rproc_max_notifyid(int id, void *p, void *data)
-{
-	int *maxid = data;
-	*maxid = max(*maxid, id);
-	return 0;
-}
-
 void rproc_free_vring(struct rproc_vring *rvring)
 {
 	int size = PAGE_ALIGN(vring_size(rvring->len, rvring->align));
 	struct rproc *rproc = rvring->rvdev->rproc;
-	int maxid = 0;
 
 	dma_free_coherent(rproc->dev.parent, size, rvring->va, rvring->dma);
 	idr_remove(&rproc->notifyids, rvring->notifyid);
-
-	/* Find the largest remaining notifyid */
-	idr_for_each(&rproc->notifyids, rproc_max_notifyid, &maxid);
-	rproc->max_notifyid = maxid;
 }
 
 /**
@@ -679,6 +664,15 @@ free_carv:
 	return ret;
 }
 
+static int rproc_handle_notifyid(struct rproc *rproc, struct fw_rsc_vdev *rsc,
+								int avail)
+{
+	/* Summerize the number of notification IDs */
+	rproc->max_notifyid += rsc->num_of_vrings;
+
+	return 0;
+}
+
 /*
  * A lookup table for resource handlers. The indices are defined in
  * enum fw_resource_type.
@@ -694,6 +688,10 @@ static rproc_handle_resource_t rproc_handle_vdev_rsc[RSC_LAST] = {
 	[RSC_VDEV] = (rproc_handle_resource_t)rproc_handle_vdev,
 };
 
+static rproc_handle_resource_t rproc_handle_notifyid_rsc[RSC_LAST] = {
+	[RSC_VDEV] = (rproc_handle_resource_t)rproc_handle_notifyid,
+};
+
 /* handle firmware resource entries before booting the remote processor */
 static int
 rproc_handle_resource(struct rproc *rproc, struct resource_table *table,
@@ -868,6 +866,12 @@ static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
 	if (!table)
 		goto out;
 
+	rproc->max_notifyid = 0;
+
+	/* count the numbe of notify-ids */
+	ret = rproc_handle_resource(rproc, table, tablesz,
+						rproc_handle_notifyid_rsc);
+
 	/* look for virtio devices and register them */
 	ret = rproc_handle_resource(rproc, table, tablesz,
 						rproc_handle_vdev_rsc);
-- 
1.7.5.4

[PATCH 9/9] remoteproc: Always perserve resource table data

[sjur.brandeland]

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

Copy resource table from first to second firmware loading.
After firmware is loaded to memory, update the vdevs resource
pointer to the resource table kept in device memory.

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 drivers/remoteproc/remoteproc_core.c |   61 +++++++++++++++++++++++++++------
 include/linux/remoteproc.h           |    3 ++
 2 files changed, 53 insertions(+), 11 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 14f40eb..13dc7b4 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -694,17 +694,16 @@ static rproc_handle_resource_t rproc_handle_notifyid_rsc[RSC_LAST] = {
 
 /* handle firmware resource entries before booting the remote processor */
 static int
-rproc_handle_resource(struct rproc *rproc, struct resource_table *table,
-				int len,
-				rproc_handle_resource_t handlers[RSC_LAST])
+rproc_handle_resource(struct rproc *rproc, 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 < table->num; i++) {
-		int offset = table->offset[i];
-		struct fw_rsc_hdr *hdr = (void *)table + offset;
+	for (i = 0; i < rproc->rsc->num; i++) {
+		int offset = rproc->rsc->offset[i];
+		struct fw_rsc_hdr *hdr = (void *)rproc->rsc + offset;
 		int avail = len - offset - sizeof(*hdr);
 		void *rsc = (void *)hdr + sizeof(*hdr);
 
@@ -783,9 +782,13 @@ 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;
+	struct rproc_vdev *rvdev;
+	struct resource_table *table, *devmem_rsc, *tmp;
 	int ret, tablesz;
 
+	if (!rproc->rsc)
+		return -ENOMEM;
+
 	ret = rproc_fw_sanity_check(rproc, fw);
 	if (ret)
 		return ret;
@@ -811,8 +814,17 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 		goto clean_up;
 	}
 
+	/* Verify that resource table in loaded fw is unchanged */
+	if (rproc->rsc_csum != ip_compute_csum(table, tablesz)) {
+		dev_err(dev, "resource checksum failed, fw changed?\n");
+		ret = -EINVAL;
+		goto clean_up;
+	}
+
+
 	/* handle fw resources which are required to boot rproc */
-	ret = rproc_handle_resource(rproc, table, tablesz, rproc_handle_rsc);
+	ret = rproc_handle_resource(rproc, tablesz,
+					rproc_handle_rsc);
 	if (ret) {
 		dev_err(dev, "Failed to process resources: %d\n", ret);
 		goto clean_up;
@@ -825,6 +837,26 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 		goto clean_up;
 	}
 
+	/* Get the resource table address in device memory */
+	devmem_rsc = rproc_get_rsctab_addr(rproc, fw);
+
+	/* Copy the updated resource table to device memory */
+	memcpy(devmem_rsc, rproc->rsc, tablesz);
+
+	/* Free the copy of the resource table */
+	tmp = rproc->rsc;
+	rproc->rsc = devmem_rsc;
+	kfree(tmp);
+
+	/* Update the vdev rsc address */
+	list_for_each_entry(rvdev, &rproc->rvdevs, node) {
+		int offset = (void *)rvdev->rsc - (void *)tmp;
+		rvdev->rsc = (void *)devmem_rsc + offset;
+	}
+
+	/* Other virtio drivers will see the rsc table in device memory */
+	rproc->rsc = devmem_rsc;
+
 	/* power up the remote processor */
 	ret = rproc->ops->start(rproc);
 	if (ret) {
@@ -866,14 +898,21 @@ static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
 	if (!table)
 		goto out;
 
-	rproc->max_notifyid = 0;
+	rproc->rsc_csum = ip_compute_csum(table, tablesz);
 
 	/* count the numbe of notify-ids */
-	ret = rproc_handle_resource(rproc, table, tablesz,
+	rproc->max_notifyid = 0;
+	rproc->rsc = table;
+	ret = rproc_handle_resource(rproc, tablesz,
 						rproc_handle_notifyid_rsc);
 
+	/* Copy resource table containing vdev config info */
+	rproc->rsc = kmalloc(tablesz, GFP_KERNEL);
+	if (rproc->rsc)
+		memcpy(rproc->rsc, table, tablesz);
+
 	/* look for virtio devices and register them */
-	ret = rproc_handle_resource(rproc, table, tablesz,
+	ret = rproc_handle_resource(rproc, tablesz,
 						rproc_handle_vdev_rsc);
 	if (ret)
 		goto out;
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index c0c363c..07deff4 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -41,6 +41,7 @@
 #include <linux/virtio.h>
 #include <linux/completion.h>
 #include <linux/idr.h>
+#include <asm/checksum.h>
 
 /**
  * struct resource_table - firmware resource table header
@@ -429,6 +430,8 @@ struct rproc {
 	struct completion crash_comp;
 	bool recovery_disabled;
 	int max_notifyid;
+	struct resource_table *rsc;
+	__sum16 rsc_csum;
 };
 
 /* we currently support only two vrings per rvdev */
-- 
1.7.5.4

Re: [PATCH 2/9] remoteproc: Refactor function rproc_elf_find_rsc_table

[Ido Yariv]

Hi Sjur,

Sorry for the (very) late reply.

On Sun, Feb 10, 2013 at 12:39:05PM +0100, sjur.brandeland@stericsson.com wrote:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
> 
> Refactor rproc_elf_find_rsc_table and split out the scanning
> for the section header named resource table. This is done to
> prepare for loading firmware once.
> 
> Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
> ---
>  drivers/remoteproc/remoteproc_elf_loader.c |   79 ++++++++++++++++++----------
>  1 files changed, 52 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_elf_loader.c b/drivers/remoteproc/remoteproc_elf_loader.c
> index 0d36f94..a958950 100644
> --- a/drivers/remoteproc/remoteproc_elf_loader.c
> +++ b/drivers/remoteproc/remoteproc_elf_loader.c
> @@ -208,38 +208,19 @@ rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
>  	return ret;
>  }
>  
> -/**
> - * rproc_elf_find_rsc_table() - find the resource table
> - * @rproc: the rproc handle
> - * @fw: the ELF firmware image
> - * @tablesz: place holder for providing back the table size
> - *
> - * This function finds the resource table inside the remote processor's
> - * firmware. It is used both upon the registration of @rproc (in order
> - * to look for and register the supported virito devices), and when the
> - * @rproc is booted.
> - *
> - * Returns the pointer to the resource table if it is found, and write its
> - * size into @tablesz. If a valid table isn't found, NULL is returned
> - * (and @tablesz isn't set).
> - */
> -static struct resource_table *
> -rproc_elf_find_rsc_table(struct rproc *rproc, const struct firmware *fw,
> -							int *tablesz)
> +static struct elf32_shdr *
> +find_rsc_shdr(struct device *dev, struct elf32_hdr *ehdr, size_t fw_size)
>  {
> -	struct elf32_hdr *ehdr;
>  	struct elf32_shdr *shdr;
> +	int i;
>  	const char *name_table;
> -	struct device *dev = &rproc->dev;
>  	struct resource_table *table = NULL;
> -	int i;
> -	const u8 *elf_data = fw->data;
> +	const u8 *elf_data = (void *)ehdr;
>  
> -	ehdr = (struct elf32_hdr *)elf_data;
> +	/* look for the resource table and handle it */
>  	shdr = (struct elf32_shdr *)(elf_data + ehdr->e_shoff);
>  	name_table = elf_data + shdr[ehdr->e_shstrndx].sh_offset;
>  
> -	/* look for the resource table and handle it */
>  	for (i = 0; i < ehdr->e_shnum; i++, shdr++) {
>  		int size = shdr->sh_size;
>  		int offset = shdr->sh_offset;
> @@ -250,7 +231,7 @@ rproc_elf_find_rsc_table(struct rproc *rproc, const struct firmware *fw,
>  		table = (struct resource_table *)(elf_data + offset);
>  
>  		/* make sure we have the entire table */
> -		if (offset + size > fw->size) {
> +		if (offset + size > fw_size) {

While we're at it, perhaps also verify that there aren't any integer overflows
here?

>  			dev_err(dev, "resource table truncated\n");
>  			return NULL;
>  		}
> @@ -280,10 +261,54 @@ rproc_elf_find_rsc_table(struct rproc *rproc, const struct firmware *fw,
>  			return NULL;
>  		}
>  
> -		*tablesz = shdr->sh_size;
> -		break;
> +		return shdr;
>  	}
>  
> +	return NULL;
> +}
> +
> +/**
> + * rproc_elf_find_rsc_table() - find the resource table
> + * @rproc: the rproc handle
> + * @fw: the ELF firmware image
> + * @tablesz: place holder for providing back the table size
> + *
> + * This function finds the resource table inside the remote processor's
> + * firmware. It is used both upon the registration of @rproc (in order
> + * to look for and register the supported virito devices), and when the
> + * @rproc is booted.
> + *
> + * Returns the pointer to the resource table if it is found, and write its
> + * size into @tablesz. If a valid table isn't found, NULL is returned
> + * (and @tablesz isn't set).
> + */
> +static struct resource_table *
> +rproc_elf_find_rsc_table(struct rproc *rproc, const struct firmware *fw,
> +							int *tablesz)
> +{
> +	struct elf32_hdr *ehdr;
> +	struct elf32_shdr *shdr;
> +
> +	struct device *dev = &rproc->dev;
> +	struct resource_table *table = NULL;
> +
> +	const u8 *elf_data = fw->data;
> +

Mind removing empty lines here?

> +	ehdr = (struct elf32_hdr *)elf_data;
> +
> +	shdr = find_rsc_shdr(dev, ehdr, fw->size);
> +	if (!shdr)
> +		return NULL;
> +
> +	/* make sure we have the entire table */
> +	if (shdr->sh_offset + shdr->sh_size > fw->size) {
> +		dev_err(dev, "resource table truncated\n");
> +		return NULL;
> +	}

Any reason for this explicit assertion? It seemed to be checked in
find_rsc_shdr.

> +
> +	table = (struct resource_table *)(elf_data + shdr->sh_offset);
> +	*tablesz = shdr->sh_size;
> +
>  	return table;
>  }
>  
> -- 
> 1.7.5.4
> 

Thanks,
Ido.

Re: [PATCH 3/9] remoteproc: Parse ELF file to find resource table address

[Ido Yariv]

Hi Sjur,

On Sun, Feb 10, 2013 at 12:39:06PM +0100, sjur.brandeland@stericsson.com wrote:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
> 
> Add function find_rsc_table_va to firmware ops. This function
> returns the location of the resource table in shared memory
> after loading.
> 
> Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
> ---
>  drivers/remoteproc/remoteproc_elf_loader.c |   17 ++++++++++++++++-
>  drivers/remoteproc/remoteproc_internal.h   |   13 +++++++++++++
>  2 files changed, 29 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_elf_loader.c b/drivers/remoteproc/remoteproc_elf_loader.c
> index a958950..3137fba 100644
> --- a/drivers/remoteproc/remoteproc_elf_loader.c
> +++ b/drivers/remoteproc/remoteproc_elf_loader.c
> @@ -312,9 +312,24 @@ rproc_elf_find_rsc_table(struct rproc *rproc, const struct firmware *fw,
>  	return table;
>  }
>  
> +struct resource_table *rproc_elf_get_rsctab_addr(struct rproc *rproc,
> +						 const struct firmware *fw)
> +{
> +	struct elf32_shdr *shdr;
> +
> +	shdr = find_rsc_shdr(&rproc->dev, (struct elf32_hdr *)fw->data,
> +				fw->size);
> +	if (!shdr)
> +		return NULL;
> +
> +	/* Find resource table in loaded segments */
> +	return rproc_da_to_va(rproc, shdr->sh_addr, shdr->sh_size);
> +}
> +
>  const struct rproc_fw_ops rproc_elf_fw_ops = {
>  	.load = rproc_elf_load_segments,
>  	.find_rsc_table = rproc_elf_find_rsc_table,
>  	.sanity_check = rproc_elf_sanity_check,
> -	.get_boot_addr = rproc_elf_get_boot_addr
> +	.get_boot_addr = rproc_elf_get_boot_addr,
> +	.get_rsctab_addr = rproc_elf_get_rsctab_addr

Since this new op returns a VA (unlike get_boot_addr), it would probably be
better to use an indicative name like get_rsctab_va.

>  };
> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
> index 7bb6648..3a5cb7d 100644
> --- a/drivers/remoteproc/remoteproc_internal.h
> +++ b/drivers/remoteproc/remoteproc_internal.h
> @@ -32,6 +32,7 @@ struct rproc;
>   *			expects to find it
>   * @sanity_check:	sanity check the fw image
>   * @get_boot_addr:	get boot address to entry point specified in firmware
> + * @get_rsctab_addr:	get resouce table address as specified in firmware
>   */
>  struct rproc_fw_ops {
>  	struct resource_table *(*find_rsc_table) (struct rproc *rproc,
> @@ -40,6 +41,8 @@ struct rproc_fw_ops {
>  	int (*load)(struct rproc *rproc, const struct firmware *fw);
>  	int (*sanity_check)(struct rproc *rproc, const struct firmware *fw);
>  	u32 (*get_boot_addr)(struct rproc *rproc, const struct firmware *fw);
> +	struct resource_table *(*get_rsctab_addr)(struct rproc *rproc,
> +						const struct firmware *fw);
>  };
>  
>  /* from remoteproc_core.c */
> @@ -102,6 +105,16 @@ struct resource_table *rproc_find_rsc_table(struct rproc *rproc,
>  	return NULL;
>  }
>  
> +static inline
> +struct resource_table *rproc_get_rsctab_addr(struct rproc *rproc,
> +				const struct firmware *fw)
> +{
> +	if (rproc->fw_ops->get_rsctab_addr)
> +		return rproc->fw_ops->get_rsctab_addr(rproc, fw);
> +
> +	return NULL;
> +}
> +
>  extern const struct rproc_fw_ops rproc_elf_fw_ops;
>  
>  #endif /* REMOTEPROC_INTERNAL_H */
> -- 
> 1.7.5.4
> 

Thanks,
Ido.

Re: [PATCH 6/9] remoteproc: Support virtio config space.

[Ido Yariv]

Hi Sjur,

On Sun, Feb 10, 2013 at 12:39:09PM +0100, sjur.brandeland@stericsson.com wrote:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
> 
> Support virtio configuration space and device status and
> feature negotiation with remote device. This virtio device
> can now access the resource table in shared memory.
> 
> Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
> ---
>  drivers/remoteproc/remoteproc_core.c   |    3 --
>  drivers/remoteproc/remoteproc_virtio.c |   38 +++++++++++++++++++++++++++++--
>  include/linux/remoteproc.h             |    2 -
>  3 files changed, 35 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index c12a385..1bf410d 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -368,9 +368,6 @@ static int rproc_handle_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc,
>  			goto free_rvdev;
>  	}
>  
> -	/* remember the device features */
> -	rvdev->dfeatures = rsc->dfeatures;
> -
>  	/* remember the resource entry */
>  	rvdev->rsc = rsc;
>  
> diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
> index 9e198e5..d98cdd8 100644
> --- a/drivers/remoteproc/remoteproc_virtio.c
> +++ b/drivers/remoteproc/remoteproc_virtio.c
> @@ -182,16 +182,21 @@ error:
>   */
>  static u8 rproc_virtio_get_status(struct virtio_device *vdev)
>  {
> -	return 0;
> +	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
> +	return rvdev->rsc->status;
>  }
>  
>  static void rproc_virtio_set_status(struct virtio_device *vdev, u8 status)
>  {
> +	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
> +	rvdev->rsc->status = status;
>  	dev_dbg(&vdev->dev, "status: %d\n", status);
>  }
>  
>  static void rproc_virtio_reset(struct virtio_device *vdev)
>  {
> +	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
> +	rvdev->rsc->status = 0;
>  	dev_dbg(&vdev->dev, "reset !\n");
>  }
>  
> @@ -200,7 +205,7 @@ static u32 rproc_virtio_get_features(struct virtio_device *vdev)
>  {
>  	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
>  
> -	return rvdev->dfeatures;
> +	return rvdev->rsc->dfeatures;
>  }
>  
>  static void rproc_virtio_finalize_features(struct virtio_device *vdev)
> @@ -219,7 +224,31 @@ static void rproc_virtio_finalize_features(struct virtio_device *vdev)
>  	 * fixed as part of a small resource table overhaul and then an
>  	 * extension of the virtio resource entries.
>  	 */
> -	rvdev->gfeatures = vdev->features[0];
> +	rvdev->rsc->gfeatures = vdev->features[0];
> +}
> +
> +void rproc_virtio_get(struct virtio_device *vdev, unsigned offset,
> +							void *buf, unsigned len)
> +{
> +	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
> +	void *cfg = &rvdev->rsc->vring[rvdev->rsc->num_of_vrings];
> +	if (offset + len > rvdev->rsc->config_len)

Perhaps check for integer overflow here?

> +		dev_err(&vdev->dev,
> +			"rproc_virtio_get: access out of bounds\n");
> +	else
> +		memcpy(buf, cfg + offset, len);
> +}
> +
> +void rproc_virtio_set(struct virtio_device *vdev, unsigned offset,
> +		      const void *buf, unsigned len)
> +{
> +	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
> +	void *cfg = &rvdev->rsc->vring[rvdev->rsc->num_of_vrings];
> +	if (offset + len > rvdev->rsc->config_len)

Here as well.

> +		dev_err(&vdev->dev,
> +			"rproc_virtio_set: access out of bounds\n");
> +	else
> +		memcpy(cfg + offset, buf, len);
>  }
>  
>  static struct virtio_config_ops rproc_virtio_config_ops = {
> @@ -230,6 +259,9 @@ static struct virtio_config_ops rproc_virtio_config_ops = {
>  	.reset		= rproc_virtio_reset,
>  	.set_status	= rproc_virtio_set_status,
>  	.get_status	= rproc_virtio_get_status,
> +	.get		= rproc_virtio_get,
> +	.set		= rproc_virtio_set,
> +
>  };
>  
>  /*
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index cdacd66..c0c363c 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -471,8 +471,6 @@ struct rproc_vdev {
>  	struct rproc *rproc;
>  	struct virtio_device vdev;
>  	struct rproc_vring vring[RVDEV_NUM_VRINGS];
> -	unsigned long dfeatures;
> -	unsigned long gfeatures;
>  	struct fw_rsc_vdev *rsc;
>  };
>  
> -- 
> 1.7.5.4
> 

Thanks,
Ido.

Re: [PATCH 8/9] remoteproc: Calculate max_notifyid by counting vrings

[Ido Yariv]

Hi Sjur,

On Sun, Feb 10, 2013 at 12:39:11PM +0100, sjur.brandeland@stericsson.com wrote:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
> 
> Simplify handling of max_notifyid by simply counting the
> number of vrings.
> 
> Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
> ---
>  drivers/remoteproc/remoteproc_core.c |   34 +++++++++++++++++++---------------
>  1 files changed, 19 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index ec9f81e..14f40eb 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -226,9 +226,6 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
>  		return ret;
>  	}
>  
> -	/* Store largest notifyid */
> -	rproc->max_notifyid = max(rproc->max_notifyid, notifyid);
> -
>  	dev_dbg(dev, "vring%d: va %p dma %llx size %x idr %d\n", i, va,
>  				(unsigned long long)dma, size, notifyid);
>  
> @@ -278,25 +275,13 @@ rproc_parse_vring(struct rproc_vdev *rvdev, struct fw_rsc_vdev *rsc, int i)
>  	return 0;
>  }
>  
> -static int rproc_max_notifyid(int id, void *p, void *data)
> -{
> -	int *maxid = data;
> -	*maxid = max(*maxid, id);
> -	return 0;
> -}
> -
>  void rproc_free_vring(struct rproc_vring *rvring)
>  {
>  	int size = PAGE_ALIGN(vring_size(rvring->len, rvring->align));
>  	struct rproc *rproc = rvring->rvdev->rproc;
> -	int maxid = 0;
>  
>  	dma_free_coherent(rproc->dev.parent, size, rvring->va, rvring->dma);
>  	idr_remove(&rproc->notifyids, rvring->notifyid);
> -
> -	/* Find the largest remaining notifyid */
> -	idr_for_each(&rproc->notifyids, rproc_max_notifyid, &maxid);
> -	rproc->max_notifyid = maxid;
>  }
>  
>  /**
> @@ -679,6 +664,15 @@ free_carv:
>  	return ret;
>  }
>  
> +static int rproc_handle_notifyid(struct rproc *rproc, struct fw_rsc_vdev *rsc,
> +								int avail)
> +{
> +	/* Summerize the number of notification IDs */
> +	rproc->max_notifyid += rsc->num_of_vrings;
> +
> +	return 0;
> +}
> +
>  /*
>   * A lookup table for resource handlers. The indices are defined in
>   * enum fw_resource_type.
> @@ -694,6 +688,10 @@ static rproc_handle_resource_t rproc_handle_vdev_rsc[RSC_LAST] = {
>  	[RSC_VDEV] = (rproc_handle_resource_t)rproc_handle_vdev,
>  };
>  
> +static rproc_handle_resource_t rproc_handle_notifyid_rsc[RSC_LAST] = {
> +	[RSC_VDEV] = (rproc_handle_resource_t)rproc_handle_notifyid,
> +};
> +
>  /* handle firmware resource entries before booting the remote processor */
>  static int
>  rproc_handle_resource(struct rproc *rproc, struct resource_table *table,
> @@ -868,6 +866,12 @@ static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
>  	if (!table)
>  		goto out;
>  
> +	rproc->max_notifyid = 0;
> +
> +	/* count the numbe of notify-ids */

Small typo there.

> +	ret = rproc_handle_resource(rproc, table, tablesz,
> +						rproc_handle_notifyid_rsc);
> +

AFAICT, this will yield a higher max_notifyid than before (by one), since the
notification ids are zero based.

>  	/* look for virtio devices and register them */
>  	ret = rproc_handle_resource(rproc, table, tablesz,
>  						rproc_handle_vdev_rsc);
> -- 
> 1.7.5.4
> 

Thanks,
Ido.

Re: [PATCH 9/9] remoteproc: Always perserve resource table data

[Ido Yariv]

Hi Sjur,

On Sun, Feb 10, 2013 at 12:39:12PM +0100, sjur.brandeland@stericsson.com wrote:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
> 
> Copy resource table from first to second firmware loading.
> After firmware is loaded to memory, update the vdevs resource
> pointer to the resource table kept in device memory.
> 
> Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
> ---
>  drivers/remoteproc/remoteproc_core.c |   61 +++++++++++++++++++++++++++------
>  include/linux/remoteproc.h           |    3 ++
>  2 files changed, 53 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 14f40eb..13dc7b4 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -694,17 +694,16 @@ static rproc_handle_resource_t rproc_handle_notifyid_rsc[RSC_LAST] = {
>  
>  /* handle firmware resource entries before booting the remote processor */
>  static int
> -rproc_handle_resource(struct rproc *rproc, struct resource_table *table,
> -				int len,
> -				rproc_handle_resource_t handlers[RSC_LAST])
> +rproc_handle_resource(struct rproc *rproc, 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 < table->num; i++) {
> -		int offset = table->offset[i];
> -		struct fw_rsc_hdr *hdr = (void *)table + offset;
> +	for (i = 0; i < rproc->rsc->num; i++) {
> +		int offset = rproc->rsc->offset[i];
> +		struct fw_rsc_hdr *hdr = (void *)rproc->rsc + offset;
>  		int avail = len - offset - sizeof(*hdr);
>  		void *rsc = (void *)hdr + sizeof(*hdr);
>  
> @@ -783,9 +782,13 @@ 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;
> +	struct rproc_vdev *rvdev;
> +	struct resource_table *table, *devmem_rsc, *tmp;
>  	int ret, tablesz;
>  
> +	if (!rproc->rsc)
> +		return -ENOMEM;
> +
>  	ret = rproc_fw_sanity_check(rproc, fw);
>  	if (ret)
>  		return ret;
> @@ -811,8 +814,17 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>  		goto clean_up;
>  	}
>  
> +	/* Verify that resource table in loaded fw is unchanged */
> +	if (rproc->rsc_csum != ip_compute_csum(table, tablesz)) {
> +		dev_err(dev, "resource checksum failed, fw changed?\n");
> +		ret = -EINVAL;
> +		goto clean_up;
> +	}
> +
> +

Remove empty line?

>  	/* handle fw resources which are required to boot rproc */
> -	ret = rproc_handle_resource(rproc, table, tablesz, rproc_handle_rsc);
> +	ret = rproc_handle_resource(rproc, tablesz,
> +					rproc_handle_rsc);

This can fit in one line now :)

>  	if (ret) {
>  		dev_err(dev, "Failed to process resources: %d\n", ret);
>  		goto clean_up;
> @@ -825,6 +837,26 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>  		goto clean_up;
>  	}
>  
> +	/* Get the resource table address in device memory */
> +	devmem_rsc = rproc_get_rsctab_addr(rproc, fw);
> +
> +	/* Copy the updated resource table to device memory */
> +	memcpy(devmem_rsc, rproc->rsc, tablesz);
> +
> +	/* Free the copy of the resource table */
> +	tmp = rproc->rsc;
> +	rproc->rsc = devmem_rsc;

This is also set a few lines below, and we can probably do without tmp
altogether.

> +	kfree(tmp);

If rproc_fw_boot is called more than once, kfree will try to free memory that
was not kmalloced, leading to a panic.

Instead of freeing the pointer here, it should be kept until rproc is
released, reverting rproc->rsc to it every time shutdown is called. It
can be freed when rproc is finally released.

> +
> +	/* Update the vdev rsc address */
> +	list_for_each_entry(rvdev, &rproc->rvdevs, node) {
> +		int offset = (void *)rvdev->rsc - (void *)tmp;
> +		rvdev->rsc = (void *)devmem_rsc + offset;
> +	}

I think it might be nicer and less error prone to hold the offsets to
the resources instead of pointers. This will require modifying
"remoteproc: Support virtio config space" accordingly, but will save the
trouble of updating pointers later on (both here, and on shutdown as
suggested above).

> +
> +	/* Other virtio drivers will see the rsc table in device memory */
> +	rproc->rsc = devmem_rsc;
> +
>  	/* power up the remote processor */
>  	ret = rproc->ops->start(rproc);
>  	if (ret) {
> @@ -866,14 +898,21 @@ static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
>  	if (!table)
>  		goto out;
>  
> -	rproc->max_notifyid = 0;
> +	rproc->rsc_csum = ip_compute_csum(table, tablesz);

Doesn't matter much and any checksum would do, but given that this is not an IP
packet perhaps use something more generic (crc32 or similar)?
In any case, an appropriate dependency should to remoteproc's Kconfig.

>  
>  	/* count the numbe of notify-ids */
> -	ret = rproc_handle_resource(rproc, table, tablesz,
> +	rproc->max_notifyid = 0;
> +	rproc->rsc = table;

Instead of temporarily setting rproc->rsc here, why not just set it up before
figuring out the maximum notification id?

> +	ret = rproc_handle_resource(rproc, tablesz,
>  						rproc_handle_notifyid_rsc);
>  
> +	/* Copy resource table containing vdev config info */
> +	rproc->rsc = kmalloc(tablesz, GFP_KERNEL);
> +	if (rproc->rsc)
> +		memcpy(rproc->rsc, table, tablesz);
> +

If the kmalloc above returned NULL, we should probably bail out here.

>  	/* look for virtio devices and register them */
> -	ret = rproc_handle_resource(rproc, table, tablesz,
> +	ret = rproc_handle_resource(rproc, tablesz,
>  						rproc_handle_vdev_rsc);

This can fit in one line now

>  	if (ret)
>  		goto out;
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index c0c363c..07deff4 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -41,6 +41,7 @@
>  #include <linux/virtio.h>
>  #include <linux/completion.h>
>  #include <linux/idr.h>
> +#include <asm/checksum.h>
>  
>  /**
>   * struct resource_table - firmware resource table header
> @@ -429,6 +430,8 @@ struct rproc {
>  	struct completion crash_comp;
>  	bool recovery_disabled;
>  	int max_notifyid;
> +	struct resource_table *rsc;
> +	__sum16 rsc_csum;
>  };
>  
>  /* we currently support only two vrings per rvdev */
> -- 
> 1.7.5.4
> 

Thanks,
Ido.

Re: [PATCH 9/9] remoteproc: Always perserve resource table data

[Sjur Brændeland]

Hi Ido,

On Wed, Feb 20, 2013 at 11:46 AM, Ido Yariv <ido@wizery.com> wrote:
> Hi Sjur,
>
> On Sun, Feb 10, 2013 at 12:39:12PM +0100, sjur.brandeland@stericsson.com wrote:
>> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
>>
>> Copy resource table from first to second firmware loading.
>> After firmware is loaded to memory, update the vdevs resource
>> pointer to the resource table kept in device memory.
>>
>> Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
>> ---
>>  drivers/remoteproc/remoteproc_core.c |   61 +++++++++++++++++++++++++++------
>>  include/linux/remoteproc.h           |    3 ++
>>  2 files changed, 53 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>> index 14f40eb..13dc7b4 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -694,17 +694,16 @@ static rproc_handle_resource_t rproc_handle_notifyid_rsc[RSC_LAST] = {
>>
>>  /* handle firmware resource entries before booting the remote processor */
>>  static int
>> -rproc_handle_resource(struct rproc *rproc, struct resource_table *table,
>> -                             int len,
>> -                             rproc_handle_resource_t handlers[RSC_LAST])
>> +rproc_handle_resource(struct rproc *rproc, 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 < table->num; i++) {
>> -             int offset = table->offset[i];
>> -             struct fw_rsc_hdr *hdr = (void *)table + offset;
>> +     for (i = 0; i < rproc->rsc->num; i++) {
>> +             int offset = rproc->rsc->offset[i];
>> +             struct fw_rsc_hdr *hdr = (void *)rproc->rsc + offset;
>>               int avail = len - offset - sizeof(*hdr);
>>               void *rsc = (void *)hdr + sizeof(*hdr);
>>
>> @@ -783,9 +782,13 @@ 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;
>> +     struct rproc_vdev *rvdev;
>> +     struct resource_table *table, *devmem_rsc, *tmp;
>>       int ret, tablesz;
>>
>> +     if (!rproc->rsc)
>> +             return -ENOMEM;
>> +
>>       ret = rproc_fw_sanity_check(rproc, fw);
>>       if (ret)
>>               return ret;
>> @@ -811,8 +814,17 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>>               goto clean_up;
>>       }
>>
>> +     /* Verify that resource table in loaded fw is unchanged */
>> +     if (rproc->rsc_csum != ip_compute_csum(table, tablesz)) {
>> +             dev_err(dev, "resource checksum failed, fw changed?\n");
>> +             ret = -EINVAL;
>> +             goto clean_up;
>> +     }
>> +
>> +
>
> Remove empty line?

Ok.

>
>>       /* handle fw resources which are required to boot rproc */
>> -     ret = rproc_handle_resource(rproc, table, tablesz, rproc_handle_rsc);
>> +     ret = rproc_handle_resource(rproc, tablesz,
>> +                                     rproc_handle_rsc);
>
> This can fit in one line now :)

Ok,

>
>>       if (ret) {
>>               dev_err(dev, "Failed to process resources: %d\n", ret);
>>               goto clean_up;
>> @@ -825,6 +837,26 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>>               goto clean_up;
>>       }
>>
>> +     /* Get the resource table address in device memory */
>> +     devmem_rsc = rproc_get_rsctab_addr(rproc, fw);
>> +
>> +     /* Copy the updated resource table to device memory */
>> +     memcpy(devmem_rsc, rproc->rsc, tablesz);
>> +
>> +     /* Free the copy of the resource table */
>> +     tmp = rproc->rsc;
>> +     rproc->rsc = devmem_rsc;
>
> This is also set a few lines below, and we can probably do without tmp
> altogether.
>
>> +     kfree(tmp);
>
> If rproc_fw_boot is called more than once, kfree will try to free memory that
> was not kmalloced, leading to a panic.
>
> Instead of freeing the pointer here, it should be kept until rproc is
> released, reverting rproc->rsc to it every time shutdown is called. It
> can be freed when rproc is finally released.

Agree. I will free the copy in rproc_trigger_recovery and in rproc_del,
and set rproc->rsc = rproc->rsc_copy at shutdown.

>
>> +
>> +     /* Update the vdev rsc address */
>> +     list_for_each_entry(rvdev, &rproc->rvdevs, node) {
>> +             int offset = (void *)rvdev->rsc - (void *)tmp;
>> +             rvdev->rsc = (void *)devmem_rsc + offset;
>> +     }
>
> I think it might be nicer and less error prone to hold the offsets to
> the resources instead of pointers. This will require modifying
> "remoteproc: Support virtio config space" accordingly, but will save the
> trouble of updating pointers later on (both here, and on shutdown as
> suggested above).

Ok, I've changed this as well. We now work with offsets from the start
of the resource table. I also updated the typedef rproc_handle_resource_t
to include the resource offset as well. In this way we're consistently handling
resource offsets.

>
>> +
>> +     /* Other virtio drivers will see the rsc table in device memory */
>> +     rproc->rsc = devmem_rsc;
>> +
>>       /* power up the remote processor */
>>       ret = rproc->ops->start(rproc);
>>       if (ret) {
>> @@ -866,14 +898,21 @@ static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
>>       if (!table)
>>               goto out;
>>
>> -     rproc->max_notifyid = 0;
>> +     rproc->rsc_csum = ip_compute_csum(table, tablesz);
>
> Doesn't matter much and any checksum would do, but given that this is not an IP
> packet perhaps use something more generic (crc32 or similar)?
> In any case, an appropriate dependency should to remoteproc's Kconfig.

OK, I'll change to crc32 and update Kconfig.

>
>>
>>       /* count the numbe of notify-ids */
>> -     ret = rproc_handle_resource(rproc, table, tablesz,
>> +     rproc->max_notifyid = 0;
>> +     rproc->rsc = table;
>
> Instead of temporarily setting rproc->rsc here, why not just set it up before
> figuring out the maximum notification id?

Yes, will do.

>
>> +     ret = rproc_handle_resource(rproc, tablesz,
>>                                               rproc_handle_notifyid_rsc);
>>
>> +     /* Copy resource table containing vdev config info */
>> +     rproc->rsc = kmalloc(tablesz, GFP_KERNEL);
>> +     if (rproc->rsc)
>> +             memcpy(rproc->rsc, table, tablesz);
>> +
>
> If the kmalloc above returned NULL, we should probably bail out here.

Agree.

>
>>       /* look for virtio devices and register them */
>> -     ret = rproc_handle_resource(rproc, table, tablesz,
>> +     ret = rproc_handle_resource(rproc, tablesz,
>>                                               rproc_handle_vdev_rsc);
>
> This can fit in one line now

Yes,

I have reworked this patch according to your review comments. I had to
reshuffle the order of patches in order to change from using pointers to
offset to resource entries.
The other comments you have on the other patches seems easy to fix.
(Please take a second look on the overflow checks though).
I will send out an updated patch-set addressing your review comments
very soon. I would appreciate a somewhat faster response this time though...

Thanks,
Sjur

Re: [PATCH 1/9] remoteproc: Bugfix: Deallocate firmware image on shutdown

[Ohad Ben-Cohen]

Hi Sjur,

On Sun, Feb 10, 2013 at 1:39 PM,  <sjur.brandeland@stericsson.com> wrote:
> From: Dmitry Tarnyagin <dmitry.tarnyagin@stericsson.com>
>
> Fixes coherent memory leakage, caused by non-deallocated
> firmware image chunk.

I'll just slightly edit the subject + commit log to indicate this fix
is STE-specific.

> Signed-off-by: Dmitry Tarnyagin <dmitry.tarnyagin@stericsson.com>

Can I add your s-o-b here?

Since you sent the patch, we should add your s-o-b after Dmitry's.

Thanks,
Ohad.

RE: [PATCH 1/9] remoteproc: Bugfix: Deallocate firmware image on shutdown

[Sjur BRENDELAND]

Hi Ohad,

> On Sun, Feb 10, 2013 at 1:39 PM,  <sjur.brandeland@stericsson.com> wrote:
> > From: Dmitry Tarnyagin <dmitry.tarnyagin@stericsson.com>
> >
> > Fixes coherent memory leakage, caused by non-deallocated
> > firmware image chunk.
> 
> I'll just slightly edit the subject + commit log to indicate this fix
> is STE-specific.

Sure, no problem.

> 
> > Signed-off-by: Dmitry Tarnyagin <dmitry.tarnyagin@stericsson.com>
> 
> Can I add your s-o-b here?
> 
> Since you sent the patch, we should add your s-o-b after Dmitry's.

Sure, you can add my: 
Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>

Thanks,
Sjur

Re: [PATCH 1/9] remoteproc: Bugfix: Deallocate firmware image on shutdown

[Ohad Ben-Cohen]

On Tue, Mar 19, 2013 at 2:40 PM, Sjur BRENDELAND
<sjur.brandeland@stericsson.com> wrote:
> Sure, you can add my:
> Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>

Thanks. I'm also cc'ing stable