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

[Vasant Hegde]

On 02/25/2019 06:12 AM, Stewart Smith wrote:
> Oliver <oohall at gmail.com> writes:
>> 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.
>> 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
>> it properly.
> 
> Yeah, I'm inclined to agree.
> 
> Let's have a high priority list that's processed first.
> 

Only issue with current code is order of synchronous message insertion. Rest of 
the flow
remains  same for normal and high priority messages. So as I commented in other 
response, how about introducing `urgent` flag to bt_msg and then insert message 
to right place in the
queue?

-Vasant