[PATCH for -tip 0/4] various irq cleanups

[PATCH for -tip 0/4] various irq cleanups

[KOSAKI Motohiro]

Hi

this patch series is irq related cleanups.

I tested on

x86_64 with SPARSE_IRQ
ia64   without !SPARSE_IRQ


I builded on (by cross compiler)

alpha
arm
m68k
m68knommu
s390


total 7 architecutre.

[PATCH for -tip 1/4] hrtimer: remove #include <linux/irq.h>

[KOSAKI Motohiro]

<linux/irq.h> can be removed and shoud be. because

  - hrtimer doesn't use any irq feature.
  - <linux/irq.h> shouldn't be include from generic code.


Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ingo Molnar <mingo@elte.hu>
---
 kernel/hrtimer.c |    1 -
 1 file changed, 1 deletion(-)

Index: b/kernel/hrtimer.c
===================================================================
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -32,7 +32,6 @@
  */
 
 #include <linux/cpu.h>
-#include <linux/irq.h>
 #include <linux/module.h>
 #include <linux/percpu.h>
 #include <linux/hrtimer.h>

[PATCH for -tip 2/4] irq: for_each_irq_desc() move to irqnr.h

[KOSAKI Motohiro]

before CONFIG_SPARSE_IRQ age, for_each_irq_desc() sit in irqnr.h and
can be called from generic code.

CONFIG_SPARSE_IRQ break this assumption. but SPARSE_IRQ version
for_each_irq_desc() also can move irqnr.h easily.

Also, this patch unify CONFIG_SPARSE_IRQ and !CONFIG_SPARSE_IRQ for_each_irq_desc().


Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
CC: Yinghai Lu <yinghai@kernel.org>
CC: Ingo Molnar <mingo@elte.hu>
---
 include/linux/irq.h   |   24 ++++--------------------
 include/linux/irqnr.h |   19 +++++++++----------
 kernel/irq/handle.c   |   13 +++++++++++--
 3 files changed, 24 insertions(+), 32 deletions(-)

Index: b/include/linux/irq.h
===================================================================
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -203,33 +203,17 @@ extern void arch_free_chip_data(struct i
 
 #ifndef CONFIG_SPARSE_IRQ
 extern struct irq_desc irq_desc[NR_IRQS];
-
-static inline struct irq_desc *irq_to_desc(unsigned int irq)
-{
-	return (irq < NR_IRQS) ? irq_desc + irq : NULL;
-}
-static inline struct irq_desc *irq_to_desc_alloc_cpu(unsigned int irq, int cpu)
-{
-	return irq_to_desc(irq);
-}
-
-#else
-
-extern struct irq_desc *irq_to_desc(unsigned int irq);
-extern struct irq_desc *irq_to_desc_alloc_cpu(unsigned int irq, int cpu);
+#else /* CONFIG_SPARSE_IRQ */
 extern struct irq_desc *move_irq_desc(struct irq_desc *old_desc, int cpu);
 
-# define for_each_irq_desc(irq, desc)		\
-	for (irq = 0, desc = irq_to_desc(irq); irq < nr_irqs; irq++, desc = irq_to_desc(irq))
-# define for_each_irq_desc_reverse(irq, desc)                          \
-	for (irq = nr_irqs - 1, desc = irq_to_desc(irq); irq >= 0; irq--, desc = irq_to_desc(irq))
-
 #define kstat_irqs_this_cpu(DESC) \
 	((DESC)->kstat_irqs[smp_processor_id()])
 #define kstat_incr_irqs_this_cpu(irqno, DESC) \
 	((DESC)->kstat_irqs[smp_processor_id()]++)
 
-#endif
+#endif /* CONFIG_SPARSE_IRQ */
+
+extern struct irq_desc *irq_to_desc_alloc_cpu(unsigned int irq, int cpu);
 
 static inline struct irq_desc *
 irq_remap_to_desc(unsigned int irq, struct irq_desc *desc)
Index: b/include/linux/irqnr.h
===================================================================
--- a/include/linux/irqnr.h
+++ b/include/linux/irqnr.h
@@ -15,20 +15,19 @@
 
 # define for_each_irq_desc_reverse(irq, desc)                          \
 	for (irq = nr_irqs - 1; irq >= 0; irq--)
-#else
+#else /* CONFIG_GENERIC_HARDIRQS */
 
 extern int nr_irqs;
+extern struct irq_desc *irq_to_desc(unsigned int irq);
 
-#ifndef CONFIG_SPARSE_IRQ
+# define for_each_irq_desc(irq, desc)					\
+	for (irq = 0, desc = irq_to_desc(irq); irq < nr_irqs;		\
+	     irq++, desc = irq_to_desc(irq))
+# define for_each_irq_desc_reverse(irq, desc)				\
+	for (irq = nr_irqs - 1, desc = irq_to_desc(irq); irq >= 0;	\
+	     irq--, desc = irq_to_desc(irq))
 
-struct irq_desc;
-# define for_each_irq_desc(irq, desc)		\
-	for (irq = 0, desc = irq_desc; irq < nr_irqs; irq++, desc++)
-# define for_each_irq_desc_reverse(irq, desc)                          \
-	for (irq = nr_irqs - 1, desc = irq_desc + (nr_irqs - 1);        \
-	    irq >= 0; irq--, desc--)
-#endif
-#endif
+#endif /* CONFIG_GENERIC_HARDIRQS */
 
 #define for_each_irq_nr(irq)                   \
        for (irq = 0; irq < nr_irqs; irq++)
Index: b/kernel/irq/handle.c
===================================================================
--- a/kernel/irq/handle.c
+++ b/kernel/irq/handle.c
@@ -203,7 +203,7 @@ out_unlock:
 	return desc;
 }
 
-#else
+#else /* !CONFIG_SPARSE_IRQ */
 
 struct irq_desc irq_desc[NR_IRQS] __cacheline_aligned_in_smp = {
 	[0 ... NR_IRQS-1] = {
@@ -218,7 +218,16 @@ struct irq_desc irq_desc[NR_IRQS] __cach
 	}
 };
 
-#endif
+struct irq_desc *irq_to_desc(unsigned int irq)
+{
+	return (irq < NR_IRQS) ? irq_desc + irq : NULL;
+}
+
+struct irq_desc *irq_to_desc_alloc_cpu(unsigned int irq, int cpu)
+{
+	return irq_to_desc(irq);
+}
+#endif /* !CONFIG_SPARSE_IRQ */
 
 /*
  * What should we do if we get a hw irq event on an illegal vector?

[PATCH for -tip 3/4] proc: remove ifdef CONFIG_SPARSE_IRQ from stat.c

[KOSAKI Motohiro]

Subject: [PATCH] proc: remove ifdef CONFIG_SPARSE_IRQ from stat.c
Impact: cleanup

introduce irq_inuse() macro and remove ifdef in stat.c

Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
CC: Yinghai Lu <yinghai@kernel.org>
CC: Ingo Molnar <mingo@elte.hu>
---
 fs/proc/stat.c        |    9 +++------
 include/linux/irqnr.h |    9 +++++++++
 2 files changed, 12 insertions(+), 6 deletions(-)

Index: b/fs/proc/stat.c
===================================================================
--- a/fs/proc/stat.c
+++ b/fs/proc/stat.c
@@ -9,6 +9,7 @@
 #include <linux/seq_file.h>
 #include <linux/slab.h>
 #include <linux/time.h>
+#include <linux/irqnr.h>
 #include <asm/cputime.h>
 
 #ifndef arch_irq_stat_cpu
@@ -45,10 +46,8 @@ static int show_stat(struct seq_file *p,
 		steal = cputime64_add(steal, kstat_cpu(i).cpustat.steal);
 		guest = cputime64_add(guest, kstat_cpu(i).cpustat.guest);
 		for_each_irq_nr(j) {
-#ifdef CONFIG_SPARSE_IRQ
-			if (!irq_to_desc(j))
+			if (!irq_inuse(j))
 				continue;
-#endif
 			sum += kstat_irqs_cpu(j, i);
 		}
 		sum += arch_irq_stat_cpu(i);
@@ -95,12 +94,10 @@ static int show_stat(struct seq_file *p,
 	/* sum again ? it could be updated? */
 	for_each_irq_nr(j) {
 		per_irq_sum = 0;
-#ifdef CONFIG_SPARSE_IRQ
-		if (!irq_to_desc(j)) {
+		if (!irq_inuse(j)) {
 			seq_printf(p, " %u", per_irq_sum);
 			continue;
 		}
-#endif
 		for_each_possible_cpu(i)
 			per_irq_sum += kstat_irqs_cpu(j, i);
 
Index: b/include/linux/irqnr.h
===================================================================
--- a/include/linux/irqnr.h
+++ b/include/linux/irqnr.h
@@ -15,6 +15,9 @@
 
 # define for_each_irq_desc_reverse(irq, desc)                          \
 	for (irq = nr_irqs - 1; irq >= 0; irq--)
+
+#define irq_inuse(irq) 1
+
 #else /* CONFIG_GENERIC_HARDIRQS */
 
 extern int nr_irqs;
@@ -27,6 +30,12 @@ extern struct irq_desc *irq_to_desc(unsi
 	for (irq = nr_irqs - 1, desc = irq_to_desc(irq); irq >= 0;	\
 	     irq--, desc = irq_to_desc(irq))
 
+#ifdef CONFIG_SPARSE_IRQ
+#define irq_inuse(irq) (!!irq_to_desc(irq))
+#else
+#define irq_inuse(irq) 1
+#endif
+
 #endif /* CONFIG_GENERIC_HARDIRQS */
 
 #define for_each_irq_nr(irq)                   \

Re: [PATCH for -tip 3/4] proc: remove ifdef CONFIG_SPARSE_IRQ from stat.c

[Yinghai Lu]

On Thu, Dec 25, 2008 at 7:28 PM, KOSAKI Motohiro
<kosaki.motohiro@jp.fujitsu.com> wrote:
> Subject: [PATCH] proc: remove ifdef CONFIG_SPARSE_IRQ from stat.c
> Impact: cleanup
>
> introduce irq_inuse() macro and remove ifdef in stat.c

should have a good name... irq_inuse is some confusing.

YH

Re: [PATCH for -tip 3/4] proc: remove ifdef CONFIG_SPARSE_IRQ from stat.c

[KOSAKI Motohiro]

> > Subject: [PATCH] proc: remove ifdef CONFIG_SPARSE_IRQ from stat.c
> > Impact: cleanup
> >
> > introduce irq_inuse() macro and remove ifdef in stat.c
> 
> should have a good name... irq_inuse is some confusing.

Why?
May I ask your perfered name?

Re: [PATCH for -tip 3/4] proc: remove ifdef CONFIG_SPARSE_IRQ from stat.c

[Yinghai Lu]

On Thu, Dec 25, 2008 at 8:21 PM, KOSAKI Motohiro
<kosaki.motohiro@jp.fujitsu.com> wrote:
>> > Subject: [PATCH] proc: remove ifdef CONFIG_SPARSE_IRQ from stat.c
>> > Impact: cleanup
>> >
>> > introduce irq_inuse() macro and remove ifdef in stat.c
>>
>> should have a good name... irq_inuse is some confusing.
>
> Why?
> May I ask your perfered name?

after freeing msi with dynamic_irq_cleanup(), that irq_desc is not used.

YH

Re: [PATCH for -tip 3/4] proc: remove ifdef CONFIG_SPARSE_IRQ from stat.c

[KOSAKI Motohiro]

> On Thu, Dec 25, 2008 at 8:21 PM, KOSAKI Motohiro
> <kosaki.motohiro@jp.fujitsu.com> wrote:
> >> > Subject: [PATCH] proc: remove ifdef CONFIG_SPARSE_IRQ from stat.c
> >> > Impact: cleanup
> >> >
> >> > introduce irq_inuse() macro and remove ifdef in stat.c
> >>
> >> should have a good name... irq_inuse is some confusing.
> >
> > Why?
> > May I ask your perfered name?
> 
> after freeing msi with dynamic_irq_cleanup(), that irq_desc is not used.

hm, instead, How about following patch?



=======
Subject: [PATCH] proc: remove ifdef CONFIG_SPARSE_IRQ from stat.c
Impact: cleanup

irq_desc can be NULL when CONFIG_SPARSE_IRQ=y only.
therefore, NULL checking can move into kstat_irqs_cpu() of SPARSE_IRQ version.


Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
---
 fs/proc/stat.c      |   11 +----------
 kernel/irq/handle.c |    2 +-
 2 files changed, 2 insertions(+), 11 deletions(-)

Index: b/fs/proc/stat.c
===================================================================
--- a/fs/proc/stat.c
+++ b/fs/proc/stat.c
@@ -9,6 +9,7 @@
 #include <linux/seq_file.h>
 #include <linux/slab.h>
 #include <linux/time.h>
+#include <linux/irqnr.h>
 #include <asm/cputime.h>
 
 #ifndef arch_irq_stat_cpu
@@ -45,10 +46,6 @@ static int show_stat(struct seq_file *p,
 		steal = cputime64_add(steal, kstat_cpu(i).cpustat.steal);
 		guest = cputime64_add(guest, kstat_cpu(i).cpustat.guest);
 		for_each_irq_nr(j) {
-#ifdef CONFIG_SPARSE_IRQ
-			if (!irq_to_desc(j))
-				continue;
-#endif
 			sum += kstat_irqs_cpu(j, i);
 		}
 		sum += arch_irq_stat_cpu(i);
@@ -95,12 +92,6 @@ static int show_stat(struct seq_file *p,
 	/* sum again ? it could be updated? */
 	for_each_irq_nr(j) {
 		per_irq_sum = 0;
-#ifdef CONFIG_SPARSE_IRQ
-		if (!irq_to_desc(j)) {
-			seq_printf(p, " %u", per_irq_sum);
-			continue;
-		}
-#endif
 		for_each_possible_cpu(i)
 			per_irq_sum += kstat_irqs_cpu(j, i);
 
Index: b/kernel/irq/handle.c
===================================================================
--- a/kernel/irq/handle.c
+++ b/kernel/irq/handle.c
@@ -445,7 +445,7 @@ void early_init_irq_lock_class(void)
 unsigned int kstat_irqs_cpu(unsigned int irq, int cpu)
 {
 	struct irq_desc *desc = irq_to_desc(irq);
-	return desc->kstat_irqs[cpu];
+	return desc ? desc->kstat_irqs[cpu] : 0;
 }
 #endif
 EXPORT_SYMBOL(kstat_irqs_cpu);

Re: [PATCH for -tip 3/4] proc: remove ifdef CONFIG_SPARSE_IRQ from stat.c

[Yinghai Lu]

On Thu, Dec 25, 2008 at 9:24 PM, KOSAKI Motohiro
<kosaki.motohiro@jp.fujitsu.com> wrote:
>> On Thu, Dec 25, 2008 at 8:21 PM, KOSAKI Motohiro
>> <kosaki.motohiro@jp.fujitsu.com> wrote:
>> >> > Subject: [PATCH] proc: remove ifdef CONFIG_SPARSE_IRQ from stat.c
>> >> > Impact: cleanup
>> >> >
>> >> > introduce irq_inuse() macro and remove ifdef in stat.c
>> >>
>> >> should have a good name... irq_inuse is some confusing.
>> >
>> > Why?
>> > May I ask your perfered name?
>>
>> after freeing msi with dynamic_irq_cleanup(), that irq_desc is not used.
>
> hm, instead, How about following patch?
>
>
>
> =======
> Subject: [PATCH] proc: remove ifdef CONFIG_SPARSE_IRQ from stat.c
> Impact: cleanup
>
> irq_desc can be NULL when CONFIG_SPARSE_IRQ=y only.
> therefore, NULL checking can move into kstat_irqs_cpu() of SPARSE_IRQ version.
>
>
> Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> ---
>  fs/proc/stat.c      |   11 +----------
>  kernel/irq/handle.c |    2 +-
>  2 files changed, 2 insertions(+), 11 deletions(-)
>
> Index: b/fs/proc/stat.c
> ===================================================================
> --- a/fs/proc/stat.c
> +++ b/fs/proc/stat.c
> @@ -9,6 +9,7 @@
>  #include <linux/seq_file.h>
>  #include <linux/slab.h>
>  #include <linux/time.h>
> +#include <linux/irqnr.h>
>  #include <asm/cputime.h>
>
>  #ifndef arch_irq_stat_cpu
> @@ -45,10 +46,6 @@ static int show_stat(struct seq_file *p,
>                steal = cputime64_add(steal, kstat_cpu(i).cpustat.steal);
>                guest = cputime64_add(guest, kstat_cpu(i).cpustat.guest);
>                for_each_irq_nr(j) {
> -#ifdef CONFIG_SPARSE_IRQ
> -                       if (!irq_to_desc(j))
> -                               continue;
> -#endif
>                        sum += kstat_irqs_cpu(j, i);
>                }
>                sum += arch_irq_stat_cpu(i);
> @@ -95,12 +92,6 @@ static int show_stat(struct seq_file *p,
>        /* sum again ? it could be updated? */
>        for_each_irq_nr(j) {
>                per_irq_sum = 0;
> -#ifdef CONFIG_SPARSE_IRQ
> -               if (!irq_to_desc(j)) {
> -                       seq_printf(p, " %u", per_irq_sum);
> -                       continue;
> -               }
> -#endif
>                for_each_possible_cpu(i)
>                        per_irq_sum += kstat_irqs_cpu(j, i);
>
> Index: b/kernel/irq/handle.c
> ===================================================================
> --- a/kernel/irq/handle.c
> +++ b/kernel/irq/handle.c
> @@ -445,7 +445,7 @@ void early_init_irq_lock_class(void)
>  unsigned int kstat_irqs_cpu(unsigned int irq, int cpu)
>  {
>        struct irq_desc *desc = irq_to_desc(irq);
> -       return desc->kstat_irqs[cpu];
> +       return desc ? desc->kstat_irqs[cpu] : 0;
>  }
>  #endif
>  EXPORT_SYMBOL(kstat_irqs_cpu);
>
>

nice. much clean.

YH

Re: [PATCH for -tip 3/4] proc: remove ifdef CONFIG_SPARSE_IRQ from stat.c

[Ingo Molnar]

* Yinghai Lu <yinghai@kernel.org> wrote:

> > hm, instead, How about following patch?
> >
> > =======
> > Subject: [PATCH] proc: remove ifdef CONFIG_SPARSE_IRQ from stat.c
> > Impact: cleanup

> nice. much clean.

applied all four patches to tip/irq/sparseirq:

 18eefed: irq: simplify for_each_irq_desc() usage
 26ddd8d: proc: remove ifdef CONFIG_SPARSE_IRQ from stat.c
 f9af0e7: irq: for_each_irq_desc() move to irqnr.h
 51bc39f: hrtimer: remove #include <linux/irq.h>

the whole series is very nice and removes quite a bit of irq_desc usage 
complexity. Thanks guys,

	Ingo

[PATCH for -tip 4/4] irq: for_each_irq_desc() makes simplify

[KOSAKI Motohiro]

Subject: [PATCH] irq: for_each_irq_desc() makes simplify
Impact: cleanup

all for_each_irq_desc() usage point have !desc check.
then its check can move into for_each_irq_desc() macro.


Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
CC: Yinghai Lu <yinghai@kernel.org>
CC: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/kernel/io_apic.c |   10 ----------
 drivers/xen/events.c      |    3 ---
 include/linux/irqnr.h     |    8 ++++++--
 kernel/irq/autoprobe.c    |   15 ---------------
 kernel/irq/handle.c       |    3 ---
 kernel/irq/spurious.c     |    5 -----
 6 files changed, 6 insertions(+), 38 deletions(-)

Index: b/arch/x86/kernel/io_apic.c
===================================================================
--- a/arch/x86/kernel/io_apic.c
+++ b/arch/x86/kernel/io_apic.c
@@ -1400,8 +1400,6 @@ void __setup_vector_irq(int cpu)
 
 	/* Mark the inuse vectors */
 	for_each_irq_desc(irq, desc) {
-		if (!desc)
-			continue;
 		cfg = desc->chip_data;
 		if (!cpumask_test_cpu(cpu, cfg->domain))
 			continue;
@@ -1783,8 +1781,6 @@ __apicdebuginit(void) print_IO_APIC(void
 	for_each_irq_desc(irq, desc) {
 		struct irq_pin_list *entry;
 
-		if (!desc)
-			continue;
 		cfg = desc->chip_data;
 		entry = cfg->irq_2_pin;
 		if (!entry)
@@ -2425,9 +2421,6 @@ static void ir_irq_migration(struct work
 	struct irq_desc *desc;
 
 	for_each_irq_desc(irq, desc) {
-		if (!desc)
-			continue;
-
 		if (desc->status & IRQ_MOVE_PENDING) {
 			unsigned long flags;
 
@@ -2713,9 +2706,6 @@ static inline void init_IO_APIC_traps(vo
 	 * 0x80, because int 0x80 is hm, kind of importantish. ;)
 	 */
 	for_each_irq_desc(irq, desc) {
-		if (!desc)
-			continue;
-
 		cfg = desc->chip_data;
 		if (IO_APIC_IRQ(irq) && cfg && !cfg->vector) {
 			/*
Index: b/drivers/xen/events.c
===================================================================
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -142,9 +142,6 @@ static void init_evtchn_cpu_bindings(voi
 
 	/* By default all event channels notify CPU#0. */
 	for_each_irq_desc(i, desc) {
-		if (!desc)
-			continue;
-
 		desc->affinity = cpumask_of_cpu(0);
 	}
 #endif
Index: b/kernel/irq/autoprobe.c
===================================================================
--- a/kernel/irq/autoprobe.c
+++ b/kernel/irq/autoprobe.c
@@ -40,9 +40,6 @@ unsigned long probe_irq_on(void)
 	 * flush such a longstanding irq before considering it as spurious.
 	 */
 	for_each_irq_desc_reverse(i, desc) {
-		if (!desc)
-			continue;
-
 		spin_lock_irq(&desc->lock);
 		if (!desc->action && !(desc->status & IRQ_NOPROBE)) {
 			/*
@@ -71,9 +68,6 @@ unsigned long probe_irq_on(void)
 	 * happened in the previous stage, it may have masked itself)
 	 */
 	for_each_irq_desc_reverse(i, desc) {
-		if (!desc)
-			continue;
-
 		spin_lock_irq(&desc->lock);
 		if (!desc->action && !(desc->status & IRQ_NOPROBE)) {
 			desc->status |= IRQ_AUTODETECT | IRQ_WAITING;
@@ -92,9 +86,6 @@ unsigned long probe_irq_on(void)
 	 * Now filter out any obviously spurious interrupts
 	 */
 	for_each_irq_desc(i, desc) {
-		if (!desc)
-			continue;
-
 		spin_lock_irq(&desc->lock);
 		status = desc->status;
 
@@ -133,9 +124,6 @@ unsigned int probe_irq_mask(unsigned lon
 	int i;
 
 	for_each_irq_desc(i, desc) {
-		if (!desc)
-			continue;
-
 		spin_lock_irq(&desc->lock);
 		status = desc->status;
 
@@ -178,9 +166,6 @@ int probe_irq_off(unsigned long val)
 	unsigned int status;
 
 	for_each_irq_desc(i, desc) {
-		if (!desc)
-			continue;
-
 		spin_lock_irq(&desc->lock);
 		status = desc->status;
 
Index: b/kernel/irq/handle.c
===================================================================
--- a/kernel/irq/handle.c
+++ b/kernel/irq/handle.c
@@ -437,9 +437,6 @@ void early_init_irq_lock_class(void)
 	int i;
 
 	for_each_irq_desc(i, desc) {
-		if (!desc)
-			continue;
-
 		lockdep_set_class(&desc->lock, &irq_desc_lock_class);
 	}
 }
Index: b/kernel/irq/spurious.c
===================================================================
--- a/kernel/irq/spurious.c
+++ b/kernel/irq/spurious.c
@@ -91,9 +91,6 @@ static int misrouted_irq(int irq)
 	int i, ok = 0;
 
 	for_each_irq_desc(i, desc) {
-		if (!desc)
-			continue;
-
 		if (!i)
 			 continue;
 
@@ -115,8 +112,6 @@ static void poll_spurious_irqs(unsigned 
 	for_each_irq_desc(i, desc) {
 		unsigned int status;
 
-		if (!desc)
-			continue;
 		if (!i)
 			 continue;
 
Index: b/include/linux/irqnr.h
===================================================================
--- a/include/linux/irqnr.h
+++ b/include/linux/irqnr.h
@@ -25,10 +25,14 @@ extern struct irq_desc *irq_to_desc(unsi
 
 # define for_each_irq_desc(irq, desc)					\
 	for (irq = 0, desc = irq_to_desc(irq); irq < nr_irqs;		\
-	     irq++, desc = irq_to_desc(irq))
+	     irq++, desc = irq_to_desc(irq))				\
+		if (desc)
+
+
 # define for_each_irq_desc_reverse(irq, desc)				\
 	for (irq = nr_irqs - 1, desc = irq_to_desc(irq); irq >= 0;	\
-	     irq--, desc = irq_to_desc(irq))
+	     irq--, desc = irq_to_desc(irq))				\
+		if (desc)
 
 #ifdef CONFIG_SPARSE_IRQ
 #define irq_inuse(irq) (!!irq_to_desc(irq))

Re: [PATCH for -tip 4/4] irq: for_each_irq_desc() makes simplify

[Yinghai Lu]

On Thu, Dec 25, 2008 at 7:29 PM, KOSAKI Motohiro
<kosaki.motohiro@jp.fujitsu.com> wrote:
> Subject: [PATCH] irq: for_each_irq_desc() makes simplify
> Impact: cleanup
>
> all for_each_irq_desc() usage point have !desc check.
> then its check can move into for_each_irq_desc() macro.
>
>
> Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> CC: Yinghai Lu <yinghai@kernel.org>
> CC: Ingo Molnar <mingo@elte.hu>
> ---
>  arch/x86/kernel/io_apic.c |   10 ----------
>  drivers/xen/events.c      |    3 ---
>  include/linux/irqnr.h     |    8 ++++++--
>  kernel/irq/autoprobe.c    |   15 ---------------
>  kernel/irq/handle.c       |    3 ---
>  kernel/irq/spurious.c     |    5 -----
>  6 files changed, 6 insertions(+), 38 deletions(-)
>
> Index: b/arch/x86/kernel/io_apic.c
> ===================================================================
> --- a/arch/x86/kernel/io_apic.c
> +++ b/arch/x86/kernel/io_apic.c
> @@ -1400,8 +1400,6 @@ void __setup_vector_irq(int cpu)
>
>        /* Mark the inuse vectors */
>        for_each_irq_desc(irq, desc) {
> -               if (!desc)
> -                       continue;
>                cfg = desc->chip_data;
>                if (!cpumask_test_cpu(cpu, cfg->domain))
>                        continue;
..
> Index: b/include/linux/irqnr.h
> ===================================================================
> --- a/include/linux/irqnr.h
> +++ b/include/linux/irqnr.h
> @@ -25,10 +25,14 @@ extern struct irq_desc *irq_to_desc(unsi
>
>  # define for_each_irq_desc(irq, desc)                                  \
>        for (irq = 0, desc = irq_to_desc(irq); irq < nr_irqs;           \
> -            irq++, desc = irq_to_desc(irq))
> +            irq++, desc = irq_to_desc(irq))                            \
> +               if (desc)
> +
> +

looks good.

YH

Re: [PATCH for -tip 4/4] irq: for_each_irq_desc() makes simplify

[Raja R Harinath]

Hi,

KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> writes:

> Subject: [PATCH] irq: for_each_irq_desc() makes simplify
> Impact: cleanup
>
> all for_each_irq_desc() usage point have !desc check.
> then its check can move into for_each_irq_desc() macro.
[snip]
> Index: b/include/linux/irqnr.h
> ===================================================================
> --- a/include/linux/irqnr.h
> +++ b/include/linux/irqnr.h
> @@ -25,10 +25,14 @@ extern struct irq_desc *irq_to_desc(unsi
>  
>  # define for_each_irq_desc(irq, desc)					\
>  	for (irq = 0, desc = irq_to_desc(irq); irq < nr_irqs;		\
> -	     irq++, desc = irq_to_desc(irq))
> +	     irq++, desc = irq_to_desc(irq))				\
> +		if (desc)
> +
> +
>  # define for_each_irq_desc_reverse(irq, desc)				\
>  	for (irq = nr_irqs - 1, desc = irq_to_desc(irq); irq >= 0;	\
> -	     irq--, desc = irq_to_desc(irq))
> +	     irq--, desc = irq_to_desc(irq))				\
> +		if (desc)

I know this has gone in, but isn't this naked 'if' unsafe.  Consider the
following hypothetical code:

  if (safe)
    for_each_irq_desc(irq, desc) {
       ...
    }
  else
    panic();

With the macro definition above, the loop would panic() each time !desc,
and _not_ panic() when !safe.  I'd consider this behaviour to be
unexpected, to say the least :-)

The fix is to change the

  if (desc)

in the macro to

  if (!desc) ; else

- Hari

Re: [PATCH for -tip 4/4] irq: for_each_irq_desc() makes simplify

[KOSAKI Motohiro]

Hi

>>  # define for_each_irq_desc(irq, desc)                                        \
>>       for (irq = 0, desc = irq_to_desc(irq); irq < nr_irqs;           \
>> -          irq++, desc = irq_to_desc(irq))
>> +          irq++, desc = irq_to_desc(irq))                            \
>> +             if (desc)
>> +
>> +
>>  # define for_each_irq_desc_reverse(irq, desc)                                \
>>       for (irq = nr_irqs - 1, desc = irq_to_desc(irq); irq >= 0;      \
>> -          irq--, desc = irq_to_desc(irq))
>> +          irq--, desc = irq_to_desc(irq))                            \
>> +             if (desc)
>
> I know this has gone in, but isn't this naked 'if' unsafe.  Consider the
> following hypothetical code:
>
>  if (safe)
>    for_each_irq_desc(irq, desc) {
>       ...
>    }
>  else
>    panic();
>
> With the macro definition above, the loop would panic() each time !desc,
> and _not_ panic() when !safe.  I'd consider this behaviour to be
> unexpected, to say the least :-)

Correct.

> The fix is to change the
>
>  if (desc)
>
> in the macro to
>
>  if (!desc) ; else

Ok. I'll do that.
Very thanks for good reviewing.

btw, actually current kernel aready have similar macros.
e.g.

#define for_each_node_with_cpus(node)                   \
        for_each_online_node(node)                      \
                if (nr_cpus_node(node))


Shoud we fixed it too? ;-)

Re: [PATCH for -tip 4/4] irq: for_each_irq_desc() makes simplify

[KOSAKI Motohiro]

> Hi
> 
> >>  # define for_each_irq_desc(irq, desc)                                        \
> >>       for (irq = 0, desc = irq_to_desc(irq); irq < nr_irqs;           \
> >> -          irq++, desc = irq_to_desc(irq))
> >> +          irq++, desc = irq_to_desc(irq))                            \
> >> +             if (desc)
> >> +
> >> +
> >>  # define for_each_irq_desc_reverse(irq, desc)                                \
> >>       for (irq = nr_irqs - 1, desc = irq_to_desc(irq); irq >= 0;      \
> >> -          irq--, desc = irq_to_desc(irq))
> >> +          irq--, desc = irq_to_desc(irq))                            \
> >> +             if (desc)
> >
> > I know this has gone in, but isn't this naked 'if' unsafe.  Consider the
> > following hypothetical code:
> >
> >  if (safe)
> >    for_each_irq_desc(irq, desc) {
> >       ...
> >    }
> >  else
> >    panic();
> >
> > With the macro definition above, the loop would panic() each time !desc,
> > and _not_ panic() when !safe.  I'd consider this behaviour to be
> > unexpected, to say the least :-)
> 
> Correct.
> 
> > The fix is to change the
> >
> >  if (desc)
> >
> > in the macro to
> >
> >  if (!desc) ; else
> 
> Ok. I'll do that.
> Very thanks for good reviewing.

Done.
Thnaks Raja.

Ingo, could you please apply this patch to -tip tree?


===
>From 6f1210e6912afd8ca07518e82f89ba92137302b2 Mon Sep 17 00:00:00 2001
From: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Date: Sun, 4 Jan 2009 02:37:26 +0900
Subject: [PATCH] sparseirq: fix for_each_irq_desc() nit

Raja reported for_each_irq_desc() has possibility unsafeness.
if anyone write folliwing code, for_each_irq_desc() doesn't works intetionally.
(Now, its code doesn't exist at all)

 if (safe)
   for_each_irq_desc(irq, desc) {
      ...
   }
 else
   panic();


Reported-by: Raja R Harinath <harinath@hurrynot.org>
Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
CC: Ingo Molnar <mingo@elte.hu>
CC: Yinghai Lu <yinghai@kernel.org>
---
 include/linux/irqnr.h |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/include/linux/irqnr.h b/include/linux/irqnr.h
index 5504a5c..99b91c6 100644
--- a/include/linux/irqnr.h
+++ b/include/linux/irqnr.h
@@ -23,13 +23,17 @@ extern struct irq_desc *irq_to_desc(unsigned int irq);
 # define for_each_irq_desc(irq, desc)					\
 	for (irq = 0, desc = irq_to_desc(irq); irq < nr_irqs;		\
 	     irq++, desc = irq_to_desc(irq))				\
-		if (desc)
+		if (!desc)						\
+			;						\
+		else
 
 
 # define for_each_irq_desc_reverse(irq, desc)				\
 	for (irq = nr_irqs - 1, desc = irq_to_desc(irq); irq >= 0;	\
 	     irq--, desc = irq_to_desc(irq))				\
-		if (desc)
+		if (!desc)						\
+			;						\
+		else
 
 #endif /* CONFIG_GENERIC_HARDIRQS */
 
-- 
1.5.3

Re: [PATCH for -tip 4/4] irq: for_each_irq_desc() makes simplify

[Ingo Molnar]

* KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:

> Ingo, could you please apply this patch to -tip tree?
> 
> 
> ===
> From 6f1210e6912afd8ca07518e82f89ba92137302b2 Mon Sep 17 00:00:00 2001
> From: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> Date: Sun, 4 Jan 2009 02:37:26 +0900
> Subject: [PATCH] sparseirq: fix for_each_irq_desc() nit

applied to tip/irq/urgent, thanks!

	Ingo