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

[Thomas Gleixner <tglx@kernel.org>]

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.

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