Re: [RFC Part2 v1 01/21] irqdomain: Introduce new interfaces to support hierarchy irqdomains

[Jiang Liu <jiang.liu@linux.intel.com>]

On 2014/9/17 1:43, Thomas Gleixner wrote:
> Jiang,
> 
> On Thu, 11 Sep 2014, Jiang Liu wrote:
>> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
>>  	/* Create mapping */
>> -	virq = irq_create_mapping(domain, hwirq);
>> +#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
>> +	if (domain->ops->alloc)
>> +		virq = irq_domain_alloc_irqs(domain, -1, 1, NUMA_NO_NODE,
>> +					     irq_data);
>> +	else
>> +#endif
>> +		virq = irq_create_mapping(domain, hwirq);
> 
> I'd prefer to get rid of the #ifdef CONFIG...s in the code. So this
> can be written:
> 
>         if (irq_domain_has_hierarchy(domain))
> 		virq = irq_domain_alloc_irqs(domain, -1, 1, NUMA_NO_NODE,
> 					     irq_data);
> 	else
> 		virq = irq_create_mapping(domain, hwirq);
Sure, will kill the ifdef. 	

> 	   
> 
>>  	if (!virq)
>>  		return virq;
>>  
>> @@ -540,7 +542,11 @@ unsigned int irq_find_mapping(struct irq_domain *domain,
>>  		return 0;
>>  
>>  	if (hwirq < domain->revmap_direct_max_irq) {
>> +#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
>> +		data = irq_domain_get_irq_data(domain, hwirq);
>> +#else
>>  		data = irq_get_irq_data(hwirq);
>> +#endif
> 
> Similar here. Make irq_domain_get_irq_data() map to irq_get_irq_data() for
> the non hierarchy mode so you end up with a single line:
> 
> -  		data = irq_get_irq_data(hwirq);
> +		data = irq_domain_get_irq_data(domain, hwirq);
Sure.

>> +#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
>> +/**
>> + * irq_domain_alloc_irqs - Allocate IRQs from domain
>> + * @domain: domain to allocate from
>> + * @irq_base: allocate specified IRQ nubmer if irq_base >= 0
>> + * @nr_irqs: number of IRQs to allocate
>> + * @node: NUMA node id for memory allocation
>> + * @arg: domain specific argument
>> + * @realloc: IRQ descriptors have already been allocated if true
>> + *
>> + * Allocate IRQ numbers and initialized all data structures to support
>> + * hiearchy IRQ domains.
>> + * Parameter @realloc is mainly to support legacy IRQs.
> 
> What's the issue with the legacy irqs? So this has the interrupt
> descriptors allocated already. Are they already wired up for serving
> interrupts and what's the state of those lines?
Function arch_early_ioapic_init() will allocate irq descriptors and
irq_cfg structures for all legacy IRQ for three purposes:
1) To support ISA IRQs managed by 8259.
2) To reserve vectors on all CPUs for legacy IRQs
3) Prepare data structures to support pre_init_apic_IRQ0().
We will kill pre_init_apic_IRQ0() soon, so item 3 above won't be needed
anymore.

When __irq_domain_alloc_irqs() is called, only irq descriptor and
irq_cfg have been allocated, but the interrupt controller hardware
should be untouched yet.

> 
>> + * Returns error code or allocated IRQ number
> 
> Can you please add some documentation how the hierarchical allocation
> is supposed to work and how the domains are connected. That should
> probably go to Documentation/IRQ-domains.txt.
Sure, I will do my best to add documentations for it.
> 
> Other than that this looks pretty good! Nice work!
Thanks!
Gerry
> 
> Thanks,
> 
> 	tglx
>