Re: [PATCH v9 05/12] iommu/arm-smmu-v3: Cache and restore MSI config

[Pranjal Shrivastava <praan@google.com>]

On Tue, Aug 25, 2026 at 01:36:11PM -0300, Jason Gunthorpe wrote:
> > [ ... 37 lines skipped ... ]
> > +static void arm_smmu_resume_msi(struct arm_smmu_device *smmu,
> > +				unsigned int irq, const char *name)
> > +{
> > +	struct msi_desc *desc;
> > +	struct msi_msg msg;
> > +
> > +	if (!irq)
> > +		return;
> > +
> > +	desc = irq_get_msi_desc(irq);
> > +	if (!desc) {
> > +		dev_err(smmu->dev, "Failed to resume msi: %s", name);
> > +		return;
> > +	}
> > +
> > +	get_cached_msi_msg(irq, &msg);
> > +	arm_smmu_write_msi_msg(desc, &msg);
> > +}
> 
> I'd really rather the irq core do this than open coding it like this,
> it should be fairly common for any driver doing power management and
> platform_device_msi stuff?
> 
> So maybe:
> 
>         platform_device_msi_rewrite(dev, irq, arm_smmu_write_msi_msg);
> 
> ?

I guess it does make sense to add another irq core helper like:

void platform_device_msi_rewrite(struct device *dev, unsigned int irq,
                 irq_write_msi_msg_t write_msi_msg)
{
    struct msi_desc *desc = irq_get_msi_desc(irq);
    struct msi_msg msg;

    if (!desc)
        return;

    __get_cached_msi_msg(desc, &msg);
    write_msi_msg(desc, &msg);
}
EXPORT_SYMBOL_GPL(platform_device_msi_rewrite);

> 
> 
>  [Severity: Medium]
>  Will this correctly detect if we have fallen back to wired irqs?
> 
>  When setting up MSIs, if the allocation fails, the driver gracefully falls
>  back to wired irqs without clearing the MSI feature flag:
> 
>  arm_smmu_setup_msis() {
>      ...
>      ret = platform_device_msi_init_and_alloc_irqs(...)
>      if (ret) {
>          dev_warn(dev, "failed to allocate MSIs - falling back to wired irqs\n");
>          return;
>      }
>      ...
>  }
> 
>  Does this mean the resume checks here will still pass during a fallback,
>  causing irq_get_msi_desc() to return NULL and trigger a spurious dev_err()
>  for each irq on every resume?
> 
> Seems believable..

Ack. Yes, I think in this case we should be ideally clearing that
feature: smmu->features &= ~ARM_SMMU_FEAT_MSI; before return.

Thanks,
Praan