ipmi: various fixes for panic notifier robustness

From the perspective of implementation, these are done by keeping

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

Quoted 1 line

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 <[email protected]>

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

drivers/char/ipmi/ipmi_msghandler.c +36 -0

index 5a2d9fe..3dcd814 100644
@@ -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

drivers/char/ipmi/ipmi_si_intf.c +4 -1

index 814b7b7..c5c7806 100644
@@ -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

include/linux/ipmi_smi.h +5 -0

index ba57fb1..1200872 100644
@@ -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 {

-----Original Message-----
From: Corey Minyard [mailto:[email protected]] On Behalf Of Corey Minyard
Sent: Wednesday, August 12, 2015 1:13 PM
To: 河合英宏 / KAWAI，HIDEHIRO
Cc: [email protected]; [email protected]
Subject: Re: [PATCH 6/7] ipmi: Handle queued messages more certainly on panic

Quoted 4 lines · 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.

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

Quoted 181 lines

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

Quoted 1 line

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 <[email protected]>

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

drivers/char/ipmi/ipmi_msghandler.c +36 -0

index 5a2d9fe..3dcd814 100644
@@ -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

drivers/char/ipmi/ipmi_si_intf.c +4 -1

index 814b7b7..c5c7806 100644
@@ -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

include/linux/ipmi_smi.h +5 -0

index ba57fb1..1200872 100644
@@ -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;
 };
 

include/linux/ipmi_smi.h struct ipmi_smi_msg { · line 85

Quoted 3 lines

include/linux/ipmi_smi.h +0 -0

 struct ipmi_smi_handlers {

Hello Corey,

Quoted 7 lines

-----Original Message-----
From: Corey Minyard [mailto:[email protected]] On Behalf Of Corey Minyard
Sent: Wednesday, August 12, 2015 1:13 PM
To: 河合英宏 / KAWAI，HIDEHIRO
Cc: [email protected]; [email protected]
Subject: Re: [PATCH 6/7] ipmi: Handle queued messages more certainly on panic

Quoted 6 lines

Quoted 4 lines · 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.

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

No problem.

Quoted 2 lines

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

Thank you for reviewing!

Quoted 3 lines

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.

Quoted 2 lines

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

Quoted 187 lines

Thank you,

-corey

Quoted 183 lines

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

Quoted 1 line

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 <[email protected]>

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

drivers/char/ipmi/ipmi_msghandler.c +36 -0

index 5a2d9fe..3dcd814 100644
@@ -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

drivers/char/ipmi/ipmi_si_intf.c +4 -1

index 814b7b7..c5c7806 100644
@@ -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

include/linux/ipmi_smi.h +5 -0

index ba57fb1..1200872 100644
@@ -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 {

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 <[email protected]>

---
 drivers/char/ipmi/ipmi_kcs_sm.c |    4 ++++
 1 file changed, 4 insertions(+)

drivers/char/ipmi/ipmi_kcs_sm.c +4 -0

index 8c25f59..0e187fb 100644
@@ -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;

From: Corey Minyard [mailto:[email protected]] On Behalf Of 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

Quoted 25 lines · 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 <[email protected]>

---
 drivers/char/ipmi/ipmi_kcs_sm.c |    4 ++++
 1 file changed, 4 insertions(+)

drivers/char/ipmi/ipmi_kcs_sm.c +0 -0

index 8c25f59..0e187fb 100644
@@ -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);
 

drivers/char/ipmi/ipmi_kcs_sm.c static enum si_sm_result kcs_event(struct si_sm_data *kcs, long time) · line 362

Quoted 9 lines

drivers/char/ipmi/ipmi_kcs_sm.c +4 -0

+	/* 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;

Quoted 3 lines

From: Corey Minyard [mailto:[email protected]] 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. */

(quoted diff) +1 -1

-	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

Quoted 41 lines

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

Quoted 34 lines · 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 <[email protected]>

---
 drivers/char/ipmi/ipmi_kcs_sm.c |    4 ++++
 1 file changed, 4 insertions(+)

drivers/char/ipmi/ipmi_kcs_sm.c +4 -0

index 8c25f59..0e187fb 100644
@@ -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;

From: Corey Minyard [mailto:[email protected]] On Behalf Of Corey Minyard

Quoted 10 lines · 河合英宏 / KAWAI，HIDEHIRO

Quoted 3 lines

From: Corey Minyard [mailto:[email protected]] 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. */

(quoted diff) +1 -1

-	if (!check_ibf(kcs, status, time))
+	if (kcs->state != KCS_IDLE && !check_ibf(kcs, status, time))

		return SI_SM_CALL_WITH_DELAY;

Quoted 2 lines

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

Quoted 38 lines

Regards,

Hidehiro Kawai
Hitachi, Ltd. Research & Development Group

Quoted 32 lines

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

Quoted 25 lines · 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 <[email protected]>

---
 drivers/char/ipmi/ipmi_kcs_sm.c |    4 ++++
 1 file changed, 4 insertions(+)

drivers/char/ipmi/ipmi_kcs_sm.c +0 -0

index 8c25f59..0e187fb 100644
@@ -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);
 

Quoted 9 lines

Quoted 9 lines

drivers/char/ipmi/ipmi_kcs_sm.c static enum si_sm_result kcs_event(struct si_sm_data *kcs, long time) · line 362

Quoted 9 lines

drivers/char/ipmi/ipmi_kcs_sm.c +4 -0

+	/* 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;

Quoted 8 lines

From: Corey Minyard [mailto:[email protected]] On Behalf Of Corey Minyard

Quoted 5 lines · 河合英宏 / KAWAI，HIDEHIRO

Quoted 3 lines

From: Corey Minyard [mailto:[email protected]] 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:

Quoted 8 lines

Quoted 7 lines

	/* All states wait for ibf, so just do it here. */

(quoted diff) +1 -1

-	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

Quoted 52 lines

One should
be there, anyway. I thought they were all covered, but I may have missed something.

-corey

Quoted 46 lines

Regards,

Hidehiro Kawai
Hitachi, Ltd. Research & Development Group

Quoted 41 lines

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

Quoted 34 lines · 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 <[email protected]>

---
 drivers/char/ipmi/ipmi_kcs_sm.c |    4 ++++
 1 file changed, 4 insertions(+)

drivers/char/ipmi/ipmi_kcs_sm.c +4 -0

index 8c25f59..0e187fb 100644
@@ -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;

From: Corey Minyard [mailto:[email protected]] On Behalf Of Corey Minyard

Quoted 19 lines · 河合英宏 / KAWAI，HIDEHIRO

Quoted 8 lines

From: Corey Minyard [mailto:[email protected]] On Behalf Of Corey Minyard

Quoted 5 lines · 河合英宏 / KAWAI，HIDEHIRO

Quoted 3 lines

From: Corey Minyard [mailto:[email protected]] 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:

Quoted 8 lines

Quoted 7 lines

	/* All states wait for ibf, so just do it here. */

(quoted diff) +1 -1

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

Quoted 6 lines

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.

Quoted 43 lines

Quoted 43 lines

One should
be there, anyway. I thought they were all covered, but I may have missed something.

-corey

Quoted 37 lines

Regards,

Hidehiro Kawai
Hitachi, Ltd. Research & Development Group

Quoted 32 lines

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

Quoted 25 lines · 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 <[email protected]>

---
 drivers/char/ipmi/ipmi_kcs_sm.c |    4 ++++
 1 file changed, 4 insertions(+)

drivers/char/ipmi/ipmi_kcs_sm.c +0 -0

index 8c25f59..0e187fb 100644
@@ -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);
 

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

drivers/char/ipmi/ipmi_kcs_sm.c static enum si_sm_result kcs_event(struct si_sm_data *kcs, long time) · line 362

Quoted 9 lines

drivers/char/ipmi/ipmi_kcs_sm.c +4 -0

+	/* 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;

Quoted 28 lines

From: Corey Minyard [mailto:[email protected]] On Behalf Of Corey Minyard

Quoted 25 lines · 河合英宏 / KAWAI，HIDEHIRO

Quoted 8 lines

From: Corey Minyard [mailto:[email protected]] On Behalf Of Corey Minyard

Quoted 5 lines · 河合英宏 / KAWAI，HIDEHIRO

Quoted 3 lines

From: Corey Minyard [mailto:[email protected]] 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:

Quoted 8 lines

Quoted 7 lines

	/* All states wait for ibf, so just do it here. */

(quoted diff) +1 -1

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

Quoted 3 lines

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.

From: Corey Minyard [mailto:[email protected]] On Behalf Of Corey Minyard

Quoted 28 lines · 河合英宏 / KAWAI，HIDEHIRO

Quoted 28 lines

From: Corey Minyard [mailto:[email protected]] On Behalf Of Corey Minyard

Quoted 25 lines · 河合英宏 / KAWAI，HIDEHIRO

Quoted 8 lines

From: Corey Minyard [mailto:[email protected]] On Behalf Of Corey Minyard

Quoted 5 lines · 河合英宏 / KAWAI，HIDEHIRO

Quoted 3 lines

From: Corey Minyard [mailto:[email protected]] 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:

Quoted 8 lines

Quoted 7 lines

	/* All states wait for ibf, so just do it here. */

(quoted diff) +1 -1

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

Quoted 8 lines

Quoted 3 lines

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.