Re: [PATCH v2 8/9] soc: qcom: mdt_loader: remove qcom_mdt_load()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] soc: qcom: mdt_loader: undocumented mapping change in
  qcom_mdt_load_no_init()
--

commit 1090e40402733b5aabfbf149a8510b247a28a308
Author: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>

soc: qcom: mdt_loader: remove qcom_mdt_load()

This commit removes the unused qcom_mdt_load() helper and moves memory
region mapping from qcom_mdt_pas_load() into qcom_mdt_load_no_init() when
mem_region is NULL.

> diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
> index aafa83466b92b..436d8bc4847a3 100644
> --- a/drivers/soc/qcom/mdt_loader.c
> +++ b/drivers/soc/qcom/mdt_loader.c
> @@ -326,7 +326,8 @@ static bool qcom_mdt_bins_are_split(const struct firmware *fw)
>   * @dev:	device handle to associate resources with
>   * @fw:		firmware object for the mdt file
>   * @fw_name:	name of the firmware, for construction of segment file names
> - * @mem_region:	allocated memory region to load firmware into
> + * @mem_region:	virtual address of the memory region to load firmware into,
> + *		or NULL to have the region mapped internally with ioremap_wc()
>   * @mem_phys:	physical address of allocated memory region
>   * @mem_size:	size of the allocated memory region
>   * @reloc_base:	adjusted physical address after relocation
> @@ -341,6 +342,7 @@ int qcom_mdt_load_no_init(struct device *dev, const struct firmware *fw,
>  	const struct elf32_phdr *phdrs;
>  	const struct elf32_phdr *phdr;
>  	const struct elf32_hdr *ehdr;
> +	void __iomem *mapped = NULL;
>  	phys_addr_t mem_reloc;
>  	phys_addr_t min_addr = PHYS_ADDR_MAX;
>  	ssize_t offset;
> @@ -350,12 +352,22 @@ int qcom_mdt_load_no_init(struct device *dev, const struct firmware *fw,
>  	int ret = 0;
>  	int i;
>  
> -	if (!fw || !mem_region || !mem_phys || !mem_size)
> +	if (!fw || !mem_phys || !mem_size)
>  		return -EINVAL;
>  
>  	if (!mdt_header_valid(fw))
>  		return -EINVAL;
>  
> +	if (!mem_region) {
> +		mapped = ioremap_wc(mem_phys, mem_size);
> +		if (!mapped) {
> +			dev_err(dev, "unable to map memory region: %pa+%zx\n",
> +				&mem_phys, mem_size);
> +			return -ENOMEM;
> +		}
> +		mem_region = (__force void *)mapped;
> +	}
> +
>  	is_split = qcom_mdt_bins_are_split(fw);
>  	ehdr = (struct elf32_hdr *)fw->data;
>  	phdrs = (struct elf32_phdr *)(fw->data + ehdr->e_phoff);
> @@ -434,39 +446,13 @@ int qcom_mdt_load_no_init(struct device *dev, const struct firmware *fw,
>  	if (reloc_base)
>  		*reloc_base = mem_reloc;
>  
> +	if (mapped)
> +		iounmap(mapped);
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(qcom_mdt_load_no_init);
[ ... ]
> @@ -486,21 +472,14 @@ EXPORT_SYMBOL_GPL(qcom_mdt_load);
>  int qcom_mdt_pas_load(struct qcom_pas_context *ctx, const struct firmware *fw,
>  		      const char *firmware, phys_addr_t *reloc_base)
>  {
> -	void __iomem *mem_region;
>  	int ret;
>  
>  	ret = __qcom_mdt_pas_init(ctx->dev, fw, firmware, ctx->pas_id, ctx->mem_phys, ctx);
>  	if (ret)
>  		return ret;
>  
> -	mem_region = qcom_pas_ctx_map(ctx);
> -	if (!mem_region)
> -		return -ENOMEM;
> -
> -	ret = qcom_mdt_load_no_init(ctx->dev, fw, firmware, (__force void *)mem_region,
> -				    ctx->mem_phys, ctx->mem_size, reloc_base);
> -	iounmap(mem_region);
> -	return ret;
> +	return qcom_mdt_load_no_init(ctx->dev, fw, firmware, NULL,
> +				     ctx->mem_phys, ctx->mem_size, reloc_base);
>  }
>  EXPORT_SYMBOL_GPL(qcom_mdt_pas_load);

[Severity: Medium]
Why move the ioremap_wc() mapping from qcom_mdt_pas_load() into
qcom_mdt_load_no_init() when this patch is only supposed to remove the
unused qcom_mdt_load()?

qcom_pas_ctx_map() in include/linux/firmware/qcom/qcom_pas.h already
encapsulates this exact ioremap_wc() and dev_err() sequence for PAS
callers:

include/linux/firmware/qcom/qcom_pas.h:qcom_pas_ctx_map() {
	void __iomem *ptr = ioremap_wc(ctx->mem_phys, ctx->mem_size);

	if (!ptr)
		dev_err(ctx->dev, "unable to map memory region: %pa+%zx\n",
			&ctx->mem_phys, ctx->mem_size);
	return ptr;
}

Open-coding it inside qcom_mdt_load_no_init() duplicates qcom_pas_ctx_map()
and drops the !mem_region -EINVAL check for non-PAS callers (such as
venus_load_fw(), adsp_load(), and q6v5_wcss_load()), so a caller that
passes a NULL mem_region after an unchecked mapping failure will silently
remap the region instead of failing with -EINVAL.  None of these changes to
qcom_mdt_load_no_init() and qcom_mdt_pas_load() are mentioned in the commit
message.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113185009.157893-1-mukesh.ojha@oss.qualcomm.com?part=8