mbox: Clarify sequence number constraints

And implement the specified behaviour.

Change-Id: I268d5896aa8dda3875cd79f4ff18929c8e3aea49
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>

3 files changed, 46 insertions, 22 deletions

diff --git a/Documentation/mbox_protocol.md b/Documentation/mbox_protocol.md
index 8a32511..a8be0f0 100644
--- a/Documentation/mbox_protocol.md
+++ b/Documentation/mbox_protocol.md
@@ -153,15 +153,14 @@ Messages usually originate from the host to the BMC. There are special
 cases for a back channel for the BMC to pass new information to the
 host which will be discussed later.
 
-To initiate a request the host must set a command code (see
-Commands) into mailbox data register 0. It is also the hosts
-responsibility to generate a unique sequence number into mailbox
-register 1. After this any command specific data should be written
-(see Layout). The host must then generate an interrupt to the BMC by
-using bit 0 of its control register and wait for an interrupt on the
-response register. Generating an interrupt automatically sets bit 7 of the
-corresponding control register. This bit can be used to poll for
-messages.
+To initiate a request the host must set a command code (see Commands) into
+mailbox data register 0, and generate a sequence number (see Sequence Numbers)
+to write to mailbox register data 1. After these two values, any
+command-specific data should be written (see Layout). The host must then
+generate an interrupt to the BMC by using bit 0 of its control register and
+wait for an interrupt on the response register.  Generating an interrupt
+automatically sets bit 7 of the corresponding control register. This bit can be
+used to poll for messages.
 
 On receiving an interrupt (or polling on bit 7 of its Control
 Register) the BMC should read the message from the general registers
@@ -312,13 +311,6 @@ BMC_EVENT_ACK        0x09
 MARK_WRITE_ERASED    0x0a	(V2)
 ```
 
-### Sequence
-
-The host must ensure a unique sequence number at the start of a
-command/response pair. The BMC must ensure the responses to
-a particular message contain the same sequence number that was in the
-command request from the host.
-
 ### Responses
 
 ```
@@ -329,8 +321,28 @@ SYSTEM_ERROR	4
 TIMEOUT		5
 BUSY		6	(V2)
 WINDOW_ERROR	7	(V2)
+SEQ_ERROR	8	(V2)
 ```
 
+### Sequence Numbers
+
+Sequence numbers are included in messages for correlation of commands and
+responses. V1 and V2 of the protocol permit either zero or one commands to be
+in progress (yet to receive a response).
+
+For generality, the host must generate a sequence number that is unique with
+respect to the previous command (one that has received a response) and any
+in-progress commands. Sequence numbers meeting this requirement are considered
+valid. The BMC's response to a command must contain the same sequence number
+issued by the host as found in the relevant command.
+
+Sequence numbers may be reused in accordance with the constraints outlined
+above, however it is not an error if the BMC receives a `GET_MBOX_INFO` with an
+invalid sequence number. For all other cases, the BMC must respond with
+`SEQ_ERROR` if the constraints are violated. If the host receives a `SEQ_ERROR`
+response it must consider any in-progress commands to have failed. The host may
+retry the affected command(s) after generating a suitable sequence number.
+
 #### Description:
 
 SUCCESS		- Command completed successfully
diff --git a/mbox.h b/mbox.h
index d9a8d03..087975d 100644
--- a/mbox.h
+++ b/mbox.h
@@ -54,6 +54,7 @@ enum api_version {
 #define MBOX_R_TIMEOUT			0x05
 #define MBOX_R_BUSY			0x06
 #define MBOX_R_WINDOW_ERROR		0x07
+#define MBOX_R_SEQ_ERROR		0x08
 
 /* Argument Flags */
 #define FLAGS_NONE			0x00
@@ -137,6 +138,7 @@ struct mbox_context {
 	sd_bus *bus;
 	bool terminate;
 	uint8_t bmc_events;
+	uint8_t prev_seq;
 
 /* Window State */
 	/* The window list struct containing all current "windows" */
diff --git a/mboxd_msg.c b/mboxd_msg.c
index 9f8aba1..a5bcf62 100644
--- a/mboxd_msg.c
+++ b/mboxd_msg.c
@@ -650,19 +650,27 @@ static int mbox_handle_ack(struct mbox_context *context, union mbox_regs *req,
 }
 
 /*
- * check_cmd_valid() - Check if the given command is a valid mbox command code
+ * check_req_valid() - Check if the given request is a valid mbox request
  * @context:	The mbox context pointer
- * @cmd:	The command code
+ * @cmd:	The request registers
  *
- * Return:	0 if command is valid otherwise negative error code
+ * Return:	0 if request is valid otherwise negative error code
  */
-static int check_cmd_valid(struct mbox_context *context, int cmd)
+static int check_req_valid(struct mbox_context *context, union mbox_regs *req)
 {
-	if (cmd <= 0 || cmd > NUM_MBOX_CMDS) {
+	uint8_t cmd = req->msg.command;
+	uint8_t seq = req->msg.seq;
+
+	if (cmd > NUM_MBOX_CMDS) {
 		MSG_ERR("Unknown mbox command: %d\n", cmd);
 		return -MBOX_R_PARAM_ERROR;
 	}
 
+	if (seq == context->prev_seq && cmd != MBOX_C_GET_MBOX_INFO) {
+		MSG_ERR("Invalid sequence number: %d\n", seq);
+		return -MBOX_R_SEQ_ERROR;
+	}
+
 	if (context->state & STATE_SUSPENDED) {
 		if (cmd != MBOX_C_GET_MBOX_INFO && cmd != MBOX_C_ACK) {
 			MSG_ERR("Cannot use that cmd while suspended: %d\n",
@@ -714,7 +722,7 @@ static int handle_mbox_req(struct mbox_context *context, union mbox_regs *req)
 	int rc = 0, len;
 
 	MSG_OUT("Got data in with command %d\n", req->msg.command);
-	rc = check_cmd_valid(context, req->msg.command);
+	rc = check_req_valid(context, req);
 	if (rc < 0) {
 		resp.response = -rc;
 	} else {
@@ -727,6 +735,8 @@ static int handle_mbox_req(struct mbox_context *context, union mbox_regs *req)
 		}
 	}
 
+	context->prev_seq = req->msg.seq;
+
 	MSG_OUT("Writing response to MBOX regs: %d\n", resp.response);
 	len = write(context->fds[MBOX_FD].fd, &resp, sizeof(resp));
 	if (len < sizeof(resp)) {