Re: [PATCH v3 1/2] remoteproc: elf_loader: Create new callback for copying segments to remote cpu

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] remoteproc: misleading memcpy_tova callback name and incomplete kernel-doc
--

commit 1f800d9eb3b123f9d100331894730d47e230cc68
Author: Uwe Kleine-König <u.kleine-koenig@baylibre.com>

remoteproc: elf_loader: Create new callback for copying segments to remote cpu

This commit introduces a new memcpy_tova callback in struct rproc_ops so
platform drivers with special memory access requirements can override the
default rproc_da_to_va() plus memcpy/memset sequence in
rproc_elf_load_segments().

> diff --git a/drivers/remoteproc/remoteproc_elf_loader.c b/drivers/remoteproc/remoteproc_elf_loader.c
> index da3cddbe7d4cb..3c749a39e5dad 100644
> --- a/drivers/remoteproc/remoteproc_elf_loader.c
> +++ b/drivers/remoteproc/remoteproc_elf_loader.c
> @@ -227,35 +227,45 @@ int rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
>  			break;
>  		}
>  
> -		/* grab the kernel address for this device address */
> -		ptr = rproc_da_to_va(rproc, da, memsz, &is_iomem);
> -		if (!ptr) {
> -			dev_err(dev, "bad phdr da 0x%llx mem 0x%llx\n", da,
> -				memsz);
> -			ret = -EINVAL;
> -			break;
> -		}
> -
> -		/* put the segment where the remote processor expects it */
> -		if (filesz) {
> -			if (is_iomem)
> -				memcpy_toio((void __iomem *)ptr, elf_data + offset, filesz);
> -			else
> -				memcpy(ptr, elf_data + offset, filesz);
> -		}
> -
> -		/*
> -		 * Zero out remaining memory for this segment.
> -		 *
> -		 * This isn't strictly required since dma_alloc_coherent already
> -		 * did this for us. albeit harmless, we may consider removing
> -		 * this.
> -		 */
> -		if (memsz > filesz) {
> -			if (is_iomem)
> -				memset_io((void __iomem *)(ptr + filesz), 0, memsz - filesz);
> -			else
> -				memset(ptr + filesz, 0, memsz - filesz);
> +		if (rproc->ops->memcpy_tova) {
> +			ret = rproc->ops->memcpy_tova(rproc, da, elf_data + offset, filesz, memsz);
> +			if (ret) {
> +				dev_err(dev, "Failed to copy segment %d to device address 0x%llx: %pe\n",
> +					i, da, ERR_PTR(ret));
> +				break;
> +			}

[ ... ]

> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 00019b1895e6e..e750a127cbc4a 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -75,6 +75,7 @@ enum rsc_handling_status {
>   * @detach:	detach from a device, leaving it powered up
>   * @kick:	kick a virtqueue (virtqueue id given as a parameter)
>   * @da_to_va:	optional platform hook to perform address translations
> + * @memcpy_tova:optional platform hook to copy data into remote memory range
>   * @parse_fw:	parse firmware to extract information (e.g. resource table)
>   * @handle_rsc:	optional platform hook to handle vendor resources. Should return
>   *		RSC_HANDLED if resource was handled, RSC_IGNORED if not handled
> @@ -99,6 +100,9 @@ struct rproc_ops {
>  	int (*detach)(struct rproc *rproc);
>  	void (*kick)(struct rproc *rproc, int vqid);
>  	void * (*da_to_va)(struct rproc *rproc, u64 da, size_t len, bool *is_iomem);
> +	int (*memcpy_tova)(struct rproc *rproc, u64 da, const void *src,
> +			   size_t filesz, size_t memsz);

[Severity: Low]
This isn't a bug, but is memcpy_tova the best name for this callback in
struct rproc_ops?

Across the remoteproc subsystem (such as in struct rproc_mem_entry and
ops->da_to_va), da denotes a remote processor device address while va denotes
a kernel virtual address. Since rproc_elf_load_segments() passes the
untranslated device address da directly to this callback (including for
devices where remote memory is not mapped into CPU virtual address space at
all), calling it memcpy_tova seems somewhat misleading.

In addition, when rproc->ops->memcpy_tova is provided,
rproc_elf_load_segments() skips both the filesz copy and the trailing
memsz - filesz zeroing step. Should the kernel-doc comment for @memcpy_tova in
include/linux/remoteproc.h document that implementations must copy filesz
bytes to da and zero-fill the remaining memsz - filesz bytes when
memsz > filesz?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793879714.git.u.kleine-koenig@baylibre.com?part=1