Re: [PATCH v10 08/15] iommu/arm-smmu-v3: Cache and restore MSI config

[Pranjal Shrivastava <praan@google.com>]

On Tue, Sep 08, 2026 at 09:56:33PM +0200, Thomas Gleixner wrote:
> On Tue, Sep 08 2026 at 17:17, Pranjal Shrivastava wrote:
> > +static void __maybe_unused arm_smmu_resume_msis(struct arm_smmu_device *smmu)
> > +{
> > +	/* Clear the MSI address regs as they reset to unknown value */
> > +	writeq_relaxed(0, smmu->base + ARM_SMMU_GERROR_IRQ_CFG0);
> > +	writeq_relaxed(0, smmu->base + ARM_SMMU_EVTQ_IRQ_CFG0);
> > +
> > +	if (smmu->features & ARM_SMMU_FEAT_PRI)
> > +		writeq_relaxed(0, smmu->base + ARM_SMMU_PRIQ_IRQ_CFG0);
> > +
> > +	if (!(smmu->features & ARM_SMMU_FEAT_MSI))
> > +		return;
> > +
> > +	if (!smmu->dev->msi.domain) {
> > +		dev_err(smmu->dev, "msi_domain absent during resume\n");
> > +		smmu->features &= ~ARM_SMMU_FEAT_MSI;
> > +		return;
> 
> If dev->msi.domain == NULL then arm_smmu_setup_msis() already cleared
> the MSI feature bit. Has it magically been set again or does resume run
> before init or does dev->msi.domain magically disappear during suspend?
> 
> I'm all for defensive programming, but this is voodoo and not structured
> defense.
> 
> Aside of that dev->msi.domain is the patently wrong condition. For
> devices which instantiate a MSI device domain dev->msi.domain points to
> the MSI parent domain and not to the actual relevant device domain.
> 
> You can't query that easily by chasing pointers (for a reason), but
> there is no point to do so. See below and the patch I sent you.
> 

Guilty of voodoo defense. You're completely right—FEAT_MSI was already
vleared on fallback during probe, and checking the parent domain here was
misguided. Dropping that entirely. (I added the check here and then
updated the setup_msis() part).

> > +	platform_device_msi_rewrite(smmu->dev, smmu->gerr_irq, arm_smmu_write_msi_msg);
> > +	platform_device_msi_rewrite(smmu->dev, smmu->evtq.q.irq, arm_smmu_write_msi_msg);
> > +
> > +	if (smmu->features & ARM_SMMU_FEAT_PRI)
> > +		platform_device_msi_rewrite(smmu->dev, smmu->priq.q.irq, arm_smmu_write_msi_msg);
> 
> And that's exactly the point I made about sprinkling this stuff all over
> the place and thereby violating all layering rules.
> 
> Done correctly this whole function boils down to:
> 
> static void __maybe_unused arm_smmu_resume_msis(struct arm_smmu_device *smmu)
> {
> 	/* Clear the MSI address regs as they reset to unknown value */
> 	writeq_relaxed(0, smmu->base + ARM_SMMU_GERROR_IRQ_CFG0);
> 	writeq_relaxed(0, smmu->base + ARM_SMMU_EVTQ_IRQ_CFG0);
> 
> 	if (smmu->features & ARM_SMMU_FEAT_PRI)
> 		writeq_relaxed(0, smmu->base + ARM_SMMU_PRIQ_IRQ_CFG0);
> 
>         msi_device_domain_restore_msi_msgs(smmu->dev, 0);
> }
> 
> It just works simply because the function returns early when there is no
> domain or the domain is not a MSI device domain, which is correct
> because there is nothing to do when nothing is set up.
> 
> But that results in too comprehensible code I fear.
> 

I certainly won't complain about code being too comprehensible! :)
This is much cleaner than what I had.

I'll drop my implementation of patch 7 (platform_device_msi_rewrite) 
completely in favor of your core msi_device_domain_restore_msi_msgs() 
helper, simplify patch 8 to this and take it for a spin.

Thanks,
Praan