mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [RESEND] firmware: stratix10-svc: Add Multi SVC clients support
@ 2026-02-03  2:58 Muhammad Amirul Asyraf Mohamad Jamian
  2026-02-03 15:24 ` kernel test robot
  2026-02-04  6:18 ` Mohamad Jamian, Muhammad Amirul Asyraf
  0 siblings, 2 replies; 3+ messages in thread
From: Muhammad Amirul Asyraf Mohamad Jamian @ 2026-02-03  2:58 UTC (permalink / raw)
  To: Dinh Nguyen, linux-kernel, Ang Tien Sung

In the current implementation, SVC client drivers such as socfpga-hwmon,
intel_fcs, stratix10-soc, stratix10-rsu each send an SMC command that
triggers a single thread in the stratix10-svc driver. Upon receiving a
callback, the initiating client driver sends a stratix10-svc-done signal,
terminating the thread without waiting for other pending SMC commands to
complete. This leads to a timeout issue in the firmware SVC mailbox service
when multiple client drivers send SMC commands concurrently.

To resolve this issue, a dedicated thread is now created per channel. The
stratix10-svc driver will supports up to the number of channels defined by
SVC_NUM_CHANNEL. Thread synchronization is handled using a mutex to prevent
simultaneous issuance of SMC commands by multiple threads.

Additionally, a thread task is now validated before invoking kthread_stop
when the user aborts, ensuring safe termination.

Timeout values have also been adjusted to accommodate the increased load
from concurrent client driver activity.

Signed-off-by: Ang Tien Sung <tien.sung.ang@intel.com>
Signed-off-by: Fong, Yan Kei <yankei.fong@altera.com>
Signed-off-by: Khairul Anuar Romli <khairul.anuar.romli@altera.com>
Signed-off-by: Muhammad Amirul Asyraf Mohamad Jamian <muhammadamirulasyraf.mohamadjamian@altera.com>
---
 drivers/firmware/stratix10-svc.c              | 200 +++++++++++-------
 .../firmware/intel/stratix10-svc-client.h     |   8 +-
 2 files changed, 128 insertions(+), 80 deletions(-)

diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c
index 515b948ff320..6bc0f9315a4e 100644
--- a/drivers/firmware/stratix10-svc.c
+++ b/drivers/firmware/stratix10-svc.c
@@ -37,9 +37,9 @@
  * service layer will return error to FPGA manager when timeout occurs,
  * timeout is set to 30 seconds (30 * 1000) at Intel Stratix10 SoC.
  */
-#define SVC_NUM_DATA_IN_FIFO			32
+#define SVC_NUM_DATA_IN_FIFO			8
 #define SVC_NUM_CHANNEL				4
-#define FPGA_CONFIG_DATA_CLAIM_TIMEOUT_MS	200
+#define FPGA_CONFIG_DATA_CLAIM_TIMEOUT_MS	2000
 #define FPGA_CONFIG_STATUS_TIMEOUT_SEC		30
 #define BYTE_TO_WORD_SIZE              4
 
@@ -252,11 +252,10 @@ struct stratix10_async_ctrl {
  * @node: list management
  * @genpool: memory pool pointing to the memory region
  * @task: pointer to the thread task which handles SMC or HVC call
- * @svc_fifo: a queue for storing service message data
  * @complete_status: state for completion
- * @svc_fifo_lock: protect access to service message data queue
  * @invoke_fn: function to issue secure monitor call or hypervisor call
  * @svc: manages the list of client svc drivers
+ * @sdm_lock: only allows a single command single response to SDM
  * @actrl: async control structure
  *
  * This struct is used to create communication channels for service clients, to
@@ -269,12 +268,10 @@ struct stratix10_svc_controller {
 	int num_active_client;
 	struct list_head node;
 	struct gen_pool *genpool;
-	struct task_struct *task;
-	struct kfifo svc_fifo;
 	struct completion complete_status;
-	spinlock_t svc_fifo_lock;
 	svc_invoke_fn *invoke_fn;
 	struct stratix10_svc *svc;
+	struct mutex *sdm_lock; /* Protect SDM transaction until response */
 	struct stratix10_async_ctrl actrl;
 };
 
@@ -283,6 +280,9 @@ struct stratix10_svc_controller {
  * @ctrl: pointer to service controller which is the provider of this channel
  * @scl: pointer to service client which owns the channel
  * @name: service client name associated with the channel
+ * @task: pointer to the thread task which handles SMC or HVC call
+ * @svc_fifo: a queue for storing service message data
+ * @svc_fifo_lock: protect access to service message data queue
  * @lock: protect access to the channel
  * @async_chan: reference to asynchronous channel object for this channel
  *
@@ -293,6 +293,10 @@ struct stratix10_svc_chan {
 	struct stratix10_svc_controller *ctrl;
 	struct stratix10_svc_client *scl;
 	char *name;
+	struct task_struct *task;
+	/* Separate fifo for every channel */
+	struct kfifo svc_fifo;
+	spinlock_t svc_fifo_lock; /* locking pending fifo */
 	spinlock_t lock;
 	struct stratix10_async_chan *async_chan;
 };
@@ -527,13 +531,14 @@ static void svc_thread_recv_status_ok(struct stratix10_svc_data *p_data,
  */
 static int svc_normal_to_secure_thread(void *data)
 {
-	struct stratix10_svc_controller
-			*ctrl = (struct stratix10_svc_controller *)data;
-	struct stratix10_svc_data *pdata;
-	struct stratix10_svc_cb_data *cbdata;
+	struct stratix10_svc_chan *chan =  (struct stratix10_svc_chan *)data;
+	struct stratix10_svc_controller	*ctrl = chan->ctrl;
+	struct stratix10_svc_data *pdata = NULL;
+	struct stratix10_svc_cb_data *cbdata = NULL;
 	struct arm_smccc_res res;
 	unsigned long a0, a1, a2, a3, a4, a5, a6, a7;
 	int ret_fifo = 0;
+	bool sdm_lock_owned = false;
 
 	pdata =  kmalloc(sizeof(*pdata), GFP_KERNEL);
 	if (!pdata)
@@ -555,12 +560,12 @@ static int svc_normal_to_secure_thread(void *data)
 	a6 = 0;
 	a7 = 0;
 
-	pr_debug("smc_hvc_shm_thread is running\n");
+	pr_debug("%s: %s: Thread is running!\n", __func__, chan->name);
 
 	while (!kthread_should_stop()) {
-		ret_fifo = kfifo_out_spinlocked(&ctrl->svc_fifo,
+		ret_fifo = kfifo_out_spinlocked(&chan->svc_fifo,
 						pdata, sizeof(*pdata),
-						&ctrl->svc_fifo_lock);
+						&chan->svc_fifo_lock);
 
 		if (!ret_fifo)
 			continue;
@@ -569,6 +574,16 @@ static int svc_normal_to_secure_thread(void *data)
 			 (unsigned int)pdata->paddr, pdata->command,
 			 (unsigned int)pdata->size);
 
+		/* SDM can only processs one command at a time */
+		if (!sdm_lock_owned) {
+			/* Must not do mutex re-lock */
+			pr_debug("%s: %s: Thread is waiting for mutex!\n",
+				 __func__, chan->name);
+			guard(mutex)(ctrl->sdm_lock);
+		}
+
+		sdm_lock_owned = true;
+
 		switch (pdata->command) {
 		case COMMAND_RECONFIG_DATA_CLAIM:
 			svc_thread_cmd_data_claim(ctrl, pdata, cbdata);
@@ -702,8 +717,8 @@ static int svc_normal_to_secure_thread(void *data)
 			pr_warn("it shouldn't happen\n");
 			break;
 		}
-		pr_debug("%s: before SMC call -- a0=0x%016x a1=0x%016x",
-			 __func__,
+		pr_debug("%s: %s: before SMC call -- a0=0x%016x a1=0x%016x",
+			 __func__, chan->name,
 			 (unsigned int)a0,
 			 (unsigned int)a1);
 		pr_debug(" a2=0x%016x\n", (unsigned int)a2);
@@ -712,8 +727,8 @@ static int svc_normal_to_secure_thread(void *data)
 		pr_debug(" a5=0x%016x\n", (unsigned int)a5);
 		ctrl->invoke_fn(a0, a1, a2, a3, a4, a5, a6, a7, &res);
 
-		pr_debug("%s: after SMC call -- res.a0=0x%016x",
-			 __func__, (unsigned int)res.a0);
+		pr_debug("%s: %s: after SMC call -- res.a0=0x%016x",
+			 __func__, chan->name, (unsigned int)res.a0);
 		pr_debug(" res.a1=0x%016x, res.a2=0x%016x",
 			 (unsigned int)res.a1, (unsigned int)res.a2);
 		pr_debug(" res.a3=0x%016x\n", (unsigned int)res.a3);
@@ -728,6 +743,7 @@ static int svc_normal_to_secure_thread(void *data)
 			cbdata->kaddr2 = NULL;
 			cbdata->kaddr3 = NULL;
 			pdata->chan->scl->receive_cb(pdata->chan->scl, cbdata);
+			sdm_lock_owned = false;
 			continue;
 		}
 
@@ -1697,21 +1713,21 @@ int stratix10_svc_send(struct stratix10_svc_chan *chan, void *msg)
 		return -ENOMEM;
 
 	/* first client will create kernel thread */
-	if (!chan->ctrl->task) {
-		chan->ctrl->task =
+	if (!chan->task) {
+		chan->task =
 			kthread_run_on_cpu(svc_normal_to_secure_thread,
-					   (void *)chan->ctrl,
+					   (void *)chan,
 					   cpu, "svc_smc_hvc_thread");
-			if (IS_ERR(chan->ctrl->task)) {
-				dev_err(chan->ctrl->dev,
-					"failed to create svc_smc_hvc_thread\n");
-				kfree(p_data);
-				return -EINVAL;
-			}
+		if (IS_ERR(chan->task)) {
+			dev_err(chan->ctrl->dev,
+				"failed to create svc_smc_hvc_thread\n");
+			kfree(p_data);
+			return -EINVAL;
+		}
 	}
 
-	pr_debug("%s: sent P-va=%p, P-com=%x, P-size=%u\n", __func__,
-		 p_msg->payload, p_msg->command,
+	pr_debug("%s: %s: sent P-va=%p, P-com=%x, P-size=%u\n", __func__,
+		 chan->name, p_msg->payload, p_msg->command,
 		 (unsigned int)p_msg->payload_length);
 
 	if (list_empty(&svc_data_mem)) {
@@ -1747,12 +1763,16 @@ int stratix10_svc_send(struct stratix10_svc_chan *chan, void *msg)
 	p_data->arg[2] = p_msg->arg[2];
 	p_data->size = p_msg->payload_length;
 	p_data->chan = chan;
-	pr_debug("%s: put to FIFO pa=0x%016x, cmd=%x, size=%u\n", __func__,
-	       (unsigned int)p_data->paddr, p_data->command,
-	       (unsigned int)p_data->size);
-	ret = kfifo_in_spinlocked(&chan->ctrl->svc_fifo, p_data,
+	pr_debug("%s: %s: put to FIFO pa=0x%016x, cmd=%x, size=%u\n",
+		 __func__,
+		 chan->name,
+		 (unsigned int)p_data->paddr,
+		 p_data->command,
+		 (unsigned int)p_data->size);
+
+	ret = kfifo_in_spinlocked(&chan->svc_fifo, p_data,
 				  sizeof(*p_data),
-				  &chan->ctrl->svc_fifo_lock);
+				  &chan->svc_fifo_lock);
 
 	kfree(p_data);
 
@@ -1773,12 +1793,21 @@ EXPORT_SYMBOL_GPL(stratix10_svc_send);
  */
 void stratix10_svc_done(struct stratix10_svc_chan *chan)
 {
-	/* stop thread when thread is running AND only one active client */
-	if (chan->ctrl->task && chan->ctrl->num_active_client <= 1) {
-		pr_debug("svc_smc_hvc_shm_thread is stopped\n");
-		kthread_stop(chan->ctrl->task);
-		chan->ctrl->task = NULL;
+	/* stop thread when thread is running */
+	if (chan->task) {
+		if (!IS_ERR(chan->task)) {
+			struct task_struct *task_to_stop = chan->task;
+
+			chan->task = NULL;
+			pr_debug("%s: %s: svc_smc_hvc_shm_thread is stopping\n",
+				 __func__, chan->name);
+			kthread_stop(task_to_stop);
+		}
+
+		chan->task = NULL;
 	}
+	pr_debug("%s: %s: svc_smc_hvc_shm_thread has stopped\n",
+		 __func__, chan->name);
 }
 EXPORT_SYMBOL_GPL(stratix10_svc_done);
 
@@ -1817,8 +1846,8 @@ void *stratix10_svc_allocate_memory(struct stratix10_svc_chan *chan,
 	pmem->paddr = pa;
 	pmem->size = s;
 	list_add_tail(&pmem->node, &svc_data_mem);
-	pr_debug("%s: va=%p, pa=0x%016x\n", __func__,
-		 pmem->vaddr, (unsigned int)pmem->paddr);
+	pr_debug("%s: %s: va=%p, pa=0x%016x\n", __func__,
+		 chan->name, pmem->vaddr, (unsigned int)pmem->paddr);
 
 	return (void *)va;
 }
@@ -1855,6 +1884,11 @@ static const struct of_device_id stratix10_svc_drv_match[] = {
 	{},
 };
 
+/**
+ * mailbox_lock protects access to the to SDM if thread is busy
+ */
+static DEFINE_MUTEX(mailbox_lock);
+
 static int stratix10_svc_drv_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -1905,7 +1939,6 @@ static int stratix10_svc_drv_probe(struct platform_device *pdev)
 	controller->num_active_client = 0;
 	controller->chans = chans;
 	controller->genpool = genpool;
-	controller->task = NULL;
 	controller->invoke_fn = invoke_fn;
 	init_completion(&controller->complete_status);
 
@@ -1917,32 +1950,63 @@ static int stratix10_svc_drv_probe(struct platform_device *pdev)
 	}
 
 	fifo_size = sizeof(struct stratix10_svc_data) * SVC_NUM_DATA_IN_FIFO;
-	ret = kfifo_alloc(&controller->svc_fifo, fifo_size, GFP_KERNEL);
+	ret = kfifo_alloc(&chans->svc_fifo, fifo_size, GFP_KERNEL);
 	if (ret) {
 		dev_err(dev, "failed to allocate FIFO\n");
 		goto err_async_exit;
 	}
-	spin_lock_init(&controller->svc_fifo_lock);
+	spin_lock_init(&chans->svc_fifo_lock);
+	/*
+	 * This mutex is used to block threads from utilizing
+	 * SDM to prevent out of order command tx
+	 */
+	controller->sdm_lock = &mailbox_lock;
+
+	fifo_size = sizeof(struct stratix10_svc_data) * SVC_NUM_DATA_IN_FIFO;
 
 	chans[0].scl = NULL;
 	chans[0].ctrl = controller;
 	chans[0].name = SVC_CLIENT_FPGA;
 	spin_lock_init(&chans[0].lock);
+	ret = kfifo_alloc(&chans[0].svc_fifo, fifo_size, GFP_KERNEL);
+	if (ret) {
+		dev_err(dev, "failed to allocate FIFO 0\n");
+		return ret;
+	}
+	spin_lock_init(&chans[0].svc_fifo_lock);
 
 	chans[1].scl = NULL;
 	chans[1].ctrl = controller;
 	chans[1].name = SVC_CLIENT_RSU;
 	spin_lock_init(&chans[1].lock);
+	ret = kfifo_alloc(&chans[1].svc_fifo, fifo_size, GFP_KERNEL);
+	if (ret) {
+		dev_err(dev, "failed to allocate FIFO 1\n");
+		return ret;
+	}
+	spin_lock_init(&chans[1].svc_fifo_lock);
 
 	chans[2].scl = NULL;
 	chans[2].ctrl = controller;
 	chans[2].name = SVC_CLIENT_FCS;
 	spin_lock_init(&chans[2].lock);
+	ret = kfifo_alloc(&chans[2].svc_fifo, fifo_size, GFP_KERNEL);
+	if (ret) {
+		dev_err(dev, "failed to allocate FIFO 2\n");
+		return ret;
+	}
+	spin_lock_init(&chans[2].svc_fifo_lock);
 
 	chans[3].scl = NULL;
 	chans[3].ctrl = controller;
 	chans[3].name = SVC_CLIENT_HWMON;
 	spin_lock_init(&chans[3].lock);
+	ret = kfifo_alloc(&chans[3].svc_fifo, fifo_size, GFP_KERNEL);
+	if (ret) {
+		dev_err(dev, "failed to allocate FIFO 3\n");
+		return ret;
+	}
+	spin_lock_init(&chans[3].svc_fifo_lock);
 
 	list_add_tail(&controller->node, &svc_ctrl);
 	platform_set_drvdata(pdev, controller);
@@ -1950,60 +2014,41 @@ static int stratix10_svc_drv_probe(struct platform_device *pdev)
 	/* add svc client device(s) */
 	svc = devm_kzalloc(dev, sizeof(*svc), GFP_KERNEL);
 	if (!svc) {
-		ret = -ENOMEM;
-		goto err_free_kfifo;
+		return -ENOMEM;
 	}
 	controller->svc = svc;
 
 	svc->stratix10_svc_rsu = platform_device_alloc(STRATIX10_RSU, 0);
 	if (!svc->stratix10_svc_rsu) {
 		dev_err(dev, "failed to allocate %s device\n", STRATIX10_RSU);
-		ret = -ENOMEM;
-		goto err_free_kfifo;
+		return -ENOMEM;
 	}
 
 	ret = platform_device_add(svc->stratix10_svc_rsu);
-	if (ret) {
-		platform_device_put(svc->stratix10_svc_rsu);
-		goto err_free_kfifo;
-	}
-
-	svc->intel_svc_fcs = platform_device_alloc(INTEL_FCS, 1);
-	if (!svc->intel_svc_fcs) {
-		dev_err(dev, "failed to allocate %s device\n", INTEL_FCS);
-		ret = -ENOMEM;
-		goto err_unregister_rsu_dev;
-	}
-
-	ret = platform_device_add(svc->intel_svc_fcs);
-	if (ret) {
-		platform_device_put(svc->intel_svc_fcs);
-		goto err_unregister_rsu_dev;
-	}
+	if (ret)
+		goto err_put_device;
 
 	ret = of_platform_default_populate(dev_of_node(dev), NULL, dev);
 	if (ret)
-		goto err_unregister_fcs_dev;
+		goto err_put_device;
 
 	pr_info("Intel Service Layer Driver Initialized\n");
 
 	return 0;
 
-err_unregister_fcs_dev:
-	platform_device_unregister(svc->intel_svc_fcs);
-err_unregister_rsu_dev:
-	platform_device_unregister(svc->stratix10_svc_rsu);
-err_free_kfifo:
-	kfifo_free(&controller->svc_fifo);
 err_async_exit:
 	stratix10_svc_async_exit(controller);
+err_put_device:
+	platform_device_put(svc->stratix10_svc_rsu);
 err_destroy_pool:
 	gen_pool_destroy(genpool);
+
 	return ret;
 }
 
 static void stratix10_svc_drv_remove(struct platform_device *pdev)
 {
+	int i;
 	struct stratix10_svc_controller *ctrl = platform_get_drvdata(pdev);
 	struct stratix10_svc *svc = ctrl->svc;
 
@@ -2014,11 +2059,14 @@ static void stratix10_svc_drv_remove(struct platform_device *pdev)
 	platform_device_unregister(svc->intel_svc_fcs);
 	platform_device_unregister(svc->stratix10_svc_rsu);
 
-	kfifo_free(&ctrl->svc_fifo);
-	if (ctrl->task) {
-		kthread_stop(ctrl->task);
-		ctrl->task = NULL;
+	for (i = 0; i < SVC_NUM_CHANNEL; i++) {
+		if (ctrl->chans[i].task) {
+			kthread_stop(ctrl->chans[i].task);
+			ctrl->chans[i].task = NULL;
+		}
+		kfifo_free(&ctrl->chans[i].svc_fifo);
 	}
+
 	if (ctrl->genpool)
 		gen_pool_destroy(ctrl->genpool);
 	list_del(&ctrl->node);
diff --git a/include/linux/firmware/intel/stratix10-svc-client.h b/include/linux/firmware/intel/stratix10-svc-client.h
index d290060f4c73..91013161e9db 100644
--- a/include/linux/firmware/intel/stratix10-svc-client.h
+++ b/include/linux/firmware/intel/stratix10-svc-client.h
@@ -68,12 +68,12 @@
  * timeout value used in Stratix10 FPGA manager driver.
  * timeout value used in RSU driver
  */
-#define SVC_RECONFIG_REQUEST_TIMEOUT_MS         300
-#define SVC_RECONFIG_BUFFER_TIMEOUT_MS          720
-#define SVC_RSU_REQUEST_TIMEOUT_MS              300
+#define SVC_RECONFIG_REQUEST_TIMEOUT_MS         5000
+#define SVC_RECONFIG_BUFFER_TIMEOUT_MS          5000
+#define SVC_RSU_REQUEST_TIMEOUT_MS              2000
 #define SVC_FCS_REQUEST_TIMEOUT_MS		2000
 #define SVC_COMPLETED_TIMEOUT_MS		30000
-#define SVC_HWMON_REQUEST_TIMEOUT_MS		300
+#define SVC_HWMON_REQUEST_TIMEOUT_MS		2000
 
 struct stratix10_svc_chan;
 
-- 
2.43.7


^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-02-04  6:18 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-02-03  2:58 [RESEND] firmware: stratix10-svc: Add Multi SVC clients support Muhammad Amirul Asyraf Mohamad Jamian
2026-02-03 15:24 ` kernel test robot
2026-02-04  6:18 ` Mohamad Jamian, Muhammad Amirul Asyraf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®