Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754837AbbHXQAU (ORCPT ); Mon, 24 Aug 2015 12:00:20 -0400 Received: from mail-pa0-f51.google.com ([209.85.220.51]:36645 "EHLO mail-pa0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750904AbbHXQAT (ORCPT ); Mon, 24 Aug 2015 12:00:19 -0400 Message-ID: <55DB3F8E.5030100@acm.org> Date: Mon, 24 Aug 2015 11:00:14 -0500 From: Corey Minyard Reply-To: minyard@acm.org User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.3.0 MIME-Version: 1.0 To: =?UTF-8?B?5rKz5ZCI6Iux5a6PIC8gS0FXQUnvvIxISURFSElSTw==?= CC: "openipmi-developer@lists.sourceforge.net" , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH 7/7] ipmi/kcs: Don't run the KCS state machine when it is KCS_IDLE References: <20150727055516.4759.34462.stgit@softrs> <20150727055516.4759.65106.stgit@softrs> <55CAC849.70902@acm.org> <04EAB7311EE43145B2D3536183D1A84454938089@GSjpTKYDCembx31.service.hitachi.net> <55D8B543.4000805@acm.org> <04EAB7311EE43145B2D3536183D1A8445493D025@GSjpTKYDCembx31.service.hitachi.net> In-Reply-To: <04EAB7311EE43145B2D3536183D1A8445493D025@GSjpTKYDCembx31.service.hitachi.net> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3846 Lines: 96 On 08/23/2015 08:52 PM, 河合英宏 / KAWAI，HIDEHIRO wrote: >> From: Corey Minyard [mailto:tcminyard@gmail.com] On Behalf Of Corey Minyard >> >> On 08/17/2015 09:54 PM, 河合英宏 / KAWAI，HIDEHIRO wrote: >>>> From: Corey Minyard [mailto:tcminyard@gmail.com] On Behalf Of Corey Minyard >>>> >>>> This patch will break ATN handling on the interfaces. So we can't do this. >>> I understand. So how about doing like this: >>> >>> /* All states wait for ibf, so just do it here. */ >>> - if (!check_ibf(kcs, status, time)) >>> + if (kcs->state != KCS_IDLE && !check_ibf(kcs, status, time)) >>> return SI_SM_CALL_WITH_DELAY; >>> >>> I think it is not necessary to wait IBF when the state is IDLE. >>> In this way, we can also handle the ATN case. >> I think it would be more reliable to go up a level and add a timeout. > It may be so, but we should address this issue separately (at least > I think above solution reasonably solves the issue). > > This issue happens after all queued messages are processed or dropped > by timeout. There is no current message. So what should we set > a timeout against? We can add a timeout into my new flush_messages(), > but that is meaningful only in panic context. That doesn't help > in normal context; we would perform a busy loop of smi_event_handler() > and schedule() in ipmi_thread(). I'm a little confused here. Is the problem that the ATN bit is stuck high? If so, it's going to be really hard to work around this without breaking ATN handling. -corey > > Regards, > > Hidehiro Kawai > >> One should >> be there, anyway. I thought they were all covered, but I may have missed >> something. >> >> -corey >> >>> Regards, >>> >>> Hidehiro Kawai >>> Hitachi, Ltd. Research & Development Group >>> >>>> It's going to be extremely hard to recover if the BMC is not working >>>> correctly when a panic happens. I'm not sure what can be done, but if >>>> you can fix it another way it would be good. >>>> >>>> -corey >>>> >>>> On 07/27/2015 12:55 AM, Hidehiro Kawai wrote: >>>>> If a BMC is unresponsive for some reason, it ends up completing >>>>> the requested message as an error, then kcs_event() is called once >>>>> to advance the state machine. However, since the BMC is >>>>> unresponsive now, the status of the KCS interface may not be >>>>> idle. As the result, the state machine can continue to run and >>>>> comsume CPU time indefinitely even if there is no more request >>>>> message. Moreover, if this happens in run-to-completion mode >>>>> (i.e. context of panic_event()), the kernel hangs up. >>>>> >>>>> To fix this problem, this patch ignores kcs_event() call if there >>>>> is no request message to be processed. >>>>> >>>>> Signed-off-by: Hidehiro Kawai >>>>> --- >>>>> drivers/char/ipmi/ipmi_kcs_sm.c | 4 ++++ >>>>> 1 file changed, 4 insertions(+) >>>>> >>>>> diff --git a/drivers/char/ipmi/ipmi_kcs_sm.c b/drivers/char/ipmi/ipmi_kcs_sm.c >>>>> index 8c25f59..0e187fb 100644 >>>>> --- a/drivers/char/ipmi/ipmi_kcs_sm.c >>>>> +++ b/drivers/char/ipmi/ipmi_kcs_sm.c >>>>> @@ -353,6 +353,10 @@ static enum si_sm_result kcs_event(struct si_sm_data *kcs, long time) >>>>> if (kcs_debug & KCS_DEBUG_STATES) >>>>> printk(KERN_DEBUG "KCS: State = %d, %x\n", kcs->state, status); >>>>> >>>>> + /* We don't want to run the state machine when the state is IDLE */ >>>>> + if (kcs->state == KCS_IDLE) >>>>> + return SI_SM_IDLE; >>>>> + >>>>> /* All states wait for ibf, so just do it here. */ >>>>> if (!check_ibf(kcs, status, time)) >>>>> return SI_SM_CALL_WITH_DELAY; >>>>> >>>>> -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/