On 2018-06-22 20:43, Corey Minyard wrote:
Signed-off-by: Haiyue Wang <haiyue.w...@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
------------------------------------------------------------------------------
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
