Re: [PATCH v11 13/16] iommu/arm-smmu-v3: Implement pm_runtime & system sleep ops

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] iommu/arm-smmu-v3: invalid synchronize_irq(0) call when gerror IRQ is absent
- [Low] iommu/arm-smmu-v3: dead code via unconditional pm_ops assignment
--

commit f1b9ff2a5159bd528e9134e997eee772c624264d
Author: Pranjal Shrivastava <praan@google.com>

iommu/arm-smmu-v3: Implement pm_runtime & system sleep ops

This commit implements pm_runtime and system sleep operations for the 
arm-smmu-v3 driver. It adds suspend and resume callbacks that coordinate 
command queue draining and device resets to manage power state transitions.

> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 89a4e756ff174..e127e3be3a6ba 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c

[ ... ]

> @@ -5830,6 +5930,150 @@ static void arm_smmu_device_shutdown(struct platform_device *pdev)
>  	arm_smmu_device_disable(smmu);
>  }
>  
> +static int __maybe_unused arm_smmu_runtime_suspend(struct device *dev)
> +{
> +	struct arm_smmu_device *smmu = dev_get_drvdata(dev);

[ ... ]

> +	/* Disable IRQ generation */
> +	arm_smmu_disable_irqs(smmu);
> +
> +	/* Wait for pending gerror handlers */
> +	synchronize_irq(smmu->combined_irq ? smmu->combined_irq : smmu->gerr_irq);

[Severity: Medium]
Is there a risk of calling synchronize_irq(0) here if both smmu->combined_irq
and smmu->gerr_irq are 0?

If the device firmware configuration lacks a combined IRQ and a gerror IRQ, 
and MSI allocation fails or falls back to wired interrupts, both variables 
would be initialized to 0. This evaluates to synchronize_irq(0), leading 
to an erroneous synchronization on an unrelated system interrupt (like a 
timer mapped to IRQ 0). Could this statement verify that the resulting IRQ 
is non-zero before invoking synchronize_irq()?

> +
> +	/* Handle any pending gerrors before powering down */
> +	arm_smmu_handle_gerror(smmu);

[ ... ]

> @@ -5846,6 +6090,7 @@ static struct platform_driver arm_smmu_driver = {
>  	.driver	= {
>  		.name			= "arm-smmu-v3",
>  		.of_match_table		= arm_smmu_of_match,
> +		.pm                     = &arm_smmu_pm_ops,

[Severity: Low]
Does this code leave dead functions compiled into the binary when CONFIG_PM
is disabled?

Assigning the arm_smmu_pm_ops structure unconditionally instead of wrapping it 
in pm_ptr() prevents the compiler from discarding the unused suspend and resume 
functions. Should this be wrapped as pm_ptr(&arm_smmu_pm_ops) to avoid 
dead code bloat?

>  		.suppress_bind_attrs	= true,
>  	},
>  	.probe	= arm_smmu_device_probe,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929034510.2023173-1-praan@google.com?part=13