From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 73FB74C97 for ; Tue, 17 Feb 2026 18:13:40 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1771352020; cv=none; b=QUNpZsChfMp3vMEcExYBD5qz9RIyIYrw/e/LoTNV+mH0edBJ2b9f36XyqD3pkBfK3GihbmCqOYSJRWJFaQRHxPwbXqEqt1lUGIp6YTpGxybQH+xPD6eKr9FvrBgvJgJbbiE4CXt+rhvqyAnQ7aNWcfi8pwwqz7evKzww7P0QBMs= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1771352020; c=relaxed/simple; bh=M9pa2zNQGCo96Gd9+GqqK65ghdd8uAaXUopBs6F2JoU=; h=Message-ID:Date:MIME-Version:From:Subject:To:Cc:References: In-Reply-To:Content-Type; b=sYJygSKk2EeyTyx59GeT/L1sB4ivUn/vXv92gJUhGeq0ui5T/+HBOcm+cActGrldAUkls1LiYHdTAZyHLn2UHQfcRvdtdLa6szgHpbIjFzqxONiuThZF/doH24nQTntTvCm25lQPFxoA4ehc/nS3ihrnWxmX5Ejojd86Vwi4DUM= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=Zfezj98Q; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="Zfezj98Q" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 33F4DC4CEF7; Tue, 17 Feb 2026 18:13:39 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1771352020; bh=M9pa2zNQGCo96Gd9+GqqK65ghdd8uAaXUopBs6F2JoU=; h=Date:From:Subject:To:Cc:References:In-Reply-To:From; b=Zfezj98QHlquU0tUtN+nIXpBMhCltpUkvJTHNJFnyGGjyOdSl2Ajybz3a9ipBi83Y 4UlCw1LU0baZx5FY81MBExGPAetHlIeqOxjfdWVcD3YQU2A12eTtjx+/xGI5zhV0jZ 1QDk8M1oMwgb2k1kDIxYJYk5u15iwSRVlsDh26BgugheuQ90V09b6OOeFq18gcIG/I CfAZTaokvtrLLOWzKRfdZ6PrY2hfVuOPxgffx8s+wYEAS6CmPWRVQEoltxlhaWxnCG uihDOiWulWTqSJgP11r3B8wN2VC3YXm7wD09HeUtKF27WC+KKbuKibZZYTtwSuNW9f AleIk2FU96aVg== Message-ID: Date: Tue, 17 Feb 2026 12:13:37 -0600 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: Dinh Nguyen Subject: Re: [PATCH v3] firmware: stratix10-svc: Add Multi SVC clients support To: Muhammad Amirul Asyraf Mohamad Jamian , linux-kernel@vger.kernel.org, Ang Tien Sung Cc: Markus Elfring References: <20260209004459.15285-1-muhammadamirulasyraf.mohamadjamian@altera.com> Content-Language: en-US In-Reply-To: <20260209004459.15285-1-muhammadamirulasyraf.mohamadjamian@altera.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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@vger.kernel.org > Signed-off-by: Ang Tien Sung > Signed-off-by: Fong, Yan Kei > Signed-off-by: Khairul Anuar Romli 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 > Reported-by: kernel test robot > Closes: https://lore.kernel.org/oe-kbuild-all/202602032325.MuglZ156-lkp@intel.com/ > --- > 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