Re: [PATCH v5] firmware: stratix10-svc: Add Multi SVC clients support

[Dinh Nguyen <dinguyen@kernel.org>]

On 3/1/26 23:39, 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 <tien.sung.ang@altera.com>
> Signed-off-by: Fong, Yan Kei <yankei.fong@altera.com>
> Signed-off-by: Muhammad Amirul Asyraf Mohamad Jamian <muhammad.amirul.asyraf.mohamad.jamian@altera.com>
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202602032325.MuglZ156-lkp@intel.com/
> ---
> Changes in v5:
> - Fix mutex locking: replace guard(mutex) with explicit lock/unlock,
>    use mutex_lock_interruptible() so kthread_stop() can unblock the
>    thread when the SDM lock is contended, fix lock/unlock imbalances,
>    and on interrupted lock notify client with SVC_STATUS_ERROR so it
>    does not block on a response that never arrives
> - Fix TOCTOU race in stratix10_svc_send(): protect chan->task creation
>    with chan->lock spinlock to prevent concurrent callers on the same
>    channel from spawning duplicate threads
> - Fix probe error path: initialize controller->node with INIT_LIST_HEAD
>    and guard list_del() so it is only called when list_add_tail() was
>    reached, preventing NULL deref on early FIFO allocation failure
> - Remove stray platform_device_unregister(svc->intel_svc_fcs) call in
>    remove function, as FCS device is not manually registered in probe;
>    also remove the now-unused INTEL_FCS macro, stale intel_svc_fcs field
>    from struct stratix10_svc, and stale @task kdoc from
>    struct stratix10_svc_controller
> - Remove unnecessary NULL check on svc pointer in err_put_device
> - Clean up: convert FIFO allocations to for loop, simplify
>    stratix10_svc_done() to a single debug message, fix whitespace,
>    typos and log messages
> 
> Changes in v4:
> - Signed-off-by chain clean up
> - Remove duplicate inline comment on sdm_lock field
> - Move structure field comments to structure header
> - Remove unnecessary checks in stratix10_svc_done()
> - Fix error handling to properly free FIFOs on probe failure
> - Reorder error labels for proper cleanup cascade
> - Remove redundant fifo_size initialization
> 
> 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              | 225 ++++++++++--------
>   .../firmware/intel/stratix10-svc-client.h     |   8 +-
>   2 files changed, 128 insertions(+), 105 deletions(-)
> 
> diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c
> index 6f5c298582ab..2af95018f06d 100644
> --- a/drivers/firmware/stratix10-svc.c
> +++ b/drivers/firmware/stratix10-svc.c
> @@ -37,15 +37,14 @@
>    * 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

Your commit message does not explain this change. I don't see how this 
change is related to this patch.


>   #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
>   
>   /* stratix10 service layer clients */
>   #define STRATIX10_RSU				"stratix10-rsu"
> -#define INTEL_FCS				"intel-fcs"
>   
>   /* Maximum number of SDM client IDs. */
>   #define MAX_SDM_CLIENT_IDS			16
> @@ -105,11 +104,9 @@ struct stratix10_svc_chan;
>   /**
>    * struct stratix10_svc - svc private data
>    * @stratix10_svc_rsu: pointer to stratix10 RSU device
> - * @intel_svc_fcs: pointer to the FCS device
>    */
>   struct stratix10_svc {
>   	struct platform_device *stratix10_svc_rsu;
> -	struct platform_device *intel_svc_fcs;
>   };
>   
>   /**
> @@ -251,12 +248,10 @@ struct stratix10_async_ctrl {
>    * @num_active_client: number of active service client
>    * @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 +264,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;
>   	struct stratix10_async_ctrl actrl;
>   };
>   
> @@ -283,6 +276,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 (separate fifo for every channel)
> + * @svc_fifo_lock: protect access to service message data queue (locking pending fifo)
>    * @lock: protect access to the channel
>    * @async_chan: reference to asynchronous channel object for this channel
>    *
> @@ -293,6 +289,9 @@ struct stratix10_svc_chan {
>   	struct stratix10_svc_controller *ctrl;
>   	struct stratix10_svc_client *scl;
>   	char *name;
> +	struct task_struct *task;
> +	struct kfifo svc_fifo;
> +	spinlock_t svc_fifo_lock;
>   	spinlock_t lock;
>   	struct stratix10_async_chan *async_chan;
>   };
> @@ -527,10 +526,10 @@ 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;
> @@ -555,12 +554,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,9 +568,25 @@ static int svc_normal_to_secure_thread(void *data)
>   			 (unsigned int)pdata->paddr, pdata->command,
>   			 (unsigned int)pdata->size);
>   
> +		/* SDM can only process one command at a time */
> +		pr_debug("%s: %s: Thread is waiting for mutex!\n",
> +			 __func__, chan->name);
> +		if (mutex_lock_interruptible(ctrl->sdm_lock)) {
> +			/* item already dequeued; notify client to unblock it */
> +			cbdata->status = BIT(SVC_STATUS_ERROR);
> +			cbdata->kaddr1 = NULL;
> +			cbdata->kaddr2 = NULL;
> +			cbdata->kaddr3 = NULL;
> +			if (pdata->chan->scl)
> +				pdata->chan->scl->receive_cb(pdata->chan->scl,
> +							     cbdata);
> +			break;
> +		}
> +
>   		switch (pdata->command) {
>   		case COMMAND_RECONFIG_DATA_CLAIM:
>   			svc_thread_cmd_data_claim(ctrl, pdata, cbdata);
> +			mutex_unlock(ctrl->sdm_lock);
>   			continue;
>   		case COMMAND_RECONFIG:
>   			a0 = INTEL_SIP_SMC_FPGA_CONFIG_START;
> @@ -700,10 +715,11 @@ static int svc_normal_to_secure_thread(void *data)
>   			break;
>   		default:
>   			pr_warn("it shouldn't happen\n");
> -			break;
> +			mutex_unlock(ctrl->sdm_lock);
> +			continue;
>   		}
> -		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 +728,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 +744,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);
> +			mutex_unlock(ctrl->sdm_lock);
>   			continue;
>   		}
>   
> @@ -801,6 +818,8 @@ static int svc_normal_to_secure_thread(void *data)
>   			break;
>   
>   		}
> +
> +		mutex_unlock(ctrl->sdm_lock);
>   	}
>   
>   	kfree(cbdata);
> @@ -1697,21 +1716,25 @@ 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 =
> +	spin_lock(&chan->lock);
> +	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)) {
> +		if (IS_ERR(chan->task)) {
> +			chan->task = NULL;
> +			spin_unlock(&chan->lock);
>   			dev_err(chan->ctrl->dev,
>   				"failed to create svc_smc_hvc_thread\n");
>   			kfree(p_data);
>   			return -EINVAL;
>   		}
>   	}
> +	spin_unlock(&chan->lock);
>   
> -	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 +1770,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,11 +1800,12 @@ 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) {
> +		pr_debug("%s: %s: svc_smc_hvc_shm_thread is stopping\n",
> +			 __func__, chan->name);
> +		kthread_stop(chan->task);
> +		chan->task = NULL;
>   	}
>   }
>   EXPORT_SYMBOL_GPL(stratix10_svc_done);
> @@ -1817,8 +1845,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 +1883,18 @@ static const struct of_device_id stratix10_svc_drv_match[] = {
>   	{},
>   };
>   
> +/**
> + * mailbox_lock protects access to the SDM if thread is busy
> + */
> +static DEFINE_MUTEX(mailbox_lock);

Should you use a global mutex? I'd consider using a local mutex that you 
already have defined in the stratix10_svc_controller->sdm_lock.

> +
> +static const char * const chan_names[SVC_NUM_CHANNEL] = {
> +	SVC_CLIENT_FPGA,
> +	SVC_CLIENT_RSU,
> +	SVC_CLIENT_FCS,
> +	SVC_CLIENT_HWMON
> +};
> +
>   static int stratix10_svc_drv_probe(struct platform_device *pdev)
>   {
>   	struct device *dev = &pdev->dev;
> @@ -1862,11 +1902,11 @@ 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;
> -	int ret;
> +	int ret, i = 0;
>   
>   	/* get SMC or HVC function */
>   	invoke_fn = get_invoke_func(dev);
> @@ -1905,8 +1945,8 @@ 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_LIST_HEAD(&controller->node);
>   	init_completion(&controller->complete_status);
>   
>   	ret = stratix10_svc_async_init(controller);
> @@ -1917,32 +1957,24 @@ 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);
> -	if (ret) {
> -		dev_err(dev, "failed to allocate FIFO\n");
> -		goto err_async_exit;
> -	}
> -	spin_lock_init(&controller->svc_fifo_lock);
> -
> -	chans[0].scl = NULL;
> -	chans[0].ctrl = controller;
> -	chans[0].name = SVC_CLIENT_FPGA;
> -	spin_lock_init(&chans[0].lock);
> -
> -	chans[1].scl = NULL;
> -	chans[1].ctrl = controller;
> -	chans[1].name = SVC_CLIENT_RSU;
> -	spin_lock_init(&chans[1].lock);
> -
> -	chans[2].scl = NULL;
> -	chans[2].ctrl = controller;
> -	chans[2].name = SVC_CLIENT_FCS;
> -	spin_lock_init(&chans[2].lock);
> +	/*
> +	 * This mutex is used to block threads from utilizing
> +	 * SDM to prevent out of order command tx
> +	 */
> +	controller->sdm_lock = &mailbox_lock;
>   
> -	chans[3].scl = NULL;
> -	chans[3].ctrl = controller;
> -	chans[3].name = SVC_CLIENT_HWMON;
> -	spin_lock_init(&chans[3].lock);
> +	for (i = 0; i < SVC_NUM_CHANNEL; i++) {
> +		chans[i].scl = NULL;
> +		chans[i].ctrl = controller;
> +		chans[i].name = (char *)chan_names[i];
> +		spin_lock_init(&chans[i].lock);
> +		ret = kfifo_alloc(&chans[i].svc_fifo, fifo_size, GFP_KERNEL);
> +		if (ret) {
> +			dev_err(dev, "failed to allocate FIFO %d\n", i);
> +			goto err_free_fifos;
> +		}
> +		spin_lock_init(&chans[i].svc_fifo_lock);
> +	}
>   
>   	list_add_tail(&controller->node, &svc_ctrl);
>   	platform_set_drvdata(pdev, controller);
> @@ -1951,7 +1983,7 @@ static int stratix10_svc_drv_probe(struct platform_device *pdev)
>   	svc = devm_kzalloc(dev, sizeof(*svc), GFP_KERNEL);
>   	if (!svc) {
>   		ret = -ENOMEM;
> -		goto err_free_kfifo;
> +		goto err_free_fifos;
>   	}
>   	controller->svc = svc;
>   
> @@ -1959,51 +1991,40 @@ static int stratix10_svc_drv_probe(struct platform_device *pdev)
>   	if (!svc->stratix10_svc_rsu) {
>   		dev_err(dev, "failed to allocate %s device\n", STRATIX10_RSU);
>   		ret = -ENOMEM;
> -		goto err_free_kfifo;
> +		goto err_free_fifos;
>   	}
>   
>   	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;

You need a new error path here because platform_device_add() has 
succeeded, but your error path is only calling platform_device_put() 
which only decrements the refcount, but does not unregister. You need an 
error path here that will call platform_device_unregister().

>   
>   	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:
> +err_put_device:
> +	platform_device_put(svc->stratix10_svc_rsu);
> +err_free_fifos:
> +	/* only remove from list if list_add_tail() was reached */
> +	if (!list_empty(&controller->node))
> +		list_del(&controller->node);
> +	/* free only the FIFOs that were successfully allocated */
> +	while (i--)
> +		kfifo_free(&chans[i].svc_fifo);
>   	stratix10_svc_async_exit(controller);
>   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;
>   
> @@ -2011,14 +2032,16 @@ static void stratix10_svc_drv_remove(struct platform_device *pdev)
>   
>   	of_platform_depopulate(ctrl->dev);
>   
> -	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;
>   

Dinh