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

[Wang, Haiyue]

On 2018-06-22 20:43, Corey Minyard wrote:
> > 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...
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