[Openipmi-developer] [PATCH ] ipmi: kcs_bmc: fix IRQ exception if the channel is not open

2018-06-21 Thread Haiyue Wang
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
   [] (unwind_backtrace) from [] (show_stack+0x20/0x24)
   [] (show_stack) from [] (dump_stack+0x8c/0xa0)
   [] (dump_stack) from [] (__report_bad_irq+0x3c/0xdc)
   [] (__report_bad_irq) from [] 
(note_interrupt+0x29c/0x2ec)
   [] (note_interrupt) from [] 
(handle_irq_event_percpu+0x5c/0x68)
   [] (handle_irq_event_percpu) from [] 
(handle_irq_event+0x48/0x6c)
   [] (handle_irq_event) from [] 
(handle_fasteoi_irq+0xc8/0x198)
   [] (handle_fasteoi_irq) from [] 
(__handle_domain_irq+0x90/0xe8)
   [] (__handle_domain_irq) from [] 
(gic_handle_irq+0x58/0x9c)
   [] (gic_handle_irq) from [] (__irq_svc+0x6c/0x90)
   Exception stack(0xc0a01de8 to 0xc0a01e30)
   1de0:   2080 c0a6fbc0    c096d294
   1e00:  0001 dc406400 f03ff100 0082 c0a01e94 c0a6fbc0 c0a01e38
   1e20: 00200102 c01015bc 6113 
   [] (__irq_svc) from [] (__do_softirq+0xbc/0x358)
   [] (__do_softirq) from [] (irq_exit+0xb8/0xec)
   [] (irq_exit) from [] (__handle_domain_irq+0x94/0xe8)
   [] (__handle_domain_irq) from [] 
(gic_handle_irq+0x58/0x9c)
   [] (gic_handle_irq) from [] (__irq_svc+0x6c/0x90)
   Exception stack(0xc0a01ef8 to 0xc0a01f40)
   1ee0:    03ae
   1f00: dcc0f338 c0111060 c0a0 c0a0cc44 c0a0cbe4 c0a1c22b c07bc218 0001
   1f20: dcffca40 c0a01f54 c0a01f58 c0a01f48 c0103524 c0103528 6013 
   [] (__irq_svc) from [] (arch_cpu_idle+0x48/0x4c)
   [] (arch_cpu_idle) from [] (default_idle_call+0x30/0x3c)
   [] (default_idle_call) from [] (do_idle+0xc8/0x134)
   [] (do_idle) from [] (cpu_startup_entry+0x28/0x2c)
   [] (cpu_startup_entry) from [] (rest_init+0x84/0x88)
   [] (rest_init) from [] (start_kernel+0x388/0x394)
   [] (start_kernel) from [<807c>] (0x807c)
   handlers:
   [] npcm7xx_kcs_irq
   Disabling IRQ #30

Signed-off-by: Haiyue Wang 
---
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.

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(_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(_bmc->lock, flags);
 
return ret;
-- 
2.7.4


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer


Re: [Openipmi-developer] [PATCH] ipmi: Cleanup oops on initialization failure

2018-06-21 Thread Corey Minyard

On 06/21/2018 01:47 AM, Meelis Roos wrote:

The corresponding dmesg:

[7.372830] IPMI System Interface driver.
[7.373034] ipmi_si dmi-ipmi-si.0: ipmi_platform: probing via SMBIOS
[7.373109] ipmi_si: SMBIOS: mem 0x0 regsize 1 spacing 1 irq 0
[7.373182] ipmi_si: Adding SMBIOS-specified kcs state machine
[7.373352] ipmi_si: Trying SMBIOS-specified kcs state machine at mem
address 0x0, slave address 0x20, irq 0
[7.373479] ipmi_si dmi-ipmi-si.0: Could not set up I/O space


BTW, can you send me at least the IPMI portion of the output of
dmidecode for your machine?  I have seen a lot of these where the
address in the SMBIOS tables is incorrect, and I'm wondering if
it's something in the driver, or if it's really the tables that
are bad.

Handle 0x001B, DMI type 38, 18 bytes
IPMI Device Information
  Interface Type: KCS (Keyboard Control Style)
  Specification Version: 2.0
  I2C Slave Address: 0x10
  NV Storage Device: Not Present
  Base Address: 0x (Memory-mapped)
  Register Spacing: Successive Byte Boundaries

Thanks a bunch.  It looks like the SMBIOS tables are wrong.  I
wonder if this is what some vendor do if there is no IPMI device
installed.  I guess I need to add a check for this.

Another machine (Sun X2100) with similar crash is also cured by the
patch, but this is slightly different (not NULL):

[8.891217] IPMI System Interface driver.
[8.898404] ipmi_si dmi-ipmi-si.0: ipmi_platform: probing via SMBIOS
[8.905635] ipmi_si: SMBIOS: io 0xca2 regsize 1 spacing 1 irq 0
[8.912895] ipmi_si: Adding SMBIOS-specified kcs state machine
[8.920246] ipmi_si: Trying SMBIOS-specified kcs state machine at i/o 
address 0xca2, slave address 0x20, irq 0
[8.934379] ipmi_si dmi-ipmi-si.0: Interface detection failed

IPMI Device Information
 Interface Type: KCS (Keyboard Control Style)
 Specification Version: 1.5
 I2C Slave Address: 0x10
 NV Storage Device: Not Present
 Base Address: 0x0CA2 (I/O)
 Register Spacing: Successive Byte Boundaries



That's even worse.  The SMBIOS table says the interface is there, but 
it's not

there.  Not much I can do about that :(.

Thanks again,

-corey

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer


Re: [Openipmi-developer] [PATCH] ipmi: Cleanup oops on initialization failure

2018-06-21 Thread Meelis Roos
> > The corresponding dmesg:
> > 
> > [7.372830] IPMI System Interface driver.
> > [7.373034] ipmi_si dmi-ipmi-si.0: ipmi_platform: probing via SMBIOS
> > [7.373109] ipmi_si: SMBIOS: mem 0x0 regsize 1 spacing 1 irq 0
> > [7.373182] ipmi_si: Adding SMBIOS-specified kcs state machine
> > [7.373352] ipmi_si: Trying SMBIOS-specified kcs state machine at mem
> > address 0x0, slave address 0x20, irq 0
> > [7.373479] ipmi_si dmi-ipmi-si.0: Could not set up I/O space
> > 
> > > BTW, can you send me at least the IPMI portion of the output of
> > > dmidecode for your machine?  I have seen a lot of these where the
> > > address in the SMBIOS tables is incorrect, and I'm wondering if
> > > it's something in the driver, or if it's really the tables that
> > > are bad.
> > Handle 0x001B, DMI type 38, 18 bytes
> > IPMI Device Information
> >  Interface Type: KCS (Keyboard Control Style)
> >  Specification Version: 2.0
> >  I2C Slave Address: 0x10
> >  NV Storage Device: Not Present
> >  Base Address: 0x (Memory-mapped)
> >  Register Spacing: Successive Byte Boundaries
> 
> Thanks a bunch.  It looks like the SMBIOS tables are wrong.  I
> wonder if this is what some vendor do if there is no IPMI device
> installed.  I guess I need to add a check for this.

Another machine (Sun X2100) with similar crash is also cured by the 
patch, but this is slightly different (not NULL):

[8.891217] IPMI System Interface driver.
[8.898404] ipmi_si dmi-ipmi-si.0: ipmi_platform: probing via SMBIOS
[8.905635] ipmi_si: SMBIOS: io 0xca2 regsize 1 spacing 1 irq 0
[8.912895] ipmi_si: Adding SMBIOS-specified kcs state machine
[8.920246] ipmi_si: Trying SMBIOS-specified kcs state machine at i/o 
address 0xca2, slave address 0x20, irq 0
[8.934379] ipmi_si dmi-ipmi-si.0: Interface detection failed

IPMI Device Information
Interface Type: KCS (Keyboard Control Style)
Specification Version: 1.5
I2C Slave Address: 0x10
NV Storage Device: Not Present
Base Address: 0x0CA2 (I/O)
Register Spacing: Successive Byte Boundaries



-- 
Meelis Roos (mr...@linux.ee)

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer