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

[Vasant Hegde]

On 02/22/2019 06:04 AM, Andrew Jeffery 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?

I'm not sure its worth having separate pointer for in-flight message.
Everything works fine except here (adding message to head of the list).

-Vasant