Re: [PATCH v2 09/10] genirq/irqdomain: fall back to default domain when creating hierarchy domain

[Marc Zyngier <marc.zyngier@arm.com>]

On Tue, 19 Feb 2019 17:48:29 +0100
Thomas Bogendoerfer <tbogendoerfer@suse.de> wrote:

> On Tue, 19 Feb 2019 16:27:16 +0000
> Marc Zyngier <marc.zyngier@arm.com> wrote:
> 
> > On Tue, 19 Feb 2019 16:57:23 +0100
> > Thomas Bogendoerfer <tbogendoerfer@suse.de> wrote:
> > 
> > Hi Thomas,
> >   
> > > When creating hierarchy domains use irq_default_domain as parent, if no
> > > parent was given by the caller. This avoids adding helper code for
> > > querying the underlying platform irq domain.
> > > 
> > > Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de>
> > > ---
> > >  kernel/irq/irqdomain.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> > > index 8b0be4bd6565..617c482d0778 100644
> > > --- a/kernel/irq/irqdomain.c
> > > +++ b/kernel/irq/irqdomain.c
> > > @@ -1021,7 +1021,10 @@ struct irq_domain *irq_domain_create_hierarchy(struct irq_domain *parent,
> > >  	else
> > >  		domain = irq_domain_create_tree(fwnode, ops, host_data);
> > >  	if (domain) {
> > > -		domain->parent = parent;
> > > +		if (parent)
> > > +			domain->parent = parent;
> > > +		else
> > > +			domain->parent = irq_default_domain;
> > >  		domain->flags |= flags;
> > >  	}
> > >    
> > 
> > I'm really not keen on this. The whole "default domain" made sense at a
> > distant point in time (when irqdomains were new and platform code was
> > blissfully ignoring it), but it really looks like a sore spot in the
> > hierarchy code, which assumes that you always know what you're building
> > your hierarchy on top of.
> > 
> > It also create a small issue in the sense that you can create a root
> > domain using irq_domain_create_hierarchy() by passing NULL as the
> > parent. With this patch, the new domain now points to the default one,
> > with unexpected consequences.
> > 
> > So let's come back to first principles: How comes you can't obtain the
> > parent domain at creation time? Because I'd rather give you a way to
> > retrieve it instead if this.  
> 
> the bridge irq domain could be stacked on different underlying irq domains
> for different platforms (HUB is IP27, HEART for IP30 and BEDROCK for IP35).
> And my idea was to set a irq default domain in the IP27/IP30/IP35 platform
> code so that bridge code will pick up the correct underlying irq domain.
> As there is no device tree I haven't found an already implemented other way.

Ah, right. We have ways to solve this kind of problem without DT
(cough... ACPI cough...), but this requires the bridge code to at least
know *something* about the underlying domain (see the use of struct
fwnode in the ACPI IORT code, and the way it uses the routing
informations to build and retrieve fwnodes that are used to match
irq domains).

Do you have such firmware tables that could be used to store sideband
data and allow the bridge code to retrieve the corresponding pointer? I
appreciate this could be quite a lot of work for a platform that has
little future...

> Right now I have two idea to solve my problem without this patch:
> 
> - implement a SGI specific helper for getting the underlying irq domain
> - use a helper function to read irq_default_domain
> 
> What do you prefer ? Or do you see something else ?

Well, short of doing the above, I'd rather have something in the common
code that allows the default domain to be retrieved. How about the
patch below?

Thanks,

	M.

From 3a9e44941c203ef2ed659838f218a0203c963828 Mon Sep 17 00:00:00 2001
From: Marc Zyngier <marc.zyngier@arm.com>
Date: Wed, 20 Feb 2019 08:59:23 +0000
Subject: [PATCH] irqdomain: Allow the default irq domain to be retrieved

The default irq domain allows legacy code to create irqdomain
mappings without having to track the domain it is allocating
from. Setting the default domain is a one shot, fire and forget
operation, and no effort was made to be able to retrieve this
information at a later point in time.

Newer irqdomain APIs (the hierarchical stuff) relies on both
the irqchip code to track the irqdomain it is allocating from,
as well as some form of firmware abstraction to easily identify
which piece of HW maps to which irq domain (DT, ACPI).

For systems without such firmware (or legacy platform that are
getting dragged into the 21st century), things are a bit harder.
For these cases (and these cases only!), let's provide a way
to retrieve the default domain, allowing the use of the v2 API
without having to resort to platform-specific hacks.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 include/linux/irqdomain.h |  1 +
 kernel/irq/irqdomain.c    | 14 ++++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index 35965f41d7be..d2130dc7c0e6 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -265,6 +265,7 @@ extern struct irq_domain *irq_find_matching_fwspec(struct irq_fwspec *fwspec,
 						   enum irq_domain_bus_token bus_token);
 extern bool irq_domain_check_msi_remap(void);
 extern void irq_set_default_host(struct irq_domain *host);
+extern struct irq_domain *irq_get_default_host(void);
 extern int irq_domain_alloc_descs(int virq, unsigned int nr_irqs,
 				  irq_hw_number_t hwirq, int node,
 				  const struct irq_affinity_desc *affinity);
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 8b0be4bd6565..80818764643d 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -458,6 +458,20 @@ void irq_set_default_host(struct irq_domain *domain)
 }
 EXPORT_SYMBOL_GPL(irq_set_default_host);
 
+/**
+ * irq_get_default_host() - Retrieve the "default" irq domain
+ *
+ * Returns: the default domain, if any.
+ *
+ * Modern code should never use this. This should only be used on
+ * systems that cannot implement a firmware->fwnode mapping (which
+ * both DT and ACPI provide).
+ */
+struct irq_domain *irq_get_default_host(void)
+{
+	return irq_default_domain;
+}
+
 static void irq_domain_clear_mapping(struct irq_domain *domain,
 				     irq_hw_number_t hwirq)
 {
-- 
2.20.1


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