firmware: arm_scpi: Properly implement wait for messages to complete

scpi_send_message is broken for a least two reasons.

1. The driver sets tx_block on the mailbox client and expects mbox_send_message
to block until the message is sent, however this isn't the case. Whether by
design or in error, mbox_send_message waits for tx_complete which will be
signalled when any message is transmitted, not the one just queued. So in the
case where two threads send messages concurrently, one thread may see the signal
from the other's message completing, whilst it's message is still in the queue.

2. The second flaw is that even if the mailbox framework was waiting for the
correct message signal the driver has set a timeout (mailbox client tx_tout)
which means the wait for that signal may be aborted whilst the message is still
in the queue for sending.

Both the above mean that when mbox_send_message returns the message may still be
pending transmission so we cannot safely do anything with its resources. The
current code however goes on to free the message with put_scpi_xfer resulting
in all sorts of bugs.

To fix this, we add code to do a proper wait for the message to complete and
if a timeout occurs BUG because we don't have any way to cancel messages in the
queue.

Signed-off-by: Jon Medhurst <tixy@linaro.org>

1 files changed, 29 insertions, 13 deletions

diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index c7d06a36b23a..3517f79d9a4a 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -73,8 +73,6 @@
 #define FW_REV_MINOR_MASK	GENMASK(23, 16)
 #define FW_REV_PATCH_MASK	GENMASK(15, 0)
 
-#define MAX_RX_TIMEOUT		(msecs_to_jiffies(30))
-
 enum scpi_error_codes {
 	SCPI_SUCCESS = 0, /* Success */
 	SCPI_ERR_PARAM = 1, /* Invalid parameter(s) */
@@ -456,6 +454,18 @@ static void scpi_tx_prepare(struct mbox_client *c, void *msg)
 		iowrite32(t->cmd, &mem->command);
 }
 
+static void scpi_tx_done(struct mbox_client *c, void *msg, int result)
+{
+	struct scpi_xfer *t = msg;
+
+	if (!t->rx_buf)
+		complete(&t->done);
+	/*
+	 * Messages with rx_buf are expecting a reply and will be on the
+	 * rx_pending list, so leave them alone.
+	 */
+}
+
 static struct scpi_xfer *get_scpi_xfer(struct scpi_chan *ch)
 {
 	struct scpi_xfer *t;
@@ -517,17 +527,24 @@ static int scpi_send_message(u8 idx, void *tx_buf, unsigned int tx_len,
 	reinit_completion(&msg->done);
 
 	ret = mbox_send_message(scpi_chan->chan, msg);
-	if (ret < 0 || !rx_buf)
-		goto out;
+	if (ret >= 0) {
+		/*
+		 * Wait for message to be processed. If we end up having to wait
+		 * for a very long time then there is a serious bug, probably in
+		 * the firmware.
+		 *
+		 * IMPORTANT: We must not try and continue after the timeout
+		 * because this driver and the mailbox framework still has data
+		 * structures referring to the failed request and further
+		 * serious bugs will result.
+		 */
+		if (!wait_for_completion_timeout(&msg->done, msecs_to_jiffies(10000)))
+			BUG();
 
-	if (!wait_for_completion_timeout(&msg->done, MAX_RX_TIMEOUT))
-		ret = -ETIMEDOUT;
-	else
 		/* first status word */
-		ret = msg->status;
-out:
-	if (ret < 0 && rx_buf) /* remove entry from the list if timed-out */
-		scpi_process_cmd(scpi_chan, msg->cmd);
+		if (rx_buf)
+			ret = le32_to_cpu(msg->status);
+	}
 
 	put_scpi_xfer(msg, scpi_chan);
 	/* SCPI error codes > 0, translate them to Linux scale*/
@@ -962,8 +979,7 @@ static int scpi_probe(struct platform_device *pdev)
 		cl->dev = dev;
 		cl->rx_callback = scpi_handle_remote_msg;
 		cl->tx_prepare = scpi_tx_prepare;
-		cl->tx_block = true;
-		cl->tx_tout = 20;
+		cl->tx_done = scpi_tx_done;
 		cl->knows_txdone = false; /* controller can't ack */
 
 		INIT_LIST_HEAD(&pchan->rx_pending);