[PATCH v3 3/3] fpga: stratix10-soc: add SVC wait helper and prefer ERROR

[Adrian Ng Ho Yin]

write_init, write and write_complete each open-code the same sequence:
reinit the completion, send a service layer command, wait for the reply,
then test status bits. The order of the status tests differs between the
call sites.

The receive callback ORs each reply's status into priv->status, so bits
from more than one reply can be pending when a caller tests them (for
example several BUFFER_DONE callbacks for one RECONFIG_DATA_CLAIM, or a
reply that arrived after an earlier wait gave up). write() and
write_complete() test a success bit before SVC_STATUS_ERROR, so a status
word holding BUFFER_DONE | ERROR or COMPLETED | ERROR is treated as
success. write_init() ignores ERROR when OK is also set.

Add s10_svc_wait(), which waits for a reply and decodes priv->status with
SVC_STATUS_ERROR taking precedence over the caller's success bits, and
s10_svc_send_and_wait(), which also reinitialises the completion and
sends the command. write_init and write_complete use
s10_svc_send_and_wait(); write() sends either a data buffer or a claim
request and uses s10_svc_wait(). The helpers return 0, -EFAULT, -EBUSY
(SVC_STATUS_BUSY), -ETIMEDOUT, or -EAGAIN for a wake-up that carried
none of those bits. The status word is read and cleared in one step with
xchg().

The callback unlocks the returned buffers before it calls complete(), so
write() can finish while the completion of its last claim callback is
still pending, and that completion can then wake write_complete() before
the RECONFIG_STATUS reply. write_complete() therefore keeps waiting on
-EAGAIN and only re-sends RECONFIG_STATUS, bounded by the deadline, on
an actual BUSY reply.

As in the existing write() loop, status bits are honoured even when the
wait times out. Visible changes: write_init() now returns -EFAULT on
SVC_STATUS_ERROR and -EAGAIN for a reply without OK, where it returned
-ETIMEDOUT for both, and write_init() and write_complete() accept a
success bit that is set when their wait times out.

Also clear priv->status at the start of write_init so that bits set by a
late reply to an aborted previous write do not affect the new request.

stratix10_svc_done() is left at the call sites since it ends the whole
reconfiguration sequence rather than a single command.

Signed-off-by: Adrian Ng Ho Yin <adrian.ho.yin.ng@xxxxxxxxxx>
---

Notes:
    Changes in v3:
    - Replaces "prefer ERROR over BUFFER_DONE". Add s10_svc_wait() and
      s10_svc_send_and_wait(); write_init and write_complete use
      s10_svc_send_and_wait(), write() uses s10_svc_wait() after its
      data/claim send (Xu Yilun).
    - ERROR now also wins over COMPLETED and over OK (Xu Yilun).
    - write_complete ignores a stray wake-up from the last claim callback
      instead of counting it against the deadline.

 drivers/fpga/stratix10-soc.c | 163 ++++++++++++++++++++---------------
 1 file changed, 94 insertions(+), 69 deletions(-)

diff --git a/drivers/fpga/stratix10-soc.c b/drivers/fpga/stratix10-soc.c
index a69f63f0236b..074a99fc3a4b 100644
--- a/drivers/fpga/stratix10-soc.c
+++ b/drivers/fpga/stratix10-soc.c
@@ -72,6 +72,69 @@ static int s10_svc_send_msg(struct s10_priv *priv,
 	return ret;
 }
 
+/*
+ * s10_svc_wait - wait for a service layer reply and decode its status
+ * priv: private data
+ * done_bits: mask of SVC_STATUS_* bits that indicate success
+ * timeout: maximum time to wait for the reply, in jiffies
+ *
+ * Status bits accumulate in priv->status until they are consumed here, and
+ * SVC_STATUS_ERROR takes precedence over any success bit set alongside it.
+ * Returns 0 if a bit in done_bits was set, -EFAULT on SVC_STATUS_ERROR,
+ * -EBUSY on SVC_STATUS_BUSY, -ETIMEDOUT if no reply arrived, or -EAGAIN if
+ * the wake-up carried none of these bits (e.g. a late completion whose
+ * status bits were already consumed).
+ */
+static int s10_svc_wait(struct s10_priv *priv, unsigned long done_bits,
+			unsigned long timeout)
+{
+	struct device *dev = priv->client.dev;
+	unsigned long time_left, status;
+
+	time_left = wait_for_completion_timeout(&priv->status_return_completion,
+						timeout);
+	status = xchg(&priv->status, 0);
+
+	if (status & BIT(SVC_STATUS_ERROR)) {
+		dev_err(dev, "ERROR - giving up - SVC_STATUS_ERROR\n");
+		return -EFAULT;
+	}
+
+	if (status & done_bits)
+		return 0;
+
+	if (status & BIT(SVC_STATUS_BUSY))
+		return -EBUSY;
+
+	if (!time_left) {
+		dev_err(dev, "timeout waiting for service layer reply\n");
+		return -ETIMEDOUT;
+	}
+
+	return -EAGAIN;
+}
+
+/*
+ * s10_svc_send_and_wait - send a command and wait for its reply
+ * Returns a negative errno from s10_svc_send_msg(), otherwise the result of
+ * s10_svc_wait().
+ */
+static int s10_svc_send_and_wait(struct s10_priv *priv,
+				 enum stratix10_svc_command_code command,
+				 void *payload, u32 payload_length,
+				 unsigned long done_bits, unsigned long timeout)
+{
+	int ret;
+
+	reinit_completion(&priv->status_return_completion);
+
+	ret = s10_svc_send_msg(priv, command, payload, payload_length);
+	if (ret < 0)
+		return ret;
+
+	return s10_svc_wait(priv, done_bits, timeout);
+}
+
 /*
  * Free buffers allocated from the service layer's pool that are not in use.
  * Return true when all buffers are freed.
@@ -159,8 +222,8 @@ static void s10_receive_callback(struct stratix10_svc_client *client,
 	status = data->status;
 
 	/*
-	 * Here we set status bits as we receive them.  Elsewhere, we always use
-	 * test_and_clear_bit() to check status in priv->status
+	 * Here we set status bits as we receive them.  s10_svc_wait() consumes
+	 * them from priv->status.
 	 */
 	for (i = 0; i <= SVC_STATUS_ERROR; i++)
 		if (status & (1 << i))
@@ -190,6 +253,9 @@ static int s10_ops_write_init(struct fpga_manager *mgr,
 	uint i;
 	int ret;
 
+	/* A reply to an aborted previous write may have left status bits. */
+	priv->status = 0;
+
 	ctype.flags = 0;
 	if (info->flags & FPGA_MGR_PARTIAL_RECONFIG) {
 		dev_dbg(dev, "Requesting partial reconfiguration.\n");
@@ -198,23 +264,11 @@ static int s10_ops_write_init(struct fpga_manager *mgr,
 		dev_dbg(dev, "Requesting full reconfiguration.\n");
 	}
 
-	reinit_completion(&priv->status_return_completion);
-	ret = s10_svc_send_msg(priv, COMMAND_RECONFIG,
-			       &ctype, sizeof(ctype));
-	if (ret < 0)
-		goto init_error;
-
-	if (!wait_for_completion_timeout(&priv->status_return_completion,
-					 S10_RECONFIG_TIMEOUT)) {
-		dev_err(dev, "timeout waiting for RECONFIG_REQUEST\n");
-		ret = -ETIMEDOUT;
-		goto init_error;
-	}
-
-	if (!test_and_clear_bit(SVC_STATUS_OK, &priv->status)) {
-		ret = -ETIMEDOUT;
+	ret = s10_svc_send_and_wait(priv, COMMAND_RECONFIG,
+				    &ctype, sizeof(ctype),
+				    BIT(SVC_STATUS_OK), S10_RECONFIG_TIMEOUT);
+	if (ret)
 		goto init_error;
-	}
 
 	/* Allocate buffers from the service layer's pool. */
 	for (i = 0; i < NUM_SVC_BUFS; i++) {
@@ -287,7 +341,6 @@ static int s10_ops_write(struct fpga_manager *mgr, const char *buf,
 {
 	struct s10_priv *priv = mgr->priv;
 	struct device *dev = priv->client.dev;
-	long wait_status;
 	int sent = 0;
 	int ret = 0;
 
@@ -320,30 +373,15 @@ static int s10_ops_write(struct fpga_manager *mgr, const char *buf,
 		 * Always wait for the completion rather than skipping it when
 		 * priv->status is already set. The callback may have fired
 		 * between the CLAIM/SUBMIT send and here; if so, complete()
-		 * has already incremented the done counter and
-		 * wait_for_completion_timeout() returns immediately.
+		 * has already incremented the done counter and s10_svc_wait()
+		 * returns immediately.
 		 */
-		wait_status = wait_for_completion_timeout(
-			&priv->status_return_completion, S10_BUFFER_TIMEOUT);
-
-		if (test_and_clear_bit(SVC_STATUS_BUFFER_DONE, &priv->status) ||
-		    test_and_clear_bit(SVC_STATUS_BUFFER_SUBMITTED,
-				       &priv->status)) {
-			ret = 0;
-			continue;
-		}
-
-		if (test_and_clear_bit(SVC_STATUS_ERROR, &priv->status)) {
-			dev_err(dev, "ERROR - giving up - SVC_STATUS_ERROR\n");
-			ret = -EFAULT;
-			break;
-		}
-
-		if (!wait_status) {
-			dev_err(dev, "timeout waiting for svc layer buffers\n");
-			ret = -ETIMEDOUT;
+		ret = s10_svc_wait(priv, BIT(SVC_STATUS_BUFFER_SUBMITTED) |
+					 BIT(SVC_STATUS_BUFFER_DONE),
+				   S10_BUFFER_TIMEOUT);
+		if (ret == -EFAULT || ret == -ETIMEDOUT)
 			break;
-		}
+		ret = 0;
 	}
 
 	if (!s10_free_buffers(mgr))
@@ -366,35 +404,22 @@ static int s10_ops_write_complete(struct fpga_manager *mgr,
 	deadline = jiffies + usecs_to_jiffies(info->config_complete_timeout_us);
 
 	do {
-		reinit_completion(&priv->status_return_completion);
-
-		ret = s10_svc_send_msg(priv, COMMAND_RECONFIG_STATUS, NULL, 0);
-		if (ret < 0)
-			break;
-
-		if (!wait_for_completion_timeout(&priv->status_return_completion,
-						 S10_RECONFIG_STATUS_TIMEOUT)) {
-			dev_err(dev, "timeout waiting for RECONFIG_STATUS\n");
-			ret = -ETIMEDOUT;
-			break;
-		}
-
-		if (test_and_clear_bit(SVC_STATUS_COMPLETED, &priv->status))
-			break;
-
-		if (test_and_clear_bit(SVC_STATUS_ERROR, &priv->status)) {
-			dev_err(dev, "ERROR - giving up - SVC_STATUS_ERROR\n");
-			ret = -EFAULT;
-			break;
-		}
+		ret = s10_svc_send_and_wait(priv, COMMAND_RECONFIG_STATUS,
+					    NULL, 0, BIT(SVC_STATUS_COMPLETED),
+					    S10_RECONFIG_STATUS_TIMEOUT);
+		/*
+		 * A late completion from the last RECONFIG_DATA_CLAIM callback
+		 * can wake us before the reply to this request arrives.
+		 */
+		while (ret == -EAGAIN)
+			ret = s10_svc_wait(priv, BIT(SVC_STATUS_COMPLETED),
+					   S10_RECONFIG_STATUS_TIMEOUT);
+	} while (ret == -EBUSY && time_before(jiffies, deadline));
 
-		if (time_after_eq(jiffies, deadline)) {
-			dev_err(dev,
-				"timeout waiting for RECONFIG_COMPLETED\n");
-			ret = -ETIMEDOUT;
-			break;
-		}
-	} while (1);
+	if (ret == -EBUSY) {
+		dev_err(dev, "timeout waiting for RECONFIG_COMPLETED\n");
+		ret = -ETIMEDOUT;
+	}
 
 	stratix10_svc_done(priv->chan);
 
-- 
2.49.GIT