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

[Bibek Kumar Patro <bibek.patro@oss.qualcomm.com>]

On 5/16/2026 6:32 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] Dead code `-ENODATA` check in `arm_smmu_icc_get()` causes probe failure on platforms without interconnects.
> - [High] Shared IRQ handlers access SMMU hardware registers while the device might be runtime suspended.
> - [Medium] Interconnect bandwidth vote is not released during system shutdown if Runtime PM is disabled.
> --
> 
> commit efe802bf0c93309aa584a5eb41c34c9788a6e837
> Author: Bibek Kumar Patro <bibek.patro@oss.qualcomm.com>
> 
> 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 register
> access failures during transitions. The bandwidth vote is enabled during
> probe and runtime resume, and released during runtime suspend and errors.
> 
>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
>> index 0bd21d206eb3e..aedf5edf8f9b2 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 int arm_smmu_icc_get(struct arm_smmu_device *smmu)
>> +{
>> +	smmu->icc_path = devm_of_icc_get(smmu->dev, NULL);
>> +	if (IS_ERR(smmu->icc_path)) {
>> +		int err = PTR_ERR(smmu->icc_path);
>> +
>> +		if (err == -ENODATA) {
> 
> Is this -ENODATA check correct for handling missing interconnects?
> 
> Looking at devm_of_icc_get(), it relies on of_icc_get(), which returns
> -ENOENT when the interconnects DT property is missing, or -ENODEV on ACPI
> platforms where dev->of_node is NULL. It appears it never returns -ENODATA.
> 
> Will this cause the driver to unconditionally call dev_err_probe() and fail
> to probe on all ACPI platforms and DT platforms that do not define
> interconnect properties?
> 

ACK, good catch.
I will fix this in the next revision. Instead of checking for -ENODATA, 
the code should handle -ENODEV to correctly cover the case where the 
interconnect path is not available, and emit a non-fatal warning 
accordingly.

>> +			smmu->icc_path = NULL;
>> +			return 0;
>> +		}
>> +		return dev_err_probe(smmu->dev, err,
>> +				     "failed to get interconnect path\n");
>> +	}
>> +	return 0;
>> +}
>> +
>> +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));
>> +}
>> +
>> +static void arm_smmu_icc_disable(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_LOW));
>> +}
> 
> Should arm_smmu_icc_disable() also be called during system shutdown?
> 
> In arm_smmu_device_shutdown(), if pm_runtime_enabled(smmu->dev) is false,
> the driver manually disables the SMMU clocks via clk_bulk_disable().
> However, a corresponding call to drop the interconnect vote doesn't appear
> to be added to that branch.
> 
> Could this leave the bandwidth vote permanently held at
> ARM_SMMU_ICC_PEAK_BW_HIGH during system halt or kexec?
> 

ACK, adding this in the shutdown path would be safer.
It should also be includes alongside clk_bulk_disable() in the "non-RPM" 
else {} path of arm_smmu_device_shutdown().

> [ ... ]
>> @@ -2308,6 +2358,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;
>>   }
> 
> With the clocks disabled and the interconnect vote dropped here, is there a
> risk of a bus hang if a shared IRQ fires while the device is suspended?
> 
> The driver registers its global_fault and context_fault handlers as
> IRQF_SHARED. If another device on the same IRQ line triggers an interrupt
> while the SMMU is in RPM_SUSPENDED, the handler might attempt to read
> hardware registers (such as calling arm_smmu_gr0_read()) while the SMMU is
> powered down without an active clock or interconnect vote.
> 
> Should the interrupt handlers use pm_runtime_get_if_active() before
> accessing registers, and should this suspend callback use synchronize_irq()?
> 

Seems like this issue predates the interconnect voting patch and is not 
introduced by this series.

The clk_bulk_disable() call in arm_smmu_runtime_suspend() was already 
present prior to this change. Introducing arm_smmu_icc_disable() does 
not alter the risk of the race condition mentioned above.

So as per my understanding this comment can be ignored for the series,
Will raise a separate patch with the proposed fix on this suspected race 
condition.

Thanks & regards,
Bibek