Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1473855 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 00/19] remoteproc: Allow platform-specific drivers to request resources Loic Pallardy <loic.pallardy@st.com> - 2016-08-31 23:00 +0200
    [PATCH v2 11/19] remoteproc: core: Add function to get resource table spare bytes information Loic Pallardy <loic.pallardy@st.com> - 2016-08-31 23:00 +0200
      Re: [PATCH v2 11/19] remoteproc: core: Add function to get resource  table spare bytes information Lee Jones <lee.jones@linaro.org> - 2016-09-08 10:40 +0200
        Re: [PATCH v2 11/19] remoteproc: core: Add function to get resource  table spare bytes information loic pallardy <loic.pallardy@st.com> - 2016-09-08 11:50 +0200
    [PATCH v2 19/19] remoteproc: core: Support empty resource tables Loic Pallardy <loic.pallardy@st.com> - 2016-08-31 23:00 +0200
    [PATCH v2 13/19] remoteproc: core: Append resource only if spare resource present Loic Pallardy <loic.pallardy@st.com> - 2016-08-31 23:00 +0200
      Re: [PATCH v2 13/19] remoteproc: core: Append resource only if spare  resource present Lee Jones <lee.jones@linaro.org> - 2016-09-08 11:40 +0200
        Re: [PATCH v2 13/19] remoteproc: core: Append resource only if spare  resource present loic pallardy <loic.pallardy@st.com> - 2016-09-08 12:00 +0200
          Re: [PATCH v2 13/19] remoteproc: core: Append resource only if spare  resource present Lee Jones <lee.jones@linaro.org> - 2016-09-08 13:00 +0200
    [PATCH v2 01/19] remoteproc: core: New API to add new resources to the resource table Loic Pallardy <loic.pallardy@st.com> - 2016-08-31 23:00 +0200
    [PATCH v2 17/19] remotecore: core: Add resource table pointer argument to rproc_handle_resource Loic Pallardy <loic.pallardy@st.com> - 2016-08-31 23:00 +0200
    [PATCH v2 09/19] remoteproc: core: Finalize dump resource table function Loic Pallardy <loic.pallardy@st.com> - 2016-08-31 23:00 +0200
      Re: [PATCH v2 09/19] remoteproc: core: Finalize dump resource table  function Lee Jones <lee.jones@linaro.org> - 2016-09-08 10:30 +0200
        Re: [PATCH v2 09/19] remoteproc: core: Finalize dump resource table  function loic pallardy <loic.pallardy@st.com> - 2016-09-08 11:50 +0200
    [PATCH v2 18/19] remoteproc: core: Add function to create remoteproc local resource table Loic Pallardy <loic.pallardy@st.com> - 2016-08-31 23:00 +0200
      Re: [PATCH v2 18/19] remoteproc: core: Add function to create  remoteproc local resource table Lee Jones <lee.jones@linaro.org> - 2016-09-08 12:20 +0200
        Re: [PATCH v2 18/19] remoteproc: core: Add function to create  remoteproc local resource table loic pallardy <loic.pallardy@st.com> - 2016-09-08 15:20 +0200
    [PATCH v2 14/19] remoteproc: core: Add resource request action support Loic Pallardy <loic.pallardy@st.com> - 2016-08-31 23:00 +0200
    [PATCH v2 03/19] remoteproc: core: Add function to amend an existing resource table entry Loic Pallardy <loic.pallardy@st.com> - 2016-08-31 23:00 +0200
    [PATCH v2 02/19] remoteproc: core: Add function to dump resource table Loic Pallardy <loic.pallardy@st.com> - 2016-08-31 23:00 +0200

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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)
>

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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);
>

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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)
>>  {
>

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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;
>>  };
>

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web