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.
<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 <[email protected]>
CC: Thomas Gleixner <[email protected]>
CC: Ingo Molnar <[email protected]>
---
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>
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 <[email protected]>
CC: Yinghai Lu <[email protected]>
CC: Ingo Molnar <[email protected]>
---
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?
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 <[email protected]>
CC: Yinghai Lu <[email protected]>
CC: Ingo Molnar <[email protected]>
---
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) \
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 <[email protected]>
CC: Yinghai Lu <[email protected]>
CC: Ingo Molnar <[email protected]>
---
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))
On Thu, Dec 25, 2008 at 7:28 PM, KOSAKI Motohiro
<[email protected]> 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
> > 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?
On Thu, Dec 25, 2008 at 7:29 PM, KOSAKI Motohiro
<[email protected]> 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 <[email protected]>
> CC: Yinghai Lu <[email protected]>
> CC: Ingo Molnar <[email protected]>
> ---
> 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
On Thu, Dec 25, 2008 at 8:21 PM, KOSAKI Motohiro
<[email protected]> 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
> On Thu, Dec 25, 2008 at 8:21 PM, KOSAKI Motohiro
> <[email protected]> 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 <[email protected]>
---
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);
On Thu, Dec 25, 2008 at 9:24 PM, KOSAKI Motohiro
<[email protected]> wrote:
>> On Thu, Dec 25, 2008 at 8:21 PM, KOSAKI Motohiro
>> <[email protected]> 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 <[email protected]>
> ---
> 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
* Yinghai Lu <[email protected]> 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
Hi,
KOSAKI Motohiro <[email protected]> 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
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? ;-)
> 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 <[email protected]>
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 <[email protected]>
Signed-off-by: KOSAKI Motohiro <[email protected]>
CC: Ingo Molnar <[email protected]>
CC: Yinghai Lu <[email protected]>
---
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
* KOSAKI Motohiro <[email protected]> wrote:
> Ingo, could you please apply this patch to -tip tree?
>
>
> ===
> From 6f1210e6912afd8ca07518e82f89ba92137302b2 Mon Sep 17 00:00:00 2001
> From: KOSAKI Motohiro <[email protected]>
> 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