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

[Thomas Gleixner]

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);
	   
	   

>  	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);


> +#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?

> + * 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.

Other than that this looks pretty good! Nice work!

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/