[PATCH] x86: io-apic - get rid of __DO_ACTION macro

[PATCH] x86: io-apic - get rid of __DO_ACTION macro

[Cyrill Gorcunov]

Replace __DO_ACTION macro with io_apic_modify_irq function.
This allow us to 'grep' definitions being hided by
the macro, ie the following:

	__unmask_IO_APIC_irq
	__mask_IO_APIC_irq
	__mask_and_edge_IO_APIC_irq
	__unmask_and_level_IO_APIC_irq

Also I removed R parameter which was to define offset inside
route table but always being zero for now and I don't think
if it ever will be other then that.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---

Please review. This modification doesn't map object file into the
same former state 'cause of 'for(entry)' cycle form changed and
new mask 'mask_and_not' added to be able to fill register with
pure zeros - ie clear register completely.

Any comments are quite welcome!

Index: linux-2.6.git/arch/x86/kernel/io_apic.c
===================================================================
--- linux-2.6.git.orig/arch/x86/kernel/io_apic.c	2008-09-09 22:27:57.000000000 +0400
+++ linux-2.6.git/arch/x86/kernel/io_apic.c	2008-09-09 22:34:03.000000000 +0400
@@ -643,65 +643,66 @@ static void __init replace_pin_at_irq(un
 		add_pin_to_irq(irq, newapic, newpin);
 }
 
-#define __DO_ACTION(R, ACTION_ENABLE, ACTION_DISABLE, FINAL)		\
-									\
-{									\
-	int pin;							\
-	struct irq_cfg *cfg;						\
-	struct irq_pin_list *entry;					\
-									\
-	cfg = irq_cfg(irq);						\
-	entry = cfg->irq_2_pin;						\
-	for (;;) {							\
-		unsigned int reg;					\
-		if (!entry)						\
-			break;						\
-		pin = entry->pin;					\
-		reg = io_apic_read(entry->apic, 0x10 + R + pin*2);	\
-		reg ACTION_DISABLE;					\
-		reg ACTION_ENABLE;					\
-		io_apic_modify(entry->apic, 0x10 + R + pin*2, reg);	\
-		FINAL;							\
-		if (!entry->next)					\
-			break;						\
-		entry = entry->next;					\
-	}								\
-}
-
-#define DO_ACTION(name,R, ACTION_ENABLE, ACTION_DISABLE, FINAL)		\
-									\
-	static void name##_IO_APIC_irq (unsigned int irq)		\
-	__DO_ACTION(R, ACTION_ENABLE, ACTION_DISABLE, FINAL)
+static inline void io_apic_modify_irq(unsigned int irq,
+				int mask_and, int mask_or,
+				int mask_and_not, int read_after_mod)
+{
+	int pin;
+	struct irq_cfg *cfg;
+	struct irq_pin_list *entry;
 
-/* mask = 0 */
-DO_ACTION(__unmask,	0, |= 0, &= ~IO_APIC_REDIR_MASKED, )
+	cfg = irq_cfg(irq);
+	for (entry = cfg->irq_2_pin; entry != NULL; entry = entry->next) {
+		unsigned int reg;
+		pin = entry->pin;
+		reg = io_apic_read(entry->apic, 0x10 + pin * 2);
+		if (mask_and)
+			reg &= mask_and;
+		if (mask_or)
+			reg |= mask_or;
+		if (mask_and_not)
+			reg &= !mask_and_not;
+		io_apic_modify(entry->apic, 0x10 + pin * 2, reg);
+		if (read_after_mod) {
+			/*
+			 * Synchronize the IO-APIC and the CPU by doing
+			 * a dummy read from the IO-APIC
+			 */
+			struct io_apic __iomem *io_apic;
+			io_apic = io_apic_base(entry->apic);
+			readl(&io_apic->data);
+		}
+	}
+}
 
-#ifdef CONFIG_X86_64
-/*
- * Synchronize the IO-APIC and the CPU by doing
- * a dummy read from the IO-APIC
- */
-static inline void io_apic_sync(unsigned int apic)
+static void __unmask_IO_APIC_irq(unsigned int irq)
 {
-	struct io_apic __iomem *io_apic = io_apic_base(apic);
-	readl(&io_apic->data);
+	io_apic_modify_irq(irq, ~IO_APIC_REDIR_MASKED, 0, 0, 0);
 }
 
-/* mask = 1 */
-DO_ACTION(__mask,	0, |= IO_APIC_REDIR_MASKED, &= ~0, io_apic_sync(entry->apic))
-
-#else
-
-/* mask = 1 */
-DO_ACTION(__mask,	0, |= IO_APIC_REDIR_MASKED, &= ~0, )
-
-/* mask = 1, trigger = 0 */
-DO_ACTION(__mask_and_edge, 0, |= IO_APIC_REDIR_MASKED, &= ~IO_APIC_REDIR_LEVEL_TRIGGER, )
+#ifdef CONFIG_X86_64
+static void __mask_IO_APIC_irq(unsigned int irq)
+{
+	io_apic_modify_irq(irq, ~0, IO_APIC_REDIR_MASKED, 0, 1);
+}
+#else /* CONFIG_X86_32 */
+static void __mask_IO_APIC_irq(unsigned int irq)
+{
+	io_apic_modify_irq(irq, ~0, IO_APIC_REDIR_MASKED, 0, 0);
+}
 
-/* mask = 0, trigger = 1 */
-DO_ACTION(__unmask_and_level, 0, |= IO_APIC_REDIR_LEVEL_TRIGGER, &= ~IO_APIC_REDIR_MASKED, )
+static void __mask_and_edge_IO_APIC_irq(unsigned int irq)
+{
+	io_apic_modify_irq(irq, ~IO_APIC_REDIR_LEVEL_TRIGGER,
+			IO_APIC_REDIR_MASKED, 0, 0);
+}
 
-#endif
+static void __unmask_and_level_IO_APIC_irq(unsigned int irq)
+{
+	io_apic_modify_irq(irq, ~IO_APIC_REDIR_MASKED,
+			IO_APIC_REDIR_LEVEL_TRIGGER, 0, 0);
+}
+#endif /* CONFIG_X86_32 */
 
 static void mask_IO_APIC_irq (unsigned int irq)
 {

Re: [PATCH] x86: io-apic - get rid of __DO_ACTION macro

[Yinghai Lu]

On Tue, Sep 9, 2008 at 11:46 AM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> Replace __DO_ACTION macro with io_apic_modify_irq function.
> This allow us to 'grep' definitions being hided by
> the macro, ie the following:
>
>        __unmask_IO_APIC_irq
>        __mask_IO_APIC_irq
>        __mask_and_edge_IO_APIC_irq
>        __unmask_and_level_IO_APIC_irq
>
> Also I removed R parameter which was to define offset inside
> route table but always being zero for now and I don't think
> if it ever will be other then that.
>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>
> Please review. This modification doesn't map object file into the
> same former state 'cause of 'for(entry)' cycle form changed and
> new mask 'mask_and_not' added to be able to fill register with
> pure zeros - ie clear register completely.
>
> Any comments are quite welcome!
>
> Index: linux-2.6.git/arch/x86/kernel/io_apic.c
> ===================================================================
> --- linux-2.6.git.orig/arch/x86/kernel/io_apic.c        2008-09-09 22:27:57.000000000 +0400
> +++ linux-2.6.git/arch/x86/kernel/io_apic.c     2008-09-09 22:34:03.000000000 +0400
> @@ -643,65 +643,66 @@ static void __init replace_pin_at_irq(un
>                add_pin_to_irq(irq, newapic, newpin);
>  }
>
> -#define __DO_ACTION(R, ACTION_ENABLE, ACTION_DISABLE, FINAL)           \
> -                                                                       \
> -{                                                                      \
> -       int pin;                                                        \
> -       struct irq_cfg *cfg;                                            \
> -       struct irq_pin_list *entry;                                     \
> -                                                                       \
> -       cfg = irq_cfg(irq);                                             \
> -       entry = cfg->irq_2_pin;                                         \
> -       for (;;) {                                                      \
> -               unsigned int reg;                                       \
> -               if (!entry)                                             \
> -                       break;                                          \
> -               pin = entry->pin;                                       \
> -               reg = io_apic_read(entry->apic, 0x10 + R + pin*2);      \
> -               reg ACTION_DISABLE;                                     \
> -               reg ACTION_ENABLE;                                      \
> -               io_apic_modify(entry->apic, 0x10 + R + pin*2, reg);     \
> -               FINAL;                                                  \
> -               if (!entry->next)                                       \
> -                       break;                                          \
> -               entry = entry->next;                                    \
> -       }                                                               \
> -}
> -
> -#define DO_ACTION(name,R, ACTION_ENABLE, ACTION_DISABLE, FINAL)                \
> -                                                                       \
> -       static void name##_IO_APIC_irq (unsigned int irq)               \
> -       __DO_ACTION(R, ACTION_ENABLE, ACTION_DISABLE, FINAL)
> +static inline void io_apic_modify_irq(unsigned int irq,
> +                               int mask_and, int mask_or,
> +                               int mask_and_not, int read_after_mod)
> +{
> +       int pin;
> +       struct irq_cfg *cfg;
> +       struct irq_pin_list *entry;
>
> -/* mask = 0 */
> -DO_ACTION(__unmask,    0, |= 0, &= ~IO_APIC_REDIR_MASKED, )
> +       cfg = irq_cfg(irq);
> +       for (entry = cfg->irq_2_pin; entry != NULL; entry = entry->next) {
> +               unsigned int reg;
> +               pin = entry->pin;
> +               reg = io_apic_read(entry->apic, 0x10 + pin * 2);
> +               if (mask_and)
> +                       reg &= mask_and;

if mask_and == 0, you don't need to clear it ?

> +               if (mask_or)
> +                       reg |= mask_or;
> +               if (mask_and_not)
> +                       reg &= !mask_and_not;

it seems you should use ~ instead of !

> +               io_apic_modify(entry->apic, 0x10 + pin * 2, reg);
> +               if (read_after_mod) {
> +                       /*
> +                        * Synchronize the IO-APIC and the CPU by doing
> +                        * a dummy read from the IO-APIC
> +                        */
> +                       struct io_apic __iomem *io_apic;
> +                       io_apic = io_apic_base(entry->apic);
> +                       readl(&io_apic->data);

hope we can keep using MACRO..

YH

Re: [PATCH] x86: io-apic - get rid of __DO_ACTION macro

[Cyrill Gorcunov]

Yinghai, the base idea with mask is to apply a mask only if you need
it. And having _and_ operation to be mandatory implemented is a
limitation and just not enough. So that is why i do check if mask is
defined (well it cold be not a good practice indeed). Thanks for
notice (and about binary operator too - will fix :)

Re: [PATCH] x86: io-apic - get rid of __DO_ACTION macro

[Cyrill Gorcunov]

On Wed, Sep 10, 2008 at 12:13 AM, Yinghai Lu <yhlu.kernel@gmail.com> wrote:
...
>
> hope we can keep using MACRO..
>
> YH
>

Btw, Yinghai, what does it mean? To not touch this macro at all?
Or you mean about implementation issue (ie the design itself)?

Re: [PATCH] x86: io-apic - get rid of __DO_ACTION macro

[Yinghai Lu]

On Tue, Sep 9, 2008 at 11:02 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> On Wed, Sep 10, 2008 at 12:13 AM, Yinghai Lu <yhlu.kernel@gmail.com> wrote:
> ...
>>
>> hope we can keep using MACRO..
>>
>> YH
>>
>
> Btw, Yinghai, what does it mean? To not touch this macro at all?
> Or you mean about implementation issue (ie the design itself)?

do not touch this macro... and may revisit after 2.6.28

YH

Re: [PATCH] x86: io-apic - get rid of __DO_ACTION macro

[Cyrill Gorcunov]

[Yinghai Lu - Tue, Sep 09, 2008 at 11:22:37PM -0700]
| On Tue, Sep 9, 2008 at 11:02 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| > On Wed, Sep 10, 2008 at 12:13 AM, Yinghai Lu <yhlu.kernel@gmail.com> wrote:
| > ...
| >>
| >> hope we can keep using MACRO..
| >>
| >> YH
| >>
| >
| > Btw, Yinghai, what does it mean? To not touch this macro at all?
| > Or you mean about implementation issue (ie the design itself)?
| 
| do not touch this macro... and may revisit after 2.6.28
| 
| YH
| 

could you please be more concrete? What will happen
with kernel after 2.6.28 that make it (this macro) possible
to be touched? The only problem I see now is that binary
operation error you pointed (thanks!) but nothing else!
actually this mask arguments are being applied in not
that obvious way indeed but I'm not sure if it'll be better
to add additional args signaling that a mask should be applied.
If _you_ are planning to continue playing with APIC/IO-APIC
'till 2.6.28 - just say that and I could stop any attempts.

		- Cyrill -

Re: [PATCH] x86: io-apic - get rid of __DO_ACTION macro

[Ingo Molnar]

* Yinghai Lu <yhlu.kernel@gmail.com> wrote:

> On Tue, Sep 9, 2008 at 11:02 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> > On Wed, Sep 10, 2008 at 12:13 AM, Yinghai Lu <yhlu.kernel@gmail.com> wrote:
> > ...
> >>
> >> hope we can keep using MACRO..
> >>
> >> YH
> >>
> >
> > Btw, Yinghai, what does it mean? To not touch this macro at all?
> > Or you mean about implementation issue (ie the design itself)?
> 
> do not touch this macro... and may revisit after 2.6.28

anything you are particularly worried about? Regressions we should be 
able to find pretty quickly, in a central macro like that - and the 
macro is quite ugly.

	Ingo

Re: [PATCH] x86: io-apic - get rid of __DO_ACTION macro

[Cyrill Gorcunov]

On Wed, Sep 10, 2008 at 1:31 PM, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Yinghai Lu <yhlu.kernel@gmail.com> wrote:
>
>> On Tue, Sep 9, 2008 at 11:02 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>> > On Wed, Sep 10, 2008 at 12:13 AM, Yinghai Lu <yhlu.kernel@gmail.com> wrote:
>> > ...
>> >>
>> >> hope we can keep using MACRO..
>> >>
>> >> YH
>> >>
>> >
>> > Btw, Yinghai, what does it mean? To not touch this macro at all?
>> > Or you mean about implementation issue (ie the design itself)?
>>
>> do not touch this macro... and may revisit after 2.6.28
>
> anything you are particularly worried about? Regressions we should be
> able to find pretty quickly, in a central macro like that - and the
> macro is quite ugly.
>
>        Ingo
>

Btw Ingo, don't apply this patch for now - I'll simplify it a bit.
I just overzealous with masks operation - 'and' and 'or' will
be more then enough without checking. I think I will do it today evening
so Yinghai could review it and say if he like it. Anyway the
patch could be just queued to satisfy any concerns :)

Re: [PATCH] x86: io-apic - get rid of __DO_ACTION macro

[Yinghai Lu]

On Wed, Sep 10, 2008 at 2:31 AM, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Yinghai Lu <yhlu.kernel@gmail.com> wrote:
>
>> On Tue, Sep 9, 2008 at 11:02 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>> > On Wed, Sep 10, 2008 at 12:13 AM, Yinghai Lu <yhlu.kernel@gmail.com> wrote:
>> > ...
>> >>
>> >> hope we can keep using MACRO..
>> >>
>> >> YH
>> >>
>> >
>> > Btw, Yinghai, what does it mean? To not touch this macro at all?
>> > Or you mean about implementation issue (ie the design itself)?
>>
>> do not touch this macro... and may revisit after 2.6.28
>
> anything you are particularly worried about? Regressions we should be
> able to find pretty quickly, in a central macro like that - and the
> macro is quite ugly.
>

ok, let remove unneeded "if", and use function pointer...

void (*extra_action_t)(struct irq_pin_list *entry);

+static inline void io_apic_modify_irq(unsigned int irq,
+                               int mask_and, int mask_or,
+                               int mask_and_not, extra_action_t action)
+{
+       int pin;
+       struct irq_cfg *cfg;
+       struct irq_pin_list *entry;
+       cfg = irq_cfg(irq);
+       for (entry = cfg->irq_2_pin; entry != NULL; entry = entry->next) {
+               unsigned int reg;
+               pin = entry->pin;
+               reg = io_apic_read(entry->apic, 0x10 + pin * 2);
+               reg &= mask_and;
+               reg |= mask_or;
+               reg &= ~mask_and_not;
+               io_apic_modify(entry->apic, 0x10 + pin * 2, reg);
+               if (action)
+                       action(entry);
+       }
+}

+void extra_read(struct irq_pin_list *entry)
+             {
+                       /*
+                        * Synchronize the IO-APIC and the CPU by doing
+                        * a dummy read from the IO-APIC
+                        */
+                       struct io_apic __iomem *io_apic;
+                       io_apic = io_apic_base(entry->apic);
+                       readl(&io_apic->data);
+               }


YH

Re: [PATCH] x86: io-apic - get rid of __DO_ACTION macro

[Cyrill Gorcunov]

[Yinghai Lu - Wed, Sep 10, 2008 at 10:30:39AM -0700]
| On Wed, Sep 10, 2008 at 2:31 AM, Ingo Molnar <mingo@elte.hu> wrote:
| >
| > * Yinghai Lu <yhlu.kernel@gmail.com> wrote:
| >
| >> On Tue, Sep 9, 2008 at 11:02 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| >> > On Wed, Sep 10, 2008 at 12:13 AM, Yinghai Lu <yhlu.kernel@gmail.com> wrote:
| >> > ...
| >> >>
| >> >> hope we can keep using MACRO..
| >> >>
| >> >> YH
| >> >>
| >> >
| >> > Btw, Yinghai, what does it mean? To not touch this macro at all?
| >> > Or you mean about implementation issue (ie the design itself)?
| >>
| >> do not touch this macro... and may revisit after 2.6.28
| >
| > anything you are particularly worried about? Regressions we should be
| > able to find pretty quickly, in a central macro like that - and the
| > macro is quite ugly.
| >
| 
| ok, let remove unneeded "if", and use function pointer...
| 
| void (*extra_action_t)(struct irq_pin_list *entry);
| 
| +static inline void io_apic_modify_irq(unsigned int irq,
| +                               int mask_and, int mask_or,
| +                               int mask_and_not, extra_action_t action)
| +{
| +       int pin;
| +       struct irq_cfg *cfg;
| +       struct irq_pin_list *entry;
| +       cfg = irq_cfg(irq);
| +       for (entry = cfg->irq_2_pin; entry != NULL; entry = entry->next) {
| +               unsigned int reg;
| +               pin = entry->pin;
| +               reg = io_apic_read(entry->apic, 0x10 + pin * 2);
| +               reg &= mask_and;
| +               reg |= mask_or;
| +               reg &= ~mask_and_not;
| +               io_apic_modify(entry->apic, 0x10 + pin * 2, reg);
| +               if (action)
| +                       action(entry);
| +       }
| +}
| 
| +void extra_read(struct irq_pin_list *entry)
| +             {
| +                       /*
| +                        * Synchronize the IO-APIC and the CPU by doing
| +                        * a dummy read from the IO-APIC
| +                        */
| +                       struct io_apic __iomem *io_apic;
| +                       io_apic = io_apic_base(entry->apic);
| +                       readl(&io_apic->data);
| +               }
| 
| 
| YH
| 

Yinghai, I've posted second version before you
proposed this. Let me update it then. (though
I don't think if we would need some extra actions
instead of syncs by additional read except for possible
erranious chips). Anyway - will repost updated version.
Thanks.

		- Cyrill -

Re: [PATCH] x86: io-apic - get rid of __DO_ACTION macro

[Cyrill Gorcunov]

[Yinghai Lu - Wed, Sep 10, 2008 at 10:30:39AM -0700]
| On Wed, Sep 10, 2008 at 2:31 AM, Ingo Molnar <mingo@elte.hu> wrote:
| >
| > * Yinghai Lu <yhlu.kernel@gmail.com> wrote:
| >
| >> On Tue, Sep 9, 2008 at 11:02 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| >> > On Wed, Sep 10, 2008 at 12:13 AM, Yinghai Lu <yhlu.kernel@gmail.com> wrote:
| >> > ...
| >> >>
| >> >> hope we can keep using MACRO..
| >> >>
| >> >> YH
| >> >>
| >> >
| >> > Btw, Yinghai, what does it mean? To not touch this macro at all?
| >> > Or you mean about implementation issue (ie the design itself)?
| >>
| >> do not touch this macro... and may revisit after 2.6.28
| >
| > anything you are particularly worried about? Regressions we should be
| > able to find pretty quickly, in a central macro like that - and the
| > macro is quite ugly.
| >
| 
| ok, let remove unneeded "if", and use function pointer...
| 
| void (*extra_action_t)(struct irq_pin_list *entry);
| 
| +static inline void io_apic_modify_irq(unsigned int irq,
| +                               int mask_and, int mask_or,
| +                               int mask_and_not, extra_action_t action)
| +{
| +       int pin;
| +       struct irq_cfg *cfg;
| +       struct irq_pin_list *entry;
| +       cfg = irq_cfg(irq);
| +       for (entry = cfg->irq_2_pin; entry != NULL; entry = entry->next) {
| +               unsigned int reg;
| +               pin = entry->pin;
| +               reg = io_apic_read(entry->apic, 0x10 + pin * 2);
| +               reg &= mask_and;
| +               reg |= mask_or;
| +               reg &= ~mask_and_not;
| +               io_apic_modify(entry->apic, 0x10 + pin * 2, reg);
| +               if (action)
| +                       action(entry);
| +       }
| +}
| 
| +void extra_read(struct irq_pin_list *entry)
| +             {
| +                       /*
| +                        * Synchronize the IO-APIC and the CPU by doing
| +                        * a dummy read from the IO-APIC
| +                        */
| +                       struct io_apic __iomem *io_apic;
| +                       io_apic = io_apic_base(entry->apic);
| +                       readl(&io_apic->data);
| +               }
| 
| 
| YH
| 

Yinghai, Ingo, what about this one?

		- Cyrill -
---
From: Cyrill Gorcunov <gorcunov@gmail.com>
Subject: x86: io-apic - get rid of __DO_ACTION macro v3

Replace __DO_ACTION macro with io_apic_modify_irq function.
This allow us to 'grep' definitions being hided by
__DO_ACTION macro:

	__unmask_IO_APIC_irq
	__mask_IO_APIC_irq
	__mask_and_edge_IO_APIC_irq
	__unmask_and_level_IO_APIC_irq

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
CC: Yinghai Lu <yhlu.kernel@gmail.com>
---

Index: linux-2.6.git/arch/x86/kernel/io_apic.c
===================================================================
--- linux-2.6.git.orig/arch/x86/kernel/io_apic.c	2008-09-09 22:27:57.000000000 +0400
+++ linux-2.6.git/arch/x86/kernel/io_apic.c	2008-09-10 22:17:25.000000000 +0400
@@ -643,65 +643,66 @@ static void __init replace_pin_at_irq(un
 		add_pin_to_irq(irq, newapic, newpin);
 }
 
-#define __DO_ACTION(R, ACTION_ENABLE, ACTION_DISABLE, FINAL)		\
-									\
-{									\
-	int pin;							\
-	struct irq_cfg *cfg;						\
-	struct irq_pin_list *entry;					\
-									\
-	cfg = irq_cfg(irq);						\
-	entry = cfg->irq_2_pin;						\
-	for (;;) {							\
-		unsigned int reg;					\
-		if (!entry)						\
-			break;						\
-		pin = entry->pin;					\
-		reg = io_apic_read(entry->apic, 0x10 + R + pin*2);	\
-		reg ACTION_DISABLE;					\
-		reg ACTION_ENABLE;					\
-		io_apic_modify(entry->apic, 0x10 + R + pin*2, reg);	\
-		FINAL;							\
-		if (!entry->next)					\
-			break;						\
-		entry = entry->next;					\
-	}								\
-}
-
-#define DO_ACTION(name,R, ACTION_ENABLE, ACTION_DISABLE, FINAL)		\
-									\
-	static void name##_IO_APIC_irq (unsigned int irq)		\
-	__DO_ACTION(R, ACTION_ENABLE, ACTION_DISABLE, FINAL)
+static inline void io_apic_modify_irq(unsigned int irq,
+				int mask_and, int mask_or,
+				void (*final)(struct irq_pin_list *entry))
+{
+	int pin;
+	struct irq_cfg *cfg;
+	struct irq_pin_list *entry;
 
-/* mask = 0 */
-DO_ACTION(__unmask,	0, |= 0, &= ~IO_APIC_REDIR_MASKED, )
+	cfg = irq_cfg(irq);
+	for (entry = cfg->irq_2_pin; entry != NULL; entry = entry->next) {
+		unsigned int reg;
+		pin = entry->pin;
+		reg = io_apic_read(entry->apic, 0x10 + pin * 2);
+		reg &= mask_and;
+		reg |= mask_or;
+		io_apic_modify(entry->apic, 0x10 + pin * 2, reg);
+		if (final)
+			final(entry);
+	}
+}
+
+static void __unmask_IO_APIC_irq(unsigned int irq)
+{
+	io_apic_modify_irq(irq, ~IO_APIC_REDIR_MASKED, 0, NULL);
+}
 
 #ifdef CONFIG_X86_64
-/*
- * Synchronize the IO-APIC and the CPU by doing
- * a dummy read from the IO-APIC
- */
-static inline void io_apic_sync(unsigned int apic)
+void io_apic_sync(struct irq_pin_list *entry)
 {
-	struct io_apic __iomem *io_apic = io_apic_base(apic);
+	/*
+	 * Synchronize the IO-APIC and the CPU by doing
+	 * a dummy read from the IO-APIC
+	 */
+	struct io_apic __iomem *io_apic;
+	io_apic = io_apic_base(entry->apic);
 	readl(&io_apic->data);
 }
 
-/* mask = 1 */
-DO_ACTION(__mask,	0, |= IO_APIC_REDIR_MASKED, &= ~0, io_apic_sync(entry->apic))
-
-#else
-
-/* mask = 1 */
-DO_ACTION(__mask,	0, |= IO_APIC_REDIR_MASKED, &= ~0, )
-
-/* mask = 1, trigger = 0 */
-DO_ACTION(__mask_and_edge, 0, |= IO_APIC_REDIR_MASKED, &= ~IO_APIC_REDIR_LEVEL_TRIGGER, )
+static void __mask_IO_APIC_irq(unsigned int irq)
+{
+	io_apic_modify_irq(irq, ~0, IO_APIC_REDIR_MASKED, &io_apic_sync);
+}
+#else /* CONFIG_X86_32 */
+static void __mask_IO_APIC_irq(unsigned int irq)
+{
+	io_apic_modify_irq(irq, ~0, IO_APIC_REDIR_MASKED, NULL);
+}
 
-/* mask = 0, trigger = 1 */
-DO_ACTION(__unmask_and_level, 0, |= IO_APIC_REDIR_LEVEL_TRIGGER, &= ~IO_APIC_REDIR_MASKED, )
+static void __mask_and_edge_IO_APIC_irq(unsigned int irq)
+{
+	io_apic_modify_irq(irq, ~IO_APIC_REDIR_LEVEL_TRIGGER,
+			IO_APIC_REDIR_MASKED, NULL);
+}
 
-#endif
+static void __unmask_and_level_IO_APIC_irq(unsigned int irq)
+{
+	io_apic_modify_irq(irq, ~IO_APIC_REDIR_MASKED,
+			IO_APIC_REDIR_LEVEL_TRIGGER, NULL);
+}
+#endif /* CONFIG_X86_32 */
 
 static void mask_IO_APIC_irq (unsigned int irq)
 {

Re: [PATCH] x86: io-apic - get rid of __DO_ACTION macro

[Yinghai Lu]

On Wed, Sep 10, 2008 at 11:19 AM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> [Yinghai Lu - Wed, Sep 10, 2008 at 10:30:39AM -0700]
> | On Wed, Sep 10, 2008 at 2:31 AM, Ingo Molnar <mingo@elte.hu> wrote:
> | >
> | > * Yinghai Lu <yhlu.kernel@gmail.com> wrote:
> | >
> | >> On Tue, Sep 9, 2008 at 11:02 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> | >> > On Wed, Sep 10, 2008 at 12:13 AM, Yinghai Lu <yhlu.kernel@gmail.com> wrote:
> | >> > ...
> | >> >>
> | >> >> hope we can keep using MACRO..
> | >> >>
> | >> >> YH
> | >> >>
> | >> >
> | >> > Btw, Yinghai, what does it mean? To not touch this macro at all?
> | >> > Or you mean about implementation issue (ie the design itself)?
> | >>
> | >> do not touch this macro... and may revisit after 2.6.28
> | >
> | > anything you are particularly worried about? Regressions we should be
> | > able to find pretty quickly, in a central macro like that - and the
> | > macro is quite ugly.
> | >
> |
> | ok, let remove unneeded "if", and use function pointer...
> |
> | void (*extra_action_t)(struct irq_pin_list *entry);
> |
> | +static inline void io_apic_modify_irq(unsigned int irq,
> | +                               int mask_and, int mask_or,
> | +                               int mask_and_not, extra_action_t action)
> | +{
> | +       int pin;
> | +       struct irq_cfg *cfg;
> | +       struct irq_pin_list *entry;
> | +       cfg = irq_cfg(irq);
> | +       for (entry = cfg->irq_2_pin; entry != NULL; entry = entry->next) {
> | +               unsigned int reg;
> | +               pin = entry->pin;
> | +               reg = io_apic_read(entry->apic, 0x10 + pin * 2);
> | +               reg &= mask_and;
> | +               reg |= mask_or;
> | +               reg &= ~mask_and_not;
> | +               io_apic_modify(entry->apic, 0x10 + pin * 2, reg);
> | +               if (action)
> | +                       action(entry);
> | +       }
> | +}
> |
> | +void extra_read(struct irq_pin_list *entry)
> | +             {
> | +                       /*
> | +                        * Synchronize the IO-APIC and the CPU by doing
> | +                        * a dummy read from the IO-APIC
> | +                        */
> | +                       struct io_apic __iomem *io_apic;
> | +                       io_apic = io_apic_base(entry->apic);
> | +                       readl(&io_apic->data);
> | +               }
> |
> |
> | YH
> |
>
> Yinghai, Ingo, what about this one?
>
>                - Cyrill -
> ---
> From: Cyrill Gorcunov <gorcunov@gmail.com>
> Subject: x86: io-apic - get rid of __DO_ACTION macro v3
>
> Replace __DO_ACTION macro with io_apic_modify_irq function.
> This allow us to 'grep' definitions being hided by
> __DO_ACTION macro:
>
>        __unmask_IO_APIC_irq
>        __mask_IO_APIC_irq
>        __mask_and_edge_IO_APIC_irq
>        __unmask_and_level_IO_APIC_irq
>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> CC: Yinghai Lu <yhlu.kernel@gmail.com>
> ---
>
> Index: linux-2.6.git/arch/x86/kernel/io_apic.c
> ===================================================================
> --- linux-2.6.git.orig/arch/x86/kernel/io_apic.c        2008-09-09 22:27:57.000000000 +0400
> +++ linux-2.6.git/arch/x86/kernel/io_apic.c     2008-09-10 22:17:25.000000000 +0400
> @@ -643,65 +643,66 @@ static void __init replace_pin_at_irq(un
>                add_pin_to_irq(irq, newapic, newpin);
>  }
>
> -#define __DO_ACTION(R, ACTION_ENABLE, ACTION_DISABLE, FINAL)           \
> -                                                                       \
> -{                                                                      \
> -       int pin;                                                        \
> -       struct irq_cfg *cfg;                                            \
> -       struct irq_pin_list *entry;                                     \
> -                                                                       \
> -       cfg = irq_cfg(irq);                                             \
> -       entry = cfg->irq_2_pin;                                         \
> -       for (;;) {                                                      \
> -               unsigned int reg;                                       \
> -               if (!entry)                                             \
> -                       break;                                          \
> -               pin = entry->pin;                                       \
> -               reg = io_apic_read(entry->apic, 0x10 + R + pin*2);      \
> -               reg ACTION_DISABLE;                                     \
> -               reg ACTION_ENABLE;                                      \
> -               io_apic_modify(entry->apic, 0x10 + R + pin*2, reg);     \
> -               FINAL;                                                  \
> -               if (!entry->next)                                       \
> -                       break;                                          \
> -               entry = entry->next;                                    \
> -       }                                                               \
> -}
> -
> -#define DO_ACTION(name,R, ACTION_ENABLE, ACTION_DISABLE, FINAL)                \
> -                                                                       \
> -       static void name##_IO_APIC_irq (unsigned int irq)               \
> -       __DO_ACTION(R, ACTION_ENABLE, ACTION_DISABLE, FINAL)
> +static inline void io_apic_modify_irq(unsigned int irq,
> +                               int mask_and, int mask_or,
> +                               void (*final)(struct irq_pin_list *entry))
> +{
> +       int pin;
> +       struct irq_cfg *cfg;
> +       struct irq_pin_list *entry;
>
> -/* mask = 0 */
> -DO_ACTION(__unmask,    0, |= 0, &= ~IO_APIC_REDIR_MASKED, )
> +       cfg = irq_cfg(irq);
> +       for (entry = cfg->irq_2_pin; entry != NULL; entry = entry->next) {
> +               unsigned int reg;
> +               pin = entry->pin;
> +               reg = io_apic_read(entry->apic, 0x10 + pin * 2);
> +               reg &= mask_and;
> +               reg |= mask_or;
> +               io_apic_modify(entry->apic, 0x10 + pin * 2, reg);
> +               if (final)
> +                       final(entry);
> +       }
> +}
> +
> +static void __unmask_IO_APIC_irq(unsigned int irq)
> +{
> +       io_apic_modify_irq(irq, ~IO_APIC_REDIR_MASKED, 0, NULL);
> +}
>
>  #ifdef CONFIG_X86_64
> -/*
> - * Synchronize the IO-APIC and the CPU by doing
> - * a dummy read from the IO-APIC
> - */
> -static inline void io_apic_sync(unsigned int apic)
> +void io_apic_sync(struct irq_pin_list *entry)
>  {
> -       struct io_apic __iomem *io_apic = io_apic_base(apic);
> +       /*
> +        * Synchronize the IO-APIC and the CPU by doing
> +        * a dummy read from the IO-APIC
> +        */
> +       struct io_apic __iomem *io_apic;
> +       io_apic = io_apic_base(entry->apic);
>        readl(&io_apic->data);
>  }
>
> -/* mask = 1 */
> -DO_ACTION(__mask,      0, |= IO_APIC_REDIR_MASKED, &= ~0, io_apic_sync(entry->apic))
> -
> -#else
> -
> -/* mask = 1 */
> -DO_ACTION(__mask,      0, |= IO_APIC_REDIR_MASKED, &= ~0, )
> -
> -/* mask = 1, trigger = 0 */
> -DO_ACTION(__mask_and_edge, 0, |= IO_APIC_REDIR_MASKED, &= ~IO_APIC_REDIR_LEVEL_TRIGGER, )
> +static void __mask_IO_APIC_irq(unsigned int irq)
> +{
> +       io_apic_modify_irq(irq, ~0, IO_APIC_REDIR_MASKED, &io_apic_sync);
> +}
> +#else /* CONFIG_X86_32 */
> +static void __mask_IO_APIC_irq(unsigned int irq)
> +{
> +       io_apic_modify_irq(irq, ~0, IO_APIC_REDIR_MASKED, NULL);
> +}
>
> -/* mask = 0, trigger = 1 */
> -DO_ACTION(__unmask_and_level, 0, |= IO_APIC_REDIR_LEVEL_TRIGGER, &= ~IO_APIC_REDIR_MASKED, )
> +static void __mask_and_edge_IO_APIC_irq(unsigned int irq)
> +{
> +       io_apic_modify_irq(irq, ~IO_APIC_REDIR_LEVEL_TRIGGER,
> +                       IO_APIC_REDIR_MASKED, NULL);
> +}
>
> -#endif
> +static void __unmask_and_level_IO_APIC_irq(unsigned int irq)
> +{
> +       io_apic_modify_irq(irq, ~IO_APIC_REDIR_MASKED,
> +                       IO_APIC_REDIR_LEVEL_TRIGGER, NULL);
> +}
> +#endif /* CONFIG_X86_32 */
>
>  static void mask_IO_APIC_irq (unsigned int irq)
>  {
>

looks good.

YH

Re: [PATCH] x86: io-apic - get rid of __DO_ACTION macro

[Ingo Molnar]

* Yinghai Lu <yhlu.kernel@gmail.com> wrote:

> > Yinghai, Ingo, what about this one?

> looks good.

applied to tip/irq/sparseirq - thanks guys!

	Ingo