* [PATCH v2] ipmi: kcs_bmc: fix IRQ exception if the channel is not open
@ 2018-06-23 13:51 Haiyue Wang
2018-06-25 19:34 ` Corey Minyard
0 siblings, 1 reply; 3+ messages in thread
From: Haiyue Wang @ 2018-06-23 13:51 UTC (permalink / raw)
To: minyard, arnd, gregkh, openipmi-developer, linux-kernel
Cc: Haiyue Wang, luis.a.silva, avi.fishman, openbmc
When kcs_bmc_handle_event calls kcs_force_abort function to handle the
not open (no user running) KCS channel transaction, the returned status
value -ENODEV causes the low level IRQ handler indicating that the irq
was not for him by returning IRQ_NONE. After some time, this IRQ will
be treated to be spurious one, and the exception dump happens.
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
It needs to change the returned status from -ENODEV to 0. The -ENODEV
was originally used to tell the low level IRQ handler that no user was
running, but not consider the IRQ handling desgin.
And multiple KCS channels share one IRQ handler, it needs to check the
IBF flag before doing force abort. If the IBF is set, after handling,
return 0 to low level IRQ handler to indicate that the IRQ is handled.
Signed-off-by: Haiyue Wang <haiyue.wang@linux.intel.com>
---
v1 -> v2:
- Change the commit message to be more understandable.
---
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;
--
2.7.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] ipmi: kcs_bmc: fix IRQ exception if the channel is not open
2018-06-23 13:51 [PATCH v2] ipmi: kcs_bmc: fix IRQ exception if the channel is not open Haiyue Wang
@ 2018-06-25 19:34 ` Corey Minyard
2018-06-26 6:29 ` Wang, Haiyue
0 siblings, 1 reply; 3+ messages in thread
From: Corey Minyard @ 2018-06-25 19:34 UTC (permalink / raw)
To: Haiyue Wang, arnd, gregkh, openipmi-developer, linux-kernel
Cc: luis.a.silva, avi.fishman, openbmc
On 06/23/2018 08:51 AM, Haiyue Wang wrote:
> When kcs_bmc_handle_event calls kcs_force_abort function to handle the
> not open (no user running) KCS channel transaction, the returned status
> value -ENODEV causes the low level IRQ handler indicating that the irq
> was not for him by returning IRQ_NONE. After some time, this IRQ will
> be treated to be spurious one, and the exception dump happens.
>
> 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
>
> It needs to change the returned status from -ENODEV to 0. The -ENODEV
> was originally used to tell the low level IRQ handler that no user was
> running, but not consider the IRQ handling desgin.
>
> And multiple KCS channels share one IRQ handler, it needs to check the
> IBF flag before doing force abort. If the IBF is set, after handling,
> return 0 to low level IRQ handler to indicate that the IRQ is handled.
I've looked at this some more, and I think it's ok, especially with the
better description.
I'm going to submit this for the current release after it sits in next
for a bit.
Thanks,
-corey
> Signed-off-by: Haiyue Wang <haiyue.wang@linux.intel.com>
> ---
> v1 -> v2:
> - Change the commit message to be more understandable.
> ---
> 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;
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] ipmi: kcs_bmc: fix IRQ exception if the channel is not open
2018-06-25 19:34 ` Corey Minyard
@ 2018-06-26 6:29 ` Wang, Haiyue
0 siblings, 0 replies; 3+ messages in thread
From: Wang, Haiyue @ 2018-06-26 6:29 UTC (permalink / raw)
To: minyard, arnd, gregkh, openipmi-developer, linux-kernel
Cc: luis.a.silva, avi.fishman, openbmc
On 2018-06-26 03:34, Corey Minyard wrote:
>> return 0 to low level IRQ handler to indicate that the IRQ is handled.
>
> I've looked at this some more, and I think it's ok, especially with
> the better description.
>
> I'm going to submit this for the current release after it sits in next
> for a bit.
>
Got it, thanks, Corey.
BR,
Haiyue
> Thanks,
>
> -corey
>
>> Signed-off-by: Haiyue Wang <haiyue.wang@linux.intel.com>
>> ---
>> v1 -> v2:
>> - Change the commit message to be more understandable.
>> ---
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-06-26 6:29 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-23 13:51 [PATCH v2] ipmi: kcs_bmc: fix IRQ exception if the channel is not open Haiyue Wang
2018-06-25 19:34 ` Corey Minyard
2018-06-26 6:29 ` Wang, Haiyue
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).