Re: [PATCH v6 06/12] media: iris: Skip DMA mask setup when the core device has no IOMMU

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] media: iris: missing IOMMU readiness check for context bank devices
- [Low] media: iris: commit message omits iris_reserve_iova_region() changes
--

commit 5b3d97b6ae9d000944779c5b28977e003ea98d0c
Author: Vikash Garodia <vikash.garodia@oss.qualcomm.com>

media: iris: Skip DMA mask setup when the core device has no IOMMU

This commit updates iris_probe(), iris_reserve_iova_region(), and
iris_unreserve_iova_region() to check device_iommu_mapped(dev) before
performing IOVA reservation and DMA mask configuration on the parent iris
device.

>     media: iris: Skip DMA mask setup when the core device has no IOMMU
>     
>     Once the streams are described as context bank subnodes, the "iommus"
>     property no longer sits on the parent iris node. That device then has no
>     IOMMU domain of its own and never performs DMA directly, so calling
>     dma_set_mask_and_coherent() on it is meaningless, and on a device with no
>     IOMMU it can fail and abort probe.
>     
>     Set the DMA mask only when device_iommu_mapped() reports an IOMMU on the
>     core device. Platforms that have not been converted still carry "iommus"
>     on the parent node and keep the existing setup. For converted platforms
>     the mask is applied to each context bank device instead, in
>     iris_create_cb_dev().
>     
>     Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
>     Co-developed-by: Vishnu Reddy <busanna.reddy@oss.qualcomm.com>
>     Signed-off-by: Vishnu Reddy <busanna.reddy@oss.qualcomm.com>
>     Signed-off-by: Vikash Garodia <vikash.garodia@oss.qualcomm.com>
> 
> diff --git a/drivers/media/platform/qcom/iris/iris_probe.c b/drivers/media/platform/qcom/iris/iris_probe.c
> index e659461716ba4..6d7147d73d40b 100644
> --- a/drivers/media/platform/qcom/iris/iris_probe.c
> +++ b/drivers/media/platform/qcom/iris/iris_probe.c
> @@ -154,6 +154,9 @@ static void iris_unreserve_iova_region(struct device *dev, struct dma_iova_state
>  {
>  	unsigned int i;
>  
> +	if (!device_iommu_mapped(dev))
> +		return;
> +
>  	for (i = 0; dma_iova_size(&iova_state[i]); i++)
>  		dma_iova_free(dev, &iova_state[i]);
>  }
> @@ -167,6 +170,9 @@ static int iris_reserve_iova_region(struct device *dev, struct dma_iova_state **
>  	unsigned int count = 0;
>  	int ret = -ENOMEM;
>  
> +	if (!device_iommu_mapped(dev))
> +		return 0;

[Severity: Low]
This isn't a bug, but the commit title and message only describe skipping
dma_set_mask_and_coherent() in iris_probe().

Should the commit message also mention skipping the IOVA region reservation in
iris_reserve_iova_region() and iris_unreserve_iova_region() when the core
device has no IOMMU (where dma_iova_try_alloc() would otherwise return false
and cause iris_probe() to fail with -ENOMEM)?

> +
>  	state = devm_kcalloc(dev, BITS_PER_TYPE(dma_addr_t) + 1, sizeof(*state), GFP_KERNEL);
>  	if (!state)
>  		return ret;
> @@ -433,9 +439,11 @@ static int iris_probe(struct platform_device *pdev)
>  
>  	dma_mask = core->iris_platform_data->dma_mask;
>  
> -	ret = dma_set_mask_and_coherent(dev, dma_mask);
> -	if (ret)
> -		goto err_vdev_unreg_enc;
> +	if (device_iommu_mapped(dev)) {
> +		ret = dma_set_mask_and_coherent(dev, dma_mask);
> +		if (ret)
> +			goto err_vdev_unreg_enc;
> +	}

[Severity: Medium]
Can this allow iris_probe() to succeed when the SMMU has not probed yet on
platforms converted to context bank subnodes?

On converted platforms, the parent iris node has no iommus property, so
platform_dma_configure(core->dev) sees -ENODEV from of_iommu_configure() and
treats it as non-fatal. In addition, fw_devlink does not block core->dev on
the child subnodes' iommus phandles because parse_iommus is marked optional
and descendant fwnode links use FW_DEVLINK_FLAGS_PERMISSIVE.

When iris_init_cb_devs() -> iris_create_cb_dev() registers the child context
bank devices:

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);
	...
}

those child platform devices have no driver bound, and
BUS_NOTIFY_ADD_DEVICE -> iommu_bus_notifier() -> iommu_probe_device() ->
iommu_init_device() ignores the -EPROBE_DEFER returned by
dev->bus->dma_configure(&pdev->dev) when the SMMU is not ready.

With both iris_reserve_iova_region() and dma_set_mask_and_coherent() now
skipped on core->dev when !device_iommu_mapped(dev), wouldn't iris_probe()
finish and register the V4L2 video devices while neither core->dev nor
core->np_dev / core->p_dev is attached to an IOMMU domain, causing subsequent
iris_open() -> iris_core_init() -> iris_hfi_queues_init() calls to run
dma_alloc_attrs() on an unmapped context bank device?

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