Re: [PATCH v3] firmware: stratix10-svc: Add Multi SVC clients support
From: Mohamad Jamian, Muhammad Amirul Asyraf
Date: Mon Feb 23 2026 - 02:19:47 EST
On 18/2/2026 2:13 am, Dinh Nguyen wrote:
>
>
> On 2/8/26 18:44, Muhammad Amirul Asyraf Mohamad Jamian wrote:
>> 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 support 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.
>>
>> Fixes: 7ca5ce896524 ("firmware: add Intel Stratix10 service layer
>> driver")
>> Cc: stable@xxxxxxxxxxxxxxx
>> Signed-off-by: Ang Tien Sung <tien.sung.ang@xxxxxxxxx>
>> Signed-off-by: Fong, Yan Kei <yankei.fong@xxxxxxxxxx>
>> Signed-off-by: Khairul Anuar Romli <khairul.anuar.romli@xxxxxxxxxx>
>
> Please clean up the above list of Signed-off-by:. I know one of the
> entries is no longer at Altera.
>
>> Signed-off-by: Muhammad Amirul Asyraf Mohamad Jamian
>> <muhammadamirulasyraf.mohamadjamian@xxxxxxxxxx>
>> Reported-by: kernel test robot <lkp@xxxxxxxxx>
>> Closes: https://nam10.safelinks.protection.outlook.com/?
>> url=https%3A%2F%2Flore.kernel.org%2Foe-kbuild-
>> all%2F202602032325.MuglZ156-
>> lkp%40intel.com%2F&data=05%7C02%7Cmuhammad.amirul.asyraf.mohamad.jamian%40altera.com%7Ca6c46069230e45c1f22e08de6e5047d8%7Cfbd72e03d4a54110adce614d51f2077a%7C0%7C0%7C639069488277371305%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=1ht5YJ3ZZbcMZGrGJ3T2VWi5yowxIIZ4X9Qyn9wEvSs%3D&reserved=0
>> ---
>> Changes in v3:
>> - Fix grammar: "will supports" -> "will support"
>> - Add Fixes tag and Cc stable as suggested by reviewer
>>
>> Changes in v2:
>> - Initialize 'svc' to NULL and add NULL check in error path to fix
>> uninitialized variable warning
>> ---
>> drivers/firmware/stratix10-svc.c | 203 +++++++++++-------
>> .../firmware/intel/stratix10-svc-client.h | 8 +-
>> 2 files changed, 130 insertions(+), 81 deletions(-)
>>
>> diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/
>> stratix10-svc.c
>> index 515b948ff320..eb37e2813860 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 */
>
> Remove the comment here, you've already documented sdm_lock.
>
>> 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 */
>
> Move these comments up to the structure header.
>
>> 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)) {
>
> Are the above checks are necessary?
>
>> + struct task_struct *task_to_stop = chan->task;
>> +
>> + chan->task = NULL;
>
> Remove this.
>
>> + 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;
>> @@ -1862,7 +1896,7 @@ static int stratix10_svc_drv_probe(struct
>> platform_device *pdev)
>> struct stratix10_svc_chan *chans;
>> struct gen_pool *genpool;
>> struct stratix10_svc_sh_memory *sh_memory;
>> - struct stratix10_svc *svc;
>> + struct stratix10_svc *svc = NULL;>
>> svc_invoke_fn *invoke_fn;
>> size_t fifo_size;
>> @@ -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,42 @@ 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;
>
> The change to the error handling doesn't look quite right. You're no
> longer freeing the FIFO on errors.
>
> Dinh
All comments have been addressed and updated in v4
https://lore.kernel.org/all/20260223070530.19187-1-muhammadamirulasyraf.mohamadjamian@xxxxxxxxxx/
Thanks,
Amirul