Messages in this thread | | | Subject | Re: [PATCH ] ipmi: kcs_bmc: fix IRQ exception if the channel is not open | From | Corey Minyard <> | Date | Fri, 22 Jun 2018 07:43:52 -0500 |
| |
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@linux.intel.com> > --- > 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;
| |