@@ -61,8 +61,7 @@ struct mbox {
void *drv_data;
void (*attn)(uint8_t bits, void *priv);
void *attn_data;
- struct lock lock; /* Protect in_flight */
- struct bmc_mbox_msg *in_flight;
+ struct lock lock;
uint8_t sequence;
unsigned long timeout;
};
@@ -124,7 +123,7 @@ int bmc_mbox_enqueue(struct bmc_mbox_msg *msg, unsigned int timeout_sec)
}
lock(&mbox.lock);
- if (mbox.in_flight) {
+ if (mbox.timeout) {
prlog(PR_DEBUG, "MBOX message already in flight\n");
if (mftb() > mbox.timeout) {
prlog(PR_ERR, "In flight message dropped on the floor\n");
@@ -135,11 +134,10 @@ int bmc_mbox_enqueue(struct bmc_mbox_msg *msg, unsigned int timeout_sec)
}
mbox.timeout = mftb() + secs_to_tb(timeout_sec);
- mbox.in_flight = msg;
- unlock(&mbox.lock);
msg->seq = ++mbox.sequence;
bmc_mbox_send_message(msg);
+ unlock(&mbox.lock);
schedule_timer(&mbox.poller, mbox.irq_ok ?
TIMER_POLL : msecs_to_tb(MBOX_DEFAULT_POLL_MS));
@@ -150,42 +148,34 @@ int bmc_mbox_enqueue(struct bmc_mbox_msg *msg, unsigned int timeout_sec)
static void mbox_poll(struct timer *t __unused, void *data __unused,
uint64_t now __unused)
{
- struct bmc_mbox_msg *msg;
+ struct bmc_mbox_msg msg;
+
+ if (!lpc_ok())
+ return;
/*
* This status bit being high means that someone touched the
* response byte (byte 13).
* There is probably a response for the previously sent commant
*/
+ lock(&mbox.lock);
if (bmc_mbox_inb(MBOX_STATUS_1) & MBOX_STATUS_1_RESP) {
/* W1C on that reg */
bmc_mbox_outb(MBOX_STATUS_1_RESP, MBOX_STATUS_1);
prlog(PR_INSANE, "Got a regular interrupt\n");
- /*
- * This should be safe lockless
- */
- msg = mbox.in_flight;
- if (msg == NULL) {
- prlog(PR_CRIT, "Couldn't find the message!!\n");
- goto out_response;
- }
- bmc_mbox_recv_message(msg);
- if (mbox.sequence != msg->seq) {
+ bmc_mbox_recv_message(&msg);
+ if (mbox.sequence != msg.seq) {
prlog(PR_ERR, "Got a response to a message we no longer care about\n");
goto out_response;
}
+ mbox.timeout = 0;
if (mbox.callback)
- mbox.callback(msg, mbox.drv_data);
+ mbox.callback(&msg, mbox.drv_data);
else
prlog(PR_ERR, "Detected NULL callback for mbox message\n");
-
- /* Yeah we'll need locks here */
- lock(&mbox.lock);
- mbox.in_flight = NULL;
- unlock(&mbox.lock);
}
out_response:
@@ -230,6 +220,8 @@ out_response:
mbox.attn(all, mbox.attn_data);
}
+ unlock(&mbox.lock);
+
schedule_timer(&mbox.poller,
mbox.irq_ok ? TIMER_POLL : msecs_to_tb(MBOX_DEFAULT_POLL_MS));
}
@@ -341,9 +333,9 @@ void mbox_init(void)
bmc_mbox_outb(MBOX_STATUS_1_RESP | MBOX_STATUS_1_ATTN, MBOX_STATUS_1);
mbox.queue_len = 0;
- mbox.in_flight = NULL;
mbox.callback = NULL;
mbox.drv_data = NULL;
+ mbox.timeout = 0;
mbox.sequence = 0;
init_lock(&mbox.lock);
Currently the hw/lpc-mbox layer keeps a pointer for the currently inflight message for the duration of the mbox call. This creates problems when messages timeout, is that pointer still valid, what can we do with it. The memory is owned by the caller but if the caller has declared a timeout, it may have freed that memory. Another problem is locking. This patch also locks around sending and receiving to avoid races with timeouts and possible resends. There was some locking previously which was likely insufficient - definitely too hard to be sure is correct All this is made much easier with the previous rework which moves sequence number allocation and verification into lpc-mbox rather than the caller. Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com> --- hw/lpc-mbox.c | 38 +++++++++++++++----------------------- 1 file changed, 15 insertions(+), 23 deletions(-)