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

[Lee Jones]

On Wed, 31 Aug 2016, Loic Pallardy wrote:

> Rproc driver has now the capability to add resources dynamically
> thanks to rproc_request_resource API.
> Depending on associated action, resource request could impact
> firmware resource table or define new local resource.
> 
> In order to preserve current remoteproc resource handling
> mechanism, all local resources are gathered in a local resource
> table which won't be shared with firmware and proceed by
> remoteproc core as firmware one.
> 
> It is rproc driver responsibility to provide the right resource
> information using rproc_request_resource API.
> 
> Signed-off-by: Loic Pallardy <loic.pallardy@xxxxxx>
> ---
>  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