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 <[email protected]> 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/[email protected]?part=5
