Re: [PATCH 09/12] iommu: qcom_iommu: allow faulting transactions to terminate

[sashiko-bot@kernel.org]

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

New issues:
- [High] The `qcom_iommu_fault` interrupt handler unconditionally writes to `ARM_SMMU_CB_RESUME`, causing CONSTRAINED UNPREDICTABLE behavior when `no_stall` is enabled.

Pre-existing issues:
- [High] The driver ignores the return value of `pm_runtime_get_sync()`, risking a synchronous external abort on QCOM platforms if power enablement fails.
--

--- Patch [9]: [PATCH 09/12] iommu: qcom_iommu: allow faulting transactions to terminate ---
commit 7786a25b4251a5907f3352e2d3045ccfe7aefaab
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

iommu: qcom_iommu: allow faulting transactions to terminate

This commit introduces a no_stall flag to qcom_iommu_cfg. When set, this
flag clears the CFCFG bit in SCTLR so that faulting transactions terminate
instead of stalling. The interrupt handler remains unchanged.

> through the existing interrupt handler; only the stall goes away, and
> with it the handler's RESUME write becomes a formality.

[Severity: High]
Does this write actually become a formality, or could it cause a regression
with unpredictable behavior?

If stalling is disabled, FSR.SS will remain 0 when a context fault occurs.
According to the ARM SMMUv2 spec, writing to CB_RESUME when the stall status
(FSR.SS) is 0 is constrained unpredictable and can trigger a synchronous
abort.

The qcom_iommu_fault() interrupt handler unconditionally writes to
ARM_SMMU_CB_RESUME:

drivers/iommu/arm/arm-smmu/qcom_iommu.c:qcom_iommu_fault() {
    ...
	iommu_writel(ctx, ARM_SMMU_CB_FSR, fsr);
	iommu_writel(ctx, ARM_SMMU_CB_RESUME, ARM_SMMU_RESUME_TERMINATE);
    ...
}

Should the unconditional write to CB_RESUME be guarded by a check for
FSR.SS?

> diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> index f27f8722ee640..2b2cf665c39db 100644
> --- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> +++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
[ ... ]
> @@ -500,6 +502,9 @@ static int qcom_iommu_init_domain(struct iommu_domain *domain,
>  		if (qcom_iommu->cfg && qcom_iommu->cfg->no_afe)
>  			reg &= ~ARM_SMMU_SCTLR_AFE;
>  
> +		if (qcom_iommu->cfg && qcom_iommu->cfg->no_stall)
> +			reg &= ~ARM_SMMU_SCTLR_CFCFG;
> +
>  		ctx->sctlr = reg;
>  
>  		qcom_iommu_program_ctx(qcom_iommu, ctx);

[Severity: High]
This is a pre-existing issue, but is it safe to proceed with initializing
the domain without checking if power enablement succeeded?

This function is called by qcom_iommu_attach_dev(), which ignores the return
value of pm_runtime_get_sync():

drivers/iommu/arm/arm-smmu/qcom_iommu.c:qcom_iommu_attach_dev() {
    ...
	pm_runtime_get_sync(qcom_iommu->dev);
	ret = qcom_iommu_init_domain(domain, qcom_iommu, dev);
	pm_runtime_put_sync(qcom_iommu->dev);
    ...
}

If pm_runtime_get_sync() fails, qcom_iommu_init_domain() will perform MMIO
accesses on unpowered hardware. On Qualcomm platforms, accessing unclocked or
unpowered IOMMU hardware registers causes a synchronous external abort.

Should qcom_iommu_attach_dev() check the return value and safely abort before
attempting to access unclocked registers?

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