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

On Tue, Sep 08, 2026 at 09:56:33PM +0200, Thomas Gleixner wrote:

On Tue, Sep 08 2026 at 17:17, Pranjal Shrivastava wrote:

quoted

+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).

quoted

+	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