Re: [PATCH v3 2/6] iommu/arm-smmu: Add interconnect bandwidth voting support

[sashiko-bot@kernel.org]

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

New issues:
- [High] If `smmu->impl->runtime_suspend()` fails, `arm_smmu_runtime_suspend()` returns the error to the PM core after clocks have already been disabled, leaving the device logically active but physically unclocked.
- [Medium] Inconsistent power sequencing between probe and runtime PM regarding the enablement of clocks and the interconnect bandwidth vote.

Pre-existing issues:
- [High] The use of `devm_krealloc()` in `qcom_smmu_create()` leaves appended struct fields uninitialized, resulting in random hardware configuration.
- [High] Shared interrupt handlers access SMMU hardware without verifying PM runtime state, and runtime suspend lacks `synchronize_irq()`.
--

commit ca13a0dc53feec15c5946623eccfe70dcf798821
Author: Bibek Kumar Patro <bibek.patro@oss.qualcomm.com>

iommu/arm-smmu: Add interconnect bandwidth voting support
    
On some SoCs the SMMU registers require an active interconnect
bandwidth vote to be accessible. While other clients typically
satisfy this requirement implicitly, certain corner cases (e.g.
during sleep/wakeup transitions) can leave the SMMU without a
vote, causing intermittent register access failures.

> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> index e2c914fccd6fc..5133d3ab023a2 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c

[ ... ]

> @@ -691,6 +738,12 @@ static struct arm_smmu_device *qcom_smmu_create(struct arm_smmu_device *smmu,
>  	qsmmu->smmu.impl = impl;
>  	qsmmu->data = data;
>  
> +	if (is_adreno_smmu) {
> +		ret = qcom_adreno_smmu_icc_init(&qsmmu->smmu);
> +		if (ret)
> +			return ERR_PTR(ret);
> +	}
> +
>  	return &qsmmu->smmu;
>  }

[Severity: High]
This is a pre-existing issue, but I noticed devm_krealloc() is used in
qcom_smmu_create() without passing __GFP_ZERO. Does this leave newly
appended fields in the struct (like qsmmu->stall_enabled) uninitialized?

When qcom_adreno_smmu_write_sctlr() later checks
if (qsmmu->stall_enabled & BIT(idx)), could it read uninitialized memory
and unpredictably enable stall-on-fault behavior?

> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> index 0bd21d206eb3e..a27804e15738c 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c

[ ... ]

> @@ -2189,6 +2189,14 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>  	if (err)
>  		return err;
>  
> +	if (smmu->impl && smmu->impl->runtime_resume) {
> +		err = smmu->impl->runtime_resume(smmu);
> +		if (err) {
> +			clk_bulk_disable_unprepare(smmu->num_clks, smmu->clks);
> +			return err;
> +		}
> +	}
> +
>  	err = arm_smmu_device_cfg_probe(smmu);

[Severity: Medium]
In arm_smmu_runtime_resume(), the interconnect bandwidth vote is
established before the clocks are enabled. However, in this probe
sequence, clk_bulk_prepare_enable() runs just before this runtime_resume
callback.

Could this sequence inversion cause hardware instability during probe if
clocks are enabled without the required active bandwidth vote?

[ ... ]

> @@ -2309,6 +2329,9 @@ static int __maybe_unused arm_smmu_runtime_suspend(struct device *dev)
>  
>  	clk_bulk_disable(smmu->num_clks, smmu->clks);
>  
> +	if (smmu->impl && smmu->impl->runtime_suspend)
> +		return smmu->impl->runtime_suspend(smmu);
> +
>  	return 0;
>  }

[Severity: High]
If smmu->impl->runtime_suspend() fails here (for example, if icc_set_bw
returns an error), the error is propagated to the PM core. The PM core
reacts to a failed suspend by leaving the device in the RPM_ACTIVE state.

Since the clocks were already disabled by clk_bulk_disable() right above
this, does this leave the device logically active but physically
unclocked? Could subsequent hardware accesses then trigger a synchronous
external abort?

[Severity: High]
This isn't a bug introduced by this patch, but since the driver requests
IRQF_SHARED for its fault interrupts, shouldn't synchronize_irq() be
called here during runtime suspend to wait for any in-flight shared IRQs?

Since the interrupt handlers (like arm_smmu_context_fault()) do not use
pm_runtime_get_if_active() to verify the device is powered before reading
hardware registers, could a shared interrupt firing while the SMMU is
fully suspended cause a crash when it accesses unclocked hardware?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706-smmu_interconnect_addition-v3-0-afdca0125a65@oss.qualcomm.com?part=2