Re: [PATCH 3/4] genirq/msi: Move prepare() call to per-device allocation

[Marc Zyngier <maz@kernel.org>]

On Mon, 12 May 2025 15:24:39 +0100,
Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> On Sun, May 11 2025 at 17:35, Marc Zyngier wrote:
> > The current device MSI infrastructure is subtly broken, as it
> > will issue an .msi_prepare() callback into the MSI controller
> > driver every time it needs to allocate an MSI. That's pretty wrong,
> > as the contract between the MSI controller and the core code is that
> > .msi_prepare() is called exactly once per device.
> 
> That contract is nowhere written in stone.

It was *definitely* there the first place, and a baked in assumption
since the ITS code was merged. You're welcome to come up with a new
scheme, but the way the HW works requires this prepare phase to take
place once per device.

If we can't have that, maybe we should consider reverting the GICv3/v4
code back to the pre-6.10 scheme that doesn't suffer from this issue.

> There are some MSI controller which get confused about that, but that's
> a problem of said controllers

No. It's an infrastructure problem. This model worked before for a
whole class of HW, until it was mutated into something else.

> 
> > diff --git a/include/linux/msi.h b/include/linux/msi.h
> > index 0a44a2cba3105..68a8b2d03eba9 100644
> > --- a/include/linux/msi.h
> > +++ b/include/linux/msi.h
> > @@ -513,12 +513,14 @@ struct msi_domain_info {
> >   * @chip:	Interrupt chip for this domain
> >   * @ops:	MSI domain ops
> >   * @info:	MSI domain info data
> > + * @arg:	MSI domain allocation data (arch specific)
> 
> arg is a horrible name. Can this please be alloc_info or such?

Because that's the name every single function that takes it as a
parameter uses? But sure, whatever name you want.

> 
> > @@ -1025,6 +1026,7 @@ bool msi_create_device_irq_domain(struct device *dev, unsigned int domid,
> >  	bundle->info.ops = &bundle->ops;
> >  	bundle->info.data = domain_data;
> >  	bundle->info.chip_data = chip_data;
> > +	bundle->info.alloc_data = &bundle->arg;
> >  
> >  	pops = parent->msi_parent_ops;
> >  	snprintf(bundle->name, sizeof(bundle->name), "%s%s-%s",
> > @@ -1053,21 +1055,28 @@ bool msi_create_device_irq_domain(struct device *dev, unsigned int domid,
> >  	msi_lock_descs(dev);
> 
> Please work against tip irq/msi which carries the guard() replacement
> for msi_lock_descs(). This patch heavily conflicts with the queued
> changes.
> 
> > +static int __populate_alloc_info(struct irq_domain *domain, struct device *dev,
> > +				 unsigned int nirqs, msi_alloc_info_t *arg)
> > +{
> 
> Why does this need double underscores?

Because it doesn't look that out of place in this file?

> 
> > +	struct msi_domain_info *info = domain->host_data;
> > +	int ret = 0;
> > +
> > +	/*
> > +	 * If the caller has provided a template alloc info, use that. Once
> > +	 * all users of msi_create_irq_domain() have been eliminated, this
> > +	 * should be the only source of allocation information, and the
> > +	 * prepare call below should be finally removed.
> 
> That's only a matter of decades :)
>
> > +	 */
> > +	if (info->alloc_data)
> > +		*arg = *info->alloc_data;
> > +	else
> > +		ret = msi_domain_prepare_irqs(domain, dev, nirqs, arg);
> > +
> > +	return ret;
> 
> 	if (!info->alloc_data)
>         	return msi_domain_prepare_irqs(domain, dev, nirqs, arg);
> 
> 	*arg = *info->alloc_data;
>         return 0;
> 
> perhaps?

Sure.

	M.

-- 
Without deviation from the norm, progress is not possible.