[PATCH 01/10] irq: move some interrupt arch_* functions into struct irq_chip.

[PATCH 01/10] irq: move some interrupt arch_* functions into struct irq_chip.

[Yinghai Lu]

From: Ian Campbell <ian.campbell@citrix.com>

Move arch_init_copy_chip_data and arch_free_chip_data into function
pointers in struct irq_chip since they operate on irq_desc->chip_data.

arch_init_chip_data cannot be moved into struct irq_chip because
irq_desc->chip is not known at the time the irq_desc is setup. Instead
rename arch_init_chip_data to arch_init_irq_desc for PowerPC, the
only other user, whose usage better matches the new name.

To replace the x86 arch_init_chip_data functionality
irq_to_desc_alloc_node now takes a pointer to a function to allocate
the chip data. This is necessary to ensure the allocation happens
under the correct locking at the core level. On PowerPC and SH
architectures (the other users of irq_to_desc_alloc_node) pass in NULL
which retains existing chip_data behaviour.

I've retained the chip_data behaviour for uv_irq although it isn't
clear to me if these interrupt types support migration or how closely
related to the APIC modes they really are. If it weren't for this the
x86_{init,copy,free}_chip_data functions could be static to
io_apic.c.

I've tested by booting on an 64 bit x86 system with sparse IRQ enabled
and 32 bit without, but it's not clear to me what actions I need to
take to actually exercise some of these code paths.

-v4: yinghai add irq_to_desc_alloc_node_x...
     so could leave default path not changed...

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Michael Ellerman <michael@ellerman.id.au> [PowerPC rename portion]
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: Yinghai Lu <yinghai@kernel.org>
Cc: Jeremy Fitzhardinge <jeremy@goop.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: x86@kernel.org
Cc: linuxppc-dev@ozlabs.org
Cc: linux-kernel@vger.kernel.org
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: lguest@ozlabs.org
Cc: Paul Mundt <lethal@linux-sh.org>
Cc: linux-sh@vger.kernel.org
---
 arch/powerpc/kernel/irq.c      |    2 +-
 arch/x86/include/asm/hw_irq.h  |    7 +++++-
 arch/x86/kernel/apic/io_apic.c |   49 ++++++++++++++++++++++++++++++++++++---
 arch/x86/kernel/uv_irq.c       |    3 ++
 drivers/xen/events.c           |    7 +++++-
 include/linux/interrupt.h      |    1 -
 include/linux/irq.h            |   21 +++++++++++++----
 kernel/irq/chip.c              |    7 +++++
 kernel/irq/handle.c            |   13 ++++++----
 kernel/irq/numa_migrate.c      |   12 ++++++++-
 kernel/softirq.c               |    5 ----
 11 files changed, 102 insertions(+), 25 deletions(-)

diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
index 64f6f20..cafd378 100644
--- a/arch/powerpc/kernel/irq.c
+++ b/arch/powerpc/kernel/irq.c
@@ -1088,7 +1088,7 @@ int arch_early_irq_init(void)
 	return 0;
 }
 
-int arch_init_chip_data(struct irq_desc *desc, int node)
+int arch_init_irq_desc(struct irq_desc *desc, int node, init_chip_data_fn fn)
 {
 	desc->status |= IRQ_NOREQUEST;
 	return 0;
diff --git a/arch/x86/include/asm/hw_irq.h b/arch/x86/include/asm/hw_irq.h
index 46c0fe0..767d3f8 100644
--- a/arch/x86/include/asm/hw_irq.h
+++ b/arch/x86/include/asm/hw_irq.h
@@ -20,9 +20,9 @@
 #include <linux/percpu.h>
 #include <linux/profile.h>
 #include <linux/smp.h>
+#include <linux/irq.h>
 
 #include <asm/atomic.h>
-#include <asm/irq.h>
 #include <asm/sections.h>
 
 /* Interrupt handlers registered during init_IRQ */
@@ -61,6 +61,11 @@ extern void init_VISWS_APIC_irqs(void);
 extern void setup_IO_APIC(void);
 extern void disable_IO_APIC(void);
 
+extern void x86_copy_chip_data(struct irq_desc *old_desc,
+				  struct irq_desc *desc, int node);
+extern void x86_free_chip_data(struct irq_desc *old_desc,
+				  struct irq_desc *desc);
+
 struct io_apic_irq_attr {
 	int ioapic;
 	int ioapic_pin;
diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 463de9a..a917fdf 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -211,7 +211,7 @@ static struct irq_cfg *get_one_free_irq_cfg(int node)
 	return cfg;
 }
 
-int arch_init_chip_data(struct irq_desc *desc, int node)
+static int x86_init_chip_data(struct irq_desc *desc, int node)
 {
 	struct irq_cfg *cfg;
 
@@ -287,8 +287,8 @@ static void free_irq_2_pin(struct irq_cfg *old_cfg, struct irq_cfg *cfg)
 	old_cfg->irq_2_pin = NULL;
 }
 
-void arch_init_copy_chip_data(struct irq_desc *old_desc,
-				 struct irq_desc *desc, int node)
+void x86_copy_chip_data(struct irq_desc *old_desc,
+			   struct irq_desc *desc, int node)
 {
 	struct irq_cfg *cfg;
 	struct irq_cfg *old_cfg;
@@ -312,7 +312,7 @@ static void free_irq_cfg(struct irq_cfg *old_cfg)
 	kfree(old_cfg);
 }
 
-void arch_free_chip_data(struct irq_desc *old_desc, struct irq_desc *desc)
+void x86_free_chip_data(struct irq_desc *old_desc, struct irq_desc *desc)
 {
 	struct irq_cfg *old_cfg, *cfg;
 
@@ -329,6 +329,14 @@ void arch_free_chip_data(struct irq_desc *old_desc, struct irq_desc *desc)
 	}
 }
 /* end for move_irq_desc */
+int arch_init_irq_desc(struct irq_desc *desc, int node,
+			 init_chip_data_fn init_chip_data)
+{
+	if (!init_chip_data)
+		return x86_init_chip_data(desc, node);
+
+	return init_chip_data(desc, node);
+}
 
 #else
 struct irq_cfg *irq_cfg(unsigned int irq)
@@ -336,6 +344,15 @@ struct irq_cfg *irq_cfg(unsigned int irq)
 	return irq < nr_irqs ? irq_cfgx + irq : NULL;
 }
 
+void x86_copy_chip_data(struct irq_desc *old_desc,
+			   struct irq_desc *desc, int node)
+{
+}
+
+void x86_free_chip_data(struct irq_desc *old_desc, struct irq_desc *desc)
+{
+}
+
 #endif
 
 struct io_apic {
@@ -2747,6 +2764,9 @@ static struct irq_chip ioapic_chip __read_mostly = {
 	.set_affinity	= set_ioapic_affinity_irq,
 #endif
 	.retrigger	= ioapic_retrigger_irq,
+
+	.copy_chip_data = x86_copy_chip_data,
+	.free_chip_data = x86_free_chip_data,
 };
 
 static struct irq_chip ir_ioapic_chip __read_mostly = {
@@ -2762,6 +2782,9 @@ static struct irq_chip ir_ioapic_chip __read_mostly = {
 #endif
 #endif
 	.retrigger	= ioapic_retrigger_irq,
+
+	.copy_chip_data = x86_copy_chip_data,
+	.free_chip_data = x86_free_chip_data,
 };
 
 static inline void init_IO_APIC_traps(void)
@@ -3474,6 +3497,9 @@ static struct irq_chip msi_chip = {
 	.set_affinity	= set_msi_irq_affinity,
 #endif
 	.retrigger	= ioapic_retrigger_irq,
+
+	.copy_chip_data = x86_copy_chip_data,
+	.free_chip_data = x86_free_chip_data,
 };
 
 static struct irq_chip msi_ir_chip = {
@@ -3487,6 +3513,9 @@ static struct irq_chip msi_ir_chip = {
 #endif
 #endif
 	.retrigger	= ioapic_retrigger_irq,
+
+	.copy_chip_data = x86_copy_chip_data,
+	.free_chip_data = x86_free_chip_data,
 };
 
 /*
@@ -3646,6 +3675,9 @@ static struct irq_chip dmar_msi_type = {
 	.set_affinity = dmar_msi_set_affinity,
 #endif
 	.retrigger = ioapic_retrigger_irq,
+
+	.copy_chip_data = x86_copy_chip_data,
+	.free_chip_data = x86_free_chip_data,
 };
 
 int arch_setup_dmar_msi(unsigned int irq)
@@ -3703,6 +3735,9 @@ static struct irq_chip ir_hpet_msi_type = {
 #endif
 #endif
 	.retrigger = ioapic_retrigger_irq,
+
+	.copy_chip_data = x86_copy_chip_data,
+	.free_chip_data = x86_free_chip_data,
 };
 
 static struct irq_chip hpet_msi_type = {
@@ -3714,6 +3749,9 @@ static struct irq_chip hpet_msi_type = {
 	.set_affinity = hpet_msi_set_affinity,
 #endif
 	.retrigger = ioapic_retrigger_irq,
+
+	.copy_chip_data = x86_copy_chip_data,
+	.free_chip_data = x86_free_chip_data,
 };
 
 int arch_setup_hpet_msi(unsigned int irq, unsigned int id)
@@ -3800,6 +3838,9 @@ static struct irq_chip ht_irq_chip = {
 	.set_affinity	= set_ht_irq_affinity,
 #endif
 	.retrigger	= ioapic_retrigger_irq,
+
+	.copy_chip_data = x86_copy_chip_data,
+	.free_chip_data = x86_free_chip_data,
 };
 
 int arch_setup_ht_irq(unsigned int irq, struct pci_dev *dev)
diff --git a/arch/x86/kernel/uv_irq.c b/arch/x86/kernel/uv_irq.c
index ece73d8..4c61f1b 100644
--- a/arch/x86/kernel/uv_irq.c
+++ b/arch/x86/kernel/uv_irq.c
@@ -55,6 +55,9 @@ struct irq_chip uv_irq_chip = {
 	.eoi		= uv_ack_apic,
 	.end		= uv_noop,
 	.set_affinity	= uv_set_irq_affinity,
+
+	.copy_chip_data = x86_copy_chip_data,
+	.free_chip_data = x86_free_chip_data,
 };
 
 /*
diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 2f84137..64cbbe4 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -329,6 +329,11 @@ static void unmask_evtchn(int port)
 	put_cpu();
 }
 
+static int xen_init_chip_data(struct irq_desc *desc, int node)
+{
+	return 0;
+}
+
 static int find_unbound_irq(void)
 {
 	int irq;
@@ -341,7 +346,7 @@ static int find_unbound_irq(void)
 	if (irq == nr_irqs)
 		panic("No available IRQ to bind to: increase nr_irqs!\n");
 
-	desc = irq_to_desc_alloc_node(irq, 0);
+	desc = irq_to_desc_alloc_node_x(irq, 0, xen_init_chip_data);
 	if (WARN_ON(desc == NULL))
 		return -1;
 
diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 75f3f00..0b0d679 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -611,6 +611,5 @@ struct irq_desc;
 extern int early_irq_init(void);
 extern int arch_probe_nr_irqs(void);
 extern int arch_early_irq_init(void);
-extern int arch_init_chip_data(struct irq_desc *desc, int node);
 
 #endif
diff --git a/include/linux/irq.h b/include/linux/irq.h
index 707ab12..60f3368 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -140,6 +140,13 @@ struct irq_chip {
 	 * Will disappear.
 	 */
 	const char	*typename;
+
+	/* for move_irq_desc */
+	void		(*copy_chip_data)(struct irq_desc *old_desc,
+					  struct irq_desc *desc, int node);
+	void		(*free_chip_data)(struct irq_desc *old_desc,
+					  struct irq_desc *desc);
+
 };
 
 struct timer_rand_state;
@@ -208,10 +215,6 @@ struct irq_desc {
 	const char		*name;
 } ____cacheline_internodealigned_in_smp;
 
-extern void arch_init_copy_chip_data(struct irq_desc *old_desc,
-					struct irq_desc *desc, int node);
-extern void arch_free_chip_data(struct irq_desc *old_desc, struct irq_desc *desc);
-
 #ifndef CONFIG_SPARSE_IRQ
 extern struct irq_desc irq_desc[NR_IRQS];
 #endif
@@ -225,7 +228,15 @@ static inline struct irq_desc *move_irq_desc(struct irq_desc *desc, int node)
 }
 #endif
 
-extern struct irq_desc *irq_to_desc_alloc_node(unsigned int irq, int node);
+typedef int (*init_chip_data_fn)(struct irq_desc *, int node);
+int arch_init_irq_desc(struct irq_desc *desc, int node, init_chip_data_fn fn);
+struct irq_desc *irq_to_desc_alloc_node_x(unsigned int irq, int node,
+					       init_chip_data_fn fn);
+static inline struct irq_desc *irq_to_desc_alloc_node(unsigned int irq,
+							 int node)
+{
+	return irq_to_desc_alloc_node_x(irq, node, NULL);
+}
 
 /*
  * Pick up the arch-dependent methods:
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index bbba585..3dcdd2f 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -758,3 +758,10 @@ void __init set_irq_probe(unsigned int irq)
 	desc->status &= ~IRQ_NOPROBE;
 	raw_spin_unlock_irqrestore(&desc->lock, flags);
 }
+
+int __weak arch_init_irq_desc(struct irq_desc *desc, int node,
+				 init_chip_data_fn init_chip_data)
+{
+	return 0;
+}
+
diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
index 76d5a67..f30c9c7 100644
--- a/kernel/irq/handle.c
+++ b/kernel/irq/handle.c
@@ -100,7 +100,8 @@ void __ref init_kstat_irqs(struct irq_desc *desc, int node, int nr)
 	}
 }
 
-static void init_one_irq_desc(int irq, struct irq_desc *desc, int node)
+static void init_one_irq_desc(int irq, struct irq_desc *desc, int node,
+			      init_chip_data_fn init_chip_data)
 {
 	memcpy(desc, &irq_desc_init, sizeof(struct irq_desc));
 
@@ -120,7 +121,7 @@ static void init_one_irq_desc(int irq, struct irq_desc *desc, int node)
 		BUG_ON(1);
 	}
 	init_desc_masks(desc);
-	arch_init_chip_data(desc, node);
+	arch_init_irq_desc(desc, node, init_chip_data);
 }
 
 /*
@@ -198,7 +199,8 @@ int __init early_irq_init(void)
 	return arch_early_irq_init();
 }
 
-struct irq_desc * __ref irq_to_desc_alloc_node(unsigned int irq, int node)
+struct irq_desc * __ref irq_to_desc_alloc_node_x(unsigned int irq, int node,
+					       init_chip_data_fn init_chip_data)
 {
 	struct irq_desc *desc;
 	unsigned long flags;
@@ -227,7 +229,7 @@ struct irq_desc * __ref irq_to_desc_alloc_node(unsigned int irq, int node)
 		printk(KERN_ERR "can not alloc irq_desc\n");
 		BUG_ON(1);
 	}
-	init_one_irq_desc(irq, desc, node);
+	init_one_irq_desc(irq, desc, node, init_chip_data);
 
 	set_irq_desc(irq, desc);
 
@@ -277,7 +279,8 @@ struct irq_desc *irq_to_desc(unsigned int irq)
 	return (irq < NR_IRQS) ? irq_desc + irq : NULL;
 }
 
-struct irq_desc *irq_to_desc_alloc_node(unsigned int irq, int node)
+struct irq_desc *irq_to_desc_alloc_node_x(unsigned int irq, int node,
+					init_chip_data_fn init_chip_data)
 {
 	return irq_to_desc(irq);
 }
diff --git a/kernel/irq/numa_migrate.c b/kernel/irq/numa_migrate.c
index 963559d..9ea09c9 100644
--- a/kernel/irq/numa_migrate.c
+++ b/kernel/irq/numa_migrate.c
@@ -47,7 +47,8 @@ static bool init_copy_one_irq_desc(int irq, struct irq_desc *old_desc,
 	lockdep_set_class(&desc->lock, &irq_desc_lock_class);
 	init_copy_kstat_irqs(old_desc, desc, node, nr_cpu_ids);
 	init_copy_desc_masks(old_desc, desc);
-	arch_init_copy_chip_data(old_desc, desc, node);
+	if (desc->chip->copy_chip_data)
+		desc->chip->copy_chip_data(old_desc, desc, node);
 	return true;
 }
 
@@ -55,7 +56,8 @@ static void free_one_irq_desc(struct irq_desc *old_desc, struct irq_desc *desc)
 {
 	free_kstat_irqs(old_desc, desc);
 	free_desc_masks(old_desc, desc);
-	arch_free_chip_data(old_desc, desc);
+	if (desc->chip->free_chip_data)
+		desc->chip->free_chip_data(old_desc, desc);
 }
 
 static struct irq_desc *__real_move_irq_desc(struct irq_desc *old_desc,
@@ -107,9 +109,15 @@ out_unlock:
 
 struct irq_desc *move_irq_desc(struct irq_desc *desc, int node)
 {
+
 	/* those static or target node is -1, do not move them */
 	if (desc->irq < NR_IRQS_LEGACY || node == -1)
 		return desc;
+	/* IRQ chip does not support movement */
+	if (desc->chip_data &&
+	    (desc->chip->copy_chip_data == NULL ||
+	     desc->chip->free_chip_data == NULL))
+		return desc;
 
 	if (desc->node != node)
 		desc = __real_move_irq_desc(desc, node);
diff --git a/kernel/softirq.c b/kernel/softirq.c
index 7c1a67e..7df0209 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -895,8 +895,3 @@ int __init __weak arch_early_irq_init(void)
 {
 	return 0;
 }
-
-int __weak arch_init_chip_data(struct irq_desc *desc, int node)
-{
-	return 0;
-}
-- 
1.6.4.2

Re: [PATCH 01/10] irq: move some interrupt arch_* functions into struct irq_chip.

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 1716 bytes --]

On Sun, 2010-03-21 at 18:36 -0700, Yinghai Lu wrote:
> From: Ian Campbell <ian.campbell@citrix.com>
...
> To replace the x86 arch_init_chip_data functionality
> irq_to_desc_alloc_node now takes a pointer to a function to allocate
> the chip data. This is necessary to ensure the allocation happens
> under the correct locking at the core level. On PowerPC and SH
> architectures (the other users of irq_to_desc_alloc_node) pass in NULL
> which retains existing chip_data behaviour.
...
> 
> -v4: yinghai add irq_to_desc_alloc_node_x...
>      so could leave default path not changed...

Apologies for not noticing this sooner, but ..

> --- a/arch/powerpc/kernel/irq.c
> +++ b/arch/powerpc/kernel/irq.c
> @@ -1088,7 +1088,7 @@ int arch_early_irq_init(void)
>  	return 0;
>  }
>  
> -int arch_init_chip_data(struct irq_desc *desc, int node)
> +int arch_init_irq_desc(struct irq_desc *desc, int node, init_chip_data_fn fn)
>  {
>  	desc->status |= IRQ_NOREQUEST;
>  	return 0;

This is a bit feral, that is the init_chip_data_fn.

It seems like it only exists to support the following on x86:

> +int arch_init_irq_desc(struct irq_desc *desc, int node,
> +			 init_chip_data_fn init_chip_data)
> +{
> +	if (!init_chip_data)
> +		return x86_init_chip_data(desc, node);
> +
> +	return init_chip_data(desc, node);
> +}

Which is really just a hack to avoid an if (xen) check isn't it?

It looks to me like this should just be done via a current machine
vector or platform routine, in the same way as powerpc and (I think)
ia64, ie:

> +int arch_init_irq_desc(struct irq_desc *desc, int node)
> +{
> +	return current_machine->init_chip_data(desc, node);
> +}

cheers


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 01/10] irq: move some interrupt arch_* functions into struct irq_chip.

[Yinghai Lu]

On 03/21/2010 06:56 PM, Michael Ellerman wrote:
> On Sun, 2010-03-21 at 18:36 -0700, Yinghai Lu wrote:
>> From: Ian Campbell <ian.campbell@citrix.com>
> ...
>> To replace the x86 arch_init_chip_data functionality
>> irq_to_desc_alloc_node now takes a pointer to a function to allocate
>> the chip data. This is necessary to ensure the allocation happens
>> under the correct locking at the core level. On PowerPC and SH
>> architectures (the other users of irq_to_desc_alloc_node) pass in NULL
>> which retains existing chip_data behaviour.
> ...
>>
>> -v4: yinghai add irq_to_desc_alloc_node_x...
>>      so could leave default path not changed...
> 
> Apologies for not noticing this sooner, but ..
> 
>> --- a/arch/powerpc/kernel/irq.c
>> +++ b/arch/powerpc/kernel/irq.c
>> @@ -1088,7 +1088,7 @@ int arch_early_irq_init(void)
>>  	return 0;
>>  }
>>  
>> -int arch_init_chip_data(struct irq_desc *desc, int node)
>> +int arch_init_irq_desc(struct irq_desc *desc, int node, init_chip_data_fn fn)
>>  {
>>  	desc->status |= IRQ_NOREQUEST;
>>  	return 0;
> 
> This is a bit feral, that is the init_chip_data_fn.
> 
> It seems like it only exists to support the following on x86:
> 
>> +int arch_init_irq_desc(struct irq_desc *desc, int node,
>> +			 init_chip_data_fn init_chip_data)
>> +{
>> +	if (!init_chip_data)
>> +		return x86_init_chip_data(desc, node);
>> +
>> +	return init_chip_data(desc, node);
>> +}
> 
> Which is really just a hack to avoid an if (xen) check isn't it?

 arch/powerpc/kernel/irq.c      |    2 +-

diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
index 64f6f20..cafd378 100644
--- a/arch/powerpc/kernel/irq.c
+++ b/arch/powerpc/kernel/irq.c
@@ -1088,7 +1088,7 @@ int arch_early_irq_init(void)
 	return 0;
 }
 
-int arch_init_chip_data(struct irq_desc *desc, int node)
+int arch_init_irq_desc(struct irq_desc *desc, int node, init_chip_data_fn fn)
 {
 	desc->status |= IRQ_NOREQUEST;
 	return 0;

so this patch only change one line with powerpc code. 
> 
> It looks to me like this should just be done via a current machine
> vector or platform routine, in the same way as powerpc and (I think)
> ia64, ie:
> 
>> +int arch_init_irq_desc(struct irq_desc *desc, int node)
>> +{
>> +	return current_machine->init_chip_data(desc, node);
>> +}
> 
looks like xen in same run time, some irqs need x86_init_chip_data,
and some may need xen_init_chip_data later.

Thanks

Yinghai

Re: [PATCH 01/10] irq: move some interrupt arch_* functions into struct irq_chip.

[Paul Mundt]

On Sun, Mar 21, 2010 at 08:32:33PM -0700, Yinghai Lu wrote:
> diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
> index 64f6f20..cafd378 100644
> --- a/arch/powerpc/kernel/irq.c
> +++ b/arch/powerpc/kernel/irq.c
> @@ -1088,7 +1088,7 @@ int arch_early_irq_init(void)
>  	return 0;
>  }
>  
> -int arch_init_chip_data(struct irq_desc *desc, int node)
> +int arch_init_irq_desc(struct irq_desc *desc, int node, init_chip_data_fn fn)
>  {
>  	desc->status |= IRQ_NOREQUEST;
>  	return 0;
> 
> so this patch only change one line with powerpc code. 

This API seems to be going from bad to worse. Initially we had
arch_init_chip_data(), which had precisely nothing to do with chip_data
and only concerned itself with irq_desc, and now you're renaming it to
something sensible but also trying to bolt on some ad-hoc chip_data
relation at the same time thereby nullifying any benefit obtained from
renaming the function in the first place.

Renaming to xxx_irq_desc() while preserving the existing prototypes would
make sense, even if it's ultimately just unecessary churn.

> > It looks to me like this should just be done via a current machine
> > vector or platform routine, in the same way as powerpc and (I think)
> > ia64, ie:
> > 
> >> +int arch_init_irq_desc(struct irq_desc *desc, int node)
> >> +{
> >> +	return current_machine->init_chip_data(desc, node);
> >> +}
> > 
> looks like xen in same run time, some irqs need x86_init_chip_data,
> and some may need xen_init_chip_data later.
> 
I'm afraid I am unable to grasp the meaning of this sentence, or what
precisely this has to do with not being able to utilize platform ops to
get this sorted out on x86.

You're effectively trying to have the hardirq code pass around a function
pointer for you that ultimately only serves to bail out on certain code
paths if you're running under xen, which is a concern for how the
platform chooses to initialize the irq desc, none of this has any value
or relevance to the hardirq code outside of that. The fact that the
hardirq code doesn't do anything with this information other than pass it
around for your platform should already be a clear indicator that this is
the wrong way to go.

>From a cursory look at the x86 code, this looks like precisely the sort
of thing that arch/x86/include/asm/x86_init.h is well suited for, and
indeed you already have a x86_init_irqs to expand on as needed.

The function pointer thing itself is also a bit unorthodox to say the
least. You're introducing and passing around an opaque type just so you
can get to a 'return 0' in the xen case as far as I can tell, so you
could also just make arch_init_chip_data() a weak symbol and clobber it
in the xen case, no?

Re: [PATCH 01/10] irq: move some interrupt arch_* functions into struct irq_chip.

[Ian Campbell]

On Tue, 2010-03-23 at 07:10 +0000, Paul Mundt wrote:

> The function pointer thing itself is also a bit unorthodox to say the
> least. You're introducing and passing around an opaque type just so you
> can get to a 'return 0' in the xen case as far as I can tell,

The ultimate aim is to have Xen use the chip data to store the event
channel information relating to each IRQ instead of keeping it in a
static NR_IRQs array, the new function only returns 0 as a placeholder
until this can be put in place (which depends on these changes), it
could as well have been left out for the time being (e.g. passing NULL
function pointer in the Xen case or whatever).

> so you
> could also just make arch_init_chip_data() a weak symbol and clobber it
> in the xen case, no?

A single kernel is able to boot native or Xen so a weak symbol doesn't
really work, having the function pointer at the arch level is one
similar option, I've replied to Thomas' similar suggestion.

Ian.

Re: [PATCH 01/10] irq: move some interrupt arch_* functions into struct irq_chip.

[Thomas Gleixner]

On Sun, 21 Mar 2010, Yinghai Lu wrote:

> From: Ian Campbell <ian.campbell@citrix.com>
> 
> Move arch_init_copy_chip_data and arch_free_chip_data into function
> pointers in struct irq_chip since they operate on irq_desc->chip_data.

Not sure about that. These functions are solely used by x86 and there
is really no need to generalize them. The problem you try to solve is
x86/xen specific and can be solved by x86_platform_ops as well w/o
adding extra function pointers to irq_chip.

> arch_init_chip_data cannot be moved into struct irq_chip because
> irq_desc->chip is not known at the time the irq_desc is setup. Instead
> rename arch_init_chip_data to arch_init_irq_desc for PowerPC, the
> only other user, whose usage better matches the new name.
> 
> To replace the x86 arch_init_chip_data functionality
> irq_to_desc_alloc_node now takes a pointer to a function to allocate
> the chip data. This is necessary to ensure the allocation happens
> under the correct locking at the core level. On PowerPC and SH

Err, that argument is totally bogus. The calling convention of
irq_to_desc_alloc_node and arch_init_chip_data/arch_init_irq_desc is
still the same. It does not explain why the heck we need that function
pointer at all.

AFAICT the function pointer to irq_to_desc_alloc_node is completely
pointless. It just solves a Xen/x86 specific problem which can be
solved by using x86_platform_ops and keeps the churn x86 internal.

> architectures (the other users of irq_to_desc_alloc_node) pass in NULL
> which retains existing chip_data behaviour.
 
> I've retained the chip_data behaviour for uv_irq although it isn't
> clear to me if these interrupt types support migration or how closely
> related to the APIC modes they really are. If it weren't for this the
> x86_{init,copy,free}_chip_data functions could be static to
> io_apic.c.
> 
> I've tested by booting on an 64 bit x86 system with sparse IRQ enabled
> and 32 bit without, but it's not clear to me what actions I need to
> take to actually exercise some of these code paths.
> 
> -v4: yinghai add irq_to_desc_alloc_node_x...

Aside of the general objection against this, please use descriptive
function names and do not distinguish functions by adding random
characters which tell us absolutely nothing about the purpose.

Thanks,

	tglx

Re: [PATCH 01/10] irq: move some interrupt arch_* functions into struct irq_chip.

[Ian Campbell]

On Mon, 2010-03-22 at 10:19 +0000, Thomas Gleixner wrote:
> On Sun, 21 Mar 2010, Yinghai Lu wrote:
> 
> > From: Ian Campbell <ian.campbell@citrix.com>
> > 
> > Move arch_init_copy_chip_data and arch_free_chip_data into function
> > pointers in struct irq_chip since they operate on irq_desc->chip_data.
> 
> Not sure about that. These functions are solely used by x86 and there
> is really no need to generalize them.

I thought the idea of struct irq_chip was to allow the potential for
multiple IRQ controllers in a system? Given that it seems that struct
irq_desc->chip_data ought to be available for use by whichever struct
irq_chip is managing a given interrupt. At the moment this is not
possible because we step around the abstraction using these arch_*
methods.

Although this might be unusual on x86 I think it is not uncommon in the
embedded world to have an architectural interrupt controller cascading
through to various different IRQ controllers/multiplexors, from random
FPGA based things, to GPIO controllers and things like superio chips
etc.

Currently the set of architectures which typically have this sort of
thing are disjoint from the ones which make use of struct
irq_desc->chip_data but with the growing use of embedded-x86 is this not
something worth considering? (Genuine question, I've been out of the
embedded space for a while now so maybe my experiences are out of date
or I'm overestimating the role of embedded-x86 etc).

Xen is a bit more of a specialised case than the above since it would
like to replace the architectural interrupt handling but I think the
broad requirements on the irq_chip interface are the same. Going forward
it is possible/likely that we would like to be able to make Xen event
channels available via a cascade model as well -- demultiplexing one (or
more?) x86 architectural interrupts into event channels would be part of
running PV Xen drivers on a fully-virtualised (i.e. native) kernel.

>  The problem you try to solve is
> x86/xen specific and can be solved by x86_platform_ops as well w/o
> adding extra function pointers to irq_chip.
[...]
> AFAICT the function pointer to irq_to_desc_alloc_node is completely
> pointless. It just solves a Xen/x86 specific problem which can be
> solved by using x86_platform_ops and keeps the churn x86 internal.

I have no problem with that if that is the x86/irq maintainer's
preference, I just thought it would be nicer to solve what I saw as an
oddity in the existing abstraction generically in the core.

> Aside of the general objection against this, please use descriptive
> function names and do not distinguish functions by adding random
> characters which tell us absolutely nothing about the purpose.

I agree on this one. More generally I would say that the number of
existing users of this interface is small enough that _if_ we decide we
need to modify it then we should just bite the bullet and do that
instead of building compatibility layers around stuff. For this reason I
think my original patch was preferable to this version (general
objections not withstanding).

Ian.

Re: [PATCH 01/10] irq: move some interrupt arch_* functions into struct irq_chip.

[Thomas Gleixner]

On Wed, 24 Mar 2010, Ian Campbell wrote:

> On Mon, 2010-03-22 at 10:19 +0000, Thomas Gleixner wrote:
> > On Sun, 21 Mar 2010, Yinghai Lu wrote:
> > 
> > > From: Ian Campbell <ian.campbell@citrix.com>
> > > 
> > > Move arch_init_copy_chip_data and arch_free_chip_data into function
> > > pointers in struct irq_chip since they operate on irq_desc->chip_data.
> > 
> > Not sure about that. These functions are solely used by x86 and there
> > is really no need to generalize them.
> 
> I thought the idea of struct irq_chip was to allow the potential for
> multiple IRQ controllers in a system? Given that it seems that struct
> irq_desc->chip_data ought to be available for use by whichever struct
> irq_chip is managing a given interrupt. At the moment this is not
> possible because we step around the abstraction using these arch_*
> methods.

Right, but you have exactly _ONE_ irq_chip associated to an irq_desc,
but that same irq_chip can be associated to several irq_descs. So
irq_desc->data is there to provide data for the irq_chip functions
depending on what irq they handle (e.g. base_address ...). 

irq_desc->chip_data is set when the irq_chip is assigned to the
irq_desc.

So there is no point in having functions in irq_chip to set
irq_desc->chip_data.

Thanks,

	tglx

Re: [PATCH 01/10] irq: move some interrupt arch_* functions into struct irq_chip.

[Ian Campbell]

On Wed, 2010-03-24 at 17:44 +0000, Thomas Gleixner wrote: 
> On Wed, 24 Mar 2010, Ian Campbell wrote:
> 
> > On Mon, 2010-03-22 at 10:19 +0000, Thomas Gleixner wrote:
> > > On Sun, 21 Mar 2010, Yinghai Lu wrote:
> > > 
> > > > From: Ian Campbell <ian.campbell@citrix.com>
> > > > 
> > > > Move arch_init_copy_chip_data and arch_free_chip_data into function
> > > > pointers in struct irq_chip since they operate on irq_desc->chip_data.
> > > 
> > > Not sure about that. These functions are solely used by x86 and there
> > > is really no need to generalize them.
> > 
> > I thought the idea of struct irq_chip was to allow the potential for
> > multiple IRQ controllers in a system? Given that it seems that struct
> > irq_desc->chip_data ought to be available for use by whichever struct
> > irq_chip is managing a given interrupt. At the moment this is not
> > possible because we step around the abstraction using these arch_*
> > methods.
> 
> Right, but you have exactly _ONE_ irq_chip associated to an irq_desc,
> but that same irq_chip can be associated to several irq_descs. So
> irq_desc->data is there to provide data for the irq_chip functions
> depending on what irq they handle (e.g. base_address ...). 
> 
> irq_desc->chip_data is set when the irq_chip is assigned to the
> irq_desc.
> 
> So there is no point in having functions in irq_chip to set
> irq_desc->chip_data.

So how do you know which is the appropriate irq_chip specific function
to call given an irq_desc that you want to copy/free/migrate? The
contents of the chip_data pointer will take different forms for
different irq_chips. The way the generic code is currently structured it
appears you can't (or at least don't) just do a shallow copy by copying
the irq_desc->chip_data pointer itself -- you need to do a deep copy
using a function which understands that type of chip_data.

How is this operation different to having pointers in irq_chip for
enabling/disabling/masking interrupts for each irq_chip?

Ian.

Re: [PATCH 01/10] irq: move some interrupt arch_* functions into struct irq_chip.

[Thomas Gleixner]

On Wed, 24 Mar 2010, Ian Campbell wrote:
> On Wed, 2010-03-24 at 17:44 +0000, Thomas Gleixner wrote: 
> > On Wed, 24 Mar 2010, Ian Campbell wrote:
> > 
> > > On Mon, 2010-03-22 at 10:19 +0000, Thomas Gleixner wrote:
> > > > On Sun, 21 Mar 2010, Yinghai Lu wrote:
> > > > 
> > > > > From: Ian Campbell <ian.campbell@citrix.com>
> > > > > 
> > > > > Move arch_init_copy_chip_data and arch_free_chip_data into function
> > > > > pointers in struct irq_chip since they operate on irq_desc->chip_data.
> > > > 
> > > > Not sure about that. These functions are solely used by x86 and there
> > > > is really no need to generalize them.
> > > 
> > > I thought the idea of struct irq_chip was to allow the potential for
> > > multiple IRQ controllers in a system? Given that it seems that struct
> > > irq_desc->chip_data ought to be available for use by whichever struct
> > > irq_chip is managing a given interrupt. At the moment this is not
> > > possible because we step around the abstraction using these arch_*
> > > methods.
> > 
> > Right, but you have exactly _ONE_ irq_chip associated to an irq_desc,
> > but that same irq_chip can be associated to several irq_descs. So
> > irq_desc->data is there to provide data for the irq_chip functions
> > depending on what irq they handle (e.g. base_address ...). 
> > 
> > irq_desc->chip_data is set when the irq_chip is assigned to the
> > irq_desc.
> > 
> > So there is no point in having functions in irq_chip to set
> > irq_desc->chip_data.
> 
> So how do you know which is the appropriate irq_chip specific function
> to call given an irq_desc that you want to copy/free/migrate? The
> contents of the chip_data pointer will take different forms for
> different irq_chips. The way the generic code is currently structured it
> appears you can't (or at least don't) just do a shallow copy by copying
> the irq_desc->chip_data pointer itself -- you need to do a deep copy
> using a function which understands that type of chip_data.

The design of sparse_irq or to be honest the lack of design grew that
crap and it's not only this detail which is a nightmare. That pointer
should probably be simply copied. Either that or if the chip data
require to be node bound we need something along the line:

struct sparse_irq_chip_data {
       void *data;
       void (*copy)(...);
       void (*free)(...);
};

and a corresponding field in irq_desc.

I'm looking into sparse_irq right now anyway because it has other way
more serious short comings.

> How is this operation different to having pointers in irq_chip for
> enabling/disabling/masking interrupts for each irq_chip?

Because that's the purpose of the irq_chip perhaps ?

Thanks,

	tglx