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

[Marc Zyngier]

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

> On Tue, 19 Feb 2019 16:27:16 +0000
> Marc Zyngier <marc.zyngier@xxxxxxx> wrote:
> 
> > On Tue, 19 Feb 2019 16:57:23 +0100
> > Thomas Bogendoerfer <tbogendoerfer@xxxxxxx> 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@xxxxxxx>
> > > ---
> > >  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.