Re: [PATCH ] ipmi: kcs_bmc: fix IRQ exception if the channel is not open

From: Corey Minyard
Date: Fri Jun 22 2018 - 08:44:02 EST

On 06/21/2018 09:56 PM, Haiyue Wang wrote:
The original core KCS IRQ handling function 'kcs_bmc_handle_event' had
a wrong logical desgin, it should force abort the KCS transaction only
if the IBF flag is set, because multiple KCS channels share the same
handle function; and it should return 0, which indicates that event of
current channel is handled. An negative value -ENODATA will cause the
IRQ handler of BMC KCS controller return IRQ_NONE, then IRQ moudle will
treat this IRQ 'nobody cared', it will trigger IRQ exception.

irq 30: nobody cared (try booting with the "irqpoll" option)
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.10.15-npcm750 #1
Hardware name: NPCMX50 Chip family
[<c010b264>] (unwind_backtrace) from [<c0106930>] (show_stack+0x20/0x24)
[<c0106930>] (show_stack) from [<c03dad38>] (dump_stack+0x8c/0xa0)
[<c03dad38>] (dump_stack) from [<c0168810>] (__report_bad_irq+0x3c/0xdc)
[<c0168810>] (__report_bad_irq) from [<c0168c34>] (note_interrupt+0x29c/0x2ec)
[<c0168c34>] (note_interrupt) from [<c0165c80>] (handle_irq_event_percpu+0x5c/0x68)
[<c0165c80>] (handle_irq_event_percpu) from [<c0165cd4>] (handle_irq_event+0x48/0x6c)
[<c0165cd4>] (handle_irq_event) from [<c0169664>] (handle_fasteoi_irq+0xc8/0x198)
[<c0169664>] (handle_fasteoi_irq) from [<c016529c>] (__handle_domain_irq+0x90/0xe8)
[<c016529c>] (__handle_domain_irq) from [<c01014bc>] (gic_handle_irq+0x58/0x9c)
[<c01014bc>] (gic_handle_irq) from [<c010752c>] (__irq_svc+0x6c/0x90)
Exception stack(0xc0a01de8 to 0xc0a01e30)
1de0: 00002080 c0a6fbc0 00000000 00000000 00000000 c096d294
1e00: 00000000 00000001 dc406400 f03ff100 00000082 c0a01e94 c0a6fbc0 c0a01e38
1e20: 00200102 c01015bc 60000113 ffffffff
[<c010752c>] (__irq_svc) from [<c01015bc>] (__do_softirq+0xbc/0x358)
[<c01015bc>] (__do_softirq) from [<c011c798>] (irq_exit+0xb8/0xec)
[<c011c798>] (irq_exit) from [<c01652a0>] (__handle_domain_irq+0x94/0xe8)
[<c01652a0>] (__handle_domain_irq) from [<c01014bc>] (gic_handle_irq+0x58/0x9c)
[<c01014bc>] (gic_handle_irq) from [<c010752c>] (__irq_svc+0x6c/0x90)
Exception stack(0xc0a01ef8 to 0xc0a01f40)
1ee0: 00000000 000003ae
1f00: dcc0f338 c0111060 c0a00000 c0a0cc44 c0a0cbe4 c0a1c22b c07bc218 00000001
1f20: dcffca40 c0a01f54 c0a01f58 c0a01f48 c0103524 c0103528 60000013 ffffffff
[<c010752c>] (__irq_svc) from [<c0103528>] (arch_cpu_idle+0x48/0x4c)
[<c0103528>] (arch_cpu_idle) from [<c0681390>] (default_idle_call+0x30/0x3c)
[<c0681390>] (default_idle_call) from [<c0156f24>] (do_idle+0xc8/0x134)
[<c0156f24>] (do_idle) from [<c015722c>] (cpu_startup_entry+0x28/0x2c)
[<c015722c>] (cpu_startup_entry) from [<c067ad74>] (rest_init+0x84/0x88)
[<c067ad74>] (rest_init) from [<c0900d44>] (start_kernel+0x388/0x394)
[<c0900d44>] (start_kernel) from [<0000807c>] (0x807c)
handlers:
[<c041c5dc>] npcm7xx_kcs_irq
Disabling IRQ #30

Signed-off-by: Haiyue Wang <haiyue.wang@xxxxxxxxxxxxxxx>
---
Hi Corey,

This patch looks introducing BIG modification, it should be easily
understandable, and makes code clean & fix an error design, which
is introduced by misunderstanding the IRQ return value.

I'm having a little trouble understanding your text above, so let me try to repeat
back to you what I'm thinking you are saying...

You have two (or more) devices using the same interrupt, and at least one is an
IPMI KCS device. And interrupt comes in to the other device when the IPMI KCS
device is not running. The original code issues an abort when that happens,
which is obviously incorrect. It then returns -ENODATA, .

When the interrupt comes in for the abort handling, the driver will then issue
another abort, and again returns -ENODATA. This time neither driver handles
the interrupt, resulting in the logs.

In general, I think the change you have here is good. You don't want to
issue an abort in this case. But...

If I am right, this fix doesn't completely solve the problem. It does solve this
immediate problem, but what if there is an OS on the other end of the
KCS interface that sets the IBF flag? The same situation will occur. In fact
it will occur even if there is only one handler for the interrupt.

Maybe it's best to have the interrupt disabled unless the device is open.
You have to handle the interrupt disable race on a close, but with the
sync functions that shouldn't be too hard.

-corey


BR,
Haiyue
---
drivers/char/ipmi/kcs_bmc.c | 31 ++++++++++---------------------
1 file changed, 10 insertions(+), 21 deletions(-)

diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
index fbfc05e..bb882ab1 100644
--- a/drivers/char/ipmi/kcs_bmc.c
+++ b/drivers/char/ipmi/kcs_bmc.c
@@ -210,34 +210,23 @@ static void kcs_bmc_handle_cmd(struct kcs_bmc *kcs_bmc)
int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc)
{
unsigned long flags;
- int ret = 0;
+ int ret = -ENODATA;
u8 status;
spin_lock_irqsave(&kcs_bmc->lock, flags);
- if (!kcs_bmc->running) {
- kcs_force_abort(kcs_bmc);
- ret = -ENODEV;
- goto out_unlock;
- }
-
- status = read_status(kcs_bmc) & (KCS_STATUS_IBF | KCS_STATUS_CMD_DAT);
-
- switch (status) {
- case KCS_STATUS_IBF | KCS_STATUS_CMD_DAT:
- kcs_bmc_handle_cmd(kcs_bmc);
- break;
-
- case KCS_STATUS_IBF:
- kcs_bmc_handle_data(kcs_bmc);
- break;
+ status = read_status(kcs_bmc);
+ if (status & KCS_STATUS_IBF) {
+ if (!kcs_bmc->running)
+ kcs_force_abort(kcs_bmc);
+ else if (status & KCS_STATUS_CMD_DAT)
+ kcs_bmc_handle_cmd(kcs_bmc);
+ else
+ kcs_bmc_handle_data(kcs_bmc);
- default:
- ret = -ENODATA;
- break;
+ ret = 0;
}
-out_unlock:
spin_unlock_irqrestore(&kcs_bmc->lock, flags);
return ret;