[Skiboot] [PATCH RFC v2 10/12] opal-prd: Add firmware_request & firmware_notify implementations

[Vasant Hegde]

On 05/26/2017 08:24 AM, Jeremy Kerr wrote:
> This change adds the implementation of firmware_request() and
> firmware_notify(). To do this, we need to add a message queue, so that
> we can properly handle out-of-order messages coming from firmware.
>
> Signed-off-by: Jeremy Kerr <jk at ozlabs.org>
> ---

.../...

>
> +uint64_t hservice_firmware_request(uint64_t req_len, void *req,
> +		uint64_t *resp_lenp, void *resp)
> +{
> +	struct opal_prd_msg *msg = ctx->msg;
> +	uint64_t resp_len;
> +	size_t size;
> +	int rc, n;
> +
> +	resp_len = be64_to_cpu(*resp_lenp);
> +
> +	pr_log(LOG_DEBUG,
> +			"HBRT: firmware request: %lu bytes req, %lu bytes resp",
> +			req_len, resp_len);
> +
> +	/* sanity check for potential overflows */
> +	if (req_len > 0xffff || resp_len > 0xffff)

Sorry. I missed to comment on this one earlier.
I think we should log error message here so that it becomes easy to debug.

> +		return -1;
> +
> +	size = sizeof(msg->hdr) + sizeof(msg->token) +
> +		sizeof(msg->fw_req) + req_len;
> +
> +	/* we need the entire message to fit within the 2-byte size field */
> +	if (size > 0xffff)

ditto.

> +		return -1;
> +


.../...

> +
> +	/* We have an "inner" poll loop here, as we want to ensure that the
> +	 * next entry into HBRT is the return from this function. So, only
> +	 * read from the prd fd, and queue anything that isn't a response
> +	 * to this request
> +	 */

This logic works fine for most cases. In worst case we may endup waiting for 
max_msgq_len messages before returning error. How about having timer here ? Say:
   run timer for 1 minute, if we don't get response then error out?


-Vasant

> +	n = 0;
> +	for (;;) {
> +		struct prd_msgq_item *item;
> +
> +		rc = read_prd_msg(ctx);
> +		if (rc)
> +			return -1;
> +
> +		msg = ctx->msg;
> +		if (msg->hdr.type == OPAL_PRD_MSG_TYPE_FIRMWARE_RESPONSE) {
> +			size = be64toh(msg->fw_resp.len);
> +			if (size > resp_len)
> +				return -1;
> +
> +			/* success! a valid response that fits into HBRT's
> +			 * resp buffer */
> +			memcpy(resp, msg->fw_resp.data, size);
> +			*resp_lenp = htobe64(size);
> +			return 0;
> +		}
> +
> +		/* not a response? queue up for later consumption */
> +		if (++n > max_msgq_len) {
> +			pr_log(LOG_ERR,
> +				"FW: too many messages queued (%d) while "
> +				"waiting for FIRMWARE_RESPONSE", n);
> +			return -1;
> +		}
> +		size = be16toh(msg->hdr.size);
> +		item = malloc(sizeof(*item) + size);
> +		memcpy(&item->msg, msg, size);
> +		list_add_tail(&ctx->msgq, &item->list);
> +	}
> +}
> +