Re: [PATCH v2 6/9] net: ipa: migrate firmware loading to qcom_mdt_pas_load() and qcom_pas_prepare_and_auth_reset()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] net: ipa: missing qcom_pas_metadata_release() in
  ipa_firmware_load()
--

commit 5c113c8e8e89734c8d281cbb95db1ce83f9398ac
Author: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>

net: ipa: migrate firmware loading to qcom_mdt_pas_load() and qcom_pas_prepare_and_auth_reset()

This commit replaces the memremap(), qcom_mdt_load(), and
qcom_pas_auth_and_reset() sequence in ipa_firmware_load() with
qcom_pas_context_init(), qcom_mdt_pas_load(), and
qcom_pas_prepare_and_auth_reset().

> diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
> index 14ac2d2faf7c2..0a37741c22858 100644
> --- a/drivers/net/ipa/ipa_main.c
> +++ b/drivers/net/ipa/ipa_main.c
> @@ -585,12 +585,11 @@ static void ipa_deconfig(struct ipa *ipa)
>  
>  static int ipa_firmware_load(struct device *dev)
>  {
> +	struct qcom_pas_context ctx;
>  	const struct firmware *fw;
>  	struct resource res;
>  	phys_addr_t phys;
>  	const char *path;
> -	ssize_t size;
> -	void *virt;
>  	int ret;
>  
>  	ret = of_reserved_mem_region_to_resource(dev->of_node, 0, &res);
> @@ -615,24 +614,18 @@ static int ipa_firmware_load(struct device *dev)
>  	}
>  
>  	phys = res.start;
> -	size = (size_t)resource_size(&res);
> -	virt = memremap(phys, size, MEMREMAP_WC);
> -	if (!virt) {
> -		dev_err(dev, "unable to remap firmware memory\n");
> -		ret = -ENOMEM;
> -		goto out_release_firmware;
> -	}
> +	qcom_pas_context_init(&ctx, dev, IPA_PAS_ID, phys, resource_size(&res));
>  
> -	ret = qcom_mdt_load(dev, fw, path, IPA_PAS_ID, virt, phys, size, NULL);
> +	ret = qcom_mdt_pas_load(&ctx, fw, path, NULL);
>  	if (ret) {
>  		dev_err(dev, "error %d loading \"%s\"\n", ret, path);
> -	} else {
> -		ret = qcom_pas_auth_and_reset(IPA_PAS_ID);
> -		if (ret)
> -			dev_err(dev, "error %d authenticating \"%s\"\n", ret, path);
> +		goto out_release_firmware;
>  	}
>  
> -	memunmap(virt);
> +	ret = qcom_pas_prepare_and_auth_reset(&ctx);
> +	if (ret)
> +		dev_err(dev, "error %d authenticating \"%s\"\n", ret, path);
> +
>  out_release_firmware:
>  	release_firmware(fw);

[Severity: Medium]
Does this leak the firmware metadata buffer stored in ctx.ptr?

Previously, qcom_mdt_load() called __qcom_mdt_pas_init() with a NULL context,
so qcom_scm_pas_init_image() freed the coherent DMA metadata buffer before
returning:

drivers/firmware/qcom/qcom_scm.c:qcom_scm_pas_init_image() {
    ...
	if (ret < 0 || !ctx) {
		dma_free_coherent(dev, size, mdata_buf, mdata_phys);
	} else if (ctx) {
		ctx->ptr = mdata_buf;
		ctx->phys = mdata_phys;
		ctx->size = size;
	}
    ...
}

Passing a non-NULL &ctx to qcom_mdt_pas_load() retains that buffer in
ctx.ptr.  Because this tree does not include 2aa8fafc08e7 ("firmware: qcom:
pas: Make MDT buffer retention explicit"), qcom_pas_init_image() does not
release ctx.ptr on its own, and ipa_firmware_load() returns without calling
qcom_pas_metadata_release(&ctx), leaking the allocation when ctx goes out of
scope.

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