* [PATCH] x86: io-apic - get rid of __DO_ACTION macro
@ 2008-09-09 18:46 Cyrill Gorcunov
2008-09-09 20:13 ` Yinghai Lu
0 siblings, 1 reply; 13+ messages in thread
From: Cyrill Gorcunov @ 2008-09-09 18:46 UTC (permalink / raw)
To: Ingo Molnar; +Cc: H. Peter Anvin, Thomas Gleixner, Maciej W. Rozycki, LKML
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)
{
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] x86: io-apic - get rid of __DO_ACTION macro
2008-09-09 18:46 [PATCH] x86: io-apic - get rid of __DO_ACTION macro Cyrill Gorcunov
@ 2008-09-09 20:13 ` Yinghai Lu
2008-09-09 20:28 ` Cyrill Gorcunov
2008-09-10 6:02 ` Cyrill Gorcunov
0 siblings, 2 replies; 13+ messages in thread
From: Yinghai Lu @ 2008-09-09 20:13 UTC (permalink / raw)
To: Cyrill Gorcunov
Cc: Ingo Molnar, H. Peter Anvin, Thomas Gleixner, Maciej W. Rozycki,
LKML
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] x86: io-apic - get rid of __DO_ACTION macro
2008-09-09 20:13 ` Yinghai Lu
@ 2008-09-09 20:28 ` Cyrill Gorcunov
2008-09-10 6:02 ` Cyrill Gorcunov
1 sibling, 0 replies; 13+ messages in thread
From: Cyrill Gorcunov @ 2008-09-09 20:28 UTC (permalink / raw)
To: Yinghai Lu
Cc: Ingo Molnar, H. Peter Anvin, Thomas Gleixner, Maciej W. Rozycki,
LKML
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 :)
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] x86: io-apic - get rid of __DO_ACTION macro
2008-09-09 20:13 ` Yinghai Lu
2008-09-09 20:28 ` Cyrill Gorcunov
@ 2008-09-10 6:02 ` Cyrill Gorcunov
2008-09-10 6:22 ` Yinghai Lu
1 sibling, 1 reply; 13+ messages in thread
From: Cyrill Gorcunov @ 2008-09-10 6:02 UTC (permalink / raw)
To: Yinghai Lu
Cc: Ingo Molnar, H. Peter Anvin, Thomas Gleixner, Maciej W. Rozycki,
LKML
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)?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] x86: io-apic - get rid of __DO_ACTION macro
2008-09-10 6:02 ` Cyrill Gorcunov
@ 2008-09-10 6:22 ` Yinghai Lu
2008-09-10 8:40 ` Cyrill Gorcunov
2008-09-10 9:31 ` Ingo Molnar
0 siblings, 2 replies; 13+ messages in thread
From: Yinghai Lu @ 2008-09-10 6:22 UTC (permalink / raw)
To: Cyrill Gorcunov
Cc: Ingo Molnar, H. Peter Anvin, Thomas Gleixner, Maciej W. Rozycki,
LKML
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] x86: io-apic - get rid of __DO_ACTION macro
2008-09-10 6:22 ` Yinghai Lu
@ 2008-09-10 8:40 ` Cyrill Gorcunov
2008-09-10 9:31 ` Ingo Molnar
1 sibling, 0 replies; 13+ messages in thread
From: Cyrill Gorcunov @ 2008-09-10 8:40 UTC (permalink / raw)
To: Yinghai Lu
Cc: Ingo Molnar, H. Peter Anvin, Thomas Gleixner, Maciej W. Rozycki,
LKML
[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 -
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] x86: io-apic - get rid of __DO_ACTION macro
2008-09-10 6:22 ` Yinghai Lu
2008-09-10 8:40 ` Cyrill Gorcunov
@ 2008-09-10 9:31 ` Ingo Molnar
2008-09-10 10:53 ` Cyrill Gorcunov
2008-09-10 17:30 ` Yinghai Lu
1 sibling, 2 replies; 13+ messages in thread
From: Ingo Molnar @ 2008-09-10 9:31 UTC (permalink / raw)
To: Yinghai Lu
Cc: Cyrill Gorcunov, H. Peter Anvin, Thomas Gleixner,
Maciej W. Rozycki, LKML
* 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] x86: io-apic - get rid of __DO_ACTION macro
2008-09-10 9:31 ` Ingo Molnar
@ 2008-09-10 10:53 ` Cyrill Gorcunov
2008-09-10 17:30 ` Yinghai Lu
1 sibling, 0 replies; 13+ messages in thread
From: Cyrill Gorcunov @ 2008-09-10 10:53 UTC (permalink / raw)
To: Ingo Molnar
Cc: Yinghai Lu, H. Peter Anvin, Thomas Gleixner, Maciej W. Rozycki,
LKML
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 :)
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] x86: io-apic - get rid of __DO_ACTION macro
2008-09-10 9:31 ` Ingo Molnar
2008-09-10 10:53 ` Cyrill Gorcunov
@ 2008-09-10 17:30 ` Yinghai Lu
2008-09-10 17:44 ` Cyrill Gorcunov
2008-09-10 18:19 ` Cyrill Gorcunov
1 sibling, 2 replies; 13+ messages in thread
From: Yinghai Lu @ 2008-09-10 17:30 UTC (permalink / raw)
To: Ingo Molnar
Cc: Cyrill Gorcunov, H. Peter Anvin, Thomas Gleixner,
Maciej W. Rozycki, LKML
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] x86: io-apic - get rid of __DO_ACTION macro
2008-09-10 17:30 ` Yinghai Lu
@ 2008-09-10 17:44 ` Cyrill Gorcunov
2008-09-10 18:19 ` Cyrill Gorcunov
1 sibling, 0 replies; 13+ messages in thread
From: Cyrill Gorcunov @ 2008-09-10 17:44 UTC (permalink / raw)
To: Yinghai Lu
Cc: Ingo Molnar, H. Peter Anvin, Thomas Gleixner, Maciej W. Rozycki,
LKML
[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 -
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] x86: io-apic - get rid of __DO_ACTION macro
2008-09-10 17:30 ` Yinghai Lu
2008-09-10 17:44 ` Cyrill Gorcunov
@ 2008-09-10 18:19 ` Cyrill Gorcunov
2008-09-10 18:40 ` Yinghai Lu
1 sibling, 1 reply; 13+ messages in thread
From: Cyrill Gorcunov @ 2008-09-10 18:19 UTC (permalink / raw)
To: Yinghai Lu
Cc: Ingo Molnar, H. Peter Anvin, Thomas Gleixner, Maciej W. Rozycki,
LKML
[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)
{
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] x86: io-apic - get rid of __DO_ACTION macro
2008-09-10 18:19 ` Cyrill Gorcunov
@ 2008-09-10 18:40 ` Yinghai Lu
2008-09-11 7:08 ` Ingo Molnar
0 siblings, 1 reply; 13+ messages in thread
From: Yinghai Lu @ 2008-09-10 18:40 UTC (permalink / raw)
To: Cyrill Gorcunov
Cc: Ingo Molnar, H. Peter Anvin, Thomas Gleixner, Maciej W. Rozycki,
LKML
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] x86: io-apic - get rid of __DO_ACTION macro
2008-09-10 18:40 ` Yinghai Lu
@ 2008-09-11 7:08 ` Ingo Molnar
0 siblings, 0 replies; 13+ messages in thread
From: Ingo Molnar @ 2008-09-11 7:08 UTC (permalink / raw)
To: Yinghai Lu
Cc: Cyrill Gorcunov, H. Peter Anvin, Thomas Gleixner,
Maciej W. Rozycki, LKML
* Yinghai Lu <yhlu.kernel@gmail.com> wrote:
> > Yinghai, Ingo, what about this one?
> looks good.
applied to tip/irq/sparseirq - thanks guys!
Ingo
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2008-09-11 7:09 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-09-09 18:46 [PATCH] x86: io-apic - get rid of __DO_ACTION macro Cyrill Gorcunov
2008-09-09 20:13 ` Yinghai Lu
2008-09-09 20:28 ` Cyrill Gorcunov
2008-09-10 6:02 ` Cyrill Gorcunov
2008-09-10 6:22 ` Yinghai Lu
2008-09-10 8:40 ` Cyrill Gorcunov
2008-09-10 9:31 ` Ingo Molnar
2008-09-10 10:53 ` Cyrill Gorcunov
2008-09-10 17:30 ` Yinghai Lu
2008-09-10 17:44 ` Cyrill Gorcunov
2008-09-10 18:19 ` Cyrill Gorcunov
2008-09-10 18:40 ` Yinghai Lu
2008-09-11 7:08 ` Ingo Molnar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox