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

Thomas Gleixner
2025-05-12T14:24:41Z

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.

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

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

> @@ -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?

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

Thanks,

tglx