Re: [PATCH 20/22] arm64: dts: qcom: sm6115: Reserve low IOVA range for Venus

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Low] The `venus-iova` node lacks required `reg` or `size` properties and relies on an undocumented property, violating the `reserved-memory` schema.

Pre-existing issues:
- [High] Reference leak and parsing desync in `of_iommu_get_resv_regions` when parsing `iommu-addresses` for unmatched devices.
- [High] NULL pointer dereference in `of_iommu_get_resv_regions` if `of_translate_dma_region` fails.
--

--- Patch [20]: [PATCH 20/22] arm64: dts: qcom: sm6115: Reserve low IOVA range for Venus ---
commit bec476bec203ee86c0c6ea293d84bdd69fa09d6f
Author: Vikash Garodia <[email protected]>

arm64: dts: qcom: sm6115: Reserve low IOVA range for Venus

Venus VPU reserves IOVA addresses below 0x25800000 (600MB), primarily for
secure non-pixel buffers. Add a reserved-memory IOVA reservation node
covering [0, 0x25800000] and reference it from the venus node.

Cc: [email protected]

> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> index 3520802d1fed5..93f4eb8d18a71 100644
> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi

[ ... ]

> @@ -532,6 +532,17 @@ rmtfs_mem: memory@89b01000 {
>  			qcom,client-id = <1>;
>  			qcom,vmid = <QCOM_SCM_VMID_MSS_MSA QCOM_SCM_VMID_NAV>;
>  		};
> +
> +		/*
> +		 * The Venus VPU reserves IOVA below 0x25800000 (600MB),
> +		 * primarily for non-pixel buffers using different Stream IDs.
> +		 * DMA into that range triggers unhandled SMMU faults and
> +		 * spontaneous reboots, so reserve it to keep IOMMU
> +		 * allocations above this boundary.
> +		 */
> +		venus_iova: venus-iova {
> +			iommu-addresses = <&venus 0x0 0x0 0x0 0x25800000>;

[Severity: Low]
Will this fail devicetree validation tooling? The venus_iova node appears
to be missing the standard reg or size properties, and relies on the
undocumented iommu-addresses property, which might violate the core
reserved-memory devicetree schema.

[Severity: High]
This isn't a bug introduced by this patch, but using the iommu-addresses
property here exposes an existing reference leak and parsing desynchronization
in drivers/iommu/of_iommu.c:of_iommu_get_resv_regions().

If the property contains a phandle that does not match dev->of_node,
the node reference is leaked:

drivers/iommu/of_iommu.c:of_iommu_get_resv_regions() {
    ...
            phandle = be32_to_cpup(maps++);
            np = of_find_node_by_phandle(phandle);

            if (np == dev->of_node) {
    ...
}

Because the if block is skipped for unmatched devices, of_node_put(np) is
never called. Furthermore, the maps pointer is not advanced past the device's
address and size cells. Would this cause the next loop iteration to read the
address/size as a phandle, corrupting all subsequent parsing?

[Severity: High]
This is also a pre-existing issue, but utilizing iommu-addresses here could
trigger a NULL pointer dereference in the same parsing function.

If of_translate_dma_region() returns NULL on a translation error (such as a
bad address or missing DMA parent):

drivers/iommu/of_iommu.c:of_iommu_get_resv_regions() {
    ...
                maps = of_translate_dma_region(np, maps, &iova, &length);
    ...
}

The maps pointer becomes NULL and is directly dereferenced in the next
iteration of the while loop. Could this cause a kernel panic during device
probe if the devicetree configuration is malformed?

> +		};
>  	};
>  
>  	smp2p-adsp {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=20