From: Ang Tien Sung <[email protected]>
hi,
In order to support N clients sending concurrent messages to svc driver,
we need to have all clients owning up to their individual threads.
And, to cater for the limitation of the mailbox device driver in
secure monitor firmware that only permits a single message at a time,
a mutex is used to prevent the multiple clients from sending a message
to SDM.
Also, added a fix on the removal of the driver that resulted in a
kernel panic due to badly used drv_set_drvdata.
changelog from V1:
- Added "Fixes:" tag to the 2nd patch to detail the fix location.
changelog from V2:
- Remove newline from the 2nd patch after the tag "Fixes:"
Ang Tien Sung (2):
firmware: stratix10-svc: Support up to N SVC clients
firmware: stratix10-svc: fix bug in saving controller data
drivers/firmware/stratix10-svc.c | 188 ++++++++++++++++++++-----------
1 file changed, 123 insertions(+), 65 deletions(-)
--
2.25.1
From: Ang Tien Sung <[email protected]>
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 <[email protected]>
---
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