lkml.org 
[lkml]   [2018]   [Jun]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH ] ipmi: kcs_bmc: fix IRQ exception if the channel is not open
From
Date


On 2018-06-22 20:43, Corey Minyard wrote:
>> 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...
>
Sorry for my bad writing.... :(
> 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.
>
In fact, in BMC chip design, the LPC controller has many devices, such as
Port 80 snoop, BT, KCS etc, they shares the same interrupt. :)

Take AST2500 BMC as an example, please search the keyword 'interrupts =
<8>' in
this file:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/aspeed-g5.dtsi?h=v4.18-rc1

And may have 4 KCS channels:
https://patchwork.kernel.org/patch/10318877/
This patch just enables kcs3 & kcs4, which use the same interrupt number 8.

So the interrupt should be enabled always.

And this IRQ issue root cause it that the IRQ handler just return IRQ_NONE
if it is not opened. And for abort actions, I just put it under its
related channel
IBF is set. Because only IBF is set, aborting makes sense.

> -corey

\
 
 \ /
  Last update: 2018-06-22 19:24    [W:0.047 / U:0.180 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site