Re: [PATCH 5/8] 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 2 potential issue(s) to consider:
- [High] net: ipa: shmbridge setup bypassed due to uninitialized ctx->use_tzmem flag
- [Medium] net: ipa: memory leak of PAS metadata DMA buffer due to missing qcom_pas_metadata_release()
--

commit 5216b43b65b5b9e8f76798ae9a011c6d5ce85515
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 open-coded memory mapping and firmware loading
sequences with devm_qcom_pas_context_alloc() and qcom_mdt_pas_load().
It then uses qcom_pas_prepare_and_auth_reset() with the goal of
performing the shmbridge prepare step required before TrustZone
authentication.

> diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
> index 14ac2d2faf7c2..6964395caedd9 100644
> --- a/drivers/net/ipa/ipa_main.c
> +++ b/drivers/net/ipa/ipa_main.c

[ ... ]

> @@ -615,24 +614,23 @@ 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;
> +	ctx = devm_qcom_pas_context_alloc(dev, IPA_PAS_ID, phys,
> +					  resource_size(&res));
> +	if (IS_ERR(ctx)) {
> +		ret = PTR_ERR(ctx);
>  		goto out_release_firmware;
>  	}

[Severity: High]
Does allocating the context here leave the use_tzmem flag implicitly
initialized to false?

The commit message states the goal is to execute the shmbridge prepare
step. However, since devm_qcom_pas_context_alloc() allocates the context
using devm_kzalloc(), the ctx->use_tzmem boolean defaults to false.

When qcom_pas_prepare_and_auth_reset(ctx) is called later, the backend
evaluates this flag. If it is false, it falls back to the legacy
authentication path and completely bypasses the intended
qcom_tzmem_shm_bridge_create() logic.

Should ctx->use_tzmem be explicitly set to true after allocation?

>  
> -	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]
Will returning from this path leak the DMA buffer containing the firmware
metadata?

When qcom_mdt_pas_load() is called, it triggers the allocation and mapping
of a DMA buffer for the metadata, storing its address in ctx->ptr. While
the context structure itself is devres-managed, the underlying PAS backend
requires callers to explicitly free the metadata buffer.

If the function returns without calling qcom_pas_metadata_release(ctx),
does this result in a permanent memory leak of the metadata buffer on
every probe attempt?

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