[Skiboot] [PATCH v2 1/3] hw/bt: Fix bt_add_ipmi_msg_head() logic

[Vasant Hegde]

On 02/22/2019 06:36 AM, Oliver wrote:
> On Fri, Feb 22, 2019 at 11:34 AM Andrew Jeffery <andrew at aj.id.au> wrote:
>>
>>
>>
>> On Fri, 22 Feb 2019, at 02:37, Vasant Hegde wrote:
>>> BT send logic always sends top of bt message list to BMC. Once BMC reads the
>>> message, it clears the interrupt and bt_idle() becomes true.
>>>
>>> bt_add_ipmi_msg_head() adds message to top of the list. If bt message list
>>> is not empty then:
>>>    - if bt_idle() is true then we will endup sending message to BMC before
>>>      getting response from BMC for inflight message. Looks like on some
>>>      BMC implementation this results in message timeout.
>>>    - else we endup starting message timer without actually sending message
>>>      to BMC.. which is not correct.
>>>
>>> This patch fixes above described issue by adding message to right place
>>> in the list.
>>>
>>> Signed-off-by: Vasant Hegde <hegdevasant at linux.vnet.ibm.com>
>>> ---
>>> Stewart,
>>>    All I'm doing is list_pop/list_add. May be we can have new function
>>>    (list_add_after()>. Let me know whether you are ok with this patch
>>>     -OR- want me to add list_add_after()>
>>>
>>> -Vasant
>>>
>>>   hw/bt.c | 11 ++++++++++-
>>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/bt.c b/hw/bt.c
>>> index 5a6bc3cde..a3561953b 100644
>>> --- a/hw/bt.c
>>> +++ b/hw/bt.c
>>> @@ -533,11 +533,20 @@ static void bt_add_msg(struct bt_msg *bt_msg)
>>>
>>>   static int bt_add_ipmi_msg_head(struct ipmi_msg *ipmi_msg)
>>>   {
>>> +     struct bt_msg *tmp_bt_msg;
>>>        struct bt_msg *bt_msg = container_of(ipmi_msg, struct bt_msg, ipmi_msg);
>>>
>>>        lock(&bt.lock);
>>>        bt_add_msg(bt_msg);
>>> -     list_add(&bt.msgq, &bt_msg->link);
>>> +
>>> +     if (list_empty(&bt.msgq)) {
>>> +             list_add(&bt.msgq, &bt_msg->link);
>>> +     } else {
>>> +             /* Add message after top message */
>>> +             tmp_bt_msg = list_pop(&bt.msgq, struct bt_msg, link);
>>> +             list_add(&bt.msgq, &bt_msg->link);
>>> +             list_add(&bt.msgq, &tmp_bt_msg->link);
>>
>> This looks a bit contorted. Is it worth having a separate pointer for the
>> in-flight message so we're not storing it at the head of the list?
> 
> Considering this is implementing a crude priority mechanism it might
> be worth having a seperate queue for high priority messages. The
> current behaviour results in first-in-last-out behaviour if we have
> multiple high priority messages are queued which doesn't sound right.

Correct. If we have multiple high priority one, then order gets messed up.
I kept it as is because chances of having such issues is very very less.

We can implement separate high priority queue -OR- add flag `urgent` flag to bt_msg
and so that we make sure to insert message to right place.

@Oliver, @Stewart,

How about something like below (untested patch) ?


-------
From: Vasant Hegde <hegdevasant at linux.vnet.ibm.com>
Date: Mon, 25 Feb 2019 22:45:39 +0530
Subject: [PATCH] hw/bt: Fix bt_add_ipmi_msg_head()

Signed-off-by: Vasant Hegde <hegdevasant at linux.vnet.ibm.com>
---
  hw/bt.c | 20 +++++++++++++++++++-
  1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/hw/bt.c b/hw/bt.c
index 5a6bc3cde..17a2fa764 100644
--- a/hw/bt.c
+++ b/hw/bt.c
@@ -94,6 +94,7 @@
  struct bt_msg {
  	struct list_node link;
  	unsigned long tb;
+	bool    urgent;
  	uint8_t seq;
  	uint8_t send_count;
  	bool disable_retry;
@@ -534,10 +535,27 @@ static void bt_add_msg(struct bt_msg *bt_msg)
  static int bt_add_ipmi_msg_head(struct ipmi_msg *ipmi_msg)
  {
  	struct bt_msg *bt_msg = container_of(ipmi_msg, struct bt_msg, ipmi_msg);
+	struct bt_msg *tmp_bt_msg, *top_bt_msg;

  	lock(&bt.lock);
  	bt_add_msg(bt_msg);
-	list_add(&bt.msgq, &bt_msg->link);
+
+	if (list_empty(&bt.msgq)) {
+		list_add(&bt.msgq, &bt_msg->link);
+	} else {
+		top_bt_msg = list_top(&bt.msgq, struct bt_msg, link);
+		list_for_each(&bt.msgq, tmp_bt_msg, link) {
+			if (tmp_bt_msg == top_bt_msg)
+				continue;
+			if (tmp_bt_msg->urgent == true)
+				continue;
+			list_add_before(&bt.msgq, &tmp_bt_msg->link, &bt_msg->link);
+			break;
+		}
+	}
+	if (tmp_bt_msg == NULL)
+		list_add_tail(&bt.msgq, &bt_msg->link);
+
  	bt_send_and_unlock();

  	return 0;
-- 
2.14.3


> The comment above ipmi_elog_poll() also points out that inserting at
> the top of the list is sort of broken anyway, so we might as well fix

Yes. Current code is broken because of the way it was inserting message. That's
what I'm fixing in this patch.

-Vasant