[Skiboot] [PATCH v2] fsp/ipmi: Add the in-band IPMI support for FSP systems

[Neelesh Gupta]

Hi Alistair,

On 07/07/2015 05:12 PM, Alistair Popple wrote:
>
> Hi Neelesh,
>
> Couple of minor comments below. I'm not at all familiar with how the FSP
>
> messages work so I can't provide too much comment there.
>
> - Alistair
>
>

<snip>

> > +static void fsp_ipmi_req_complete(struct fsp_msg *msg)
>
> > +{
>
> > + uint8_t status = (msg->resp->word1 >> 8) & 0xff;
>
> > + uint32_t length = msg->resp->data.words[0];
>
> > + struct fsp_ipmi_msg *fsp_ipmi_msg;
>
> > + struct ipmi_msg *ipmi_msg;
>
> > +
>
> > + fsp_freemsg(msg);
>
> > +
>
> > + if (status != FSP_STATUS_SUCCESS) {
>
> > + fsp_ipmi_msg = msg->user_data;
>
> > + assert(fsp_ipmi_msg == fsp_ipmi.cur_msg);
>
> > +
>
> > + ipmi_msg = &fsp_ipmi_msg->ipmi_msg;
>
> > +
>
> > + if (length != (ipmi_msg->req_size + FSP_IPMI_REQ_MIN_LEN))
>
> > + prlog(PR_DEBUG, "IPMI: Length mismatch in req completion "
>
> > + "(%d, %d)\n", ipmi_msg->req_size, length);
>
> > +
>
> > + log_simple_error(&e_info(OPAL_RC_IPMI_REQ), "IPMI: Request "
>
> > + "failed with status:0x%02x\n", status);
>
> > + /* FSP will not send the response now, so clear the current
>
> > + * outstanding request
>
> > + */
>
> > + fsp_ipmi_cmd_done(ipmi_msg->cmd, ipmi_msg->netfn + (1 << 2),
>
> > + IPMI_INVALID_COMMAND_ERR);
>
> Does FSP differentiate between failure types? Ie. does it send a
>
> different response for invalid IPMI command versus some other kind of
>
> error? Userspace tools (ie. ipmitool) will attempt sending potentially
>
> invalid IPMI commands to probe capabilities so if we can't
>
> differentiate the failure you probably don't want to log them as they
>
> will just be noise.
>

Yes, FSP differentiates between the errors and would come as the 'cc' 
field of
response buffer. This error path is caused due to 'mbox' error like 
invalid data
field in mbox command, DMA operation etc.. and not related to 'ipmi' error.
So, probably I shouldn't use IPMI_INVALID_COMMAND_ERR and return a generic
code like IPMI_ERR_UNSPECIFIED..

> > + /* Send the next request in the queue */
>
> > + fsp_ipmi_send_request();
>
> > + }
>
> > +}
>
> > +
>
> > +static int fsp_ipmi_send_request(void)
>
> > +{
>
> > + uint8_t *req_buf = fsp_ipmi.ipmi_req_buf;
>
> > + struct ipmi_msg *ipmi_msg;
>
> > + struct fsp_msg *msg;
>
> > + int rc;
>
> > +
>
> > + lock(&fsp_ipmi.lock);
>
> > + /* An outstanding request is still pending */
>
> > + if (fsp_ipmi.cur_msg) {
>
> > + unlock(&fsp_ipmi.lock);
>
> > + return 0;
>
> > + }
>
> > +
>
> > + fsp_ipmi.cur_msg = list_top(&fsp_ipmi.msg_queue, struct fsp_ipmi_msg,
>
> > + link);
>
> > + unlock(&fsp_ipmi.lock);
>
> > +
>
> > + if (!fsp_ipmi.cur_msg)
>
> > + return 0;
>
> > +
>
> > + ipmi_msg = &fsp_ipmi.cur_msg->ipmi_msg;
>
> > + prlog(PR_TRACE, "IPMI: Send request, netfn:0x%02x, cmd:0x%02x, "
>
> > + "req_len:%d\n", ipmi_msg->netfn, ipmi_msg->cmd, ipmi_msg->req_size);
>
> > +
>
> > + /* KCS request message format */
>
> > + *req_buf++ = ipmi_msg->netfn; /* BYTE 1 */
>
> > + *req_buf++ = ipmi_msg->cmd; /* BYTE 2 */
>
> > + if (ipmi_msg->req_size)
>
> > + memcpy(req_buf, ipmi_msg->data, ipmi_msg->req_size);
>
> > +
>
> > + msg = fsp_mkmsg(FSP_CMD_FETCH_PLAT_DATA, 5, 0, PSI_DMA_PLAT_REQ_BUF,
>
> > + 0, PSI_DMA_PLAT_RESP_BUF,
>
> > + ipmi_msg->req_size + FSP_IPMI_REQ_MIN_LEN);
>
> > + if (!msg) {
>
> > + log_simple_error(&e_info(OPAL_RC_IPMI_REQ), "IPMI: Failed to "
>
> > + "allocate request message\n");
>
> > + return OPAL_NO_MEM;
>
> > + }
>
> > +
>
> > + msg->user_data = fsp_ipmi.cur_msg;
>
> > + rc = fsp_queue_msg(msg, fsp_ipmi_req_complete);
>
> > + if (rc) {
>
> > + fsp_freemsg(msg);
>
> > + log_simple_error(&e_info(OPAL_RC_IPMI_REQ), "IPMI: Failed to "
>
> > + "queue request message (%d)\n", rc);
>
> If the message fails to send we need to call ipmi_cmd_done with an
>
> error response to allow the error handler to free the ipmi
>
> message. Otherwise we leak that message.
>
> Also wont this prevent retrying another message, as fsp_ipmi.cur_msg
>
> will always be != NULL?
>

Yes, this should be handled in the error path.

Thanks,
Neelesh.

> > + return OPAL_INTERNAL_ERROR;
>
> > + }
>
> > +
>
> > + return OPAL_SUCCESS;
>
> > +}
>
> > +
>
>

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