On 27/06/15 04:52, Hanjun Guo wrote:

On 06/24/2015 01:38 AM, Lorenzo Pieralisi wrote:

quoted

On Tue, Jun 23, 2015 at 04:11:38PM +0100, Hanjun Guo wrote:

[...]

quoted

quoted

quoted

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 8fc67bc..d1b2131 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c

@@ -851,15 +851,22 @@ static struct notifier_block gic_cpu_notifier = {
   static int gic_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
   				unsigned int nr_irqs, void *arg)
   {
-	int i, ret;
+	int i;
   	irq_hw_number_t hwirq;
-	unsigned int type = IRQ_TYPE_NONE;
-	struct of_phandle_args *irq_data = arg;

-	ret = gic_irq_domain_xlate(domain, irq_data->np, irq_data->args,
-				   irq_data->args_count, &hwirq, &type);
-	if (ret)
-		return ret;
+	if (domain->of_node) {	/* DT case */
+		int ret;
+		unsigned int type = IRQ_TYPE_NONE;
+		struct of_phandle_args *irq_data = arg;
+
+		ret = gic_irq_domain_xlate(domain, irq_data->np,
+					irq_data->args,
+					irq_data->args_count, &hwirq, &type);
+		if (ret)
+			return ret;
+	} else {	/* ACPI case */
+		hwirq = (irq_hw_number_t)*(u32 *)arg;
+	}

If domain->of_node is NULL and system booted with DT the code above
does not fail (and if it fails almost certainly that won't be graceful)
but it should.

how about the following logic?

if (!domain->of_node && acpi_disabled)
	return -ENODEV;
else if (domain->of_node)
	dt related code;
else
	ACPI related code;

Code is not checking the node at present so:

if (acpi_disabled)
	dt code;
else
	ACPI code;

would do, but that's a nit.

quoted

quoted

quoted

   	for (i = 0; i < nr_irqs; i++)
   		gic_irq_domain_map(domain, virq + i, hwirq + i);

@@ -945,11 +952,11 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
   		gic_irqs = 1020;
   	gic->gic_irqs = gic_irqs;

-	if (node) {		/* DT case */
+	if (node || !acpi_disabled) {		/* DT or ACPI case */
   		gic->domain = irq_domain_add_linear(node, gic_irqs,
   						    &gic_irq_domain_hierarchy_ops,
   						    gic);

I think this is a bit more worrying, I mean passing a NULL node pointer to
the irqdomain layer which basically means you are booting out of ACPI

I'm little confused here, would you mind explaining more for your
worrying? To me, node pointer is optional and it's ok for ACPI
case.

quoted

(for you, if that's true for the irq_domain_add_linear implementation
that's another story), the node pointer should be optional but you
need feedback from IRQ layer maintainers here.

Sure.

Frankly, I'd really like to see ACPI using the "node" parameter for
something useful. This would save having to cache pointers all over the
place, will make find_irq_host() work as expected... etc.

See the comment at the top of linux/irqdomain.h :

"... This code could thus be used on other architectures by replacing
those two by some sort of arch-specific void * "token" used to identify
interrupt controllers."

Maybe it is time to bite the bullet.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...