Re: [PATCH 11/12] iommu: qcom_iommu: add the MSM8974 instances

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The commit message claims to add per-instance configurations for five MSM8974 IOMMUs (explicitly mentioning camera blocks), but only three are provided.
- [Low] The Venus IOMMU is configured to halt the micro-MMU, but the commit message only justifies this behavior for the display (MDP) IOMMU.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the qcom_iommu_fault interrupt handler accesses device hardware registers directly without checking the device's PM state.
--

--- Patch [11]: [PATCH 11/12] iommu: qcom_iommu: add the MSM8974 instances ---
commit bfa29c5d93fde62303c2c0ea8deeb5f5b59f6431
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

iommu: qcom_iommu: add the MSM8974 instances

This commit adds per-instance configurations for MSM8974 IOMMUs. It configures
them to terminate faulting transactions rather than stalling.

> Add the per-instance configurations for the five MSM8974 IOMMUs. MDP and
> Venus are TZ-secured (SCM secure IDs 1 and 0), so only their context
> banks are programmed, like on MSM8916. GPU is fully OS-managed. The
> stream ID maps come from the downstream MSM8974-v2 DT. All instances use
> the short-descriptor pagetable format with the AFE quirk.

[Severity: Medium]
The commit message claims to add per-instance configurations for five
MSM8974 IOMMUs, but only three are provided in this patch. Are the other
instances missing from the patch, or does the commit message need updating?

> All instances terminate faulting transactions rather than stalling them,
> as downstream does. The display IOMMU also halts the micro-MMU while its
> context banks are programmed: a hung GPU cannot drain, so halting it
> during GPU recovery resets the SoC.

[Severity: Low]
Does the Venus IOMMU also need to halt the micro-MMU? The commit message
only justifies this behavior for the display (MDP) IOMMU, but the Venus
configuration in msm8974_venus_cfg sets halt = true. Could the commit
message be expanded to document this?

> diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> index 9523ae95fa125..bc7d692a20436 100644
> --- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> +++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c

[ ... ]

> +static const struct qcom_iommu_cfg msm8974_venus_cfg = {
> +	.halt = true,
> +	.no_stall = true,
> +	.fmt = ARM_V7S,
> +	.no_afe = true,
> +	.ctx_restore = true,
> +	.bfb = msm8974_venus_bfb,
> +	.num_bfb = ARRAY_SIZE(msm8974_venus_bfb),
> +};

[ ... ]

>  static const struct of_device_id qcom_iommu_of_match[] = {
>  	{ .compatible = "qcom,msm-iommu-v1" },
>  	{ .compatible = "qcom,msm-iommu-v2" },
> +	{ .compatible = "qcom,msm8974-gpu-iommu", .data = &msm8974_gpu_cfg },
> +	{ .compatible = "qcom,msm8974-mdp-iommu", .data = &msm8974_mdp_cfg },
> +	{ .compatible = "qcom,msm8974-venus-iommu", .data = &msm8974_venus_cfg },
>  	{ /* sentinel */ }
>  };

[Severity: High]
This is a pre-existing issue, but the qcom_iommu_fault interrupt handler
accesses hardware registers directly without checking the device PM state:

drivers/iommu/arm/arm-smmu/qcom_iommu.c:qcom_iommu_fault() {
    ...
	fsr = iommu_readl(ctx, ARM_SMMU_CB_FSR);

	if (!(fsr & ARM_SMMU_CB_FSR_FAULT))
		return IRQ_NONE;
    ...
}

Because the interrupt is shared (IRQF_SHARED), an interrupt from another
device on the same line could trigger this handler while the IOMMU is in a
suspended PM state. Can reading from the unpowered hardware register
ARM_SMMU_CB_FSR cause a bus stall or return 0xffffffff? Should the handler
use pm_runtime_get_if_active before accessing the registers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260809-msm8974-iommu-upstream-v1-0-87f5cd492560@oss.qualcomm.com?part=11