Re: [PATCH v6 12/12] arm64: dts: qcom: sm8750: Add Iris context bank subnodes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] media: iris: missing IOMMU and deferral check in iris_create_cb_dev()
--

commit 9f019e2759c9a77cf0f6ce3f5725ca965b2fadcb
Author: Vikash Garodia <vikash.garodia@oss.qualcomm.com>

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

This commit moves the iommus property from the sm8750 iris video-codec node
into new non-pixel and pixel child subnodes with per-stream iommu-ranges,
along with a 1:1 dma-ranges mapping on the parent node.

> diff --git a/arch/arm64/boot/dts/qcom/sm8750.dtsi b/arch/arm64/boot/dts/qcom/sm8750.dtsi
> index cf947acc57602..eca9944ff2228 100644
> --- a/arch/arm64/boot/dts/qcom/sm8750.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8750.dtsi
> @@ -3246,8 +3246,6 @@ iris: video-codec@aa00000 {
>  				      "vcodec0_core_freerun";
>  
>  			dma-coherent;
> -			iommus = <&apps_smmu 0x1940 0>,
> -				 <&apps_smmu 0x1947 0>;
>  
>  			interconnects = <&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ACTIVE_ONLY
>  					 &config_noc SLAVE_VENUS_CFG QCOM_ICC_TAG_ACTIVE_ONLY>,
> @@ -3280,6 +3278,10 @@ &config_noc SLAVE_VENUS_CFG QCOM_ICC_TAG_ACTIVE_ONLY>,
>  				      "core",
>  				      "vcodec0_core";
>  
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			dma-ranges = <0x0 0x0 0x0 0xe0000000>;
> +
>  			/*
>  			 * IRIS firmware is signed by vendors, only
>  			 * enable in boards where the proper signed firmware
> @@ -3287,6 +3289,15 @@ &config_noc SLAVE_VENUS_CFG QCOM_ICC_TAG_ACTIVE_ONLY>,
>  			 */
>  			status = "disabled";
>  
> +			non-pixel {
> +				iommus = <&apps_smmu 0x1940 0x0>;
> +				iommu-ranges = <0x25800000 0xba800000>;
> +			};
> +
> +			pixel {
> +				iommus = <&apps_smmu 0x1947 0x0>;
> +			};

[Severity: High]
With the iommus property moved from the parent video-codec@aa00000 node into
the driverless non-pixel and pixel child subnodes, what happens if qcom-iris
probes before apps_smmu is ready or if IOMMU attachment fails on the child
devices?

Previously, the parent video-codec device had the iommus property directly,
so really_probe() -> platform_dma_configure() -> of_dma_configure_id() ran
before iris_probe() and propagated -EPROBE_DEFER if apps_smmu had not probed
yet.

Now, the child context bank devices are registered dynamically via
iris_probe() -> iris_init_cb_devs() -> iris_create_cb_dev():

drivers/media/platform/qcom/iris/iris_probe.c:iris_create_cb_dev() {
	...
	pdev = platform_device_register_full(&plat_dev_info);
	of_node_put(child_of_node);
	if (IS_ERR(pdev))
		return ERR_CAST(pdev);
	...
	return &pdev->dev;
}

Because these child platform devices have no driver bound, their DMA and
IOMMU configuration happens inside the bus notifier path:

platform_device_register_full()
  -> device_add()
    -> iommu_bus_notifier()
      -> iommu_probe_device()
        -> iommu_init_device()

In drivers/iommu/iommu.c:iommu_init_device(), the return value of
dev->bus->dma_configure(dev) is ignored:

	if (!dev->iommu->fwspec && !dev->driver && dev->bus->dma_configure) {
		mutex_unlock(&iommu_probe_device_lock);
		dev->bus->dma_configure(dev);
		mutex_lock(&iommu_probe_device_lock);
	}

When of_iommu_configure() returns -EPROBE_DEFER inside
drivers/of/device.c:of_dma_configure_id(), it clears dev->dma_range_map and
returns before calling arch_setup_dma_ops():

	ret = of_iommu_configure(dev, np, id);
	if (ret == -EPROBE_DEFER) {
		/* Don't touch range map if it wasn't set from a valid dma-ranges */
		if (set_map)
			dev->dma_range_map = NULL;
		kfree(map);
		return -EPROBE_DEFER;
	}

Since iommu_init_device() and iommu_bus_notifier() discard that error,
platform_device_register_full() still succeeds. Can iris_probe() then
complete and register V4L2 devices while core->np_dev and core->p_dev have
no IOMMU attached, a NULL dma_range_map, and unconfigured DMA ops?

Should iris_create_cb_dev() verify that IOMMU configuration succeeded (or
propagate probe deferral) before returning the child device?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-vpu_iommu_iova_handling-v6-0-e4b6ad864f8f@oss.qualcomm.com?part=12