Re: [PATCH v10 07/15] platform-msi: Introduce platform_device_msi_rewrite()

[Pranjal Shrivastava <praan@google.com>]

On Tue, Sep 08, 2026 at 09:40:49PM +0200, Thomas Gleixner wrote:
> On Tue, Sep 08 2026 at 17:17, Pranjal Shrivastava wrote:
> 
> ...
> > Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> 
> Jason. You really should know better by now :(
> 

Apologies for the confusion, Jason just directed me to avoid open coding
this in the iommu driver and have the irq core handle it instead. [1]
Jason had nothing to do with the specific implementation / design here,
that misintepretation / mess is mine :( 

I'm relatively less familiar with the MSI pieces, and gave it a go
(probably should've prefixed this patch with RFC). Sorry about that!

> > +/**
> > + * platform_device_msi_rewrite - Rewrite the MSI config for a platform device
> > + * @dev:		The device for which to rewrite interrupt
> > + * @irq:		The interrupt number to be rewritten.
> > + * @write_msi_msg:	Callback to write the interrupt message for @dev
> > + *
> > + * Rewrites the cached MSI message for a platform device.
> > + *
> > + * Note: Platform MSI does not automatically cache composed messages. The caller's
> > + * @write_msi_msg callback is expected to cache the message (e.g. into desc->msg)
> > + * during initial configuration so it can be rewritten on resume.
> > + */
> > +void platform_device_msi_rewrite(struct device *dev, unsigned int irq,
> > +				 irq_write_msi_msg_t write_msi_msg)
> 
> Why is this a platform device specific function and why does this need to
> hand in the write_msi_msg() callback, which is already known through the
> interrupt descriptor and the top level interrupt chip?
> 
> I spent an awful lot of time and effort to get rid of these platform MSI
> layering violations and now you start adding the same mess again.
> 
> Not going to happen.
> 

Ack. I'll address the layering violations

> > +{
> > +	struct msi_desc *desc;
> > +	struct msi_msg msg;
> > +
> > +	if (!irq || !write_msi_msg)
> > +		return;
> 
> Oh well.
> 
> > +	desc = irq_get_msi_desc(irq);
> > +	if (!desc) {
> > +		dev_err(dev, "Failed to get MSI descriptor for irq %u\n", irq);
> > +		return;
> > +	}
> 
> Doing this without having the underlying interrupt descriptor locked is
> a recipe for an undebuggable disaster waiting to happen. It might be
> "safe" in the context you are calling it but it's absolutely not safe in
> general.
> 

Ack. I was thinking about races but I assumed the descriptor shoudln't
change but that's a "happy" / unsafe assumption.

> > +	__get_cached_msi_msg(desc, &msg);
> > +	if (!msg.address_hi && !msg.address_lo) {
> > +		dev_warn(dev, "No cached MSI message found for irq %u\n", irq);
> 
> That's just wrong. A message with a zero address is valid, e.g. when an
> interrupt is shut down. So if there is random crap after resume in the
> message store and the interrupt is valid, but not requested, then the
> cached message still has to be written even if it is zero.
> 
> So this want's to be a function in the MSI core code. Also this is not a
> per interrupt problem it is obviously a per device domain problem.
> Simply because the device provides the message store for all MSI interrupts
> which originate from that same device and therefore _all_ MSI interrupts
> are affected by that, no?
> 
> So this all can be solved at the device domain level without sprinkling
> per interrupt invocations including conditionals all over the place.
> 

Ack. I was wondering if the irq core should also cache the message for
platform MSIs like it's done for PCI ? Would that be a bad idea? Or is
it this way by design? (I'm having to cache the msg in the iommu driver
atm).

> Something like the completely untested below should just work.
> 

Thanks for sharing this! I'll give it a go.

> Thanks,
> 
>         tglx
> ---
> --- a/include/linux/msi.h
> +++ b/include/linux/msi.h
> @@ -669,6 +669,8 @@ void msi_domain_free_irqs_all(struct dev
>  
>  struct msi_domain_info *msi_get_domain_info(struct irq_domain *domain);
>  
> +void msi_device_domain_restore_msi_msgs(struct device *dev, unsigned int domid);
> +
>  /* Per device platform MSI */
>  int platform_device_msi_init_and_alloc_irqs(struct device *dev, unsigned int nvec,
>  					    irq_write_msi_msg_t write_msi_msg);
> --- a/kernel/irq/msi.c
> +++ b/kernel/irq/msi.c
> @@ -1775,3 +1775,34 @@ bool msi_device_has_isolated_msi(struct
>  	return arch_is_isolated_msi();
>  }
>  EXPORT_SYMBOL_GPL(msi_device_has_isolated_msi);
> +
> +void msi_device_domain_restore_msi_msgs(struct device *dev, unsigned int domid)
> +{
> +	if (!dev->msi.data)
> +		return;
> +
> +	guard(msi_descs_lock)(dev);
> +	struct irq_domain *domain = msi_get_device_domain(dev, domid);
> +
> +	if (!domain || !irq_domain_is_msi_device(domain))
> +		return;
> +
> +	struct xarray *xa = &dev->msi.data->__domains[domid].store;
> +	struct msi_domain_info *info = domain->host_data;
> +	struct msi_desc *msi_desc;
> +	unsigned long idx;
> +
> +	xa_for_each_range(xa, idx, msi_desc, 0, info->hwsize) {
> +		/* Only handle MSI entries which have an interrupt associated */
> +		if (!msi_desc_match(msi_desc, MSI_DESC_ASSOCIATED))
> +			continue;
> +
> +		scoped_irqdesc_get_and_lock(msi_desc->irq, 0) {
> +			struct irq_data *data = irq_desc_get_irq_data(scoped_irqdesc);
> +			struct msi_msg msg = msi_desc->msg;
> +
> +			if (data->chip)
> +				irq_chip_write_msi_msg(data, &msg);
> +		}
> +	}
> +}
> 
> 
> 
>

Thanks,
Praan