Re: [PATCH 02/11] genirq/msi: Add helper for creating MSI-parent irq domains

[Thomas Gleixner <tglx@linutronix.de>]

On Wed, Dec 04 2024 at 12:45, Marc Zyngier wrote:
> Creating an irq domain that serves as an MSI parent requires
> a substantial amount of esoteric boiler-plate code, some of
> which is often provided twice (such as the bus token).
>
> To make things a bit simpler for the unsuspecting MSI tinkerer,
> provide a helper that does it for them, and serves as documentation
> of what needs to be provided.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  include/linux/msi.h |  7 +++++++
>  kernel/irq/msi.c    | 40 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 47 insertions(+)
>
> diff --git a/include/linux/msi.h b/include/linux/msi.h
> index b10093c4d00ea..f08d14cf07103 100644
> --- a/include/linux/msi.h
> +++ b/include/linux/msi.h
> @@ -594,6 +594,13 @@ struct irq_domain *msi_create_irq_domain(struct fwnode_handle *fwnode,
>  					 struct msi_domain_info *info,
>  					 struct irq_domain *parent);
>  
> +struct irq_domain *msi_create_parent_irq_domain(struct fwnode_handle *fwnode,
> +						const struct msi_parent_ops *msi_parent_ops,
> +						const struct irq_domain_ops *ops,
> +						unsigned long flags, unsigned long size,
> +						void *host_data,
> +						struct irq_domain *parent);

Can we please make this a template based interface similar to
msi_create_device_irq_domain()?

> +/**
> + * msi_create_parent_irq_domain - Create an MSI-parent interrupt domain
> + * @fwnode:		Optional fwnode of the interrupt controller
> + * @msi_parent_ops:	MSI parent callbacks and configuration
> + * @ops:		Interrupt domain ballbacks
> + * @flags:		Interrupt domain flags
> + * @size:		Interrupt domain size (0 if arbitrarily large)
> + * @host_data:		Interrupt domain private data
> + * @parent:		Parent irq domain
> + *
> + * Return: pointer to the created &struct irq_domain or %NULL on failure
> + */
> +struct irq_domain *msi_create_parent_irq_domain(struct fwnode_handle *fwnode,
> +						const struct msi_parent_ops *msi_parent_ops,
> +						const struct irq_domain_ops *ops,
> +						unsigned long flags, unsigned long size,
> +						void *host_data,
> +						struct irq_domain *parent)
> +{
> +	struct irq_domain_info info = {
> +		.fwnode		= fwnode,
> +		.size		= size,
> +		.hwirq_max	= size,
> +		.ops		= ops,
> +		.host_data	= host_data,
> +		.domain_flags	= flags | IRQ_DOMAIN_FLAG_MSI_PARENT,
> +		.parent		= parent,
> +		.bus_token	= msi_parent_ops->bus_select_token,
> +	};

Instead of hiding the template in the function?

We've been burnt with interfaces which might require extensions over
time before and I just converted the GIC patch (3/11) over to a template
at call site model. It results in the same code size reduction at the
call sites, but allows us to expand the template without touching any
existing driver in the future. See below.

It might be a good idea to have a specific msi_irq_domain_info template
which only contains the information required instead of reusing and
expanding irq_domain_info.

Hmm?

Thanks,

        tglx
---
--- a/drivers/irqchip/irq-gic-v2m.c
+++ b/drivers/irqchip/irq-gic-v2m.c
@@ -263,24 +263,25 @@ static struct msi_parent_ops gicv2m_msi_
 
 static __init int gicv2m_allocate_domains(struct irq_domain *parent)
 {
-	struct irq_domain *inner_domain;
 	struct v2m_data *v2m;
+	struct irq_domain_info info = {
+		.ops		= &gic2m_domain_ops,
+		.parent		= parent,
+		.msi_parent_ops = &gicv2m_msi_parent_ops,
+	};
 
 	v2m = list_first_entry_or_null(&v2m_nodes, struct v2m_data, entry);
 	if (!v2m)
 		return 0;
 
-	inner_domain = irq_domain_create_hierarchy(parent, 0, 0, v2m->fwnode,
-						   &gicv2m_domain_ops, v2m);
-	if (!inner_domain) {
-		pr_err("Failed to create GICv2m domain\n");
-		return -ENOMEM;
-	}
-
-	irq_domain_update_bus_token(inner_domain, DOMAIN_BUS_NEXUS);
-	inner_domain->flags |= IRQ_DOMAIN_FLAG_MSI_PARENT;
-	inner_domain->msi_parent_ops = &gicv2m_msi_parent_ops;
-	return 0;
+	info->fwnode = v2m->fwnode;
+	info->host_data = v2m;
+
+	if (msi_create_parent_irq_domain(&info))
+		return 0;
+
+	pr_err("Failed to create GICv2m domain\n");
+	return -ENOMEM;
 }
 
 static int __init gicv2m_init_one(struct fwnode_handle *fwnode,
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -5076,31 +5076,27 @@ static void __init __iomem *its_map_one(
 
 static int its_init_domain(struct its_node *its)
 {
-	struct irq_domain *inner_domain;
-	struct msi_domain_info *info;
+	struct msi_domain_info *msi_info;
+	struct irq_domain_info info = {
+		.fwnode		= its->fwnode_handle,
+		.ops		= &its_domain_ops,
+		.parent		= its_parent,
+		.msi_parent_ops = &gic_v3_its_msi_parent_ops,
+		.flags		= its->msi_domain_flags,
+	};
 
-	info = kzalloc(sizeof(*info), GFP_KERNEL);
-	if (!info)
+	msi_info = kzalloc(sizeof(*info), GFP_KERNEL);
+	if (!msi_info)
 		return -ENOMEM;
 
-	info->ops = &its_msi_domain_ops;
-	info->data = its;
-
-	inner_domain = irq_domain_create_hierarchy(its_parent,
-						   its->msi_domain_flags, 0,
-						   its->fwnode_handle, &its_domain_ops,
-						   info);
-	if (!inner_domain) {
-		kfree(info);
-		return -ENOMEM;
-	}
-
-	irq_domain_update_bus_token(inner_domain, DOMAIN_BUS_NEXUS);
-
-	inner_domain->msi_parent_ops = &gic_v3_its_msi_parent_ops;
-	inner_domain->flags |= IRQ_DOMAIN_FLAG_MSI_PARENT;
-
-	return 0;
+	msi_info->ops = &its_msi_domain_ops;
+	msi_info->data = its;
+	info.host_data = msi_info;
+
+	if (msi_create_parent_irq_domain(&info))
+		return 0;
+	kfree(info);
+	return -ENOMEM;
 }
 
 static int its_init_vpe_domain(void)
--- a/drivers/irqchip/irq-gic-v3-mbi.c
+++ b/drivers/irqchip/irq-gic-v3-mbi.c
@@ -209,17 +209,14 @@ static const struct msi_parent_ops gic_v
 
 static int mbi_allocate_domain(struct irq_domain *parent)
 {
-	struct irq_domain *nexus_domain;
+	struct irq_domain_info info = {
+		.fwnode		= parent->fwnode,
+		.ops		= &mbi_domain_ops,
+		.parent		= parent,
+		.msi_parent_ops = &gic_v3_mbi_msi_parent_ops,
+	};
 
-	nexus_domain = irq_domain_create_hierarchy(parent, 0, 0, parent->fwnode,
-						   &mbi_domain_ops, NULL);
-	if (!nexus_domain)
-		return -ENOMEM;
-
-	irq_domain_update_bus_token(nexus_domain, DOMAIN_BUS_NEXUS);
-	nexus_domain->flags |= IRQ_DOMAIN_FLAG_MSI_PARENT;
-	nexus_domain->msi_parent_ops = &gic_v3_mbi_msi_parent_ops;
-	return 0;
+	return msi_create_parent_irq_domain(&info) ? 0 : -ENOMEM;
 }
 
 int __init mbi_init(struct fwnode_handle *fwnode, struct irq_domain *parent)