[PATCH 0/3] fpga: stratix10-soc: harden wait, ERROR order, and timeout

[PATCH 0/3] fpga: stratix10-soc: harden wait, ERROR order, and timeout

[Adrian Ng Ho Yin]

Stratix10 FPGA reconfiguration can fail or hang for three related
reasons in the FPGA manager. This series hardens s10_ops_write() and
s10_ops_write_complete() so those paths cannot skip a completion wait,
treat BUFFER_DONE | ERROR as success, or return immediately when the
DT-supplied config_complete_timeout_us is left at 0. No dependency on
the stratix10-svc accumulate changes.

  1) always wait for svc buffer completion

     s10_ops_write() skips wait_for_completion_timeout() when
     priv->status is already set. That skip is unnecessary and unsafe:
     the callback may fire between the CLAIM/SUBMIT send and the wait
     check. Always call the wait; if complete() already ran, the done
     counter is raised and the wait returns immediately.

  2) prefer ERROR over BUFFER_DONE

     BUFFER_DONE is checked before ERROR, so a combined
     BUFFER_DONE | ERROR status is treated as success. Check ERROR
     first so that status aborts the write. Also clear priv->status at
     the start of write_init so stale bits from an aborted transfer
     cannot leak into the next reconfiguration.

  3) floor config_complete_timeout_us

     of-fpga-region leaves config_complete_timeout_us at 0 when the DT
     property is absent. usecs_to_jiffies(0) makes write_complete return
     immediately, while reconfiguration takes ~600ms in practice.
     Enforce S10_RECONFIG_TIMEOUT as a minimum floor while still
     honouring any larger caller-supplied value. Note this prevents a
     DT property from lowering the timeout below that floor.

Adrian Ng Ho Yin (3):
  fpga: stratix10-soc: always wait for svc buffer completion
  fpga: stratix10-soc: prefer ERROR over BUFFER_DONE
  fpga: stratix10-soc: floor config_complete_timeout_us

 drivers/fpga/stratix10-soc.c | 41 ++++++++++++++++++++++++------------
 1 file changed, 27 insertions(+), 14 deletions(-)

-- 
2.49.GIT

[PATCH 1/3] fpga: stratix10-soc: always wait for svc buffer completion

[Adrian Ng Ho Yin]

Always call wait_for_completion_timeout() in s10_ops_write(). Skipping
the wait when priv->status is already set is unnecessary: if the
callback already ran, complete() raised the done counter and the wait
returns immediately.

Signed-off-by: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>
---
 drivers/fpga/stratix10-soc.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/fpga/stratix10-soc.c b/drivers/fpga/stratix10-soc.c
index b8ec2e6f615f..3ba0ffa1d9e0 100644
--- a/drivers/fpga/stratix10-soc.c
+++ b/drivers/fpga/stratix10-soc.c
@@ -310,14 +310,14 @@ static int s10_ops_write(struct fpga_manager *mgr, const char *buf,
 		}
 
 		/*
-		 * If callback hasn't already happened, wait for buffers to be
-		 * returned from service layer
+		 * 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.
 		 */
-		wait_status = 1; /* not timed out */
-		if (!priv->status)
-			wait_status = wait_for_completion_timeout(
-				&priv->status_return_completion,
-				S10_BUFFER_TIMEOUT);
+		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,
-- 
2.49.GIT

[PATCH 2/3] fpga: stratix10-soc: prefer ERROR over BUFFER_DONE

[Adrian Ng Ho Yin]

Check SVC_STATUS_ERROR before BUFFER_DONE so a combined
BUFFER_DONE | ERROR callback aborts instead of being treated as success.
Clear priv->status at the start of write_init so stale bits cannot leak
into the next reconfiguration.

Signed-off-by: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>
---
 drivers/fpga/stratix10-soc.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/fpga/stratix10-soc.c b/drivers/fpga/stratix10-soc.c
index 3ba0ffa1d9e0..c16ed571630b 100644
--- a/drivers/fpga/stratix10-soc.c
+++ b/drivers/fpga/stratix10-soc.c
@@ -183,6 +183,9 @@ static int s10_ops_write_init(struct fpga_manager *mgr,
 	uint i;
 	int ret;
 
+	/* Drop stale status bits left by a previous aborted write. */
+	priv->status = 0;
+
 	ctype.flags = 0;
 	if (info->flags & FPGA_MGR_PARTIAL_RECONFIG) {
 		dev_dbg(dev, "Requesting partial reconfiguration.\n");
@@ -319,6 +322,13 @@ static int s10_ops_write(struct fpga_manager *mgr, const char *buf,
 		wait_status = wait_for_completion_timeout(
 			&priv->status_return_completion, S10_BUFFER_TIMEOUT);
 
+		/* ERROR must win over BUFFER_DONE when both are set. */
+		if (test_and_clear_bit(SVC_STATUS_ERROR, &priv->status)) {
+			dev_err(dev, "ERROR - giving up - SVC_STATUS_ERROR\n");
+			ret = -EFAULT;
+			break;
+		}
+
 		if (test_and_clear_bit(SVC_STATUS_BUFFER_DONE, &priv->status) ||
 		    test_and_clear_bit(SVC_STATUS_BUFFER_SUBMITTED,
 				       &priv->status)) {
@@ -326,12 +336,6 @@ static int s10_ops_write(struct fpga_manager *mgr, const char *buf,
 			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;
-- 
2.49.GIT

[PATCH 3/3] fpga: stratix10-soc: floor config_complete_timeout_us

[Adrian Ng Ho Yin]

Keep info->config_complete_timeout_us configurable. of-fpga-region
leaves it at 0 when the DT property is absent, and usecs_to_jiffies(0)
makes the wait return immediately. Reconfiguration also takes ~600ms in
practice, so enforce S10_RECONFIG_TIMEOUT as a minimum floor while still
honouring any larger caller-supplied value. Note this prevents a DT
property from lowering the timeout below that floor.

Signed-off-by: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>
---
 drivers/fpga/stratix10-soc.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/fpga/stratix10-soc.c b/drivers/fpga/stratix10-soc.c
index c16ed571630b..83590e6b405f 100644
--- a/drivers/fpga/stratix10-soc.c
+++ b/drivers/fpga/stratix10-soc.c
@@ -7,6 +7,7 @@
 #include <linux/completion.h>
 #include <linux/fpga/fpga-mgr.h>
 #include <linux/firmware/intel/stratix10-svc-client.h>
+#include <linux/minmax.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_platform.h>
@@ -360,7 +361,15 @@ static int s10_ops_write_complete(struct fpga_manager *mgr,
 	unsigned long timeout;
 	int ret;
 
-	timeout = usecs_to_jiffies(info->config_complete_timeout_us);
+	/*
+	 * Keep config_complete_timeout_us configurable. of-fpga-region leaves
+	 * it at 0 when the DT property is absent, and usecs_to_jiffies(0)
+	 * makes the wait return immediately. Reconfiguration also takes
+	 * ~600ms in practice, so enforce S10_RECONFIG_TIMEOUT as a minimum
+	 * floor while still honouring any larger caller-supplied value.
+	 */
+	timeout = max(usecs_to_jiffies(info->config_complete_timeout_us),
+		      S10_RECONFIG_TIMEOUT);
 
 	do {
 		reinit_completion(&priv->status_return_completion);
-- 
2.49.GIT