Re: [PATCH v2 09/12] arm64: dts: qcom: sm8550: Add Iris context bank subnodes

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] In `of_iommu_get_resv_regions()`, the code fails to advance the `maps` pointer past a node's specifier cells if the node does not match `dev->of_node`, causing the parser to fall out of sync.
- [High] If `of_translate_dma_region()` fails to translate the address, it returns `NULL`, which is assigned to `maps` without a NULL check, leading to a guaranteed kernel panic on the next loop iteration.
--

--- Patch [9]: [PATCH v2 09/12] arm64: dts: qcom: sm8550: Add Iris context bank subnodes ---
commit d594fb5947e7e4fe6f238d480eadd3c2b7770369
Author: Vikash Garodia <vikash.garodia@oss.qualcomm.com>

arm64: dts: qcom: sm8550: Add Iris context bank subnodes

The VPU issues DMA through several SMMU streams, and the hardware does
not give every stream the same addressable range. The non-pixel stream
is restricted to use 0-600MB of IOVA space, while the pixel stream can
address the full range. This commit separates the streams into two
subnodes, pointing the non-pixel subnode to a reserved-memory node to
exclude the lower 600 MB from its IOVA allocator.

> diff --git a/arch/arm64/boot/dts/qcom/sm8550.dtsi b/arch/arm64/boot/dts/qcom/sm8550.dtsi
> index 7599b7b7e645f..4c8ad7f026fe5 100644
> --- a/arch/arm64/boot/dts/qcom/sm8550.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8550.dtsi
> @@ -1087,6 +1087,10 @@ hyp_ext_reserved_mem: hyp-ext-reserved-region@ff700000 {
>  			reg = <0 0xff700000 0 0x100000>;
>  			no-map;
>  		};
> +
> +		iris_resv: reservation-iris {
> +			iommu-addresses = <&iris_non_pixel 0x0 0x0 0x0 0x25800000>;

[Severity: High]
This isn't a bug introduced by this patch, but adding this property exposes
a pre-existing issue in of_iommu_get_resv_regions().

If a memory-region specifies an iommu-addresses list with multiple devices,
the parsing breaks for all entries after the first one because the code fails
to advance the maps pointer past a node's specifier cells if the node does
not match dev->of_node:

drivers/iommu/of_iommu.c:of_iommu_get_resv_regions() {
    ...
			if (np == dev->of_node) {
				int prot = IOMMU_READ | IOMMU_WRITE;
    ...
			}
    ...
}

There is no else branch to compute the specifier size and advance the maps
pointer over it. The next iteration incorrectly reads the first cell of the
specifier as the next phandle, throwing the parser out of sync.

Will this be an issue if additional devices are added to this list in the
future?

[Severity: High]
This is a pre-existing issue, but adding this property could trigger a kernel
panic if the device tree is ever misconfigured.

If of_translate_dma_region() fails to translate the address (for example,
due to a missing dma-ranges property), it returns NULL. This NULL is directly
assigned to maps without a check:

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

Because length is uninitialized, the loop eventually proceeds to the next
iteration where the condition while (maps < end) evaluates to true since maps
is NULL. Dereferencing maps with be32_to_cpup(maps++) then causes a kernel
panic.

Should we add a NULL check in of_iommu_get_resv_regions() to prevent this?

> +		};
>  	};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731-vpu_iommu_iova_handling-v2-0-da52b5228dbd@oss.qualcomm.com?part=9