[GITPULL+PATCH 0/2] irq: move some interrupt arch_* functions into struct irq_chip.

[GITPULL+PATCH 0/2] irq: move some interrupt arch_* functions into struct irq_chip.

[Ian Campbell]

This small series ensures that struct irq_desc->chip_data is available
for alternative irq_chip implementations.

There is an outstanding issue wrt when/how the chip_data field is
initialised. I am continuing to investigate this but the solution is not
turning out as easy/low-impact as expected.

Since last time I've dropped the renaming portion of the series since it
was basically wrong, the functions I'd implicated as ioapic specific are
not at all.

Ian.

The following changes since commit 1ebbdcc83e75697c0d75eb091df172b7d93c84c1:
  Ingo Molnar (1):
        Merge branch 'perf/urgent'

are available in the git repository at:

  git://xenbits.xensource.com/people/ianc/linux-2.6.git for-x86/irq

Ian Campbell (2):
      irq: move some interrupt arch_* functions into struct irq_chip.
      x86: irq_desc->chip_data is always correct whether or not SPARSE_IRQ is enabled.

 arch/powerpc/kernel/irq.c      |    2 +-
 arch/x86/include/asm/hw_irq.h  |   11 ++++++-
 arch/x86/kernel/apic/io_apic.c |   61 ++++++++++++++++++++++++++++++++++-----
 arch/x86/kernel/uv_irq.c       |    5 +++
 include/linux/interrupt.h      |    2 +-
 include/linux/irq.h            |   12 +++++--
 kernel/irq/handle.c            |    2 +-
 kernel/irq/numa_migrate.c      |   12 ++++++-
 kernel/softirq.c               |    3 +-
 9 files changed, 90 insertions(+), 20 deletions(-)

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

[Ian Campbell]

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 at this time
because irq_desc->chip is not known at the time the irq_desc is
setup. For now rename arch_init_chip_data to arch_init_irq_desc (for
PowerPC, the only other user, whose usage better matches the new name)
and on x86 convert arch_init_chip_data to x86_init_chip_data and
call this whenever a new struct irq_desc is allocated.

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 a 64 bit system, but it's not clear to me
what actions I need to take to actually exercise some of these code
paths.

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
---
 arch/powerpc/kernel/irq.c      |    2 +-
 arch/x86/include/asm/hw_irq.h  |   11 +++++++-
 arch/x86/kernel/apic/io_apic.c |   58 +++++++++++++++++++++++++++++++++++++---
 arch/x86/kernel/uv_irq.c       |    5 +++
 include/linux/interrupt.h      |    2 +-
 include/linux/irq.h            |   12 +++++---
 kernel/irq/handle.c            |    2 +-
 kernel/irq/numa_migrate.c      |   12 +++++++-
 kernel/softirq.c               |    3 +-
 9 files changed, 91 insertions(+), 16 deletions(-)

diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
index 64f6f20..002d87f 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)
 {
 	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 a929c9e..1bc7063 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,15 @@ extern void init_VISWS_APIC_irqs(void);
 extern void setup_IO_APIC(void);
 extern void disable_IO_APIC(void);
 
+extern int x86_init_chip_data(struct irq_desc *desc, int node);
+
+#ifdef CONFIG_SPARSE_IRQ
+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);
+#endif
+
 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 e4e0ddc..12e5cf4 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)
+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;
 
@@ -336,6 +336,11 @@ struct irq_cfg *irq_cfg(unsigned int irq)
 	return irq < nr_irqs ? irq_cfgx + irq : NULL;
 }
 
+int x86_init_chip_data(struct irq_desc *desc, int node)
+{
+	return 0;
+}
+
 #endif
 
 struct io_apic {
@@ -1520,6 +1525,7 @@ static void __init setup_IO_APIC_irqs(void)
 			printk(KERN_INFO "can not get irq_desc for %d\n", irq);
 			continue;
 		}
+		x86_init_chip_data(desc, node);
 		cfg = desc->chip_data;
 		add_pin_to_irq_node(cfg, node, apic_id, pin);
 		/*
@@ -1570,6 +1576,7 @@ void setup_IO_APIC_irq_extra(u32 gsi)
 		printk(KERN_INFO "can not get irq_desc for %d\n", irq);
 		return;
 	}
+	x86_init_chip_data(desc, node);
 
 	cfg = desc->chip_data;
 	add_pin_to_irq_node(cfg, node, apic_id, pin);
@@ -2739,6 +2746,11 @@ static struct irq_chip ioapic_chip __read_mostly = {
 	.set_affinity	= set_ioapic_affinity_irq,
 #endif
 	.retrigger	= ioapic_retrigger_irq,
+
+#ifdef CONFIG_SPARSE_IRQ
+	.copy_chip_data = x86_copy_chip_data,
+	.free_chip_data = x86_free_chip_data,
+#endif
 };
 
 static struct irq_chip ir_ioapic_chip __read_mostly = {
@@ -2754,6 +2766,11 @@ static struct irq_chip ir_ioapic_chip __read_mostly = {
 #endif
 #endif
 	.retrigger	= ioapic_retrigger_irq,
+
+#ifdef CONFIG_SPARSE_IRQ
+	.copy_chip_data = x86_copy_chip_data,
+	.free_chip_data = x86_free_chip_data,
+#endif
 };
 
 static inline void init_IO_APIC_traps(void)
@@ -3261,6 +3278,7 @@ unsigned int create_irq_nr(unsigned int irq_want, int node)
 			printk(KERN_INFO "can not get irq_desc for %d\n", new);
 			continue;
 		}
+		x86_init_chip_data(desc_new, node);
 		cfg_new = desc_new->chip_data;
 
 		if (cfg_new->vector != 0)
@@ -3466,6 +3484,11 @@ static struct irq_chip msi_chip = {
 	.set_affinity	= set_msi_irq_affinity,
 #endif
 	.retrigger	= ioapic_retrigger_irq,
+
+#ifdef CONFIG_SPARSE_IRQ
+	.copy_chip_data = x86_copy_chip_data,
+	.free_chip_data = x86_free_chip_data,
+#endif
 };
 
 static struct irq_chip msi_ir_chip = {
@@ -3479,6 +3502,11 @@ static struct irq_chip msi_ir_chip = {
 #endif
 #endif
 	.retrigger	= ioapic_retrigger_irq,
+
+#ifdef CONFIG_SPARSE_IRQ
+	.copy_chip_data = x86_copy_chip_data,
+	.free_chip_data = x86_free_chip_data,
+#endif
 };
 
 /*
@@ -3638,6 +3666,11 @@ static struct irq_chip dmar_msi_type = {
 	.set_affinity = dmar_msi_set_affinity,
 #endif
 	.retrigger = ioapic_retrigger_irq,
+
+#ifdef CONFIG_SPARSE_IRQ
+	.copy_chip_data = x86_copy_chip_data,
+	.free_chip_data = x86_free_chip_data,
+#endif
 };
 
 int arch_setup_dmar_msi(unsigned int irq)
@@ -3695,6 +3728,11 @@ static struct irq_chip ir_hpet_msi_type = {
 #endif
 #endif
 	.retrigger = ioapic_retrigger_irq,
+
+#ifdef CONFIG_SPARSE_IRQ
+	.copy_chip_data = x86_copy_chip_data,
+	.free_chip_data = x86_free_chip_data,
+#endif
 };
 
 static struct irq_chip hpet_msi_type = {
@@ -3706,6 +3744,11 @@ static struct irq_chip hpet_msi_type = {
 	.set_affinity = hpet_msi_set_affinity,
 #endif
 	.retrigger = ioapic_retrigger_irq,
+
+#ifdef CONFIG_SPARSE_IRQ
+	.copy_chip_data = x86_copy_chip_data,
+	.free_chip_data = x86_free_chip_data,
+#endif
 };
 
 int arch_setup_hpet_msi(unsigned int irq, unsigned int id)
@@ -3792,6 +3835,11 @@ static struct irq_chip ht_irq_chip = {
 	.set_affinity	= set_ht_irq_affinity,
 #endif
 	.retrigger	= ioapic_retrigger_irq,
+
+#ifdef CONFIG_SPARSE_IRQ
+	.copy_chip_data = x86_copy_chip_data,
+	.free_chip_data = x86_free_chip_data,
+#endif
 };
 
 int arch_setup_ht_irq(unsigned int irq, struct pci_dev *dev)
@@ -3919,6 +3967,7 @@ static int __io_apic_set_pci_routing(struct device *dev, int irq,
 		printk(KERN_INFO "can not get irq_desc %d\n", irq);
 		return 0;
 	}
+	x86_init_chip_data(desc, node);
 
 	pin = irq_attr->ioapic_pin;
 	trigger = irq_attr->trigger;
@@ -4313,6 +4362,7 @@ void __init pre_init_apic_IRQ0(void)
 	phys_cpu_present_map = physid_mask_of_physid(boot_cpu_physical_apicid);
 #endif
 	desc = irq_to_desc_alloc_node(0, 0);
+	x86_init_chip_data(desc, 0);
 
 	setup_local_APIC();
 
diff --git a/arch/x86/kernel/uv_irq.c b/arch/x86/kernel/uv_irq.c
index ece73d8..df2c6d6 100644
--- a/arch/x86/kernel/uv_irq.c
+++ b/arch/x86/kernel/uv_irq.c
@@ -55,6 +55,11 @@ struct irq_chip uv_irq_chip = {
 	.eoi		= uv_ack_apic,
 	.end		= uv_noop,
 	.set_affinity	= uv_set_irq_affinity,
+
+#ifdef CONFIG_SPARSE_IRQ
+	.copy_chip_data = x86_copy_chip_data,
+	.free_chip_data = x86_free_chip_data,
+#endif
 };
 
 /*
diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 75f3f00..cc4cd22 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -611,6 +611,6 @@ 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);
+extern int arch_init_irq_desc(struct irq_desc *desc, int node);
 
 #endif
diff --git a/include/linux/irq.h b/include/linux/irq.h
index 707ab12..558bd2d 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -131,6 +131,14 @@ struct irq_chip {
 	void		(*bus_lock)(unsigned int irq);
 	void		(*bus_sync_unlock)(unsigned int irq);
 
+	/* for move_irq_desc */
+#ifdef CONFIG_SPARSE_IRQ
+	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);
+#endif
+
 	/* Currently used only by UML, might disappear one day.*/
 #ifdef CONFIG_IRQ_RELEASE_METHOD
 	void		(*release)(unsigned int irq, void *dev_id);
@@ -208,10 +216,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
diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
index 76d5a67..168e2f8 100644
--- a/kernel/irq/handle.c
+++ b/kernel/irq/handle.c
@@ -120,7 +120,6 @@ 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);
 }
 
 /*
@@ -228,6 +227,7 @@ struct irq_desc * __ref irq_to_desc_alloc_node(unsigned int irq, int node)
 		BUG_ON(1);
 	}
 	init_one_irq_desc(irq, desc, node);
+	arch_init_irq_desc(desc, node);
 
 	set_irq_desc(irq, desc);
 
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..3f4b80e 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -895,8 +895,7 @@ int __init __weak arch_early_irq_init(void)
 {
 	return 0;
 }
-
-int __weak arch_init_chip_data(struct irq_desc *desc, int node)
+int __weak arch_init_irq_desc(struct irq_desc *desc, int node)
 {
 	return 0;
 }
-- 
1.5.6.5

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

[Yinghai Lu]

On 03/12/2010 01:45 AM, Ian Campbell wrote:
> 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 at this time
> because irq_desc->chip is not known at the time the irq_desc is
> setup. For now rename arch_init_chip_data to arch_init_irq_desc (for
> PowerPC, the only other user, whose usage better matches the new name)
> and on x86 convert arch_init_chip_data to x86_init_chip_data and
> call this whenever a new struct irq_desc is allocated.
> 
> 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 a 64 bit system, but it's not clear to me
> what actions I need to take to actually exercise some of these code
> paths.
> 
> 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
> ---
>  arch/powerpc/kernel/irq.c      |    2 +-
>  arch/x86/include/asm/hw_irq.h  |   11 +++++++-
>  arch/x86/kernel/apic/io_apic.c |   58 +++++++++++++++++++++++++++++++++++++---
>  arch/x86/kernel/uv_irq.c       |    5 +++
>  include/linux/interrupt.h      |    2 +-
>  include/linux/irq.h            |   12 +++++---
>  kernel/irq/handle.c            |    2 +-
>  kernel/irq/numa_migrate.c      |   12 +++++++-
>  kernel/softirq.c               |    3 +-
>  9 files changed, 91 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
> index 64f6f20..002d87f 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)
>  {
>  	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 a929c9e..1bc7063 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,15 @@ extern void init_VISWS_APIC_irqs(void);
>  extern void setup_IO_APIC(void);
>  extern void disable_IO_APIC(void);
>  
> +extern int x86_init_chip_data(struct irq_desc *desc, int node);
> +
> +#ifdef CONFIG_SPARSE_IRQ
> +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);
> +#endif
> +
>  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 e4e0ddc..12e5cf4 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)
> +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;
>  
> @@ -336,6 +336,11 @@ struct irq_cfg *irq_cfg(unsigned int irq)
>  	return irq < nr_irqs ? irq_cfgx + irq : NULL;
>  }
>  
> +int x86_init_chip_data(struct irq_desc *desc, int node)
> +{
> +	return 0;
> +}
> +

you could put one dummy x86_copy_chip_data and x86_free_chip_data here too.

>  #endif
>  
>  struct io_apic {
> @@ -1520,6 +1525,7 @@ static void __init setup_IO_APIC_irqs(void)
>  			printk(KERN_INFO "can not get irq_desc for %d\n", irq);
>  			continue;
>  		}
> +		x86_init_chip_data(desc, node);
>  		cfg = desc->chip_data;
>  		add_pin_to_irq_node(cfg, node, apic_id, pin);
>  		/*
> @@ -1570,6 +1576,7 @@ void setup_IO_APIC_irq_extra(u32 gsi)
>  		printk(KERN_INFO "can not get irq_desc for %d\n", irq);
>  		return;
>  	}
> +	x86_init_chip_data(desc, node);
>  
>  	cfg = desc->chip_data;
>  	add_pin_to_irq_node(cfg, node, apic_id, pin);
> @@ -2739,6 +2746,11 @@ static struct irq_chip ioapic_chip __read_mostly = {
>  	.set_affinity	= set_ioapic_affinity_irq,
>  #endif
>  	.retrigger	= ioapic_retrigger_irq,
> +
> +#ifdef CONFIG_SPARSE_IRQ
> +	.copy_chip_data = x86_copy_chip_data,
> +	.free_chip_data = x86_free_chip_data,
> +#endif
>  };
>  
>  static struct irq_chip ir_ioapic_chip __read_mostly = {
> @@ -2754,6 +2766,11 @@ static struct irq_chip ir_ioapic_chip __read_mostly = {
>  #endif
>  #endif
>  	.retrigger	= ioapic_retrigger_irq,
> +
> +#ifdef CONFIG_SPARSE_IRQ
> +	.copy_chip_data = x86_copy_chip_data,
> +	.free_chip_data = x86_free_chip_data,
> +#endif
>  };
>  
>  static inline void init_IO_APIC_traps(void)
> @@ -3261,6 +3278,7 @@ unsigned int create_irq_nr(unsigned int irq_want, int node)
>  			printk(KERN_INFO "can not get irq_desc for %d\n", new);
>  			continue;
>  		}
> +		x86_init_chip_data(desc_new, node);
>  		cfg_new = desc_new->chip_data;
>  
>  		if (cfg_new->vector != 0)
> @@ -3466,6 +3484,11 @@ static struct irq_chip msi_chip = {
>  	.set_affinity	= set_msi_irq_affinity,
>  #endif
>  	.retrigger	= ioapic_retrigger_irq,
> +
> +#ifdef CONFIG_SPARSE_IRQ
> +	.copy_chip_data = x86_copy_chip_data,
> +	.free_chip_data = x86_free_chip_data,
> +#endif
>  };
>  
>  static struct irq_chip msi_ir_chip = {
> @@ -3479,6 +3502,11 @@ static struct irq_chip msi_ir_chip = {
>  #endif
>  #endif
>  	.retrigger	= ioapic_retrigger_irq,
> +
> +#ifdef CONFIG_SPARSE_IRQ
> +	.copy_chip_data = x86_copy_chip_data,
> +	.free_chip_data = x86_free_chip_data,
> +#endif
>  };
>  
>  /*
> @@ -3638,6 +3666,11 @@ static struct irq_chip dmar_msi_type = {
>  	.set_affinity = dmar_msi_set_affinity,
>  #endif
>  	.retrigger = ioapic_retrigger_irq,
> +
> +#ifdef CONFIG_SPARSE_IRQ
> +	.copy_chip_data = x86_copy_chip_data,
> +	.free_chip_data = x86_free_chip_data,
> +#endif
>  };
>  
>  int arch_setup_dmar_msi(unsigned int irq)
> @@ -3695,6 +3728,11 @@ static struct irq_chip ir_hpet_msi_type = {
>  #endif
>  #endif
>  	.retrigger = ioapic_retrigger_irq,
> +
> +#ifdef CONFIG_SPARSE_IRQ
> +	.copy_chip_data = x86_copy_chip_data,
> +	.free_chip_data = x86_free_chip_data,
> +#endif
>  };
>  
>  static struct irq_chip hpet_msi_type = {
> @@ -3706,6 +3744,11 @@ static struct irq_chip hpet_msi_type = {
>  	.set_affinity = hpet_msi_set_affinity,
>  #endif
>  	.retrigger = ioapic_retrigger_irq,
> +
> +#ifdef CONFIG_SPARSE_IRQ
> +	.copy_chip_data = x86_copy_chip_data,
> +	.free_chip_data = x86_free_chip_data,
> +#endif
>  };
>  
>  int arch_setup_hpet_msi(unsigned int irq, unsigned int id)
> @@ -3792,6 +3835,11 @@ static struct irq_chip ht_irq_chip = {
>  	.set_affinity	= set_ht_irq_affinity,
>  #endif
>  	.retrigger	= ioapic_retrigger_irq,
> +
> +#ifdef CONFIG_SPARSE_IRQ
> +	.copy_chip_data = x86_copy_chip_data,
> +	.free_chip_data = x86_free_chip_data,
> +#endif
>  };
>  
>  int arch_setup_ht_irq(unsigned int irq, struct pci_dev *dev)
> @@ -3919,6 +3967,7 @@ static int __io_apic_set_pci_routing(struct device *dev, int irq,
>  		printk(KERN_INFO "can not get irq_desc %d\n", irq);
>  		return 0;
>  	}
> +	x86_init_chip_data(desc, node);
>  
>  	pin = irq_attr->ioapic_pin;
>  	trigger = irq_attr->trigger;
> @@ -4313,6 +4362,7 @@ void __init pre_init_apic_IRQ0(void)
>  	phys_cpu_present_map = physid_mask_of_physid(boot_cpu_physical_apicid);
>  #endif
>  	desc = irq_to_desc_alloc_node(0, 0);
> +	x86_init_chip_data(desc, 0);
>  
>  	setup_local_APIC();
>  
> diff --git a/arch/x86/kernel/uv_irq.c b/arch/x86/kernel/uv_irq.c
> index ece73d8..df2c6d6 100644
> --- a/arch/x86/kernel/uv_irq.c
> +++ b/arch/x86/kernel/uv_irq.c
> @@ -55,6 +55,11 @@ struct irq_chip uv_irq_chip = {
>  	.eoi		= uv_ack_apic,
>  	.end		= uv_noop,
>  	.set_affinity	= uv_set_irq_affinity,
> +
> +#ifdef CONFIG_SPARSE_IRQ
> +	.copy_chip_data = x86_copy_chip_data,
> +	.free_chip_data = x86_free_chip_data,
> +#endif
>  };
>  
>  /*
> diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
> index 75f3f00..cc4cd22 100644
> --- a/include/linux/interrupt.h
> +++ b/include/linux/interrupt.h
> @@ -611,6 +611,6 @@ 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);
> +extern int arch_init_irq_desc(struct irq_desc *desc, int node);
>  
>  #endif
> diff --git a/include/linux/irq.h b/include/linux/irq.h
> index 707ab12..558bd2d 100644
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -131,6 +131,14 @@ struct irq_chip {
>  	void		(*bus_lock)(unsigned int irq);
>  	void		(*bus_sync_unlock)(unsigned int irq);
>  
> +	/* for move_irq_desc */
> +#ifdef CONFIG_SPARSE_IRQ
> +	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);
> +#endif
> +
>  	/* Currently used only by UML, might disappear one day.*/
>  #ifdef CONFIG_IRQ_RELEASE_METHOD
>  	void		(*release)(unsigned int irq, void *dev_id);
> @@ -208,10 +216,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
> diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
> index 76d5a67..168e2f8 100644
> --- a/kernel/irq/handle.c
> +++ b/kernel/irq/handle.c
> @@ -120,7 +120,6 @@ 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);
>  }
>  
>  /*
> @@ -228,6 +227,7 @@ struct irq_desc * __ref irq_to_desc_alloc_node(unsigned int irq, int node)
>  		BUG_ON(1);
>  	}
>  	init_one_irq_desc(irq, desc, node);
> +	arch_init_irq_desc(desc, node);

you move out init_chip_data for x86 out of irq_to_desc_alloc_node
current init_chip_data in irq_to_desc_alloc_node is protected by sparse_irq_lock.


wonder if you could let arch_init_irq_desc for x86 to call x86_init_chip_data?

or let add another irq_to_desc_alloc_node_f to take extra func call.

like 
typedef int (*init_chip_data_f)(struct irq_desc *desc, int node);
struct irq_desc * __ref irq_to_desc_alloc_node_f(unsigned int irq, int node, init_chip_data_f *f);

struct irq_desc * __ref irq_to_desc_alloc_node_f(unsigned int irq, int node)
{
 irq_to_desc_alloc_node(irq, node, NULL);
}

then for x86

int arch_init_irq_desc(struct irq_desc *desc, int node, init_chip_data_f *f)
{
	if (!f)
		return x86_init_chip_data(desc, node);
 	f(desc, node);
}

after that xen could use
irq_to_desc_alloc_node_f(irq, node, xen_init_chip_data);

as need...

at last we don't need to call x86_init_chip_data everywhere.

Thanks

Yinghai

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

[Eric W. Biederman]

Yinghai Lu <yinghai@kernel.org> writes:

> On 03/12/2010 01:45 AM, Ian Campbell wrote:
>> 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 at this time
>> because irq_desc->chip is not known at the time the irq_desc is
>> setup. For now rename arch_init_chip_data to arch_init_irq_desc (for
>> PowerPC, the only other user, whose usage better matches the new name)
>> and on x86 convert arch_init_chip_data to x86_init_chip_data and
>> call this whenever a new struct irq_desc is allocated.
>> 
>> 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 a 64 bit system, but it's not clear to me
>> what actions I need to take to actually exercise some of these code
>> paths.
>> 
>> 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
>> ---
>>  arch/powerpc/kernel/irq.c      |    2 +-
>>  arch/x86/include/asm/hw_irq.h  |   11 +++++++-
>>  arch/x86/kernel/apic/io_apic.c |   58 +++++++++++++++++++++++++++++++++++++---
>>  arch/x86/kernel/uv_irq.c       |    5 +++
>>  include/linux/interrupt.h      |    2 +-
>>  include/linux/irq.h            |   12 +++++---
>>  kernel/irq/handle.c            |    2 +-
>>  kernel/irq/numa_migrate.c      |   12 +++++++-
>>  kernel/softirq.c               |    3 +-
>>  9 files changed, 91 insertions(+), 16 deletions(-)
>> 
>> diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
>> index 64f6f20..002d87f 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)
>>  {
>>  	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 a929c9e..1bc7063 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,15 @@ extern void init_VISWS_APIC_irqs(void);
>>  extern void setup_IO_APIC(void);
>>  extern void disable_IO_APIC(void);
>>  
>> +extern int x86_init_chip_data(struct irq_desc *desc, int node);
>> +
>> +#ifdef CONFIG_SPARSE_IRQ
>> +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);
>> +#endif
>> +
>>  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 e4e0ddc..12e5cf4 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)
>> +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;
>>  
>> @@ -336,6 +336,11 @@ struct irq_cfg *irq_cfg(unsigned int irq)
>>  	return irq < nr_irqs ? irq_cfgx + irq : NULL;
>>  }
>>  
>> +int x86_init_chip_data(struct irq_desc *desc, int node)
>> +{
>> +	return 0;
>> +}
>> +
>
> you could put one dummy x86_copy_chip_data and x86_free_chip_data here too.
>
>>  #endif
>>  
>>  struct io_apic {
>> @@ -1520,6 +1525,7 @@ static void __init setup_IO_APIC_irqs(void)
>>  			printk(KERN_INFO "can not get irq_desc for %d\n", irq);
>>  			continue;
>>  		}
>> +		x86_init_chip_data(desc, node);
>>  		cfg = desc->chip_data;
>>  		add_pin_to_irq_node(cfg, node, apic_id, pin);
>>  		/*
>> @@ -1570,6 +1576,7 @@ void setup_IO_APIC_irq_extra(u32 gsi)
>>  		printk(KERN_INFO "can not get irq_desc for %d\n", irq);
>>  		return;
>>  	}
>> +	x86_init_chip_data(desc, node);
>>  
>>  	cfg = desc->chip_data;
>>  	add_pin_to_irq_node(cfg, node, apic_id, pin);
>> @@ -2739,6 +2746,11 @@ static struct irq_chip ioapic_chip __read_mostly = {
>>  	.set_affinity	= set_ioapic_affinity_irq,
>>  #endif
>>  	.retrigger	= ioapic_retrigger_irq,
>> +
>> +#ifdef CONFIG_SPARSE_IRQ
>> +	.copy_chip_data = x86_copy_chip_data,
>> +	.free_chip_data = x86_free_chip_data,
>> +#endif
>>  };
>>  
>>  static struct irq_chip ir_ioapic_chip __read_mostly = {
>> @@ -2754,6 +2766,11 @@ static struct irq_chip ir_ioapic_chip __read_mostly = {
>>  #endif
>>  #endif
>>  	.retrigger	= ioapic_retrigger_irq,
>> +
>> +#ifdef CONFIG_SPARSE_IRQ
>> +	.copy_chip_data = x86_copy_chip_data,
>> +	.free_chip_data = x86_free_chip_data,
>> +#endif
>>  };
>>  
>>  static inline void init_IO_APIC_traps(void)
>> @@ -3261,6 +3278,7 @@ unsigned int create_irq_nr(unsigned int irq_want, int node)
>>  			printk(KERN_INFO "can not get irq_desc for %d\n", new);
>>  			continue;
>>  		}
>> +		x86_init_chip_data(desc_new, node);
>>  		cfg_new = desc_new->chip_data;
>>  
>>  		if (cfg_new->vector != 0)
>> @@ -3466,6 +3484,11 @@ static struct irq_chip msi_chip = {
>>  	.set_affinity	= set_msi_irq_affinity,
>>  #endif
>>  	.retrigger	= ioapic_retrigger_irq,
>> +
>> +#ifdef CONFIG_SPARSE_IRQ
>> +	.copy_chip_data = x86_copy_chip_data,
>> +	.free_chip_data = x86_free_chip_data,
>> +#endif
>>  };
>>  
>>  static struct irq_chip msi_ir_chip = {
>> @@ -3479,6 +3502,11 @@ static struct irq_chip msi_ir_chip = {
>>  #endif
>>  #endif
>>  	.retrigger	= ioapic_retrigger_irq,
>> +
>> +#ifdef CONFIG_SPARSE_IRQ
>> +	.copy_chip_data = x86_copy_chip_data,
>> +	.free_chip_data = x86_free_chip_data,
>> +#endif
>>  };
>>  
>>  /*
>> @@ -3638,6 +3666,11 @@ static struct irq_chip dmar_msi_type = {
>>  	.set_affinity = dmar_msi_set_affinity,
>>  #endif
>>  	.retrigger = ioapic_retrigger_irq,
>> +
>> +#ifdef CONFIG_SPARSE_IRQ
>> +	.copy_chip_data = x86_copy_chip_data,
>> +	.free_chip_data = x86_free_chip_data,
>> +#endif
>>  };
>>  
>>  int arch_setup_dmar_msi(unsigned int irq)
>> @@ -3695,6 +3728,11 @@ static struct irq_chip ir_hpet_msi_type = {
>>  #endif
>>  #endif
>>  	.retrigger = ioapic_retrigger_irq,
>> +
>> +#ifdef CONFIG_SPARSE_IRQ
>> +	.copy_chip_data = x86_copy_chip_data,
>> +	.free_chip_data = x86_free_chip_data,
>> +#endif
>>  };
>>  
>>  static struct irq_chip hpet_msi_type = {
>> @@ -3706,6 +3744,11 @@ static struct irq_chip hpet_msi_type = {
>>  	.set_affinity = hpet_msi_set_affinity,
>>  #endif
>>  	.retrigger = ioapic_retrigger_irq,
>> +
>> +#ifdef CONFIG_SPARSE_IRQ
>> +	.copy_chip_data = x86_copy_chip_data,
>> +	.free_chip_data = x86_free_chip_data,
>> +#endif
>>  };
>>  
>>  int arch_setup_hpet_msi(unsigned int irq, unsigned int id)
>> @@ -3792,6 +3835,11 @@ static struct irq_chip ht_irq_chip = {
>>  	.set_affinity	= set_ht_irq_affinity,
>>  #endif
>>  	.retrigger	= ioapic_retrigger_irq,
>> +
>> +#ifdef CONFIG_SPARSE_IRQ
>> +	.copy_chip_data = x86_copy_chip_data,
>> +	.free_chip_data = x86_free_chip_data,
>> +#endif
>>  };
>>  
>>  int arch_setup_ht_irq(unsigned int irq, struct pci_dev *dev)
>> @@ -3919,6 +3967,7 @@ static int __io_apic_set_pci_routing(struct device *dev, int irq,
>>  		printk(KERN_INFO "can not get irq_desc %d\n", irq);
>>  		return 0;
>>  	}
>> +	x86_init_chip_data(desc, node);
>>  
>>  	pin = irq_attr->ioapic_pin;
>>  	trigger = irq_attr->trigger;
>> @@ -4313,6 +4362,7 @@ void __init pre_init_apic_IRQ0(void)
>>  	phys_cpu_present_map = physid_mask_of_physid(boot_cpu_physical_apicid);
>>  #endif
>>  	desc = irq_to_desc_alloc_node(0, 0);
>> +	x86_init_chip_data(desc, 0);
>>  
>>  	setup_local_APIC();
>>  
>> diff --git a/arch/x86/kernel/uv_irq.c b/arch/x86/kernel/uv_irq.c
>> index ece73d8..df2c6d6 100644
>> --- a/arch/x86/kernel/uv_irq.c
>> +++ b/arch/x86/kernel/uv_irq.c
>> @@ -55,6 +55,11 @@ struct irq_chip uv_irq_chip = {
>>  	.eoi		= uv_ack_apic,
>>  	.end		= uv_noop,
>>  	.set_affinity	= uv_set_irq_affinity,
>> +
>> +#ifdef CONFIG_SPARSE_IRQ
>> +	.copy_chip_data = x86_copy_chip_data,
>> +	.free_chip_data = x86_free_chip_data,
>> +#endif
>>  };
>>  
>>  /*
>> diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
>> index 75f3f00..cc4cd22 100644
>> --- a/include/linux/interrupt.h
>> +++ b/include/linux/interrupt.h
>> @@ -611,6 +611,6 @@ 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);
>> +extern int arch_init_irq_desc(struct irq_desc *desc, int node);
>>  
>>  #endif
>> diff --git a/include/linux/irq.h b/include/linux/irq.h
>> index 707ab12..558bd2d 100644
>> --- a/include/linux/irq.h
>> +++ b/include/linux/irq.h
>> @@ -131,6 +131,14 @@ struct irq_chip {
>>  	void		(*bus_lock)(unsigned int irq);
>>  	void		(*bus_sync_unlock)(unsigned int irq);
>>  
>> +	/* for move_irq_desc */
>> +#ifdef CONFIG_SPARSE_IRQ
>> +	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);
>> +#endif
>> +
>>  	/* Currently used only by UML, might disappear one day.*/
>>  #ifdef CONFIG_IRQ_RELEASE_METHOD
>>  	void		(*release)(unsigned int irq, void *dev_id);
>> @@ -208,10 +216,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
>> diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
>> index 76d5a67..168e2f8 100644
>> --- a/kernel/irq/handle.c
>> +++ b/kernel/irq/handle.c
>> @@ -120,7 +120,6 @@ 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);
>>  }
>>  
>>  /*
>> @@ -228,6 +227,7 @@ struct irq_desc * __ref irq_to_desc_alloc_node(unsigned int irq, int node)
>>  		BUG_ON(1);
>>  	}
>>  	init_one_irq_desc(irq, desc, node);
>> +	arch_init_irq_desc(desc, node);
>
> you move out init_chip_data for x86 out of irq_to_desc_alloc_node
> current init_chip_data in irq_to_desc_alloc_node is protected by sparse_irq_lock.
>
>
> wonder if you could let arch_init_irq_desc for x86 to call x86_init_chip_data?
>
> or let add another irq_to_desc_alloc_node_f to take extra func call.
>
> like 
> typedef int (*init_chip_data_f)(struct irq_desc *desc, int node);
> struct irq_desc * __ref irq_to_desc_alloc_node_f(unsigned int irq, int node, init_chip_data_f *f);
>
> struct irq_desc * __ref irq_to_desc_alloc_node_f(unsigned int irq, int node)
> {
>  irq_to_desc_alloc_node(irq, node, NULL);
> }
>
> then for x86
>
> int arch_init_irq_desc(struct irq_desc *desc, int node, init_chip_data_f *f)
> {
> 	if (!f)
> 		return x86_init_chip_data(desc, node);
>  	f(desc, node);
> }
>
> after that xen could use
> irq_to_desc_alloc_node_f(irq, node, xen_init_chip_data);
>
> as need...
>
> at last we don't need to call x86_init_chip_data everywhere.

Either that or we could have an arch specific wrapper say
"x86_irq_to_desc_alloc_node" that simply wraps irq_to_desc_alloc_node,
with the little extras we need.  This assumes that none of the
initialization has to happen inside of the lock an arch specific
wrapper makes sense.

Let's evaluate this set of patches on it's merits.  What this patchset
allows is a Xen implementation whose irq functions are fully decoupled
from the rest of x86 that supports SPARSE_IRQ.    That is a huge
maintenance improvement that far outweighs the worst damage this
patch can do to maintenance.

So trying to evaluate races.  The worse case for this particular piece
of code appears to be create_irq_nr.  As this is the only place where
we are setting up irqs and possibly repurposing the structure.  Today
we figure out if an irq has been assigned by looking at irq_cfg->vector,
and if it is non-zero the irq has been assigned.

The logic in x86_init_chip_data is correct we only assign desc->chip_data
if the generic layers are above it.  However we need a lock to ensure that
two paths don't race in that comparison and that assignment.  There is
no lock in x86_init_chip_data.  Which unfortunately means as it stands
this patchset is buggy.

Eric

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

[Ian Campbell]

On Sat, 2010-03-13 at 00:29 +0000, Eric W. Biederman wrote: 
> [...]
> > after that xen could use
> > irq_to_desc_alloc_node_f(irq, node, xen_init_chip_data);
> >
> > as need...
> >
> > at last we don't need to call x86_init_chip_data everywhere.

This was one of the things I was considering. It seems like one of the
easiest solutions to make work correctly with the current locking in
e.g. irq_to_desc_alloc_node since the callback would always happen under
the lock taken in that function.

> So trying to evaluate races.  The worse case for this particular piece
> of code appears to be create_irq_nr.  As this is the only place where
> we are setting up irqs and possibly repurposing the structure.

Yes, create_irq_nr was one of the functions I was struggling to solve
cleanly. There is a similar construct in the Xen code as well.

Part of the problem I'm having is the combination of lookup and allocate
in irq_to_desc_alloc_node but also the kind of "implicit" repurposing is
tricky to deal with. (by implicit I just mean that I can't find where
the previous user explicitly says they are finished with it, if you see
what I mean)

What do you think of adding an explicit free operation for the irq_desc
structs? (does one already exist? I couldn't find it). This would go
along with some tracking of allocation state, trivial in the sparse case
where you can treat a NULL node in the radix tree as unallocated, I
guess a flag would suffice in the static array non-sparse case?

Going further could we split the alloc and lookup functions into
separate operations instead of combining them in irq_to_desc_alloc_node?
We already have irq_to_desc for the lookup portion so this would largely
involve changes to the semantics of irq_to_desc_alloc_node, perhaps
returning ERR_PTR(-EBUSY) if the node was already allocated.

Having a variant which found a free IRQ rather than operating on a
specific requested IRQ could also be useful for create_irq_nr as well as
find_unbound_irq on the Xen side. I'm not convinced irq_alloc_virt on
powerpc isn't implementing broadly the same concept as well, although it
seems to work very differently from the other two.

>   Today
> we figure out if an irq has been assigned by looking at irq_cfg->vector,
> and if it is non-zero the irq has been assigned.

Which is tricky to move into generic code hence my suggestions of
explicitly freeing the irq_desc and tracking the allocation status in
the generic code. 

> The logic in x86_init_chip_data is correct we only assign desc->chip_data
> if the generic layers are above it.  However we need a lock to ensure that
> two paths don't race in that comparison and that assignment.  There is
> no lock in x86_init_chip_data.  Which unfortunately means as it stands
> this patchset is buggy.

Yes, unfortunately I think you are right. The callback idea fixes this.
I'll respin with that.

Ian.

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

[Eric W. Biederman]

Ian Campbell <Ian.Campbell@citrix.com> writes:

> On Sat, 2010-03-13 at 00:29 +0000, Eric W. Biederman wrote: 
>> [...]
>> > after that xen could use
>> > irq_to_desc_alloc_node_f(irq, node, xen_init_chip_data);
>> >
>> > as need...
>> >
>> > at last we don't need to call x86_init_chip_data everywhere.
>
> This was one of the things I was considering. It seems like one of the
> easiest solutions to make work correctly with the current locking in
> e.g. irq_to_desc_alloc_node since the callback would always happen under
> the lock taken in that function.
>
>> So trying to evaluate races.  The worse case for this particular piece
>> of code appears to be create_irq_nr.  As this is the only place where
>> we are setting up irqs and possibly repurposing the structure.
>
> Yes, create_irq_nr was one of the functions I was struggling to solve
> cleanly. There is a similar construct in the Xen code as well.
>
> Part of the problem I'm having is the combination of lookup and allocate
> in irq_to_desc_alloc_node but also the kind of "implicit" repurposing is
> tricky to deal with. (by implicit I just mean that I can't find where
> the previous user explicitly says they are finished with it, if you see
> what I mean)
>
> What do you think of adding an explicit free operation for the irq_desc
> structs? (does one already exist? I couldn't find it). This would go
> along with some tracking of allocation state, trivial in the sparse case
> where you can treat a NULL node in the radix tree as unallocated, I
> guess a flag would suffice in the static array non-sparse case?

We have free_one_irq_desc used in the numa irq migration code.
We have dynamic_irq_cleanup.

I don't expect it would be fundamentally hard to put all of the pieces
together.

> Going further could we split the alloc and lookup functions into
> separate operations instead of combining them in irq_to_desc_alloc_node?
> We already have irq_to_desc for the lookup portion so this would largely
> involve changes to the semantics of irq_to_desc_alloc_node, perhaps
> returning ERR_PTR(-EBUSY) if the node was already allocated.
>
> Having a variant which found a free IRQ rather than operating on a
> specific requested IRQ could also be useful for create_irq_nr as well as
> find_unbound_irq on the Xen side. I'm not convinced irq_alloc_virt on
> powerpc isn't implementing broadly the same concept as well, although it
> seems to work very differently from the other two.
>
>>   Today
>> we figure out if an irq has been assigned by looking at irq_cfg->vector,
>> and if it is non-zero the irq has been assigned.
>
> Which is tricky to move into generic code hence my suggestions of
> explicitly freeing the irq_desc and tracking the allocation status in
> the generic code. 

Agreed.  Getting an allocation assist into the generic code sounds
nlike something for the next round of patches after your current one.
At least for msi irqs, hi irqs and other irqs that are totally software
constructs this makes a lot of sense.

We really need to get to the point on x86 where sparse irq has no penalties
and stop making it an option.  Otherwise it gets a bit tricky to rely
on sparse irq for an allocation helper.

I need to go look up my old patchset.  I did something different with
create_irq(), that I think was cleaner and it may be worth resurrecting.
At the very least I generalized the msi case and the ht irq case into
using the same pointer in struct irq_desc.

>> The logic in x86_init_chip_data is correct we only assign desc->chip_data
>> if the generic layers are above it.  However we need a lock to ensure that
>> two paths don't race in that comparison and that assignment.  There is
>> no lock in x86_init_chip_data.  Which unfortunately means as it stands
>> this patchset is buggy.
>
> Yes, unfortunately I think you are right. The callback idea fixes this.
> I'll respin with that.

Thanks.

Eric

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

[ijc]

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 at this time
because irq_desc->chip is not known at the time the irq_desc is
setup. For now rename arch_init_chip_data to arch_init_irq_desc (for
PowerPC, the only other user, whose usage better matches the new name)
and on x86 convert arch_init_chip_data to ioapic_init_chip_data and
call this whenever the IO APIC code allocates a new IRQ.

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
ioapic_{init,copy,free}_chip_data functions could be static to
io_apic.c.

I've tested by booting on a 64 bit system, but it's not clear to me
what actions I need to take to actually exercise some of these code
paths.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
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
---
 arch/powerpc/kernel/irq.c      |    2 +-
 arch/x86/include/asm/hw_irq.h  |   11 +++++++-
 arch/x86/kernel/apic/io_apic.c |   58 +++++++++++++++++++++++++++++++++++++---
 arch/x86/kernel/uv_irq.c       |    5 +++
 include/linux/interrupt.h      |    2 +-
 include/linux/irq.h            |   12 +++++---
 kernel/irq/handle.c            |    2 +-
 kernel/irq/numa_migrate.c      |   12 +++++++-
 kernel/softirq.c               |    3 +-
 9 files changed, 91 insertions(+), 16 deletions(-)

diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
index 64f6f20..002d87f 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)
 {
 	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 0642186..b9d7310 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,15 @@ extern void init_VISWS_APIC_irqs(void);
 extern void setup_IO_APIC(void);
 extern void disable_IO_APIC(void);
 
+extern int ioapic_init_chip_data(struct irq_desc *desc, int node);
+
+#ifdef CONFIG_SPARSE_IRQ
+extern void ioapic_copy_chip_data(struct irq_desc *old_desc,
+				  struct irq_desc *desc, int node);
+extern void ioapic_free_chip_data(struct irq_desc *old_desc,
+				  struct irq_desc *desc);
+#endif
+
 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 a57d974..74d5d96 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -211,7 +211,7 @@ static struct ioapic_irq_cfg *get_one_free_irq_cfg(int node)
 	return cfg;
 }
 
-int arch_init_chip_data(struct irq_desc *desc, int node)
+int ioapic_init_chip_data(struct irq_desc *desc, int node)
 {
 	struct ioapic_irq_cfg *cfg;
 
@@ -289,8 +289,8 @@ static void free_irq_2_pin(struct ioapic_irq_cfg *old_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 ioapic_copy_chip_data(struct irq_desc *old_desc,
+			   struct irq_desc *desc, int node)
 {
 	struct ioapic_irq_cfg *cfg;
 	struct ioapic_irq_cfg *old_cfg;
@@ -314,7 +314,7 @@ static void free_irq_cfg(struct ioapic_irq_cfg *old_cfg)
 	kfree(old_cfg);
 }
 
-void arch_free_chip_data(struct irq_desc *old_desc, struct irq_desc *desc)
+void ioapic_free_chip_data(struct irq_desc *old_desc, struct irq_desc *desc)
 {
 	struct ioapic_irq_cfg *old_cfg, *cfg;
 
@@ -338,6 +338,11 @@ struct ioapic_irq_cfg *ioapic_irq_cfg(unsigned int irq)
 	return irq < nr_irqs ? irq_cfgx + irq : NULL;
 }
 
+int ioapic_init_chip_data(struct irq_desc *desc, int node)
+{
+	return 0;
+}
+
 #endif
 
 struct io_apic {
@@ -1526,6 +1531,7 @@ static void __init setup_IO_APIC_irqs(void)
 			printk(KERN_INFO "can not get irq_desc for %d\n", irq);
 			continue;
 		}
+		ioapic_init_chip_data(desc, node);
 		cfg = desc->chip_data;
 		add_pin_to_irq_node(cfg, node, apic_id, pin);
 		/*
@@ -1576,6 +1582,7 @@ void setup_IO_APIC_irq_extra(u32 gsi)
 		printk(KERN_INFO "can not get irq_desc for %d\n", irq);
 		return;
 	}
+	ioapic_init_chip_data(desc, node);
 
 	cfg = desc->chip_data;
 	add_pin_to_irq_node(cfg, node, apic_id, pin);
@@ -2746,6 +2753,11 @@ static struct irq_chip ioapic_chip __read_mostly = {
 	.set_affinity	= set_ioapic_affinity_irq,
 #endif
 	.retrigger	= ioapic_retrigger_irq,
+
+#ifdef CONFIG_SPARSE_IRQ
+	.copy_chip_data = ioapic_copy_chip_data,
+	.free_chip_data = ioapic_free_chip_data,
+#endif
 };
 
 static struct irq_chip ir_ioapic_chip __read_mostly = {
@@ -2761,6 +2773,11 @@ static struct irq_chip ir_ioapic_chip __read_mostly = {
 #endif
 #endif
 	.retrigger	= ioapic_retrigger_irq,
+
+#ifdef CONFIG_SPARSE_IRQ
+	.copy_chip_data = ioapic_copy_chip_data,
+	.free_chip_data = ioapic_free_chip_data,
+#endif
 };
 
 static inline void init_IO_APIC_traps(void)
@@ -3268,6 +3285,7 @@ unsigned int create_irq_nr(unsigned int irq_want, int node)
 			printk(KERN_INFO "can not get irq_desc for %d\n", new);
 			continue;
 		}
+		ioapic_init_chip_data(desc_new, node);
 		cfg_new = desc_new->chip_data;
 
 		if (cfg_new->vector != 0)
@@ -3474,6 +3492,11 @@ static struct irq_chip msi_chip = {
 	.set_affinity	= set_msi_irq_affinity,
 #endif
 	.retrigger	= ioapic_retrigger_irq,
+
+#ifdef CONFIG_SPARSE_IRQ
+	.copy_chip_data = ioapic_copy_chip_data,
+	.free_chip_data = ioapic_free_chip_data,
+#endif
 };
 
 static struct irq_chip msi_ir_chip = {
@@ -3487,6 +3510,11 @@ static struct irq_chip msi_ir_chip = {
 #endif
 #endif
 	.retrigger	= ioapic_retrigger_irq,
+
+#ifdef CONFIG_SPARSE_IRQ
+	.copy_chip_data = ioapic_copy_chip_data,
+	.free_chip_data = ioapic_free_chip_data,
+#endif
 };
 
 /*
@@ -3646,6 +3674,11 @@ static struct irq_chip dmar_msi_type = {
 	.set_affinity = dmar_msi_set_affinity,
 #endif
 	.retrigger = ioapic_retrigger_irq,
+
+#ifdef CONFIG_SPARSE_IRQ
+	.copy_chip_data = ioapic_copy_chip_data,
+	.free_chip_data = ioapic_free_chip_data,
+#endif
 };
 
 int arch_setup_dmar_msi(unsigned int irq)
@@ -3703,6 +3736,11 @@ static struct irq_chip ir_hpet_msi_type = {
 #endif
 #endif
 	.retrigger = ioapic_retrigger_irq,
+
+#ifdef CONFIG_SPARSE_IRQ
+	.copy_chip_data = ioapic_copy_chip_data,
+	.free_chip_data = ioapic_free_chip_data,
+#endif
 };
 
 static struct irq_chip hpet_msi_type = {
@@ -3714,6 +3752,11 @@ static struct irq_chip hpet_msi_type = {
 	.set_affinity = hpet_msi_set_affinity,
 #endif
 	.retrigger = ioapic_retrigger_irq,
+
+#ifdef CONFIG_SPARSE_IRQ
+	.copy_chip_data = ioapic_copy_chip_data,
+	.free_chip_data = ioapic_free_chip_data,
+#endif
 };
 
 int arch_setup_hpet_msi(unsigned int irq, unsigned int id)
@@ -3800,6 +3843,11 @@ static struct irq_chip ht_irq_chip = {
 	.set_affinity	= set_ht_irq_affinity,
 #endif
 	.retrigger	= ioapic_retrigger_irq,
+
+#ifdef CONFIG_SPARSE_IRQ
+	.copy_chip_data = ioapic_copy_chip_data,
+	.free_chip_data = ioapic_free_chip_data,
+#endif
 };
 
 int arch_setup_ht_irq(unsigned int irq, struct pci_dev *dev)
@@ -3927,6 +3975,7 @@ static int __io_apic_set_pci_routing(struct device *dev, int irq,
 		printk(KERN_INFO "can not get irq_desc %d\n", irq);
 		return 0;
 	}
+	ioapic_init_chip_data(desc, node);
 
 	pin = irq_attr->ioapic_pin;
 	trigger = irq_attr->trigger;
@@ -4321,6 +4370,7 @@ void __init pre_init_apic_IRQ0(void)
 	phys_cpu_present_map = physid_mask_of_physid(boot_cpu_physical_apicid);
 #endif
 	desc = irq_to_desc_alloc_node(0, 0);
+	ioapic_init_chip_data(desc, 0);
 
 	setup_local_APIC();
 
diff --git a/arch/x86/kernel/uv_irq.c b/arch/x86/kernel/uv_irq.c
index 3520564..96020cb 100644
--- a/arch/x86/kernel/uv_irq.c
+++ b/arch/x86/kernel/uv_irq.c
@@ -55,6 +55,11 @@ struct irq_chip uv_irq_chip = {
 	.eoi		= uv_ack_apic,
 	.end		= uv_noop,
 	.set_affinity	= uv_set_irq_affinity,
+
+#ifdef CONFIG_SPARSE_IRQ
+	.copy_chip_data = ioapic_copy_chip_data,
+	.free_chip_data = ioapic_free_chip_data,
+#endif
 };
 
 /*
diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 75f3f00..cc4cd22 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -611,6 +611,6 @@ 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);
+extern int arch_init_irq_desc(struct irq_desc *desc, int node);
 
 #endif
diff --git a/include/linux/irq.h b/include/linux/irq.h
index 707ab12..558bd2d 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -131,6 +131,14 @@ struct irq_chip {
 	void		(*bus_lock)(unsigned int irq);
 	void		(*bus_sync_unlock)(unsigned int irq);
 
+	/* for move_irq_desc */
+#ifdef CONFIG_SPARSE_IRQ
+	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);
+#endif
+
 	/* Currently used only by UML, might disappear one day.*/
 #ifdef CONFIG_IRQ_RELEASE_METHOD
 	void		(*release)(unsigned int irq, void *dev_id);
@@ -208,10 +216,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
diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
index 76d5a67..168e2f8 100644
--- a/kernel/irq/handle.c
+++ b/kernel/irq/handle.c
@@ -120,7 +120,6 @@ 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);
 }
 
 /*
@@ -228,6 +227,7 @@ struct irq_desc * __ref irq_to_desc_alloc_node(unsigned int irq, int node)
 		BUG_ON(1);
 	}
 	init_one_irq_desc(irq, desc, node);
+	arch_init_irq_desc(desc, node);
 
 	set_irq_desc(irq, desc);
 
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..3f4b80e 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -895,8 +895,7 @@ int __init __weak arch_early_irq_init(void)
 {
 	return 0;
 }
-
-int __weak arch_init_chip_data(struct irq_desc *desc, int node)
+int __weak arch_init_irq_desc(struct irq_desc *desc, int node)
 {
 	return 0;
 }
-- 
1.5.6.5

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

[Ian Campbell]

On Wed, 2010-03-10 at 10:55 +0000, ijc@hellion.org.uk wrote:
> 
> arch_init_chip_data cannot be moved into struct irq_chip at this time
> because irq_desc->chip is not known at the time the irq_desc is
> setup. For now rename arch_init_chip_data to arch_init_irq_desc (for
> PowerPC, the only other user, whose usage better matches the new name)
> and on x86 convert arch_init_chip_data to ioapic_init_chip_data and
> call this whenever the IO APIC code allocates a new IRQ.

One idea I had to improve this was to add a struct irq_chip * as a
parameter to irq_to_desc_alloc_node. The new parameter potentially could
be NULL for current behaviour. Does that sound like a reasonable
approach?

Ian.

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

[Eric W. Biederman]

Ian Campbell <Ian.Campbell@citrix.com> writes:

> On Wed, 2010-03-10 at 10:55 +0000, ijc@hellion.org.uk wrote:
>> 
>> arch_init_chip_data cannot be moved into struct irq_chip at this time
>> because irq_desc->chip is not known at the time the irq_desc is
>> setup. For now rename arch_init_chip_data to arch_init_irq_desc (for
>> PowerPC, the only other user, whose usage better matches the new name)
>> and on x86 convert arch_init_chip_data to ioapic_init_chip_data and
>> call this whenever the IO APIC code allocates a new IRQ.
>
> One idea I had to improve this was to add a struct irq_chip * as a
> parameter to irq_to_desc_alloc_node. The new parameter potentially could
> be NULL for current behaviour. Does that sound like a reasonable
> approach?

I don't follow why we have the restriction that irq_to_desc_alloc_node
must call arch_init_chip_data.  Assuming that requirement to call arch_init_chip_data
is valid, passing something into init_one_irq_desc seems appropriate.

Eric

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

[Ian Campbell]

On Wed, 2010-03-10 at 17:18 +0000, Eric W. Biederman wrote:
> Ian Campbell <Ian.Campbell@citrix.com> writes:
> 
> > On Wed, 2010-03-10 at 10:55 +0000, ijc@hellion.org.uk wrote:
> >> 
> >> arch_init_chip_data cannot be moved into struct irq_chip at this time
> >> because irq_desc->chip is not known at the time the irq_desc is
> >> setup. For now rename arch_init_chip_data to arch_init_irq_desc (for
> >> PowerPC, the only other user, whose usage better matches the new name)
> >> and on x86 convert arch_init_chip_data to ioapic_init_chip_data and
> >> call this whenever the IO APIC code allocates a new IRQ.
> >
> > One idea I had to improve this was to add a struct irq_chip * as a
> > parameter to irq_to_desc_alloc_node. The new parameter potentially could
> > be NULL for current behaviour. Does that sound like a reasonable
> > approach?
> 
> I don't follow why we have the restriction that irq_to_desc_alloc_node
> must call arch_init_chip_data.  Assuming that requirement to call arch_init_chip_data
> is valid, passing something into init_one_irq_desc seems appropriate.

Yes, I suspect that could also be made to work.

The lifecycle of the irq_desc and chip_data isn't really clear to me --
I guess once allocated an irq_desc never gets freed (at least
currently)? The associated chip_data can be freed on migrate and
replaced with a new one, but is not freed otherwise.

My concern is that if the caller asks for an IRQ which already exists
(is that valid?) then you will get that existing irq_desc back,
including its existing chip_data, which potentially leaks the new one
which was passed in. Or is it the case that the only way this could
happen would be for legacy IRQs? In which case perhaps it is simply
invalid to pass a new chip data in for such an IRQ.

Ian.

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

[Eric W. Biederman]

Ian Campbell <Ian.Campbell@citrix.com> writes:

> On Wed, 2010-03-10 at 17:18 +0000, Eric W. Biederman wrote:
>> Ian Campbell <Ian.Campbell@citrix.com> writes:
>> 
>> > On Wed, 2010-03-10 at 10:55 +0000, ijc@hellion.org.uk wrote:
>> >> 
>> >> arch_init_chip_data cannot be moved into struct irq_chip at this time
>> >> because irq_desc->chip is not known at the time the irq_desc is
>> >> setup. For now rename arch_init_chip_data to arch_init_irq_desc (for
>> >> PowerPC, the only other user, whose usage better matches the new name)
>> >> and on x86 convert arch_init_chip_data to ioapic_init_chip_data and
>> >> call this whenever the IO APIC code allocates a new IRQ.
>> >
>> > One idea I had to improve this was to add a struct irq_chip * as a
>> > parameter to irq_to_desc_alloc_node. The new parameter potentially could
>> > be NULL for current behaviour. Does that sound like a reasonable
>> > approach?
>> 
>> I don't follow why we have the restriction that irq_to_desc_alloc_node
>> must call arch_init_chip_data.  Assuming that requirement to call arch_init_chip_data
>> is valid, passing something into init_one_irq_desc seems appropriate.
>
> Yes, I suspect that could also be made to work.
>
> The lifecycle of the irq_desc and chip_data isn't really clear to me --
> I guess once allocated an irq_desc never gets freed (at least
> currently)? The associated chip_data can be freed on migrate and
> replaced with a new one, but is not freed otherwise.

Yes.  That actually looks like a bug.

> My concern is that if the caller asks for an IRQ which already exists
> (is that valid?) then you will get that existing irq_desc back,
> including its existing chip_data, which potentially leaks the new one
> which was passed in. Or is it the case that the only way this could
> happen would be for legacy IRQs? In which case perhaps it is simply
> invalid to pass a new chip data in for such an IRQ.

The only irqs that should be allocated/freed are probably the msi
irqs as those are the only ones that dynamically come and go in the
system.

Unfortunately there appears to be a bigger mess here than first appeared.

Eric

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

[Yinghai Lu]

On Wed, Mar 10, 2010 at 2:55 AM,  <ijc@hellion.org.uk> 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.
>
> arch_init_chip_data cannot be moved into struct irq_chip at this time
> because irq_desc->chip is not known at the time the irq_desc is
> setup. For now rename arch_init_chip_data to arch_init_irq_desc (for
> PowerPC, the only other user, whose usage better matches the new name)
> and on x86 convert arch_init_chip_data to ioapic_init_chip_data and
> call this whenever the IO APIC code allocates a new IRQ.
>
> 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
> ioapic_{init,copy,free}_chip_data functions could be static to
> io_apic.c.
>
> I've tested by booting on a 64 bit system, but it's not clear to me
> what actions I need to take to actually exercise some of these code
> paths.
>

can you just add another pointer field in irq_desc?

some kind of *irq_info etc.

YH

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

[Ian Campbell]

On Wed, 2010-03-10 at 12:06 +0000, Yinghai Lu wrote:
> On Wed, Mar 10, 2010 at 2:55 AM,  <ijc@hellion.org.uk> 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.
> >
> > arch_init_chip_data cannot be moved into struct irq_chip at this time
> > because irq_desc->chip is not known at the time the irq_desc is
> > setup. For now rename arch_init_chip_data to arch_init_irq_desc (for
> > PowerPC, the only other user, whose usage better matches the new name)
> > and on x86 convert arch_init_chip_data to ioapic_init_chip_data and
> > call this whenever the IO APIC code allocates a new IRQ.
> >
> > 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
> > ioapic_{init,copy,free}_chip_data functions could be static to
> > io_apic.c.
> >
> > I've tested by booting on a 64 bit system, but it's not clear to me
> > what actions I need to take to actually exercise some of these code
> > paths.
> >
> 
> can you just add another pointer field in irq_desc?
> 
> some kind of *irq_info etc.

I think I don't understand what you are suggesting.

There is already a pointer for irq_chip specific use i.e.
irq_desc->chip_data. This patchset is just about ensuring that the field
really is available to any chip implementation rather than just assuming
it is always used for the acpi chip types (on x86 at least).

Does adding a second pointer with the same (intended?) semantics as the
existing one buy us anything?

Ian.

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

[Eric W. Biederman]

Ian Campbell <Ian.Campbell@citrix.com> writes:

> On Wed, 2010-03-10 at 12:06 +0000, Yinghai Lu wrote:
>> On Wed, Mar 10, 2010 at 2:55 AM,  <ijc@hellion.org.uk> 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.
>> >
>> > arch_init_chip_data cannot be moved into struct irq_chip at this time
>> > because irq_desc->chip is not known at the time the irq_desc is
>> > setup. For now rename arch_init_chip_data to arch_init_irq_desc (for
>> > PowerPC, the only other user, whose usage better matches the new name)
>> > and on x86 convert arch_init_chip_data to ioapic_init_chip_data and
>> > call this whenever the IO APIC code allocates a new IRQ.
>> >
>> > 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
>> > ioapic_{init,copy,free}_chip_data functions could be static to
>> > io_apic.c.
>> >
>> > I've tested by booting on a 64 bit system, but it's not clear to me
>> > what actions I need to take to actually exercise some of these code
>> > paths.
>> >
>> 
>> can you just add another pointer field in irq_desc?
>> 
>> some kind of *irq_info etc.
>
> I think I don't understand what you are suggesting.

YH another field doesn't make much sense.  Xen is a bizarre subarch
with an incompatible irq model.  Xen simply needs the ability to
handle the entire lifetime of an irq_chip.

All we need between the Xen and the rest of x86 is a convention
so that we never manage the same irqs.   At least for domU we are
in an either/or situation so I don't see even that being a problem.

> There is already a pointer for irq_chip specific use i.e.
> irq_desc->chip_data. This patchset is just about ensuring that the field
> really is available to any chip implementation rather than just assuming
> it is always used for the acpi chip types (on x86 at least).

Ian Xen in this sense is simply not x86.  irq_cfg is not acpi or ioapic
or anything but x86 specific.  It has everything to do with having a per
cpu vector table of 256 entries and architecturally receiving a vector
number when an interrupt is fired.

It totally makes sense for Xen to do something different because
architecturally it has a completely different irq subsystem.

At the same time let's not pretend that the reason for this is anything
except that Xen has a completely different notion of interrupt delivery
than the rest of x86 and so it is it's own bizarre subarch.

This is not a case where you simply need a driver because something is
a bit different but fits into the existing model.

So the best solution here seems to be a parameter that we pass into
irq_to_desc_alloc_node that does what is needed.  The second best
would be to have arch_init_chip_data to call something like
platfrom_init_chip_data().    But I think we can avoid that in
this case.

Eric

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

[Ian Campbell]

On Wed, 2010-03-10 at 17:42 +0000, Eric W. Biederman wrote:
> 
> 
> Ian Xen in this sense is simply not x86.  irq_cfg is not acpi or
> ioapic or anything but x86 specific.  It has everything to do with
> having a per cpu vector table of 256 entries and architecturally
> receiving a vector number when an interrupt is fired.
> 
> It totally makes sense for Xen to do something different because
> architecturally it has a completely different irq subsystem.

OK, so that sounds like you would like the same patchset but without the
irq_cfg renaming? or potentially with renaming to x86_blah instead (I'll
rework to your preference).

Ian.

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

[Eric W. Biederman]

Ian Campbell <Ian.Campbell@citrix.com> writes:

> On Wed, 2010-03-10 at 17:42 +0000, Eric W. Biederman wrote:
>> 
>> 
>> Ian Xen in this sense is simply not x86.  irq_cfg is not acpi or
>> ioapic or anything but x86 specific.  It has everything to do with
>> having a per cpu vector table of 256 entries and architecturally
>> receiving a vector number when an interrupt is fired.
>> 
>> It totally makes sense for Xen to do something different because
>> architecturally it has a completely different irq subsystem.
>
> OK, so that sounds like you would like the same patchset but without the
> irq_cfg renaming? or potentially with renaming to x86_blah instead (I'll
> rework to your preference).

Currently the renaming really makes it unclear what you are doing and for
some reason the description of the renaming rubbed me the wrong way.

Eric

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

[Ian Campbell]

On Wed, 2010-03-10 at 18:15 +0000, Eric W. Biederman wrote: 
> Ian Campbell <Ian.Campbell@citrix.com> writes:
> 
> > On Wed, 2010-03-10 at 17:42 +0000, Eric W. Biederman wrote:
> >> 
> >> 
> >> Ian Xen in this sense is simply not x86.  irq_cfg is not acpi or
> >> ioapic or anything but x86 specific.  It has everything to do with
> >> having a per cpu vector table of 256 entries and architecturally
> >> receiving a vector number when an interrupt is fired.
> >> 
> >> It totally makes sense for Xen to do something different because
> >> architecturally it has a completely different irq subsystem.
> >
> > OK, so that sounds like you would like the same patchset but without the
> > irq_cfg renaming? or potentially with renaming to x86_blah instead (I'll
> > rework to your preference).
> 
> Currently the renaming really makes it unclear what you are doing and for
> some reason the description of the renaming rubbed me the wrong way.

Sorry, I started off a bit confused and then totally misunderstood what
related to what and I think that came through in the description.

I'll respin without the first patch.

Ian.

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

[Jeremy Fitzhardinge]

On 03/10/2010 09:42 AM, Eric W. Biederman wrote:
> All we need between the Xen and the rest of x86 is a convention
> so that we never manage the same irqs.   At least for domU we are
> in an either/or situation so I don't see even that being a problem.
>    

Dom0 too.  This is part of the work implementing what we discussed a 
while back - Xen now completely owns the local and IO apics, so dom0 
only deals with Xen, not the hardware.  Xen has a completely different 
interrupt setup path, but at least it isn't a mishmash of Xen stuff and 
native APIC stuff.

     J

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

[Yinghai Lu]

On 03/10/2010 04:51 AM, Ian Campbell wrote:
> On Wed, 2010-03-10 at 12:06 +0000, Yinghai Lu wrote:
>> On Wed, Mar 10, 2010 at 2:55 AM,  <ijc@hellion.org.uk> 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.
>>>
>>> arch_init_chip_data cannot be moved into struct irq_chip at this time
>>> because irq_desc->chip is not known at the time the irq_desc is
>>> setup. For now rename arch_init_chip_data to arch_init_irq_desc (for
>>> PowerPC, the only other user, whose usage better matches the new name)
>>> and on x86 convert arch_init_chip_data to ioapic_init_chip_data and
>>> call this whenever the IO APIC code allocates a new IRQ.
>>>
>>> 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
>>> ioapic_{init,copy,free}_chip_data functions could be static to
>>> io_apic.c.
>>>
>>> I've tested by booting on a 64 bit system, but it's not clear to me
>>> what actions I need to take to actually exercise some of these code
>>> paths.
>>>
>>
>> can you just add another pointer field in irq_desc?
>>
>> some kind of *irq_info etc.
> 
> I think I don't understand what you are suggesting.
> 
> There is already a pointer for irq_chip specific use i.e.
> irq_desc->chip_data. This patchset is just about ensuring that the field
> really is available to any chip implementation rather than just assuming
> it is always used for the acpi chip types (on x86 at least).
> 
> Does adding a second pointer with the same (intended?) semantics as the
> existing one buy us anything?


#ifdef CONFIG_INTR_REMAP
        struct irq_2_iommu      *irq_2_iommu;
#endif
        struct irq_chip         *chip;
        struct msi_desc         *msi_desc;

we already have that for irq_2_iommu and msi_desc

YH

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

[Eric W. Biederman]

Yinghai Lu <yinghai@kernel.org> writes:

> On 03/10/2010 04:51 AM, Ian Campbell wrote:
>> On Wed, 2010-03-10 at 12:06 +0000, Yinghai Lu wrote:
>>> On Wed, Mar 10, 2010 at 2:55 AM,  <ijc@hellion.org.uk> 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.
>>>>
>>>> arch_init_chip_data cannot be moved into struct irq_chip at this time
>>>> because irq_desc->chip is not known at the time the irq_desc is
>>>> setup. For now rename arch_init_chip_data to arch_init_irq_desc (for
>>>> PowerPC, the only other user, whose usage better matches the new name)
>>>> and on x86 convert arch_init_chip_data to ioapic_init_chip_data and
>>>> call this whenever the IO APIC code allocates a new IRQ.
>>>>
>>>> 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
>>>> ioapic_{init,copy,free}_chip_data functions could be static to
>>>> io_apic.c.
>>>>
>>>> I've tested by booting on a 64 bit system, but it's not clear to me
>>>> what actions I need to take to actually exercise some of these code
>>>> paths.
>>>>
>>>
>>> can you just add another pointer field in irq_desc?
>>>
>>> some kind of *irq_info etc.
>> 
>> I think I don't understand what you are suggesting.
>> 
>> There is already a pointer for irq_chip specific use i.e.
>> irq_desc->chip_data. This patchset is just about ensuring that the field
>> really is available to any chip implementation rather than just assuming
>> it is always used for the acpi chip types (on x86 at least).
>> 
>> Does adding a second pointer with the same (intended?) semantics as the
>> existing one buy us anything?
>
>
> #ifdef CONFIG_INTR_REMAP
>         struct irq_2_iommu      *irq_2_iommu;
> #endif
>         struct irq_chip         *chip;
>         struct msi_desc         *msi_desc;
>
> we already have that for irq_2_iommu and msi_desc

Those are at different levels of the hierarchy.  Adding another pointer
for Xen is like having a different iommu and so adding another pointer
to handle that kind of iommu.

Eric

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

[Michael Ellerman]

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

On Wed, 2010-03-10 at 10:55 +0000, ijc@hellion.org.uk 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.
> 
> arch_init_chip_data cannot be moved into struct irq_chip at this time
> because irq_desc->chip is not known at the time the irq_desc is
> setup. For now rename arch_init_chip_data to arch_init_irq_desc (for
> PowerPC, the only other user, whose usage better matches the new name)
> and on x86 convert arch_init_chip_data to ioapic_init_chip_data and
> call this whenever the IO APIC code allocates a new IRQ.

Ack on the name change, it should be called arch_init_irq_desc(), the
existing name clearly comes from the fact that sparse IRQ was
implemented first on x86, and on x86 that routine init's the chip data
for a new irq_desc.

But semantically arch_init_irq_desc() is the right name, I was just too
lazy to change it when I enabled sparse IRQ for powerpc.

Can't comment on the rest of the patch.

cheers


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