Re: [PATCH v3 8/9] mailbox: mediatek: Add CMDQ secure mailbox driver

CK Hu (胡俊光)
2023-12-26T06:15:34Z

Hi, Jason:

On Fri, 2023-12-22 at 12:52 +0800, Jason-JH.Lin wrote:
> To support secure video path feature, GCE have to read/write
> registgers
> in the secure world. GCE will enable the secure access permission to
> the
> HW who wants to access the secure content buffer.
>
> Add CMDQ secure mailbox driver to make CMDQ client user is able to
> sending their HW settings to the secure world. So that GCE can
> execute
> all instructions to configure HW in the secure world.
>
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> ---

[snip]

> +
> +static void cmdq_sec_task_exec_work(struct work_struct *work_item)
> +{
> + struct cmdq_sec_task *sec_task = container_of(work_item,
> + struct
> cmdq_sec_task, exec_work);
> + struct cmdq_sec_thread *sec_thread = container_of(sec_task-
> >task.thread,
> + struct
> cmdq_sec_thread, thread);
> + struct cmdq_sec *cmdq = container_of(sec_thread->thread.chan-
> >mbox,
> + struct cmdq_sec, mbox);
> + struct cmdq_sec_data *data;
> + unsigned long flags;
> + int err;
> +
> + dev_dbg(cmdq->mbox.dev, "%s gce:%#lx sec_task:%p pkt:%p
> thread:%u",
> + __func__, (unsigned long)cmdq->base_pa,
> + sec_task, sec_task->task.pkt, sec_thread->idx);
> +
> + if (!sec_task->task.pkt->sec_data) {
> + dev_err(cmdq->mbox.dev, "pkt:%p without sec_data",
> sec_task->task.pkt);
> + return;
> + }
> + data = (struct cmdq_sec_data *)sec_task->task.pkt->sec_data;
> +
> + mutex_lock(&cmdq->exec_lock);
> +
> + spin_lock_irqsave(&sec_thread->thread.chan->lock, flags);
> + if (!sec_thread->task_cnt) {
> + mod_timer(&sec_thread->timeout, jiffies +
> + msecs_to_jiffies(sec_thread->timeout_ms));
> + sec_thread->wait_cookie = 1;
> + sec_thread->next_cookie = 1;
> + sec_thread->task_cnt = 0;
> + __raw_writel(0, (void __iomem *)cmdq->shared_mem->va +
> + CMDQ_SEC_SHARED_THR_CNT_OFFSET +
> sec_thread->idx * sizeof(u32));
> + }
> +
> + sec_task->reset_exec = sec_thread->task_cnt ? false : true;
> + sec_task->wait_cookie = sec_thread->next_cookie;
> + sec_thread->next_cookie = (sec_thread->next_cookie + 1) %
> CMDQ_MAX_COOKIE_VALUE;
> + list_add_tail(&sec_task->task.list_entry, &sec_thread-
> >thread.task_busy_list);
> + sec_thread->task_cnt += 1;
> + spin_unlock_irqrestore(&sec_thread->thread.chan->lock, flags);
> + sec_task->trigger = sched_clock();
> +
> + if (!atomic_cmpxchg(&cmdq_path_res, 0, 1)) {
> + err = cmdq_sec_task_submit(cmdq, NULL,
> CMD_CMDQ_IWC_PATH_RES_ALLOCATE,
> + CMDQ_INVALID_THREAD, NULL);
> + if (err) {
> + atomic_set(&cmdq_path_res, 0);
> + goto task_end;
> + }
> + }
> +
> + if (sec_thread->task_cnt > CMDQ_MAX_TASK_IN_SECURE_THREAD) {
> + dev_err(cmdq->mbox.dev, "task_cnt:%u cannot more than
> %u sec_task:%p thread:%u",
> + sec_thread->task_cnt,
> CMDQ_MAX_TASK_IN_SECURE_THREAD,
> + sec_task, sec_thread->idx);
> + err = -EMSGSIZE;
> + goto task_end;
> + }
> +
> + err = cmdq_sec_task_submit(cmdq, sec_task,
> CMD_CMDQ_IWC_SUBMIT_TASK,
> + sec_thread->idx, NULL);
> + if (err)
> + dev_err(cmdq->mbox.dev, "cmdq_sec_task_submit err:%d
> sec_task:%p thread:%u",
> + err, sec_task, sec_thread->idx);
> +
> +task_end:
> + if (err) {
> + struct cmdq_cb_data cb_data;
> +
> + cb_data.sta = err;
> + cb_data.pkt = sec_task->task.pkt;
> + mbox_chan_received_data(sec_thread->thread.chan,
> &cb_data);
> +
> + spin_lock_irqsave(&sec_thread->thread.chan->lock,
> flags);
> + if (!sec_thread->task_cnt)
> + dev_err(cmdq->mbox.dev, "thread:%u task_cnt:%u
> cannot below zero",
> + sec_thread->idx, sec_thread->task_cnt);
> + else
> + sec_thread->task_cnt -= 1;
> +
> + sec_thread->next_cookie = (sec_thread->next_cookie - 1
> +
> + CMDQ_MAX_COOKIE_VALUE) % CMDQ_MAX_COOKIE_VALUE;
> + list_del(&sec_task->task.list_entry);
> + dev_dbg(cmdq->mbox.dev, "gce:%#lx err:%d sec_task:%p
> pkt:%p",
> + (unsigned long)cmdq->base_pa, err, sec_task,
> sec_task->task.pkt);
> + dev_dbg(cmdq->mbox.dev, "thread:%u task_cnt:%u
> wait_cookie:%u next_cookie:%u",
> + sec_thread->idx, sec_thread->task_cnt,
> + sec_thread->wait_cookie, sec_thread-
> >next_cookie);
> + spin_unlock_irqrestore(&sec_thread->thread.chan->lock,
> flags);
> +
> + kfree(sec_task);
> + }
> +
> + mutex_unlock(&cmdq->exec_lock);
> +}
> +
> +static int cmdq_sec_mbox_send_data(struct mbox_chan *chan, void
> *data)
> +{
> + struct cmdq_pkt *pkt = (struct cmdq_pkt *)data;
> + struct cmdq_sec_data *sec_data = (struct cmdq_sec_data *)pkt-
> >sec_data;
> + struct cmdq_thread *thread = (struct cmdq_thread *)chan-
> >con_priv;
> + struct cmdq_sec_thread *sec_thread = container_of(thread,
> struct cmdq_sec_thread, thread);
> + struct cmdq_sec_task *sec_task;
> +
> + if (!sec_data)
> + return -EINVAL;
> +
> + sec_task = kzalloc(sizeof(*sec_task), GFP_ATOMIC);
> + if (!sec_task)
> + return -ENOMEM;
> +
> + sec_task->task.pkt = pkt;
> + sec_task->task.thread = thread;
> + sec_task->scenario = sec_data->scenario;
> + sec_task->engine_flag = sec_data->engs_need_dapc | sec_data-
> >engs_need_sec_port;
> +
> + INIT_WORK(&sec_task->exec_work, cmdq_sec_task_exec_work);
> + queue_work(sec_thread->task_exec_wq, &sec_task->exec_work);

It's not necessary to queue a work here. Squash
cmdq_sec_task_exec_work() into this function.

Regards,
CK

> + return 0;
> +}
> +