Messages in this thread | | | Subject | Re: [PATCH ] ipmi: kcs_bmc: fix IRQ exception if the channel is not open | From | "Wang, Haiyue" <> | Date | Sat, 23 Jun 2018 01:23:55 +0800 |
| |
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
| |