* [PATCH v2 1/2] firmware: stratix10-svc: Support up to N SVC clients
2023-06-21 6:46 [PATCH v2 0/2] firmware: stratix10-svc: support N clients tien.sung.ang
@ 2023-06-21 6:46 ` tien.sung.ang
2023-06-21 6:46 ` [PATCH v2 2/2] firmware: stratix10-svc: fix bug in saving controller data tien.sung.ang
1 sibling, 0 replies; 4+ messages in thread
From: tien.sung.ang @ 2023-06-21 6:46 UTC (permalink / raw)
To: Dinh Nguyen; +Cc: linux-kernel, Ang Tien Sung
From: Ang Tien Sung <tien.sung.ang@intel.com>
This fixes the SVC Time-out issue on Intel's firmware SVC mailbox service
when more than 1 client driver sends SMC commands concurrently. A thread
is created now per channel. Current stratix10-svc driver supports N
channels. Thread synchronization with mutex is used to prevent more
than one thread from calling SMC command. The time-out are adjusted
to cater for multiple drivers.
In the old implementation, the respective SVC client drivers like
intel_fcs and stratix10-soc sends a SMC command
and this triggers a single thread at the stratix10-svc driver.
Upon receiving a callback, the caller driver sends
stratix10-svc-done and it stops the thread without waiting
for the other SMC commands to complete.
Signed-off-by: Ang Tien Sung <tien.sung.ang@intel.com>
---
drivers/firmware/stratix10-svc.c | 181 ++++++++++++++++++++-----------
1 file changed, 119 insertions(+), 62 deletions(-)
diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c
index 80f4e2d14e04..ca713f39107e 100644
--- a/drivers/firmware/stratix10-svc.c
+++ b/drivers/firmware/stratix10-svc.c
@@ -33,9 +33,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 3
-#define FPGA_CONFIG_DATA_CLAIM_TIMEOUT_MS 200
+#define FPGA_CONFIG_DATA_CLAIM_TIMEOUT_MS 2000
#define FPGA_CONFIG_STATUS_TIMEOUT_SEC 30
/* stratix10 service layer clients */
@@ -125,14 +125,11 @@ struct stratix10_svc_data {
* @dev: device
* @chans: array of service channels
* @num_chans: number of channels in 'chans' array
- * @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
+ * @sdm_lock: a mutex lock to allow only one pending sdm message per client
*
* This struct is used to create communication channels for service clients, to
* handle secure monitor or hypervisor call.
@@ -141,14 +138,12 @@ struct stratix10_svc_controller {
struct device *dev;
struct stratix10_svc_chan *chans;
int num_chans;
- 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;
+ /* Enforces atomic command sending to SDM */
+ struct mutex *sdm_lock;
};
/**
@@ -156,6 +151,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
*
* This struct is used by service client to communicate with service layer, each
@@ -165,6 +163,11 @@ struct stratix10_svc_chan {
struct stratix10_svc_controller *ctrl;
struct stratix10_svc_client *scl;
char *name;
+ struct task_struct *task;
+ struct kfifo svc_fifo;
+ /* Access protection to the message data queue */
+ spinlock_t svc_fifo_lock;
+ /* Access protection to the channel */
spinlock_t lock;
};
@@ -382,13 +385,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)
@@ -410,12 +414,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;
@@ -424,6 +428,16 @@ 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 */
+ if (!sdm_lock_owned) {
+ /* Must not do mutex re-lock */
+ pr_debug("%s: %s: Thread is waiting for mutex!\n",
+ __func__, chan->name);
+ mutex_lock(ctrl->sdm_lock);
+ }
+
+ sdm_lock_owned = true;
+
switch (pdata->command) {
case COMMAND_RECONFIG_DATA_CLAIM:
svc_thread_cmd_data_claim(ctrl, pdata, cbdata);
@@ -538,8 +552,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);
@@ -548,8 +562,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);
@@ -564,6 +578,8 @@ 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);
+ sdm_lock_owned = false;
continue;
}
@@ -637,7 +653,9 @@ static int svc_normal_to_secure_thread(void *data)
}
}
-
+ pr_debug("%s: %s: Exit thread\n", __func__, chan->name);
+ if (sdm_lock_owned)
+ mutex_unlock(ctrl->sdm_lock);
kfree(cbdata);
kfree(pdata);
@@ -699,7 +717,7 @@ static int svc_get_sh_memory(struct platform_device *pdev,
/* smc or hvc call happens on cpu 0 bound kthread */
sh_memory_task = kthread_create_on_node(svc_normal_to_secure_shm_thread,
- (void *)sh_memory,
+ (void *)sh_memory,
cpu_to_node(cpu),
"svc_smc_hvc_shm_thread");
if (IS_ERR(sh_memory_task)) {
@@ -897,7 +915,6 @@ struct stratix10_svc_chan *stratix10_svc_request_channel_byname(
spin_lock_irqsave(&chan->lock, flag);
chan->scl = client;
- chan->ctrl->num_active_client++;
spin_unlock_irqrestore(&chan->lock, flag);
return chan;
@@ -916,7 +933,6 @@ void stratix10_svc_free_channel(struct stratix10_svc_chan *chan)
spin_lock_irqsave(&chan->lock, flag);
chan->scl = NULL;
- chan->ctrl->num_active_client--;
module_put(chan->ctrl->dev->driver->owner);
spin_unlock_irqrestore(&chan->lock, flag);
}
@@ -947,24 +963,24 @@ 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_create_on_node(svc_normal_to_secure_thread,
- (void *)chan->ctrl,
- cpu_to_node(cpu),
- "svc_smc_hvc_thread");
- if (IS_ERR(chan->ctrl->task)) {
+ (void *)chan,
+ cpu_to_node(cpu),
+ "svc_smc_hvc_thread");
+ if (IS_ERR(chan->task)) {
dev_err(chan->ctrl->dev,
"failed to create svc_smc_hvc_thread\n");
kfree(p_data);
return -EINVAL;
}
- kthread_bind(chan->ctrl->task, cpu);
- wake_up_process(chan->ctrl->task);
+ kthread_bind(chan->task, cpu);
+ wake_up_process(chan->task);
}
- 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)) {
@@ -999,12 +1015,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);
@@ -1025,12 +1045,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);
@@ -1068,8 +1097,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;
}
@@ -1089,7 +1118,7 @@ void stratix10_svc_free_memory(struct stratix10_svc_chan *chan, void *kaddr)
list_for_each_entry(pmem, &svc_data_mem, node)
if (pmem->vaddr == kaddr) {
gen_pool_free(chan->ctrl->genpool,
- (unsigned long)kaddr, pmem->size);
+ (unsigned long)kaddr, pmem->size);
pmem->vaddr = NULL;
list_del(&pmem->node);
return;
@@ -1105,6 +1134,23 @@ static const struct of_device_id stratix10_svc_drv_match[] = {
{},
};
+static DEFINE_MUTEX(mailbox_lock);
+
+static void stratix10_svc_free_channels(struct stratix10_svc_controller *ctrl)
+{
+ int i;
+
+ for (i = 0; i < SVC_NUM_CHANNEL; i++) {
+ if (ctrl->chans[i].task) {
+ kthread_stop(ctrl->chans[i].task);
+ ctrl->chans[i].task = NULL;
+ }
+
+ if (&ctrl->chans[i].svc_fifo)
+ kfifo_free(&ctrl->chans[i].svc_fifo);
+ }
+}
+
static int stratix10_svc_drv_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
@@ -1152,35 +1198,50 @@ static int stratix10_svc_drv_probe(struct platform_device *pdev)
controller->dev = dev;
controller->num_chans = SVC_NUM_CHANNEL;
- controller->num_active_client = 0;
controller->chans = chans;
controller->genpool = genpool;
- controller->task = NULL;
controller->invoke_fn = invoke_fn;
init_completion(&controller->complete_status);
+ /* 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;
- ret = kfifo_alloc(&controller->svc_fifo, fifo_size, GFP_KERNEL);
- if (ret) {
- dev_err(dev, "failed to allocate FIFO\n");
- goto err_destroy_pool;
- }
- 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);
+ 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);
list_add_tail(&controller->node, &svc_ctrl);
platform_set_drvdata(pdev, controller);
@@ -1227,7 +1288,7 @@ static int stratix10_svc_drv_probe(struct platform_device *pdev)
err_unregister_dev:
platform_device_unregister(svc->stratix10_svc_rsu);
err_free_kfifo:
- kfifo_free(&controller->svc_fifo);
+ stratix10_svc_free_channels(controller);
err_destroy_pool:
gen_pool_destroy(genpool);
return ret;
@@ -1241,11 +1302,7 @@ static int 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;
- }
+ stratix10_svc_free_channels(ctrl);
if (ctrl->genpool)
gen_pool_destroy(ctrl->genpool);
list_del(&ctrl->node);
--
2.25.1
^ permalink raw reply related [flat|nested] 4+ messages in thread