astlpc: Implement async support via buffer state tracking
To remove the unbounded loop in mctp_astlpc_kcs_send() we need the
capability to retry sending the pending command once the data value has
been consumed. However, the link is duplex and we may enter the state
where we have multiple pending commands.
Rather than explicitly track the set of pending commands, track the
buffer state in order to derive both the pollfd state needed by the
caller _and_ the pending buffer synchronisation commands.
Unfortunately due to the structure of the code the integration of the
state tracking is a little messy, but is capable of correctly booting a
P10 system.
I've put up a less constrained and perhaps aspirational implementation
of how the state tracking could be implemented if we feel the need to
rework things going forward:
https://github.com/amboar/libmctp-shmb/
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
Change-Id: I693946dae5336aa5d0f07bcd66c36f1ccd391054
diff --git a/astlpc.c b/astlpc.c
index b576ba7..0e7e9f8 100644
--- a/astlpc.c
+++ b/astlpc.c
@@ -41,9 +41,62 @@
#endif
+enum mctp_astlpc_buffer_state {
+ /*
+ * Prior to "Channel Ready" we mark the buffers as "idle" to catch illegal accesses. In this
+ * state neither side is considered the owner of the buffer.
+ *
+ * Upon "Channel Ready", each side transitions the buffers from the initial "idle" state
+ * to the following target states:
+ *
+ * Tx buffer: "acquired"
+ * Rx buffer: "released"
+ */
+ buffer_state_idle,
+
+ /*
+ * Beyond initialisation by "Channel Ready", buffers are in the "acquired" state once:
+ *
+ * 1. We dequeue a control command transferring the buffer to our ownership out of the KCS
+ * interface, and
+ * 2. We are yet to complete all of our required accesses to the buffer
+ *
+ * * The Tx buffer enters the "acquired" state when we dequeue the "Rx Complete" command
+ * * The Rx buffer enters the "acquired" state when we dequeue the "Tx Begin" command
+ *
+ * It is a failure of implementation if it's possible for both sides to simultaneously
+ * consider a buffer as "acquired".
+ */
+ buffer_state_acquired,
+
+ /*
+ * Buffers are in the "prepared" state when:
+ *
+ * 1. We have completed all of our required accesses (read or write) for the buffer, and
+ * 2. We have not yet successfully enqueued the control command to hand off ownership
+ */
+ buffer_state_prepared,
+
+ /*
+ * Beyond initialisation by "Channel Ready", buffers are in the "released" state once:
+ *
+ * 1. We successfully enqueue the control command transferring ownership to the remote
+ * side in to the KCS interface
+ *
+ * * The Tx buffer enters the "released" state when we enqueue the "Tx Begin" command
+ * * The Rx buffer enters the "released" state when we enqueue the "Rx Complete" command
+ *
+ * It may be the case that both sides simultaneously consider a buffer to be in the
+ * "released" state. However, if this is true, it must also be true that a buffer ownership
+ * transfer command has been enqueued in the KCS interface and is yet to be dequeued.
+ */
+ buffer_state_released,
+};
+
struct mctp_astlpc_buffer {
uint32_t offset;
uint32_t size;
+ enum mctp_astlpc_buffer_state state;
};
struct mctp_astlpc_layout {
@@ -733,22 +786,19 @@
uint8_t status;
int rc;
- for (;;) {
- rc = mctp_astlpc_kcs_read(astlpc, MCTP_ASTLPC_KCS_REG_STATUS,
- &status);
- if (rc) {
- astlpc_prwarn(astlpc, "KCS status read failed");
- return -1;
- }
- if (mctp_astlpc_kcs_write_ready(astlpc, status))
- break;
- /* todo: timeout */
+ rc = mctp_astlpc_kcs_read(astlpc, MCTP_ASTLPC_KCS_REG_STATUS,
+ &status);
+ if (rc) {
+ astlpc_prwarn(astlpc, "KCS status read failed");
+ return -EIO;
}
+ if (!mctp_astlpc_kcs_write_ready(astlpc, status))
+ return -EBUSY;
rc = mctp_astlpc_kcs_write(astlpc, MCTP_ASTLPC_KCS_REG_DATA, data);
if (rc) {
astlpc_prwarn(astlpc, "KCS data write failed");
- return -1;
+ return -EIO;
}
return 0;
@@ -760,6 +810,7 @@
struct mctp_binding_astlpc *astlpc = binding_to_astlpc(b);
uint32_t len, len_be;
struct mctp_hdr *hdr;
+ int rc;
hdr = mctp_pktbuf_hdr(pkt);
len = mctp_pktbuf_size(pkt);
@@ -775,6 +826,8 @@
return -1;
}
+ mctp_binding_set_tx_enabled(b, false);
+
len_be = htobe32(len);
mctp_astlpc_lpc_write(astlpc, &len_be, astlpc->layout.tx.offset,
sizeof(len_be));
@@ -784,11 +837,13 @@
mctp_astlpc_lpc_write(astlpc, hdr, astlpc->layout.tx.offset + 4, len);
- mctp_binding_set_tx_enabled(b, false);
+ astlpc->layout.tx.state = buffer_state_prepared;
- mctp_astlpc_kcs_send(astlpc, 0x1);
+ rc = mctp_astlpc_kcs_send(astlpc, 0x1);
+ if (!rc)
+ astlpc->layout.tx.state = buffer_state_released;
- return 0;
+ return rc == -EBUSY ? 0 : rc;
}
static uint32_t mctp_astlpc_calculate_mtu(struct mctp_binding_astlpc *astlpc,
@@ -897,6 +952,10 @@
offsetof(struct mctp_lpcmap_hdr, negotiated_ver),
sizeof(negotiated_be));
+ /* Track buffer ownership */
+ astlpc->layout.tx.state = buffer_state_acquired;
+ astlpc->layout.rx.state = buffer_state_released;
+
/* Finalise the configuration */
status = KCS_STATUS_BMC_READY | KCS_STATUS_OBF;
if (negotiated > 0) {
@@ -948,10 +1007,13 @@
mctp_astlpc_lpc_read(astlpc, mctp_pktbuf_hdr(pkt),
astlpc->layout.rx.offset + 4, packet);
+ astlpc->layout.rx.state = buffer_state_prepared;
+
/* Inform the other side of the MCTP interface that we have read
* the packet off the bus before handling the contents of the packet.
*/
- mctp_astlpc_kcs_send(astlpc, 0x2);
+ if (!mctp_astlpc_kcs_send(astlpc, 0x2))
+ astlpc->layout.rx.state = buffer_state_released;
/*
* v3 will validate the CRC32 in the medium-specific trailer and adjust
@@ -969,6 +1031,7 @@
static void mctp_astlpc_tx_complete(struct mctp_binding_astlpc *astlpc)
{
+ astlpc->layout.tx.state = buffer_state_acquired;
mctp_binding_set_tx_enabled(&astlpc->binding, true);
}
@@ -1013,6 +1076,10 @@
astlpc->binding.pkt_size =
astlpc->proto->body_size(astlpc->layout.tx.size);
+ /* Track buffer ownership */
+ astlpc->layout.tx.state = buffer_state_acquired;
+ astlpc->layout.rx.state = buffer_state_released;
+
return 0;
}
@@ -1034,6 +1101,9 @@
astlpc->kcs_status = status;
return astlpc->binding.start(&astlpc->binding);
} else {
+ /* Shut down the channel */
+ astlpc->layout.rx.state = buffer_state_idle;
+ astlpc->layout.tx.state = buffer_state_idle;
mctp_binding_set_tx_enabled(&astlpc->binding, false);
}
}
@@ -1042,9 +1112,10 @@
updated & KCS_STATUS_CHANNEL_ACTIVE) {
bool enable;
+ astlpc->layout.rx.state = buffer_state_idle;
+ astlpc->layout.tx.state = buffer_state_idle;
rc = mctp_astlpc_finalise_channel(astlpc);
enable = (status & KCS_STATUS_CHANNEL_ACTIVE) && rc == 0;
-
mctp_binding_set_tx_enabled(&astlpc->binding, enable);
}
@@ -1058,6 +1129,14 @@
uint8_t status, data;
int rc;
+ if (astlpc->layout.rx.state == buffer_state_prepared)
+ if (!mctp_astlpc_kcs_send(astlpc, 0x2))
+ astlpc->layout.rx.state = buffer_state_released;
+
+ if (astlpc->layout.tx.state == buffer_state_prepared)
+ if (!mctp_astlpc_kcs_send(astlpc, 0x1))
+ astlpc->layout.tx.state = buffer_state_released;
+
rc = mctp_astlpc_kcs_read(astlpc, MCTP_ASTLPC_KCS_REG_STATUS, &status);
if (rc) {
astlpc_prwarn(astlpc, "KCS read error");
@@ -1088,9 +1167,21 @@
mctp_astlpc_init_channel(astlpc);
break;
case 0x1:
+ if (astlpc->layout.rx.state != buffer_state_released) {
+ astlpc_prerr(astlpc,
+ "Protocol error: Invalid Rx buffer state for event %d: %d\n",
+ data, astlpc->layout.rx.state);
+ return 0;
+ }
mctp_astlpc_rx_start(astlpc);
break;
case 0x2:
+ if (astlpc->layout.tx.state != buffer_state_released) {
+ astlpc_prerr(astlpc,
+ "Protocol error: Invalid Tx buffer state for event %d: %d\n",
+ data, astlpc->layout.tx.state);
+ return 0;
+ }
mctp_astlpc_tx_complete(astlpc);
break;
case 0xff:
@@ -1130,6 +1221,8 @@
memset(astlpc, 0, sizeof(*astlpc));
astlpc->mode = mode;
astlpc->lpc_map = NULL;
+ astlpc->layout.rx.state = buffer_state_idle;
+ astlpc->layout.tx.state = buffer_state_idle;
astlpc->requested_mtu = mtu;
astlpc->binding.name = "astlpc";
astlpc->binding.version = 1;
@@ -1310,8 +1403,15 @@
int mctp_astlpc_init_pollfd(struct mctp_binding_astlpc *astlpc,
struct pollfd *pollfd)
{
+ bool release;
+
pollfd->fd = astlpc->kcs_fd;
- pollfd->events = POLLIN;
+ pollfd->events = 0;
+
+ release = astlpc->layout.rx.state == buffer_state_prepared ||
+ astlpc->layout.tx.state == buffer_state_prepared;
+
+ pollfd->events = release ? POLLOUT : POLLIN;
return 0;
}
diff --git a/tests/test_astlpc.c b/tests/test_astlpc.c
index f781fa5..a485016 100644
--- a/tests/test_astlpc.c
+++ b/tests/test_astlpc.c
@@ -1295,6 +1295,68 @@
network_destroy(&ctx);
}
+static void astlpc_test_async_exchange(void)
+{
+ struct astlpc_test ctx = { 0 };
+ uint8_t msg[MCTP_BTU];
+ struct pollfd pollfd;
+ uint8_t tag = 0;
+
+ network_init(&ctx);
+
+ memset(&msg[0], 0x5a, MCTP_BTU);
+
+ /* (1)
+ * Fill the KCS transmit buffer by sending a message from the BMC to the host without
+ * dequeuing it on the host side
+ */
+ mctp_message_tx(ctx.bmc.mctp, 9, MCTP_MESSAGE_TO_SRC, tag, msg,
+ sizeof(msg));
+
+ /* (2)
+ * Assert that we're still listening for in-bound messages on the BMC
+ */
+ mctp_astlpc_init_pollfd(ctx.bmc.astlpc, &pollfd);
+ assert(pollfd.events & POLLIN);
+ assert(!(pollfd.events & POLLOUT));
+
+ /* (3)
+ * Send a message from the host to the BMC and dequeue the message on the BMC, triggering a
+ * buffer ownership transfer command back to the host
+ */
+ mctp_message_tx(ctx.host.mctp, 8, MCTP_MESSAGE_TO_SRC, tag, msg,
+ sizeof(msg));
+ mctp_astlpc_poll(ctx.bmc.astlpc);
+
+ /* (4)
+ * Assert that the BMC has to wait for the host to dequeue the ownership transfer command
+ * from (1) before further transfers take place.
+ */
+ mctp_astlpc_init_pollfd(ctx.bmc.astlpc, &pollfd);
+ assert(!(pollfd.events & POLLIN));
+ assert(pollfd.events & POLLOUT);
+
+ /* (5)
+ * Dequeue the message from (1) on the host side, allowing transmisson of the outstanding
+ * ownership transfer command from (3)
+ */
+ mctp_astlpc_poll(ctx.host.astlpc);
+
+ /* (6)
+ * Emulate a POLLOUT event on the BMC side
+ */
+ mctp_astlpc_poll(ctx.bmc.astlpc);
+
+ /* (7)
+ * Assert that we're again listening for in-bound messages on the BMC.
+ */
+ mctp_astlpc_init_pollfd(ctx.bmc.astlpc, &pollfd);
+ assert(pollfd.events & POLLIN);
+ assert(!(pollfd.events & POLLOUT));
+
+ network_destroy(&ctx);
+}
+
/* clang-format off */
#define TEST_CASE(test) { #test, test }
static const struct {
@@ -1338,6 +1400,7 @@
TEST_CASE(astlpc_test_tx_before_channel_init),
TEST_CASE(astlpc_test_corrupt_host_tx),
TEST_CASE(astlpc_test_corrupt_bmc_tx),
+ TEST_CASE(astlpc_test_async_exchange),
};
/* clang-format on */