[ORLinux] [PATCH] openrisc: fix irq_mask_ack for spec compliant PIC

[Dmitry Kalinkin]

The bug was introduced in d23b5799b608112bb799c9b0e1e11ee1da692d76
OpenRISC 1000 Architecture Manual specifies mask values to be 1 for
enabled interrupt and 0 for disabled. or1200 complies to the spec here,
so, in non-or1200 case, mask value should not be generated in some
opposite way.

Signed-off-by: Dmitry Kalinkin <dmitry.kalinkin at gmail.com>
---
arch/openrisc/kernel/irq.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/openrisc/kernel/irq.c b/arch/openrisc/kernel/irq.c
index 8ec77bc..6d1744a 100644
--- a/arch/openrisc/kernel/irq.c
+++ b/arch/openrisc/kernel/irq.c
@@ -84,14 +84,14 @@ static void or1k_pic_ack(struct irq_data *data)

static void or1k_pic_mask_ack(struct irq_data *data)
{
+ mtspr(SPR_PICMR, mfspr(SPR_PICMR) & ~(1UL << data->hwirq));
+
/* Comments for pic_ack apply here, too */

#ifdef CONFIG_OR1K_1200
- mtspr(SPR_PICMR, mfspr(SPR_PICMR) & ~(1UL << data->hwirq));
mtspr(SPR_PICSR, mfspr(SPR_PICSR) & ~(1UL << data->hwirq));
#else
WARN(1, "Interrupt handling possibly broken\n");
- mtspr(SPR_PICMR, (1UL << data->hwirq));
mtspr(SPR_PICSR, (1UL << data->hwirq));
#endif
}

[Jonas Bonn]

Hi Dmitry,

Post by Dmitry Kalinkin
The bug was introduced in d23b5799b608112bb799c9b0e1e11ee1da692d76
OpenRISC 1000 Architecture Manual specifies mask values to be 1 for
enabled interrupt and 0 for disabled. or1200 complies to the spec here,
so, in non-or1200 case, mask value should not be generated in some
opposite way.

Seems reasonable. I should obviously apply this and will do so...

However, I'm a bit curious as to why nobody has reported this earlier.
Are the mor1kx guys running the kernel with the OR1200 option set? Does
mor1kx have the same "bug" WRT clearing interrupts? Have you been
running the mor1kx all this time without ever having been able to mask
interrupts (and things still work???)? Or is there a patch floating
about somewhere that hasn't found its way to me?

Stefan: can you comment on this?

/Jonas

Post by Dmitry Kalinkin
Signed-off-by: Dmitry Kalinkin <dmitry.kalinkin at gmail.com>
---
arch/openrisc/kernel/irq.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/openrisc/kernel/irq.c b/arch/openrisc/kernel/irq.c
index 8ec77bc..6d1744a 100644
--- a/arch/openrisc/kernel/irq.c
+++ b/arch/openrisc/kernel/irq.c
@@ -84,14 +84,14 @@ static void or1k_pic_ack(struct irq_data *data)
static void or1k_pic_mask_ack(struct irq_data *data)
{
+ mtspr(SPR_PICMR, mfspr(SPR_PICMR) & ~(1UL << data->hwirq));
+
/* Comments for pic_ack apply here, too */
#ifdef CONFIG_OR1K_1200
- mtspr(SPR_PICMR, mfspr(SPR_PICMR) & ~(1UL << data->hwirq));
mtspr(SPR_PICSR, mfspr(SPR_PICSR) & ~(1UL << data->hwirq));
#else
WARN(1, "Interrupt handling possibly broken\n");
- mtspr(SPR_PICMR, (1UL << data->hwirq));
mtspr(SPR_PICSR, (1UL << data->hwirq));
#endif
}

[Stefan Kristiansson]

Post by Jonas Bonn
Hi Dmitry,

Post by Dmitry Kalinkin
The bug was introduced in d23b5799b608112bb799c9b0e1e11ee1da692d76
OpenRISC 1000 Architecture Manual specifies mask values to be 1 for
enabled interrupt and 0 for disabled. or1200 complies to the spec here,
so, in non-or1200 case, mask value should not be generated in some
opposite way.

Seems reasonable. I should obviously apply this and will do so...
However, I'm a bit curious as to why nobody has reported this earlier.
Are the mor1kx guys running the kernel with the OR1200 option set? Does
mor1kx have the same "bug" WRT clearing interrupts? Have you been
running the mor1kx all this time without ever having been able to mask
interrupts (and things still work???)? Or is there a patch floating
about somewhere that hasn't found its way to me?
Stefan: can you comment on this?

Sure, I'll try.
mor1kx have a "or1200-pic compatibility" switch,
so in some cases it has been running with the OR1200 option set.
It also have config options to act like the spec states.
The only patch I have had floating around, has been to remove the WARNings,
but even when touching the lines above the bug I've missed it.
Note though, this is in the irq_mask_ack function, the irq_mask
function have always been correct.
So it's possible that bugs caused by this have gone unnoticed.

Stefan

[Dmitry Kalinkin]

Post by Jonas Bonn
Hi Dmitry,
Seems reasonable. I should obviously apply this and will do so...
However, I'm a bit curious as to why nobody has reported this earlier.
Are the mor1kx guys running the kernel with the OR1200 option set? Does
mor1kx have the same "bug" WRT clearing interrupts? Have you been
running the mor1kx all this time without ever having been able to mask
interrupts (and things still work???)? Or is there a patch floating
about somewhere that hasn't found its way to me?

Firstly, as Stefan said, only a fraction of people will use Linux with
EDGE trigger, plus not everyone likes warnings. Secondly, the bug
exposure also depends on what effect you are supposed to see. If you
only have an uart in the system, then it probably won't affect any
user experience. Some people will try to use it with ethmac, then, in
most cases, uart might just win and then it will look like the
ethernet is not working, which may be then somehow resolved without
any extra thought. In my case I had a half complete driver which lead
to an interrupt lane holding active after rmmod, winning the race on
the next modprobe and breaking my uart input. The effect became
obvious.

This is only explanation I have. There doesn't seem to be any second
path which would lead around this bug: handle_level_irq() from
kernel/irq/chip.c always calls mask_ack_irq(), which will always call
our irq_mask_ack implementation just because we have one.