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

[Alistair Popple]

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;

> +		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
>