Re: [Patch Part2 v3 01/24] irqdomain: Introduce new interfaces to support hierarchy irqdomains

[Thomas Gleixner]

On Tue, 28 Oct 2014, Yingjoe Chen wrote:
> On Tue, 2014-10-28 at 16:26 +0800, Jiang Liu wrote:
> <deleted...>
> > @@ -471,7 +469,7 @@ unsigned int irq_create_of_mapping(struct of_phandle_args *irq_data)
> >  	struct irq_domain *domain;
> >  	irq_hw_number_t hwirq;
> >  	unsigned int type = IRQ_TYPE_NONE;
> > -	unsigned int virq;
> > +	int virq;
> >  
> >  	domain = irq_data->np ? irq_find_host(irq_data->np) : irq_default_domain;
> >  	if (!domain) {
> > @@ -480,6 +478,11 @@ unsigned int irq_create_of_mapping(struct of_phandle_args *irq_data)
> >  		return 0;
> >  	}
> >  
> > +	if (irq_domain_is_hierarchy(domain)) {
> > +		virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, irq_data);
> > +		return virq <= 0 ? 0 : virq;
> > +	}
> > +
> >  	/* If domain has no translation, then we assume interrupt line */
> >  	if (domain->ops->xlate == NULL)
> >  		hwirq = irq_data->args[0];
> > @@ -540,8 +543,8 @@ unsigned int irq_find_mapping(struct irq_domain *domain,
> >  		return 0;
> >  
> >  	if (hwirq < domain->revmap_direct_max_irq) {
> > -		data = irq_get_irq_data(hwirq);
> > -		if (data && (data->domain == domain) && (data->hwirq == hwirq))
> > +		data = irq_domain_get_irq_data(domain, hwirq);
> > +		if (data && data->hwirq == hwirq)
> >  			return hwirq;
> >  	}
> 
> Why do you want to move irq_domain_alloc_irqs up there?

That's the wrong question. The proper question is whether we can move
it after the xlate part. I don't see a reason why we cannot do that.

Jiang was right not to incorporate your patch to do that simply
because we want to keep the history of the evolution of this
code. Your patch is an enhancement and it needs to be discussed and
applied seperately.

So while we are at it:

> +	if (irq_domain_is_hierarchy(domain)) {
> +		if (domain->ops->xlate) {
> +			/*
> +			 * If we've already configured this interrupt,
> +			 * don't do it again, or hell will break loose.
> +			 */
> +			virq = irq_find_mapping(domain, hwirq);
> +			if (virq)
> +				return virq;

I can understand that it is an issue if the mapping exists already,
but I have to ask WHY is it correct behaviour to call into that code
for an existing mapping.

And why would this check only apply if domain->ops->xlate is set?
irq_create_mapping() does it unconditionally.

> Joe.C
> >  
> > @@ -709,3 +712,341 @@ const struct irq_domain_ops irq_domain_simple_ops = {

Please remove the rest of the mail to which you are not answering next time.

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/