[PATCH] firmware: stratix10-svc: accumulate up to 4 FPGA config buffer claims

Adrian Ng Ho Yin posted 1 patch 1 day, 20 hours ago
drivers/firmware/stratix10-svc.c              | 109 +++++++++++++++---
drivers/fpga/stratix10-soc.c                  |   1 +
.../firmware/intel/stratix10-svc-client.h     |   2 +
3 files changed, 99 insertions(+), 13 deletions(-)
[PATCH] firmware: stratix10-svc: accumulate up to 4 FPGA config buffer claims
Posted by Adrian Ng Ho Yin 1 day, 20 hours ago
Each COMPLETED_WRITE SMC returns at most three addresses, but the ATF
FPGA config pool holds four. Calling receive_cb(BUFFER_DONE) on every
SMC races reinit_completion() in s10_ops_write() and can hang until
S10_BUFFER_TIMEOUT.

Accumulate addresses across SMCs and deliver one BUFFER_DONE when
firmware sets res.a1 == 0 (end of reclaim). Empty reclaim skips the
client callback. On early poll end or claim-array overflow, return
already-collected addresses with BUFFER_DONE | ERROR.

Extend stratix10_svc_cb_data with kaddr4, unlock it in the FPGA manager,
and clear it wherever kaddr1..3 are cleared so the reused cbdata object
cannot leak a stale pointer.

BUFFER_DONE | ERROR requires the fpga stratix10-soc harden series
(ERROR checked before BUFFER_DONE).

Depends-on: <cover.1790090287.git.adrian.ho.yin.ng@altera.com>
Signed-off-by: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>
---
 drivers/firmware/stratix10-svc.c              | 109 +++++++++++++++---
 drivers/fpga/stratix10-soc.c                  |   1 +
 .../firmware/intel/stratix10-svc-client.h     |   2 +
 3 files changed, 99 insertions(+), 13 deletions(-)

diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c
index f7928ef6b6db..9dc621b15cbf 100644
--- a/drivers/firmware/stratix10-svc.c
+++ b/drivers/firmware/stratix10-svc.c
@@ -46,11 +46,17 @@
  * FPGA_CONFIG_STATUS_TIMEOUT_SEC - poll the FPGA configuration status,
  * service layer will return error to FPGA manager when timeout occurs,
  * timeout is set to 30 seconds (30 * 1000) at Intel Stratix10 SoC.
+ *
+ * FPGA_CONFIG_NUM_BUFS - number of FPGA config buffers in the ATF pool.
+ * Matches NUM_SVC_BUFS in the Stratix10 FPGA manager. Each COMPLETED_WRITE
+ * SMC returns at most three physical addresses (smccc res.a1/a2/a3), so a
+ * full pool reclaim needs more than one SMC round-trip.
  */
 #define SVC_NUM_DATA_IN_FIFO			8
 #define SVC_NUM_CHANNEL				4
 #define FPGA_CONFIG_DATA_CLAIM_TIMEOUT_MS	2000
 #define FPGA_CONFIG_STATUS_TIMEOUT_SEC		30
+#define FPGA_CONFIG_NUM_BUFS			4
 #define BYTE_TO_WORD_SIZE              4
 
 /*
@@ -396,6 +402,29 @@ static void *svc_pa_to_va(unsigned long addr)
 	return NULL;
 }
 
+/**
+ * svc_data_claim_notify() - report claimed buffers (and status) to the client
+ * @p_data: pointer to service data structure
+ * @cb_data: pointer to callback data structure to service client
+ * @status: SVC_STATUS_* bit mask for the client
+ * @kaddr1: address of 1st completed data block (or NULL)
+ * @kaddr2: address of 2nd completed data block (or NULL)
+ * @kaddr3: address of 3rd completed data block (or NULL)
+ * @kaddr4: address of 4th completed data block (or NULL)
+ */
+static void svc_data_claim_notify(struct stratix10_svc_data *p_data,
+				  struct stratix10_svc_cb_data *cb_data,
+				  u32 status, void *kaddr1, void *kaddr2,
+				  void *kaddr3, void *kaddr4)
+{
+	cb_data->status = status;
+	cb_data->kaddr1 = kaddr1;
+	cb_data->kaddr2 = kaddr2;
+	cb_data->kaddr3 = kaddr3;
+	cb_data->kaddr4 = kaddr4;
+	p_data->chan->scl->receive_cb(p_data->chan->scl, cb_data);
+}
+
 /**
  * svc_thread_cmd_data_claim() - claim back buffer from the secure world
  * @ctrl: pointer to service layer controller
@@ -411,6 +440,10 @@ static void svc_thread_cmd_data_claim(struct stratix10_svc_controller *ctrl,
 {
 	struct arm_smccc_res res;
 	unsigned long timeout;
+	void *buf_claim_addr[FPGA_CONFIG_NUM_BUFS] = {NULL};
+	unsigned int buf_claim_count = 0;
+	unsigned int n_addrs;
+	u32 err_status = BIT(SVC_STATUS_BUFFER_DONE) | BIT(SVC_STATUS_ERROR);
 
 	reinit_completion(&ctrl->complete_status);
 	timeout = msecs_to_jiffies(FPGA_CONFIG_DATA_CLAIM_TIMEOUT_MS);
@@ -422,23 +455,52 @@ static void svc_thread_cmd_data_claim(struct stratix10_svc_controller *ctrl,
 
 		if (res.a0 == INTEL_SIP_SMC_STATUS_OK) {
 			if (!res.a1) {
+				/* Firmware: no more buffers in this reclaim. */
 				complete(&ctrl->complete_status);
-				break;
+				if (buf_claim_count)
+					svc_data_claim_notify(
+						p_data, cb_data,
+						BIT(SVC_STATUS_BUFFER_DONE),
+						buf_claim_addr[0],
+						buf_claim_addr[1],
+						buf_claim_addr[2],
+						buf_claim_addr[3]);
+				return;
 			}
-			cb_data->status = BIT(SVC_STATUS_BUFFER_DONE);
+
+			n_addrs = 1 + (res.a2 ? 1 : 0) + (res.a3 ? 1 : 0);
+			if (buf_claim_count + n_addrs >
+			    ARRAY_SIZE(buf_claim_addr)) {
+				/*
+				 * Protocol violation: more addresses than the
+				 * client pool. Return what was already collected
+				 * with BUFFER_DONE | ERROR so the client unlocks
+				 * and aborts; do not issue a second notify.
+				 */
+				pr_err("%s: buffer reclaim overflow (have=%u got=%u)\n",
+				       __func__, buf_claim_count, n_addrs);
+				complete(&ctrl->complete_status);
+				svc_data_claim_notify(p_data, cb_data,
+						      err_status,
+						      buf_claim_addr[0],
+						      buf_claim_addr[1],
+						      buf_claim_addr[2],
+						      buf_claim_addr[3]);
+				return;
+			}
+
 			/*
-			 * The firmware COMPLETED_WRITE response returns the
-			 * raw IOVA (without dma_addr_offset). Add it back to
-			 * match the key stored in pmem->paddr at allocation
-			 * time. dma_addr_offset is zero on non-SMMU paths.
+			 * COMPLETED_WRITE returns raw IOVA; restore offset
+			 * for pmem lookup. Zero on non-SMMU paths.
 			 */
-			cb_data->kaddr1 = svc_pa_to_va(res.a1 + ctrl->dma_addr_offset);
-			cb_data->kaddr2 = (res.a2) ?
-					  svc_pa_to_va(res.a2 + ctrl->dma_addr_offset) : NULL;
-			cb_data->kaddr3 = (res.a3) ?
-					  svc_pa_to_va(res.a3 + ctrl->dma_addr_offset) : NULL;
-			p_data->chan->scl->receive_cb(p_data->chan->scl,
-						      cb_data);
+			buf_claim_addr[buf_claim_count++] =
+				svc_pa_to_va(res.a1 + ctrl->dma_addr_offset);
+			if (res.a2)
+				buf_claim_addr[buf_claim_count++] =
+					svc_pa_to_va(res.a2 + ctrl->dma_addr_offset);
+			if (res.a3)
+				buf_claim_addr[buf_claim_count++] =
+					svc_pa_to_va(res.a3 + ctrl->dma_addr_offset);
 		} else {
 			pr_debug("%s: secure world busy, polling again\n",
 				 __func__);
@@ -446,6 +508,19 @@ static void svc_thread_cmd_data_claim(struct stratix10_svc_controller *ctrl,
 	} while (res.a0 == INTEL_SIP_SMC_STATUS_OK ||
 		 res.a0 == INTEL_SIP_SMC_STATUS_BUSY ||
 		 wait_for_completion_timeout(&ctrl->complete_status, timeout));
+
+	/*
+	 * Poll ended without a1 == 0 (timeout or unexpected SMC status) after
+	 * some addresses were collected. Return them for unlock and fail the
+	 * transaction so they are not leaked.
+	 */
+	if (buf_claim_count) {
+		pr_err("%s: reclaim ended early (status=0x%lx, have=%u)\n",
+		       __func__, res.a0, buf_claim_count);
+		svc_data_claim_notify(p_data, cb_data, err_status,
+				      buf_claim_addr[0], buf_claim_addr[1],
+				      buf_claim_addr[2], buf_claim_addr[3]);
+	}
 }
 
 /**
@@ -468,6 +543,7 @@ static void svc_thread_cmd_config_status(struct stratix10_svc_controller *ctrl,
 	cb_data->kaddr1 = NULL;
 	cb_data->kaddr2 = NULL;
 	cb_data->kaddr3 = NULL;
+	cb_data->kaddr4 = NULL;
 	cb_data->status = BIT(SVC_STATUS_ERROR);
 
 	pr_debug("%s: polling config status\n", __func__);
@@ -533,6 +609,7 @@ static void svc_thread_recv_status_ok(struct stratix10_svc_data *p_data,
 	cb_data->kaddr1 = NULL;
 	cb_data->kaddr2 = NULL;
 	cb_data->kaddr3 = NULL;
+	cb_data->kaddr4 = NULL;
 
 	switch (p_data->command) {
 	case COMMAND_RECONFIG:
@@ -599,6 +676,7 @@ static void svc_thread_recv_status_ok(struct stratix10_svc_data *p_data,
 		cb_data->kaddr1 = res12;
 		cb_data->kaddr2 = NULL;
 		cb_data->kaddr3 = NULL;
+		cb_data->kaddr4 = NULL;
 		break;
 	default:
 		pr_warn("it shouldn't happen\n");
@@ -677,6 +755,7 @@ static int svc_normal_to_secure_thread(void *data)
 			cbdata->kaddr1 = NULL;
 			cbdata->kaddr2 = NULL;
 			cbdata->kaddr3 = NULL;
+			cbdata->kaddr4 = NULL;
 			if (pdata->chan->scl)
 				pdata->chan->scl->receive_cb(pdata->chan->scl,
 							     cbdata);
@@ -870,6 +949,7 @@ static int svc_normal_to_secure_thread(void *data)
 			cbdata->kaddr1 = &res;
 			cbdata->kaddr2 = NULL;
 			cbdata->kaddr3 = NULL;
+			cbdata->kaddr4 = NULL;
 			pdata->chan->scl->receive_cb(pdata->chan->scl, cbdata);
 			mutex_unlock(&ctrl->sdm_lock);
 			continue;
@@ -916,6 +996,7 @@ static int svc_normal_to_secure_thread(void *data)
 				cbdata->kaddr1 = NULL;
 				cbdata->kaddr2 = NULL;
 				cbdata->kaddr3 = NULL;
+				cbdata->kaddr4 = NULL;
 				pdata->chan->scl->receive_cb(pdata->chan->scl,
 							     cbdata);
 				break;
@@ -929,6 +1010,7 @@ static int svc_normal_to_secure_thread(void *data)
 				cbdata->kaddr1 = &res12;
 				cbdata->kaddr2 = NULL;
 				cbdata->kaddr3 = NULL;
+				cbdata->kaddr4 = NULL;
 			} else {
 				cbdata->kaddr1 = &res.a1;
 				cbdata->kaddr2 = (res.a2) ?
@@ -944,6 +1026,7 @@ static int svc_normal_to_secure_thread(void *data)
 			cbdata->kaddr1 = NULL;
 			cbdata->kaddr2 = NULL;
 			cbdata->kaddr3 = NULL;
+			cbdata->kaddr4 = NULL;
 			pdata->chan->scl->receive_cb(pdata->chan->scl, cbdata);
 			break;
 
diff --git a/drivers/fpga/stratix10-soc.c b/drivers/fpga/stratix10-soc.c
index b8ec2e6f615f..456b30064395 100644
--- a/drivers/fpga/stratix10-soc.c
+++ b/drivers/fpga/stratix10-soc.c
@@ -163,6 +163,7 @@ static void s10_receive_callback(struct stratix10_svc_client *client,
 		s10_unlock_bufs(priv, data->kaddr1);
 		s10_unlock_bufs(priv, data->kaddr2);
 		s10_unlock_bufs(priv, data->kaddr3);
+		s10_unlock_bufs(priv, data->kaddr4);
 	}
 
 	complete(&priv->status_return_completion);
diff --git a/include/linux/firmware/intel/stratix10-svc-client.h b/include/linux/firmware/intel/stratix10-svc-client.h
index af13dacdf5ac..8872003cbebc 100644
--- a/include/linux/firmware/intel/stratix10-svc-client.h
+++ b/include/linux/firmware/intel/stratix10-svc-client.h
@@ -237,12 +237,14 @@ struct stratix10_svc_command_config_type {
  *	firmware that does not handle this command), kaddr1 is NULL.
  * @kaddr2: address of 2nd completed data block
  * @kaddr3: address of 3rd completed data block
+ * @kaddr4: address of 4th completed data block
  */
 struct stratix10_svc_cb_data {
 	u32 status;
 	void *kaddr1;
 	void *kaddr2;
 	void *kaddr3;
+	void *kaddr4;
 };
 
 /**
-- 
2.49.GIT