[Skiboot] [PATCH] ipmi: Fix the opal_ipmi_recv() call to handle the error path

[Neelesh Gupta]

Hi Alistair,

On 07/31/2015 10:56 AM, Alistair Popple wrote:
> Hi Neelesh,
>
> Comments below.
>
> On Thu, 16 Jul 2015 16:41:35 Neelesh Gupta wrote:
>> In the error path of the OPAL function to receive the ipmi message,
>> the function returns the error code without deleting the message
>> containing response. Though the kernel doesn't claim this message
>> later and continue with the subsequent ipmi commands. This leads to
>> a scenario when there is a mismatch between the ipmi command and its
>> response for all the subsequent ipmi commands.
>>
>> Signed-off-by: Neelesh Gupta <neelegup at linux.vnet.ibm.com>
>> Cc: Alistair Popple <alistair at popple.id.au>
>> ---
>>   hw/ipmi/ipmi-opal.c |   19 ++++++++++++-------
>>   1 file changed, 12 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/ipmi/ipmi-opal.c b/hw/ipmi/ipmi-opal.c
>> index 237f8c0..d37f151 100644
>> --- a/hw/ipmi/ipmi-opal.c
>> +++ b/hw/ipmi/ipmi-opal.c
>> @@ -75,25 +75,25 @@ static int64_t opal_ipmi_recv(uint64_t interface,
>>   	msg = list_top(&msgq, struct ipmi_msg, link);
>>   
>>   	if (!msg) {
>> -		rc = OPAL_EMPTY;
>> -		goto out_unlock;
> My preference would be to keep out_unlock and add out_del_msg. Eg:
>
> out_del_msg:
> 	list_del(&msg->link);
> 	if (list_empty(&msgq))
> 		opal_update_pending_evt(ipmi_backend->opal_event_ipmi_recv,
> 0);
> 	ipmi_free_msg(msg);
>
> out_unlock:
> 	unlock(&msgq_lock);
> 	return rc;

I earlier thought of not calling an interface (ipmi_free_msg) while 
taking the lock,
but not a concern in this case, so will change as you suggested.

Thanks,
Neelesh.

>
>> +		unlock(&msgq_lock);
>> +		return OPAL_EMPTY;
>>   	}
>>   
>>   	if (opal_ipmi_msg->version != OPAL_IPMI_MSG_FORMAT_VERSION_1) {
>>   		prerror("OPAL IPMI: Incorrect version\n");
>>   		rc = OPAL_UNSUPPORTED;
>> -		goto out_unlock;
>> +		goto out_del_msg;
>>   	}
>>   
>>   	if (interface != IPMI_DEFAULT_INTERFACE) {
>>   		prerror("IPMI: Invalid interface 0x%llx in opal_ipmi_recv\n",
> interface);
>> -		rc = OPAL_EMPTY;
>> -		goto out_unlock;
>> +		rc = OPAL_PARAMETER;
>> +		goto out_del_msg;
>>   	}
>>   
>>   	if (*msg_len - sizeof(struct opal_ipmi_msg) < msg->resp_size + 1) {
>>   		rc = OPAL_RESOURCE;
>> -		goto out_unlock;
>> +		goto out_del_msg;
>>   	}
>>   
>>   	list_del(&msg->link);
>> @@ -115,8 +115,13 @@ static int64_t opal_ipmi_recv(uint64_t interface,
>>   
>>   	return OPAL_SUCCESS;
>>   
>> -out_unlock:
>> +out_del_msg:
>> +	list_del(&msg->link);
>> +	if (list_empty(&msgq))
>> +		opal_update_pending_evt(ipmi_backend->opal_event_ipmi_recv,
> 0);
>>   	unlock(&msgq_lock);
>> +
>> +	ipmi_free_msg(msg);
>>   	return rc;
>>   }
>>   
>>
>> _______________________________________________
>> Skiboot mailing list
>> Skiboot at lists.ozlabs.org
>> https://lists.ozlabs.org/listinfo/skiboot
>>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/skiboot/attachments/20150731/f3152b57/attachment.html>