[PATCH 0/7] ipmi: various fixes for panic notifier robustness

[PATCH 0/7] ipmi: various fixes for panic notifier robustness

[Hidehiro Kawai]

This patche set fixes multiple bugs in IPMI driver especially for
the panic notifier callback panic_event() and related functions.
This callback is called before kdump if crash_kexec_post_notifiers
boot option is specified, so its robustness is very important.

panic_event() first tries to flush queued messages before panic,
and this can cause multiple trouble such as hang-up.  PATCH 3 to 6 
addresses this kind of problems.

PATCH 7 addresses a problem caused by malfunctioning BMC.

NOTE: I'm planning to post another patch set which includes a patch
to drop all queued messages at the beginning of panic_event() if
crash_kexec_post_notifiers boot option is specified.  This will
improve the robustness of the panic notifier callback.

---

Hidehiro Kawai (7):
      ipmi: Remove unneeded set_run_to_completion call
      ipmi: Factor out message flushing procedure
      ipmi: Don't flush messages in sneder() in run-to-completion mode
      ipmi: Avoid touching possible corrupted lists in the panic context
      ipmi: Don't call receive handler in the panic context
      ipmi: Handle queued messages more certainly on panic
      ipmi/kcs: Don't run the KCS state machine when it is KCS_IDLE


 drivers/char/ipmi/ipmi_kcs_sm.c     |    4 ++
 drivers/char/ipmi/ipmi_msghandler.c |   66 +++++++++++++++++++++++++++++++++--
 drivers/char/ipmi/ipmi_si_intf.c    |   45 ++++++++++++------------
 include/linux/ipmi_smi.h            |   10 +++++
 4 files changed, 100 insertions(+), 25 deletions(-)


-- 
Hidehiro Kawai
Hitachi, Ltd. Research & Development Group

[PATCH 5/7] ipmi: Don't call receive handler in the panic context

[Hidehiro Kawai]

Received handlers defined as ipmi_recv_hndl member of struct
ipmi_user_hndl can take a spinlock.  This means that if the kernel
panics while holding the lock, a deadlock may happen on the lock
while flushing queued messages in the panic context.

Calling the receive handler doesn't make much meanings in the panic
context, simply skip it to avoid possible deadlocks.

Signed-off-by: Hidehiro Kawai <hidehiro.kawai.ez@hitachi.com>
---
 drivers/char/ipmi/ipmi_msghandler.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
index e7d84482..5a2d9fe 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -744,7 +744,13 @@ static void deliver_response(struct ipmi_recv_msg *msg)
 			ipmi_inc_stat(intf, unhandled_local_responses);
 		}
 		ipmi_free_recv_msg(msg);
-	} else {
+	} else if (!oops_in_progress) {
+		/*
+		 * If we are running in the panic context, calling the
+		 * receive handler doesn't much meaning and has a deadlock
+		 * risk.  At this moment, simply skip it in that case.
+		 */
+
 		ipmi_user_t user = msg->user;
 		user->handler->ipmi_recv_hndl(msg, user->handler_data);
 	}

[PATCH 6/7] ipmi: Handle queued messages more certainly on panic

[Hidehiro Kawai]

panic_event() called as a panic notifier tries to flush queued
messages, but it can't handle them if the kernel panic happens
while processing a message.  What happens depends on when the
kernel panics.

Here is the summary of message sending process.

    smi_send()
     smi_add_send_msg()
(1)   intf->curr_msg = msg
     sender()
(2)   smi_info->waiting_msg = msg

    <asynchronously called>
    check_start_timer_thread()
     start_next_msg()
      smi_info->curr_msg = smi_info->waiting_msg
(3)   smi_info->waiting_msg = NULL
(4)   smi_info->handlers->start_transaction()

    <asynchronously called>
    smi_event_handler()
(5)  handle_transaction_done()
      smi_info->curr_msg = NULL
      deliver_recv_msg()
       ipmi_smi_msg_received()
        intf->curr_msg = NULL

If the kernel panics before (1), the requested message will be
lost.  But it can't be helped.

If the kernel panics before (2), new message sent by
send_panic_events() is queued to intf->xmit_msgs because
intf->curr_msg is non-NULL.  But the new message will be never
sent because no one sends intf->curr_msg.  As the result, the
kernel hangs up.

If the kernel panics before (3), intf->curr_msg will be sent by
set_run_to_completion().  It's no problem.

If the kernel panics before (4), intf->curr_msg will be lost.
However, messages on intf->xmit_msgs will be handled.

If the kernel panics before (5), we try to continue running the
state machine.  It may successfully complete.

If the kernel panics after (5), we will miss the response message
handling, but it's not much problem in the panic context.

This patch tries to handle messages in intf->curr_msg and
intf->xmit_msgs only once without losing them.  To achieve this,
this patch does that:
  - if a message is in intf->curr_msg or intf->xmit_msgs and
    start_transaction() for the message hasn't been done yet,
    resend it
  - if start_transaction() for a message has been called,
    just continue to run the state machine
  - if the transaction has been completed, do nothing

>From the perspective of implementation, these are done by keeping
smi_info->waiting_msg until start_transaction() is completed and
by keeping new flag IPMI_MSG_RESEND_ON_PANIC just before starting
the state machine.

Signed-off-by: Hidehiro Kawai <hidehiro.kawai.ez@hitachi.com>
---
 drivers/char/ipmi/ipmi_msghandler.c |   36 +++++++++++++++++++++++++++++++++++
 drivers/char/ipmi/ipmi_si_intf.c    |    5 ++++-
 include/linux/ipmi_smi.h            |    5 +++++
 3 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
index 5a2d9fe..3dcd814 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -1493,6 +1493,8 @@ static struct ipmi_smi_msg *smi_add_send_msg(ipmi_smi_t intf,
 					     struct ipmi_smi_msg *smi_msg,
 					     int priority)
 {
+	smi_msg->flags |= IPMI_MSG_RESEND_ON_PANIC;
+
 	if (intf->curr_msg) {
 		if (priority > 0)
 			list_add_tail(&smi_msg->link, &intf->hp_xmit_msgs);
@@ -4223,6 +4225,7 @@ struct ipmi_smi_msg *ipmi_alloc_smi_msg(void)
 		rv->done = free_smi_msg;
 		rv->user_data = NULL;
 		atomic_inc(&smi_msg_inuse_count);
+		rv->flags = 0;
 	}
 	return rv;
 }
@@ -4531,7 +4534,40 @@ static int panic_event(struct notifier_block *this,
 			spin_unlock(&intf->waiting_rcv_msgs_lock);
 
 		intf->run_to_completion = 1;
+restart:
 		intf->handlers->set_run_to_completion(intf->send_info, 1);
+
+		if (intf->curr_msg) {
+			/*
+			 * This can happen if the kernel panics before
+			 * setting msg to smi_info->waiting_msg or while
+			 * processing a response.  For the former case, we
+			 * resend the message by re-queueing it.  For the
+			 * latter case, we simply ignore it because handling
+			 * response is not much meaningful in the panic
+			 * context.
+			 */
+
+			/*
+			 * Since we want to send the current message first,
+			 * re-queue it into the high-prioritized queue.
+			 */
+			if (intf->curr_msg->flags & IPMI_MSG_RESEND_ON_PANIC)
+				list_add(&intf->curr_msg->link,
+					 &intf->hp_xmit_msgs);
+
+			intf->curr_msg = NULL;
+		}
+
+		if (!list_empty(&intf->hp_xmit_msgs) ||
+		    !list_empty(&intf->xmit_msgs)) {
+			/*
+			 * This can happen if the kernel panics while
+			 * processing a response.  Kick the queue and restart.
+			 */
+			smi_recv_tasklet((unsigned long)intf);
+			goto restart;
+		}
 	}
 
 #ifdef CONFIG_IPMI_PANIC_EVENT
diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 814b7b7..c5c7806 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -383,7 +383,6 @@ static enum si_sm_result start_next_msg(struct smi_info *smi_info)
 		int err;
 
 		smi_info->curr_msg = smi_info->waiting_msg;
-		smi_info->waiting_msg = NULL;
 		debug_timestamp("Start2");
 		err = atomic_notifier_call_chain(&xaction_notifier_list,
 				0, smi_info);
@@ -401,6 +400,7 @@ static enum si_sm_result start_next_msg(struct smi_info *smi_info)
 		rv = SI_SM_CALL_WITHOUT_DELAY;
 	}
  out:
+	smi_info->waiting_msg = NULL;
 	return rv;
 }
 
@@ -804,6 +804,9 @@ static enum si_sm_result smi_event_handler(struct smi_info *smi_info,
 {
 	enum si_sm_result si_sm_result;
 
+	if (smi_info->curr_msg)
+		smi_info->curr_msg->flags &= ~(IPMI_MSG_RESEND_ON_PANIC);
+
  restart:
 	/*
 	 * There used to be a loop here that waited a little while
diff --git a/include/linux/ipmi_smi.h b/include/linux/ipmi_smi.h
index ba57fb1..1200872 100644
--- a/include/linux/ipmi_smi.h
+++ b/include/linux/ipmi_smi.h
@@ -47,6 +47,9 @@
 /* Structure for the low-level drivers. */
 typedef struct ipmi_smi *ipmi_smi_t;
 
+/* Flags for flags member of struct ipmi_smi_msg */
+#define IPMI_MSG_RESEND_ON_PANIC	1 /* If set, resend in panic_event() */
+
 /*
  * Messages to/from the lower layer.  The smi interface will take one
  * of these to send. After the send has occurred and a response has
@@ -75,6 +78,8 @@ struct ipmi_smi_msg {
 	/* Will be called when the system is done with the message
 	   (presumably to free it). */
 	void (*done)(struct ipmi_smi_msg *msg);
+
+	int flags;
 };
 
 struct ipmi_smi_handlers {

Re: [PATCH 6/7] ipmi: Handle queued messages more certainly on panic

[Corey Minyard]

On 07/27/2015 12:55 AM, Hidehiro Kawai wrote:
> panic_event() called as a panic notifier tries to flush queued
> messages, but it can't handle them if the kernel panic happens
> while processing a message.  What happens depends on when the
> kernel panics.

Sorry this took so long, I've been traveling.

I have queued the patches before this one.  They all look good and
necessary.

I'm not so sure about this patch.  It looks like the only thing that is
a real issue is #2 below.
It's not so important to avoid dropping messages.

Can this be simplified somehow to work around the issue at panic time if
intf->curr_msg is set and smi_info->waiting_msg is not?

Thank you,

-corey

>
> Here is the summary of message sending process.
>
>     smi_send()
>      smi_add_send_msg()
> (1)   intf->curr_msg = msg
>      sender()
> (2)   smi_info->waiting_msg = msg
>
>     <asynchronously called>
>     check_start_timer_thread()
>      start_next_msg()
>       smi_info->curr_msg = smi_info->waiting_msg
> (3)   smi_info->waiting_msg = NULL
> (4)   smi_info->handlers->start_transaction()
>
>     <asynchronously called>
>     smi_event_handler()
> (5)  handle_transaction_done()
>       smi_info->curr_msg = NULL
>       deliver_recv_msg()
>        ipmi_smi_msg_received()
>         intf->curr_msg = NULL
>
> If the kernel panics before (1), the requested message will be
> lost.  But it can't be helped.
>
> If the kernel panics before (2), new message sent by
> send_panic_events() is queued to intf->xmit_msgs because
> intf->curr_msg is non-NULL.  But the new message will be never
> sent because no one sends intf->curr_msg.  As the result, the
> kernel hangs up.
>
> If the kernel panics before (3), intf->curr_msg will be sent by
> set_run_to_completion().  It's no problem.
>
> If the kernel panics before (4), intf->curr_msg will be lost.
> However, messages on intf->xmit_msgs will be handled.
>
> If the kernel panics before (5), we try to continue running the
> state machine.  It may successfully complete.
>
> If the kernel panics after (5), we will miss the response message
> handling, but it's not much problem in the panic context.
>
> This patch tries to handle messages in intf->curr_msg and
> intf->xmit_msgs only once without losing them.  To achieve this,
> this patch does that:
>   - if a message is in intf->curr_msg or intf->xmit_msgs and
>     start_transaction() for the message hasn't been done yet,
>     resend it
>   - if start_transaction() for a message has been called,
>     just continue to run the state machine
>   - if the transaction has been completed, do nothing
>
> >From the perspective of implementation, these are done by keeping
> smi_info->waiting_msg until start_transaction() is completed and
> by keeping new flag IPMI_MSG_RESEND_ON_PANIC just before starting
> the state machine.
>
> Signed-off-by: Hidehiro Kawai <hidehiro.kawai.ez@hitachi.com>
> ---
>  drivers/char/ipmi/ipmi_msghandler.c |   36 +++++++++++++++++++++++++++++++++++
>  drivers/char/ipmi/ipmi_si_intf.c    |    5 ++++-
>  include/linux/ipmi_smi.h            |    5 +++++
>  3 files changed, 45 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
> index 5a2d9fe..3dcd814 100644
> --- a/drivers/char/ipmi/ipmi_msghandler.c
> +++ b/drivers/char/ipmi/ipmi_msghandler.c
> @@ -1493,6 +1493,8 @@ static struct ipmi_smi_msg *smi_add_send_msg(ipmi_smi_t intf,
>  					     struct ipmi_smi_msg *smi_msg,
>  					     int priority)
>  {
> +	smi_msg->flags |= IPMI_MSG_RESEND_ON_PANIC;
> +
>  	if (intf->curr_msg) {
>  		if (priority > 0)
>  			list_add_tail(&smi_msg->link, &intf->hp_xmit_msgs);
> @@ -4223,6 +4225,7 @@ struct ipmi_smi_msg *ipmi_alloc_smi_msg(void)
>  		rv->done = free_smi_msg;
>  		rv->user_data = NULL;
>  		atomic_inc(&smi_msg_inuse_count);
> +		rv->flags = 0;
>  	}
>  	return rv;
>  }
> @@ -4531,7 +4534,40 @@ static int panic_event(struct notifier_block *this,
>  			spin_unlock(&intf->waiting_rcv_msgs_lock);
>  
>  		intf->run_to_completion = 1;
> +restart:
>  		intf->handlers->set_run_to_completion(intf->send_info, 1);
> +
> +		if (intf->curr_msg) {
> +			/*
> +			 * This can happen if the kernel panics before
> +			 * setting msg to smi_info->waiting_msg or while
> +			 * processing a response.  For the former case, we
> +			 * resend the message by re-queueing it.  For the
> +			 * latter case, we simply ignore it because handling
> +			 * response is not much meaningful in the panic
> +			 * context.
> +			 */
> +
> +			/*
> +			 * Since we want to send the current message first,
> +			 * re-queue it into the high-prioritized queue.
> +			 */
> +			if (intf->curr_msg->flags & IPMI_MSG_RESEND_ON_PANIC)
> +				list_add(&intf->curr_msg->link,
> +					 &intf->hp_xmit_msgs);
> +
> +			intf->curr_msg = NULL;
> +		}
> +
> +		if (!list_empty(&intf->hp_xmit_msgs) ||
> +		    !list_empty(&intf->xmit_msgs)) {
> +			/*
> +			 * This can happen if the kernel panics while
> +			 * processing a response.  Kick the queue and restart.
> +			 */
> +			smi_recv_tasklet((unsigned long)intf);
> +			goto restart;
> +		}
>  	}
>  
>  #ifdef CONFIG_IPMI_PANIC_EVENT
> diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> index 814b7b7..c5c7806 100644
> --- a/drivers/char/ipmi/ipmi_si_intf.c
> +++ b/drivers/char/ipmi/ipmi_si_intf.c
> @@ -383,7 +383,6 @@ static enum si_sm_result start_next_msg(struct smi_info *smi_info)
>  		int err;
>  
>  		smi_info->curr_msg = smi_info->waiting_msg;
> -		smi_info->waiting_msg = NULL;
>  		debug_timestamp("Start2");
>  		err = atomic_notifier_call_chain(&xaction_notifier_list,
>  				0, smi_info);
> @@ -401,6 +400,7 @@ static enum si_sm_result start_next_msg(struct smi_info *smi_info)
>  		rv = SI_SM_CALL_WITHOUT_DELAY;
>  	}
>   out:
> +	smi_info->waiting_msg = NULL;
>  	return rv;
>  }
>  
> @@ -804,6 +804,9 @@ static enum si_sm_result smi_event_handler(struct smi_info *smi_info,
>  {
>  	enum si_sm_result si_sm_result;
>  
> +	if (smi_info->curr_msg)
> +		smi_info->curr_msg->flags &= ~(IPMI_MSG_RESEND_ON_PANIC);
> +
>   restart:
>  	/*
>  	 * There used to be a loop here that waited a little while
> diff --git a/include/linux/ipmi_smi.h b/include/linux/ipmi_smi.h
> index ba57fb1..1200872 100644
> --- a/include/linux/ipmi_smi.h
> +++ b/include/linux/ipmi_smi.h
> @@ -47,6 +47,9 @@
>  /* Structure for the low-level drivers. */
>  typedef struct ipmi_smi *ipmi_smi_t;
>  
> +/* Flags for flags member of struct ipmi_smi_msg */
> +#define IPMI_MSG_RESEND_ON_PANIC	1 /* If set, resend in panic_event() */
> +
>  /*
>   * Messages to/from the lower layer.  The smi interface will take one
>   * of these to send. After the send has occurred and a response has
> @@ -75,6 +78,8 @@ struct ipmi_smi_msg {
>  	/* Will be called when the system is done with the message
>  	   (presumably to free it). */
>  	void (*done)(struct ipmi_smi_msg *msg);
> +
> +	int flags;
>  };
>  
>  struct ipmi_smi_handlers {
>
>

RE: [PATCH 6/7] ipmi: Handle queued messages more certainly on panic

[河合英宏 / KAWAI，HIDEHIRO]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 8531 bytes --]

Hello Corey,

> -----Original Message-----
> From: Corey Minyard [mailto:tcminyard@gmail.com] On Behalf Of Corey Minyard
> Sent: Wednesday, August 12, 2015 1:13 PM
> To: 河合英宏 / KAWAI，HIDEHIRO
> Cc: openipmi-developer@lists.sourceforge.net; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 6/7] ipmi: Handle queued messages more certainly on panic
> 
> On 07/27/2015 12:55 AM, Hidehiro Kawai wrote:
> > panic_event() called as a panic notifier tries to flush queued
> > messages, but it can't handle them if the kernel panic happens
> > while processing a message.  What happens depends on when the
> > kernel panics.
> 
> Sorry this took so long, I've been traveling.

No problem.

> I have queued the patches before this one.  They all look good and
> necessary.

Thank you for reviewing!
 
> I'm not so sure about this patch.  It looks like the only thing that is
> a real issue is #2 below.
> It's not so important to avoid dropping messages.

Initially I thought dropping middle of queued messages breaks
some consistencies if a message depends on the preceding dropped
message.  However, userland tools normally issue request messages
in sequential manner, so the above situation wouldn't happen.
Now, I think dropping a message is OK.

> Can this be simplified somehow to work around the issue at panic time if
> intf->curr_msg is set and smi_info->waiting_msg is not?

There are two cases where intf->curr_msg is set and
smi_info->waiting_msg is not; one is before (2) and the other
is after (3).  If we decide to drop intf->curr_msg in both cases,
I can simplify this patch somewhat.

Regards,

Hidehiro Kawai
Hitachi, Ltd. Research & Development Group

> 
> Thank you,
> 
> -corey
> 
> >
> > Here is the summary of message sending process.
> >
> >     smi_send()
> >      smi_add_send_msg()
> > (1)   intf->curr_msg = msg
> >      sender()
> > (2)   smi_info->waiting_msg = msg
> >
> >     <asynchronously called>
> >     check_start_timer_thread()
> >      start_next_msg()
> >       smi_info->curr_msg = smi_info->waiting_msg
> > (3)   smi_info->waiting_msg = NULL
> > (4)   smi_info->handlers->start_transaction()
> >
> >     <asynchronously called>
> >     smi_event_handler()
> > (5)  handle_transaction_done()
> >       smi_info->curr_msg = NULL
> >       deliver_recv_msg()
> >        ipmi_smi_msg_received()
> >         intf->curr_msg = NULL
> >
> > If the kernel panics before (1), the requested message will be
> > lost.  But it can't be helped.
> >
> > If the kernel panics before (2), new message sent by
> > send_panic_events() is queued to intf->xmit_msgs because
> > intf->curr_msg is non-NULL.  But the new message will be never
> > sent because no one sends intf->curr_msg.  As the result, the
> > kernel hangs up.
> >
> > If the kernel panics before (3), intf->curr_msg will be sent by
> > set_run_to_completion().  It's no problem.
> >
> > If the kernel panics before (4), intf->curr_msg will be lost.
> > However, messages on intf->xmit_msgs will be handled.
> >
> > If the kernel panics before (5), we try to continue running the
> > state machine.  It may successfully complete.
> >
> > If the kernel panics after (5), we will miss the response message
> > handling, but it's not much problem in the panic context.
> >
> > This patch tries to handle messages in intf->curr_msg and
> > intf->xmit_msgs only once without losing them.  To achieve this,
> > this patch does that:
> >   - if a message is in intf->curr_msg or intf->xmit_msgs and
> >     start_transaction() for the message hasn't been done yet,
> >     resend it
> >   - if start_transaction() for a message has been called,
> >     just continue to run the state machine
> >   - if the transaction has been completed, do nothing
> >
> > >From the perspective of implementation, these are done by keeping
> > smi_info->waiting_msg until start_transaction() is completed and
> > by keeping new flag IPMI_MSG_RESEND_ON_PANIC just before starting
> > the state machine.
> >
> > Signed-off-by: Hidehiro Kawai <hidehiro.kawai.ez@hitachi.com>
> > ---
> >  drivers/char/ipmi/ipmi_msghandler.c |   36 +++++++++++++++++++++++++++++++++++
> >  drivers/char/ipmi/ipmi_si_intf.c    |    5 ++++-
> >  include/linux/ipmi_smi.h            |    5 +++++
> >  3 files changed, 45 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
> > index 5a2d9fe..3dcd814 100644
> > --- a/drivers/char/ipmi/ipmi_msghandler.c
> > +++ b/drivers/char/ipmi/ipmi_msghandler.c
> > @@ -1493,6 +1493,8 @@ static struct ipmi_smi_msg *smi_add_send_msg(ipmi_smi_t intf,
> >  					     struct ipmi_smi_msg *smi_msg,
> >  					     int priority)
> >  {
> > +	smi_msg->flags |= IPMI_MSG_RESEND_ON_PANIC;
> > +
> >  	if (intf->curr_msg) {
> >  		if (priority > 0)
> >  			list_add_tail(&smi_msg->link, &intf->hp_xmit_msgs);
> > @@ -4223,6 +4225,7 @@ struct ipmi_smi_msg *ipmi_alloc_smi_msg(void)
> >  		rv->done = free_smi_msg;
> >  		rv->user_data = NULL;
> >  		atomic_inc(&smi_msg_inuse_count);
> > +		rv->flags = 0;
> >  	}
> >  	return rv;
> >  }
> > @@ -4531,7 +4534,40 @@ static int panic_event(struct notifier_block *this,
> >  			spin_unlock(&intf->waiting_rcv_msgs_lock);
> >
> >  		intf->run_to_completion = 1;
> > +restart:
> >  		intf->handlers->set_run_to_completion(intf->send_info, 1);
> > +
> > +		if (intf->curr_msg) {
> > +			/*
> > +			 * This can happen if the kernel panics before
> > +			 * setting msg to smi_info->waiting_msg or while
> > +			 * processing a response.  For the former case, we
> > +			 * resend the message by re-queueing it.  For the
> > +			 * latter case, we simply ignore it because handling
> > +			 * response is not much meaningful in the panic
> > +			 * context.
> > +			 */
> > +
> > +			/*
> > +			 * Since we want to send the current message first,
> > +			 * re-queue it into the high-prioritized queue.
> > +			 */
> > +			if (intf->curr_msg->flags & IPMI_MSG_RESEND_ON_PANIC)
> > +				list_add(&intf->curr_msg->link,
> > +					 &intf->hp_xmit_msgs);
> > +
> > +			intf->curr_msg = NULL;
> > +		}
> > +
> > +		if (!list_empty(&intf->hp_xmit_msgs) ||
> > +		    !list_empty(&intf->xmit_msgs)) {
> > +			/*
> > +			 * This can happen if the kernel panics while
> > +			 * processing a response.  Kick the queue and restart.
> > +			 */
> > +			smi_recv_tasklet((unsigned long)intf);
> > +			goto restart;
> > +		}
> >  	}
> >
> >  #ifdef CONFIG_IPMI_PANIC_EVENT
> > diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> > index 814b7b7..c5c7806 100644
> > --- a/drivers/char/ipmi/ipmi_si_intf.c
> > +++ b/drivers/char/ipmi/ipmi_si_intf.c
> > @@ -383,7 +383,6 @@ static enum si_sm_result start_next_msg(struct smi_info *smi_info)
> >  		int err;
> >
> >  		smi_info->curr_msg = smi_info->waiting_msg;
> > -		smi_info->waiting_msg = NULL;
> >  		debug_timestamp("Start2");
> >  		err = atomic_notifier_call_chain(&xaction_notifier_list,
> >  				0, smi_info);
> > @@ -401,6 +400,7 @@ static enum si_sm_result start_next_msg(struct smi_info *smi_info)
> >  		rv = SI_SM_CALL_WITHOUT_DELAY;
> >  	}
> >   out:
> > +	smi_info->waiting_msg = NULL;
> >  	return rv;
> >  }
> >
> > @@ -804,6 +804,9 @@ static enum si_sm_result smi_event_handler(struct smi_info *smi_info,
> >  {
> >  	enum si_sm_result si_sm_result;
> >
> > +	if (smi_info->curr_msg)
> > +		smi_info->curr_msg->flags &= ~(IPMI_MSG_RESEND_ON_PANIC);
> > +
> >   restart:
> >  	/*
> >  	 * There used to be a loop here that waited a little while
> > diff --git a/include/linux/ipmi_smi.h b/include/linux/ipmi_smi.h
> > index ba57fb1..1200872 100644
> > --- a/include/linux/ipmi_smi.h
> > +++ b/include/linux/ipmi_smi.h
> > @@ -47,6 +47,9 @@
> >  /* Structure for the low-level drivers. */
> >  typedef struct ipmi_smi *ipmi_smi_t;
> >
> > +/* Flags for flags member of struct ipmi_smi_msg */
> > +#define IPMI_MSG_RESEND_ON_PANIC	1 /* If set, resend in panic_event() */
> > +
> >  /*
> >   * Messages to/from the lower layer.  The smi interface will take one
> >   * of these to send. After the send has occurred and a response has
> > @@ -75,6 +78,8 @@ struct ipmi_smi_msg {
> >  	/* Will be called when the system is done with the message
> >  	   (presumably to free it). */
> >  	void (*done)(struct ipmi_smi_msg *msg);
> > +
> > +	int flags;
> >  };
> >
> >  struct ipmi_smi_handlers {
> >
> >

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 6/7] ipmi: Handle queued messages more certainly on panic

[Corey Minyard]

On 08/17/2015 08:59 PM, 河合英宏 / KAWAI，HIDEHIRO wrote:
> Hello Corey,
>
>> -----Original Message-----
>> From: Corey Minyard [mailto:tcminyard@gmail.com] On Behalf Of Corey Minyard
>> Sent: Wednesday, August 12, 2015 1:13 PM
>> To: 河合英宏 / KAWAI，HIDEHIRO
>> Cc: openipmi-developer@lists.sourceforge.net; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH 6/7] ipmi: Handle queued messages more certainly on panic
>>
>> On 07/27/2015 12:55 AM, Hidehiro Kawai wrote:
>>> panic_event() called as a panic notifier tries to flush queued
>>> messages, but it can't handle them if the kernel panic happens
>>> while processing a message.  What happens depends on when the
>>> kernel panics.
>> Sorry this took so long, I've been traveling.
> No problem.
>
>> I have queued the patches before this one.  They all look good and
>> necessary.
> Thank you for reviewing!
>  
>> I'm not so sure about this patch.  It looks like the only thing that is
>> a real issue is #2 below.
>> It's not so important to avoid dropping messages.
> Initially I thought dropping middle of queued messages breaks
> some consistencies if a message depends on the preceding dropped
> message.  However, userland tools normally issue request messages
> in sequential manner, so the above situation wouldn't happen.
> Now, I think dropping a message is OK.
>
>> Can this be simplified somehow to work around the issue at panic time if
>> intf->curr_msg is set and smi_info->waiting_msg is not?
> There are two cases where intf->curr_msg is set and
> smi_info->waiting_msg is not; one is before (2) and the other
> is after (3).  If we decide to drop intf->curr_msg in both cases,
> I can simplify this patch somewhat.

Yes, please do.

-corey

> Regards,
>
> Hidehiro Kawai
> Hitachi, Ltd. Research & Development Group
>
>> Thank you,
>>
>> -corey
>>
>>> Here is the summary of message sending process.
>>>
>>>     smi_send()
>>>      smi_add_send_msg()
>>> (1)   intf->curr_msg = msg
>>>      sender()
>>> (2)   smi_info->waiting_msg = msg
>>>
>>>     <asynchronously called>
>>>     check_start_timer_thread()
>>>      start_next_msg()
>>>       smi_info->curr_msg = smi_info->waiting_msg
>>> (3)   smi_info->waiting_msg = NULL
>>> (4)   smi_info->handlers->start_transaction()
>>>
>>>     <asynchronously called>
>>>     smi_event_handler()
>>> (5)  handle_transaction_done()
>>>       smi_info->curr_msg = NULL
>>>       deliver_recv_msg()
>>>        ipmi_smi_msg_received()
>>>         intf->curr_msg = NULL
>>>
>>> If the kernel panics before (1), the requested message will be
>>> lost.  But it can't be helped.
>>>
>>> If the kernel panics before (2), new message sent by
>>> send_panic_events() is queued to intf->xmit_msgs because
>>> intf->curr_msg is non-NULL.  But the new message will be never
>>> sent because no one sends intf->curr_msg.  As the result, the
>>> kernel hangs up.
>>>
>>> If the kernel panics before (3), intf->curr_msg will be sent by
>>> set_run_to_completion().  It's no problem.
>>>
>>> If the kernel panics before (4), intf->curr_msg will be lost.
>>> However, messages on intf->xmit_msgs will be handled.
>>>
>>> If the kernel panics before (5), we try to continue running the
>>> state machine.  It may successfully complete.
>>>
>>> If the kernel panics after (5), we will miss the response message
>>> handling, but it's not much problem in the panic context.
>>>
>>> This patch tries to handle messages in intf->curr_msg and
>>> intf->xmit_msgs only once without losing them.  To achieve this,
>>> this patch does that:
>>>   - if a message is in intf->curr_msg or intf->xmit_msgs and
>>>     start_transaction() for the message hasn't been done yet,
>>>     resend it
>>>   - if start_transaction() for a message has been called,
>>>     just continue to run the state machine
>>>   - if the transaction has been completed, do nothing
>>>
>>> >From the perspective of implementation, these are done by keeping
>>> smi_info->waiting_msg until start_transaction() is completed and
>>> by keeping new flag IPMI_MSG_RESEND_ON_PANIC just before starting
>>> the state machine.
>>>
>>> Signed-off-by: Hidehiro Kawai <hidehiro.kawai.ez@hitachi.com>
>>> ---
>>>  drivers/char/ipmi/ipmi_msghandler.c |   36 +++++++++++++++++++++++++++++++++++
>>>  drivers/char/ipmi/ipmi_si_intf.c    |    5 ++++-
>>>  include/linux/ipmi_smi.h            |    5 +++++
>>>  3 files changed, 45 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
>>> index 5a2d9fe..3dcd814 100644
>>> --- a/drivers/char/ipmi/ipmi_msghandler.c
>>> +++ b/drivers/char/ipmi/ipmi_msghandler.c
>>> @@ -1493,6 +1493,8 @@ static struct ipmi_smi_msg *smi_add_send_msg(ipmi_smi_t intf,
>>>  					     struct ipmi_smi_msg *smi_msg,
>>>  					     int priority)
>>>  {
>>> +	smi_msg->flags |= IPMI_MSG_RESEND_ON_PANIC;
>>> +
>>>  	if (intf->curr_msg) {
>>>  		if (priority > 0)
>>>  			list_add_tail(&smi_msg->link, &intf->hp_xmit_msgs);
>>> @@ -4223,6 +4225,7 @@ struct ipmi_smi_msg *ipmi_alloc_smi_msg(void)
>>>  		rv->done = free_smi_msg;
>>>  		rv->user_data = NULL;
>>>  		atomic_inc(&smi_msg_inuse_count);
>>> +		rv->flags = 0;
>>>  	}
>>>  	return rv;
>>>  }
>>> @@ -4531,7 +4534,40 @@ static int panic_event(struct notifier_block *this,
>>>  			spin_unlock(&intf->waiting_rcv_msgs_lock);
>>>
>>>  		intf->run_to_completion = 1;
>>> +restart:
>>>  		intf->handlers->set_run_to_completion(intf->send_info, 1);
>>> +
>>> +		if (intf->curr_msg) {
>>> +			/*
>>> +			 * This can happen if the kernel panics before
>>> +			 * setting msg to smi_info->waiting_msg or while
>>> +			 * processing a response.  For the former case, we
>>> +			 * resend the message by re-queueing it.  For the
>>> +			 * latter case, we simply ignore it because handling
>>> +			 * response is not much meaningful in the panic
>>> +			 * context.
>>> +			 */
>>> +
>>> +			/*
>>> +			 * Since we want to send the current message first,
>>> +			 * re-queue it into the high-prioritized queue.
>>> +			 */
>>> +			if (intf->curr_msg->flags & IPMI_MSG_RESEND_ON_PANIC)
>>> +				list_add(&intf->curr_msg->link,
>>> +					 &intf->hp_xmit_msgs);
>>> +
>>> +			intf->curr_msg = NULL;
>>> +		}
>>> +
>>> +		if (!list_empty(&intf->hp_xmit_msgs) ||
>>> +		    !list_empty(&intf->xmit_msgs)) {
>>> +			/*
>>> +			 * This can happen if the kernel panics while
>>> +			 * processing a response.  Kick the queue and restart.
>>> +			 */
>>> +			smi_recv_tasklet((unsigned long)intf);
>>> +			goto restart;
>>> +		}
>>>  	}
>>>
>>>  #ifdef CONFIG_IPMI_PANIC_EVENT
>>> diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
>>> index 814b7b7..c5c7806 100644
>>> --- a/drivers/char/ipmi/ipmi_si_intf.c
>>> +++ b/drivers/char/ipmi/ipmi_si_intf.c
>>> @@ -383,7 +383,6 @@ static enum si_sm_result start_next_msg(struct smi_info *smi_info)
>>>  		int err;
>>>
>>>  		smi_info->curr_msg = smi_info->waiting_msg;
>>> -		smi_info->waiting_msg = NULL;
>>>  		debug_timestamp("Start2");
>>>  		err = atomic_notifier_call_chain(&xaction_notifier_list,
>>>  				0, smi_info);
>>> @@ -401,6 +400,7 @@ static enum si_sm_result start_next_msg(struct smi_info *smi_info)
>>>  		rv = SI_SM_CALL_WITHOUT_DELAY;
>>>  	}
>>>   out:
>>> +	smi_info->waiting_msg = NULL;
>>>  	return rv;
>>>  }
>>>
>>> @@ -804,6 +804,9 @@ static enum si_sm_result smi_event_handler(struct smi_info *smi_info,
>>>  {
>>>  	enum si_sm_result si_sm_result;
>>>
>>> +	if (smi_info->curr_msg)
>>> +		smi_info->curr_msg->flags &= ~(IPMI_MSG_RESEND_ON_PANIC);
>>> +
>>>   restart:
>>>  	/*
>>>  	 * There used to be a loop here that waited a little while
>>> diff --git a/include/linux/ipmi_smi.h b/include/linux/ipmi_smi.h
>>> index ba57fb1..1200872 100644
>>> --- a/include/linux/ipmi_smi.h
>>> +++ b/include/linux/ipmi_smi.h
>>> @@ -47,6 +47,9 @@
>>>  /* Structure for the low-level drivers. */
>>>  typedef struct ipmi_smi *ipmi_smi_t;
>>>
>>> +/* Flags for flags member of struct ipmi_smi_msg */
>>> +#define IPMI_MSG_RESEND_ON_PANIC	1 /* If set, resend in panic_event() */
>>> +
>>>  /*
>>>   * Messages to/from the lower layer.  The smi interface will take one
>>>   * of these to send. After the send has occurred and a response has
>>> @@ -75,6 +78,8 @@ struct ipmi_smi_msg {
>>>  	/* Will be called when the system is done with the message
>>>  	   (presumably to free it). */
>>>  	void (*done)(struct ipmi_smi_msg *msg);
>>> +
>>> +	int flags;
>>>  };
>>>
>>>  struct ipmi_smi_handlers {
>>>
>>>

[PATCH 4/7] ipmi: Avoid touching possible corrupted lists in the panic context

[Hidehiro Kawai]

When processing queued messages in the panic context, IPMI driver
tries to do it without any locking to avoid deadlocks.  However,
this means we can touch a corrupted list if the kernel panicked
while manipulating the list.  Fortunately, current `add-tail and
del-from-head' style implementation won't touch the corrupted part,
but it is inherently risky.

To get rid of the risk, this patch re-initializes the message lists
on panic if the related spinlock has already been acquired.  As the
result, we may lose queued messages, but it's not so painful.
Droping messages on the received message list is also less
problematic because no one can respond the received messages.

Signed-off-by: Hidehiro Kawai <hidehiro.kawai.ez@hitachi.com>
---
 drivers/char/ipmi/ipmi_msghandler.c |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
index f1ecd25..e7d84482 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -4507,6 +4507,23 @@ static int panic_event(struct notifier_block *this,
 			/* Interface is not ready. */
 			continue;
 
+		/*
+		 * If we were interrupted while locking xmit_msgs_lock or
+		 * waiting_rcv_msgs_lock, the corresponding list may be
+		 * corrupted.  In this case, drop itmes on the list for
+		 * the safety.
+		 */
+		if (!spin_trylock(&intf->xmit_msgs_lock)) {
+			INIT_LIST_HEAD(&intf->xmit_msgs);
+			INIT_LIST_HEAD(&intf->hp_xmit_msgs);
+		} else
+			spin_unlock(&intf->xmit_msgs_lock);
+
+		if (!spin_trylock(&intf->waiting_rcv_msgs_lock))
+			INIT_LIST_HEAD(&intf->waiting_rcv_msgs);
+		else
+			spin_unlock(&intf->waiting_rcv_msgs_lock);
+
 		intf->run_to_completion = 1;
 		intf->handlers->set_run_to_completion(intf->send_info, 1);
 	}

[PATCH 3/7] ipmi: Don't flush messages in sneder() in run-to-completion mode

[Hidehiro Kawai]

When flushing queued messages in run-to-completion mode,
smi_event_handler() is recursively called.

flush_messages()
 smi_event_handler()
  handle_transaction_done()
   deliver_recv_msg()
    ipmi_smi_msg_received()
     smi_recv_tasklet()
      sender()
       flush_messages()
        smi_event_handler()
         ...

The depth of the recursive call depends on the number of queued
messages, so it can cause a stack overflow if many messages have
been queued.

To solve this problem, this patch removes flush_messages()
from sender()@ipmi_si_intf.c.  Instead, add flush_messages() to
caller side of sender() if needed.  Additionally, to implement this,
add new handler flush_messages to struct ipmi_smi_handlers.

Signed-off-by: Hidehiro Kawai <hidehiro.kawai.ez@hitachi.com>
---
 drivers/char/ipmi/ipmi_msghandler.c |    3 +++
 drivers/char/ipmi/ipmi_si_intf.c    |    5 +++--
 include/linux/ipmi_smi.h            |    5 +++++
 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
index a6e6ec0..f1ecd25 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -4291,6 +4291,9 @@ static void ipmi_panic_request_and_wait(ipmi_smi_t           intf,
 			    0, 1); /* Don't retry, and don't wait. */
 	if (rv)
 		atomic_sub(2, &panic_done_count);
+	else if (intf->handlers->flush_messages)
+		intf->handlers->flush_messages(intf->send_info);
+
 	while (atomic_read(&panic_done_count) != 0)
 		ipmi_poll(intf);
 }
diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 660e53b..814b7b7 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -928,8 +928,9 @@ static void check_start_timer_thread(struct smi_info *smi_info)
 	}
 }
 
-static void flush_messages(struct smi_info *smi_info)
+static void flush_messages(void *send_info)
 {
+	struct smi_info *smi_info = send_info;
 	enum si_sm_result result;
 
 	/*
@@ -958,7 +959,6 @@ static void sender(void                *send_info,
 		 */
 		smi_info->waiting_msg = msg;
 
-		flush_messages(smi_info);
 		return;
 	}
 
@@ -1264,6 +1264,7 @@ static void set_maintenance_mode(void *send_info, bool enable)
 	.set_need_watch		= set_need_watch,
 	.set_maintenance_mode   = set_maintenance_mode,
 	.set_run_to_completion  = set_run_to_completion,
+	.flush_messages		= flush_messages,
 	.poll			= poll,
 };
 
diff --git a/include/linux/ipmi_smi.h b/include/linux/ipmi_smi.h
index 0b1e569..ba57fb1 100644
--- a/include/linux/ipmi_smi.h
+++ b/include/linux/ipmi_smi.h
@@ -115,6 +115,11 @@ struct ipmi_smi_handlers {
 	   implement it. */
 	void (*set_need_watch)(void *send_info, bool enable);
 
+	/*
+	 * Called when flushing all pending messages.
+	 */
+	void (*flush_messages)(void *send_info);
+
 	/* Called when the interface should go into "run to
 	   completion" mode.  If this call sets the value to true, the
 	   interface should make sure that all messages are flushed

[PATCH 1/7] ipmi: Remove unneeded set_run_to_completion call

[Hidehiro Kawai]

send_panic_events() calls intf->handlers->set_run_to_completion(),
but it has already been done in the caller function panic_event().
Remove it from send_panic_events().

Signed-off-by: Hidehiro Kawai <hidehiro.kawai.ez@hitachi.com>
---
 drivers/char/ipmi/ipmi_msghandler.c |    2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
index bf75f63..a6e6ec0 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -4364,9 +4364,7 @@ static void send_panic_events(char *str)
 			/* Interface is not ready. */
 			continue;
 
-		intf->run_to_completion = 1;
 		/* Send the event announcing the panic. */
-		intf->handlers->set_run_to_completion(intf->send_info, 1);
 		ipmi_panic_request_and_wait(intf, &addr, &msg);
 	}
 

[PATCH 2/7] ipmi: Factor out message flushing procedure

[Hidehiro Kawai]

Factor out message flushing procedure which is used in run-to-completion
mode.  This patch doesn't change the logic.

Signed-off-by: Hidehiro Kawai <hidehiro.kawai.ez@hitachi.com>
---
 drivers/char/ipmi/ipmi_si_intf.c |   39 ++++++++++++++++++--------------------
 1 file changed, 18 insertions(+), 21 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 8a45e92..660e53b 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -928,11 +928,25 @@ static void check_start_timer_thread(struct smi_info *smi_info)
 	}
 }
 
+static void flush_messages(struct smi_info *smi_info)
+{
+	enum si_sm_result result;
+
+	/*
+	 * Currently, this function is called only in run-to-completion
+	 * mode.  This means we are single-threaded, no need for locks.
+	 */
+	result = smi_event_handler(smi_info, 0);
+	while (result != SI_SM_IDLE) {
+		udelay(SI_SHORT_TIMEOUT_USEC);
+		result = smi_event_handler(smi_info, SI_SHORT_TIMEOUT_USEC);
+	}
+}
+
 static void sender(void                *send_info,
 		   struct ipmi_smi_msg *msg)
 {
 	struct smi_info   *smi_info = send_info;
-	enum si_sm_result result;
 	unsigned long     flags;
 
 	debug_timestamp("Enqueue");
@@ -944,17 +958,7 @@ static void sender(void                *send_info,
 		 */
 		smi_info->waiting_msg = msg;
 
-		/*
-		 * Run to completion means we are single-threaded, no
-		 * need for locks.
-		 */
-
-		result = smi_event_handler(smi_info, 0);
-		while (result != SI_SM_IDLE) {
-			udelay(SI_SHORT_TIMEOUT_USEC);
-			result = smi_event_handler(smi_info,
-						   SI_SHORT_TIMEOUT_USEC);
-		}
+		flush_messages(smi_info);
 		return;
 	}
 
@@ -975,17 +979,10 @@ static void sender(void                *send_info,
 static void set_run_to_completion(void *send_info, bool i_run_to_completion)
 {
 	struct smi_info   *smi_info = send_info;
-	enum si_sm_result result;
 
 	smi_info->run_to_completion = i_run_to_completion;
-	if (i_run_to_completion) {
-		result = smi_event_handler(smi_info, 0);
-		while (result != SI_SM_IDLE) {
-			udelay(SI_SHORT_TIMEOUT_USEC);
-			result = smi_event_handler(smi_info,
-						   SI_SHORT_TIMEOUT_USEC);
-		}
-	}
+	if (i_run_to_completion)
+		flush_messages(smi_info);
 }
 
 /*

[PATCH 7/7] ipmi/kcs: Don't run the KCS state machine when it is KCS_IDLE

[Hidehiro Kawai]

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 <hidehiro.kawai.ez@hitachi.com>
---
 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;

Re: [PATCH 7/7] ipmi/kcs: Don't run the KCS state machine when it is KCS_IDLE

[Corey Minyard]

This patch will break ATN handling on the interfaces.  So we can't do this.

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 <hidehiro.kawai.ez@hitachi.com>
> ---
>  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;
>
>

RE: [PATCH 7/7] ipmi/kcs: Don't run the KCS state machine when it is KCS_IDLE

[河合英宏 / KAWAI，HIDEHIRO]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2492 bytes --]

> 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.

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 <hidehiro.kawai.ez@hitachi.com>
> > ---
> >  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;
> >
> >

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 7/7] ipmi/kcs: Don't run the KCS state machine when it is KCS_IDLE

[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. 
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 <hidehiro.kawai.ez@hitachi.com>
>>> ---
>>>  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;
>>>
>>>

RE: [PATCH 7/7] ipmi/kcs: Don't run the KCS state machine when it is KCS_IDLE

[河合英宏 / KAWAI，HIDEHIRO]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3501 bytes --]

> 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().

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 <hidehiro.kawai.ez@hitachi.com>
> >>> ---
> >>>  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;
> >>>
> >>>

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 7/7] ipmi/kcs: Don't run the KCS state machine when it is KCS_IDLE

[Corey Minyard]

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 <hidehiro.kawai.ez@hitachi.com>
>>>>> ---
>>>>>  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;
>>>>>
>>>>>

RE: [PATCH 7/7] ipmi/kcs: Don't run the KCS state machine when it is KCS_IDLE

[河合英宏 / KAWAI，HIDEHIRO]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 4206 bytes --]

> From: Corey Minyard [mailto:tcminyard@gmail.com] On Behalf Of Corey Minyard
> 
> 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.

Sorry for my insufficient explanation.  I assume the case where
IBF bit is always 1.  I don't know what happens when
BMC hangs up, but I guess IBF stays in 1 because my server's
BMC behaves as such while rebooting.

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 <hidehiro.kawai.ez@hitachi.com>
> >>>>> ---
> >>>>>  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;
> >>>>>
> >>>>>

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 7/7] ipmi/kcs: Don't run the KCS state machine when it is KCS_IDLE

[Corey Minyard]

On 08/24/2015 10:53 PM, 河合英宏 / KAWAI，HIDEHIRO wrote:
>> From: Corey Minyard [mailto:tcminyard@gmail.com] On Behalf Of Corey Minyard
>>
>> 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.
> Sorry for my insufficient explanation.  I assume the case where
> IBF bit is always 1.  I don't know what happens when
> BMC hangs up, but I guess IBF stays in 1 because my server's
> BMC behaves as such while rebooting.
>
Ok, your patch above makes sense, then.  IBF is irrelevant when in idle
state,
so ignore it then, and then in your case it will return KCS_IDLE and
cause that
operation to complete.  I'm ok with the patch you posted above, I think
it will
work correctly and solve the problem.

I would like a detailed comment, though, so people (forgetful people
like me :)
can figure out why it is there.  I'd also like to save this one until
4.4 to give it some
time in linux-next for people to find issues.

Thanks,

-corey

RE: [PATCH 7/7] ipmi/kcs: Don't run the KCS state machine when it is KCS_IDLE

[河合英宏 / KAWAI，HIDEHIRO]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2792 bytes --]

> From: Corey Minyard [mailto:tcminyard@gmail.com] On Behalf Of Corey Minyard
> 
> On 08/24/2015 10:53 PM, 河合英宏 / KAWAI，HIDEHIRO wrote:
> >> From: Corey Minyard [mailto:tcminyard@gmail.com] On Behalf Of Corey Minyard
> >>
> >> 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.
> > Sorry for my insufficient explanation.  I assume the case where
> > IBF bit is always 1.  I don't know what happens when
> > BMC hangs up, but I guess IBF stays in 1 because my server's
> > BMC behaves as such while rebooting.
> >
> Ok, your patch above makes sense, then.  IBF is irrelevant when in idle
> state,
> so ignore it then, and then in your case it will return KCS_IDLE and
> cause that
> operation to complete.  I'm ok with the patch you posted above, I think
> it will
> work correctly and solve the problem.
> 
> I would like a detailed comment, though, so people (forgetful people
> like me :)
> can figure out why it is there.

Sure.  I'll post the revised version with detailed comment and
description later including PATCH 6/7.

Thanks,

Hidehiro Kawai

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥