Re: [PATCH v10 08/15] iommu/arm-smmu-v3: Cache and restore MSI config
From: Thomas Gleixner <tglx@kernel.org>
Date: 2026-09-08 19:56:45
Also in:
linux-arm-kernel, linux-iommu
On Tue, Sep 08 2026 at 17:17, Pranjal Shrivastava wrote:
quoted hunk ↗ jump to hunk
+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.
quoted hunk ↗ jump to hunk
+ 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.
Thanks,
tglx