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

[Loic PALLARDY]

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

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

Regards,
Loic

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