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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Missing cleanup of clock and interconnect resources on probe error paths.
- [High] Failures to enable the interconnect bandwidth vote are ignored, risking a system crash.
- [Medium] Interconnect bandwidth is voted after clocks are enabled during probe, contradicting architectural requirements and resume ordering.

Pre-existing issues:
- [High] Shared interrupt handlers access hardware registers without verifying if the device is active, violating PM guidelines and causing crashes.
- [High] The IOMMU device is exposed to the system before Runtime PM is fully initialized, creating a race condition that can lead to a PM usage counter underflow.
--

commit a35c4344b83f53cca1914e7921a505dbdaa63eb5
Author: Bibek Kumar Patro <bibek.patro@oss.qualcomm.com>
Subject: iommu/arm-smmu: Add interconnect bandwidth voting support

This commit adds support for an optional interconnect path to the arm-smmu
driver. It votes for bandwidth while the SMMU is active to prevent
intermittent register access failures during sleep/wakeup transitions.

> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> index 0bd21d206eb3e..07c7e44ec6a5b 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
[ ... ]
> @@ -86,6 +91,36 @@ static inline void arm_smmu_rpm_put(struct arm_smmu_device *smmu)
[ ... ]
> +static void arm_smmu_icc_enable(struct arm_smmu_device *smmu)
> +{
> +	if (smmu->icc_path)
> +		WARN_ON(icc_set_bw(smmu->icc_path, ARM_SMMU_ICC_AVG_BW,
> +				   ARM_SMMU_ICC_PEAK_BW_HIGH));
> +}

[Severity: High]
Does this code need to propagate the return value of icc_set_bw() instead of
ignoring it?

If the bandwidth vote fails due to aggregation failures or invalid constraints,
the hardware might lack the vote required for register access.

Proceeding to access SMMU registers in arm_smmu_device_probe() or
arm_smmu_runtime_resume() without the required bandwidth vote could result in
an unrecoverable bus fault.

Could we handle the error and propagate it to fail the probe or resume
sequence gracefully?

[ ... ]
> @@ -2189,6 +2224,17 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>  	if (err)
>  		return err;
>  
> +	/*
> +	 * Acquire and vote the interconnect path before accessing any SMMU
> +	 * registers (including ARM_SMMU_GR0_ID0 in arm_smmu_device_cfg_probe).
> +	 */
> +	err = arm_smmu_icc_get(smmu);

[Severity: Medium]
Could this ordering cause an architectural violation?

The commit message notes that the interconnect must have an active vote
whenever the adreno_smmu block is powered on. In arm_smmu_runtime_resume(),
this is handled correctly by calling arm_smmu_icc_enable() before
clk_bulk_enable().

However, in arm_smmu_device_probe(), clk_bulk_prepare_enable() is called
just before this code, before arm_smmu_icc_get() and arm_smmu_icc_enable().

Would it be better to match the resume path ordering to ensure the SMMU clocks
are not powered on without the required interconnect bandwidth vote?

> +	if (err) {
> +		clk_bulk_disable_unprepare(smmu->num_clks, smmu->clks);
> +		return err;
> +	}
> +	arm_smmu_icc_enable(smmu);
> +
>  	err = arm_smmu_device_cfg_probe(smmu);
>  	if (err)
>  		return err;

[Severity: High]
Are we leaking the clock references and interconnect vote on subsequent error
paths?

If arm_smmu_device_cfg_probe() or other subsequent initialization steps fail,
the function returns directly without calling clk_bulk_disable_unprepare() or
arm_smmu_icc_disable().

Should there be a cleanup path that properly releases these resources when
probe fails?

[Severity: High]
This is a pre-existing issue, but does arm_smmu_device_probe() expose the
IOMMU device to the system before Runtime PM is fully initialized?

Later in arm_smmu_device_probe(), iommu_device_register() is called before
pm_runtime_enable(). Registering the IOMMU makes it visible to the system,
allowing concurrent device probes to attach to it and trigger mapping
operations, which call arm_smmu_rpm_get().

Since pm_runtime_enable() hasn't run yet, pm_runtime_enabled() evaluates to
false, and arm_smmu_rpm_get() returns 0 without incrementing the usage counter.

If the probe thread subsequently executes pm_runtime_enable(), the concurrent
thread's matching arm_smmu_rpm_put() will see pm_runtime_enabled() as true and
decrement the counter, causing a structural underflow.

[ ... ]
> @@ -2308,6 +2360,7 @@ static int __maybe_unused arm_smmu_runtime_suspend(struct device *dev)
>  	struct arm_smmu_device *smmu = dev_get_drvdata(dev);
>  
>  	clk_bulk_disable(smmu->num_clks, smmu->clks);
> +	arm_smmu_icc_disable(smmu);
>  
>  	return 0;
>  }

[Severity: High]
This is a pre-existing issue, but does disabling the interconnect vote and
clocks here expose a crash in the shared interrupt handler?

The driver requests its IRQs (such as arm_smmu_global_fault) using the
IRQF_SHARED flag, and arm_smmu_global_fault() reads hardware registers
directly without checking the power state.

Since the handler doesn't use pm_runtime_get_if_active() to verify the device
is active, if another device on the shared IRQ line triggers an interrupt
while the SMMU is suspended, won't the SMMU's handler access unclocked and
unvoted hardware, leading to a synchronous bus abort?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260526-smmu_interconnect_addition-v2-0-2a6d8ca30d63@oss.qualcomm.com?part=2