lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <aa71f976a864e8813d8feacc99ea6ba0b35196a9.camel@mediatek.com>
Date:   Wed, 25 Oct 2023 06:20:13 +0000
From:   Jason-JH Lin (林睿祥) 
        <Jason-JH.Lin@...iatek.com>
To:     "jassisinghbrar@...il.com" <jassisinghbrar@...il.com>,
        "angelogioacchino.delregno@...labora.com" 
        <angelogioacchino.delregno@...labora.com>,
        "chunkuang.hu@...nel.org" <chunkuang.hu@...nel.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "krzysztof.kozlowski+dt@...aro.org" 
        <krzysztof.kozlowski+dt@...aro.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-mediatek@...ts.infradead.org" 
        <linux-mediatek@...ts.infradead.org>,
        Singo Chang (張興國) 
        <Singo.Chang@...iatek.com>,
        Johnson Wang (王聖鑫) 
        <Johnson.Wang@...iatek.com>,
        Jason-ch Chen (陳建豪) 
        <Jason-ch.Chen@...iatek.com>,
        Shawn Sung (宋孝謙) 
        <Shawn.Sung@...iatek.com>,
        Nancy Lin (林欣螢) <Nancy.Lin@...iatek.com>,
        "jkardatzke@...gle.com" <jkardatzke@...gle.com>,
        "conor+dt@...nel.org" <conor+dt@...nel.org>,
        Project_Global_Chrome_Upstream_Group 
        <Project_Global_Chrome_Upstream_Group@...iatek.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "matthias.bgg@...il.com" <matthias.bgg@...il.com>
Subject: Re: [PATCH v2 8/9] mailbox: mediatek: Add CMDQ secure mailbox driver

Hi Angelo,

Thanks for the reviews.

On Mon, 2023-10-23 at 12:48 +0200, AngeloGioacchino Del Regno wrote:
> Il 23/10/23 06:37, Jason-JH.Lin ha scritto:
> > 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@...iatek.com>
> 
> This driver needs to be cleaned up quite a bit, and I'd expect this
> to take a few
> versions before being upstreamable.
> 
Sure, it's hard to make it upstreamable. It will get better and better
after your reviews. :)

> > ---
> >   drivers/mailbox/Makefile                      |    2 +-
> >   drivers/mailbox/mtk-cmdq-sec-mailbox.c        | 1102
> > +++++++++++++++++
> >   drivers/mailbox/mtk-cmdq-sec-tee.c            |  202 +++
> >   include/linux/mailbox/mtk-cmdq-mailbox.h      |    2 +
> >   .../linux/mailbox/mtk-cmdq-sec-iwc-common.h   |  293 +++++
> >   include/linux/mailbox/mtk-cmdq-sec-mailbox.h  |   83 ++
> >   include/linux/mailbox/mtk-cmdq-sec-tee.h      |   31 +
> >   7 files changed, 1714 insertions(+), 1 deletion(-)
> >   create mode 100644 drivers/mailbox/mtk-cmdq-sec-mailbox.c
> >   create mode 100644 drivers/mailbox/mtk-cmdq-sec-tee.c
> >   create mode 100644 include/linux/mailbox/mtk-cmdq-sec-iwc-
> > common.h
> >   create mode 100644 include/linux/mailbox/mtk-cmdq-sec-mailbox.h
> >   create mode 100644 include/linux/mailbox/mtk-cmdq-sec-tee.h
> > 
> > diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> > index fc9376117111..82da2f4ee81a 100644
> > --- a/drivers/mailbox/Makefile
> > +++ b/drivers/mailbox/Makefile
> > @@ -51,7 +51,7 @@ obj-$(CONFIG_STM32_IPCC) 	+= stm32-ipcc.o
> >   
> >   obj-$(CONFIG_MTK_ADSP_MBOX)	+= mtk-adsp-mailbox.o
> >   
> > -obj-$(CONFIG_MTK_CMDQ_MBOX)	+= mtk-cmdq-mailbox.o
> > +obj-$(CONFIG_MTK_CMDQ_MBOX)	+= mtk-cmdq-mailbox.o mtk-cmdq-
> > sec-mailbox.o mtk-cmdq-sec-tee.o
> >   
> >   obj-$(CONFIG_ZYNQMP_IPI_MBOX)	+= zynqmp-ipi-mailbox.o
> >   
> > diff --git a/drivers/mailbox/mtk-cmdq-sec-mailbox.c
> > b/drivers/mailbox/mtk-cmdq-sec-mailbox.c
> > new file mode 100644
> > index 000000000000..dac867386cf1
> > --- /dev/null
> > +++ b/drivers/mailbox/mtk-cmdq-sec-mailbox.c
> > @@ -0,0 +1,1102 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +//
> > +// Copyright (c) 2019 MediaTek Inc.
> > +
> 
> Fix comment style please:
> 
> // SPDX...
> /*
>   * Copyright ...
>   */
> 
OK, I'll fix this.

> > +#include <linux/clk.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/io.h>
> > +#include <linux/mailbox_controller.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/sched/clock.h>
> > +#include <linux/timer.h>
> > +
> > +#include <linux/mailbox/mtk-cmdq-sec-mailbox.h>
> > +
> > +#define CMDQ_THR_BASE		(0x100)
> > +#define CMDQ_THR_SIZE		(0x80)
> 
> Those two definitions are the same as in mtk-cmdq-mailbox.c: please
> avoid code
> duplication around, it's harder to maintain.

OK, I'll move this to header file.

> 
> > +#define CMDQ_THR_EXEC_CNT_PA	(0x28)
> > +
> > +#define CMDQ_BUF_ALLOC_SIZE		(PAGE_SIZE)
> > +#define CMDQ_TIMEOUT_DEFAULT		(1000)
> > +#define CMDQ_NO_TIMEOUT			(0xffffffff)
> > +#define ADDR_METADATA_MAX_COUNT_ORIGIN	(8)
> > +
> > +/*
> > + * CMDQ secure context struct
> > + * note it is not global data, each process has its own CMDQ sec
> > context
> > + */
> 
> This structure is practically fully documented: please use kerneldoc
> at this point.
> 
OK, I'll refine these structures here.
By the way, is there any tools can scan the structures or function that
are met to kernel doc?

> > +struct cmdq_sec_context {
> > +	struct list_head		listEntry;
> 
> listEntry -> list_entry or listentry - no camelcase please.
> 
> > +
> > +	/* basic info */
> > +	u32				tgid; /* tgid of process context */
> > +	u32				referCount; /* reference count for
> > open cmdq device node */
> > +
> 
> referCount -> refcount?
> 
> > +	/* iwc state */
> > +	enum cmdq_iwc_state_enum	state;
> 
> iwc_state
> 
> > +
> > +	/* iwc information */
> > +	void				*iwc_msg; /* message buffer
> > */
> > +
> > +	struct cmdq_sec_tee_context	tee; /* trustzone parameters */
> 
> tee_ctx
> 
> > +};
> > +
> > +struct cmdq_sec_task {
> > +	struct list_head	list_entry;
> > +	dma_addr_t		pa_base;
> > +	struct cmdq_sec_thread	*thread;
> > +	struct cmdq_pkt		*pkt;
> > +	u64			exec_time;
> > +	struct work_struct	exec_work;
> > +
> > +	bool			resetExecCnt;
> 
> Again no camelcase - here and everywhere else.
> 
> Also, I see a pattern in all of the _sec_ structures: those seem to
> be the same
> as the non-sec ones, as defined in mtk-cmdq-mailbox, but with more
> params.
> 
> I understand the memory concerns, but there's an easy way out of
> this... we could
> get this done like:
> 
> struct cmdq_sec_task {
> 	struct cmdq_task	tsk;
> 
> 	/* Secure CMDQ */
> 	u32			wait_cookie;
> 	s32			scenario;
> 	u64			engine_flag;
> 	u64			trigger;
> }
> 
> ...and this can be done for all of the _sec_ variants of the cmdq
> mbox structs
> as far as I can see.

OK, Thanks for the advices.
I'll reuse the same part of struct cmdq_task.

> > +	u32			waitCookie;
> > +
> > +	u64			engineFlag;
> > +	s32			scenario;
> > +	u64			trigger;
> > +};
> > +
> > +struct cmdq_sec_thread {
> > +	struct mbox_chan	*chan;
> > +	void __iomem		*base;
> > +	struct list_head	task_list;
> > +	struct timer_list	timeout;
> > +	u32			timeout_ms;
> > +	struct work_struct	timeout_work;
> > +	u32			priority;
> > +	u32			idx;
> > +	bool			occupied;
> > +
> > +	/* following part only secure ctrl */
> > +	u32			wait_cookie;
> > +	u32			next_cookie;
> > +	u32			task_cnt;
> > +	struct workqueue_struct	*task_exec_wq;
> > +};
> > +
> > +/**
> 
> This is not valid kerneldoc.

OK, I'll refine it.

> 
> > + * shared memory between normal and secure world
> > + */
> > +struct cmdq_sec_shared_mem {
> > +	void		*va;
> > +	dma_addr_t	pa;
> > +	u32		size;
> > +};
> > +
> > +struct cmdq_sec {
> > +	struct mbox_controller		mbox;
> > +	void __iomem			*base;
> > +	phys_addr_t			base_pa;
> > +	u32				hwid;
> > +	u32				gce_num;
> > +	struct clk_bulk_data		*clocks;
> > +	u32				thread_nr;
> > +	struct cmdq_sec_thread		*thread;
> > +	struct cmdq_client		*clt;
> > +	struct cmdq_pkt			*clt_pkt;
> > +
> > +	atomic_t			path_res;
> > +	struct cmdq_sec_shared_mem	*shared_mem;
> > +	struct cmdq_sec_context		*context;
> > +	struct iwcCmdqCancelTask_t	cancel;
> > +
> > +	struct workqueue_struct		*timeout_wq;
> > +	u64				sec_invoke;
> > +	u64				sec_done;
> > +
> > +	bool				notify_run;
> > +	struct work_struct		irq_notify_work;
> > +	struct workqueue_struct		*notify_wq;
> > +	u32				cmdq_event;
> > +	/* mutex for cmdq_sec_thread excuting cmdq_sec_task */
> > +	struct mutex			exec_lock;
> > +};
> > +
> > +static atomic_t cmdq_path_res = ATOMIC_INIT(0);
> > +
> > +static int cmdq_sec_task_submit(struct cmdq_sec *cmdq, struct
> > cmdq_sec_task *task,
> > +				const u32 iwc_cmd, const u32 thrd_idx,
> > void *data);
> > +
> > +static inline void cmdq_sec_setup_tee_context_base(struct
> > cmdq_sec_context *context)
> 
> You don't need this function, as it's just wrapping over another one:
> you can just
> call cmdq_sec_setup_tee_context.

OK, I'll remove this wrapping function.

> 
> > +{
> > +	cmdq_sec_setup_tee_context(&context->tee);
> > +}
> > +
> > +static inline int cmdq_sec_init_context_base(struct
> > cmdq_sec_context *context)
> > +{
> > +	int status;
> > +
> > +	status = cmdq_sec_init_context(&context->tee);
> > +	if (status < 0)
> > +		return status;
> 
> if (a < 0) return a;
> else return a;
> 
> really? :-)
> 
Oh, this is not correct...

> Besides, again, you don't need this wrapper function.
> 
OK, I'll remove this wrapping function and avoid the wrong status
checking. Thanks!

> > +
> > +	return status;
> > +}
> > +
> > +int cmdq_sec_insert_backup_cookie(struct cmdq_pkt *pkt)
> > +{
> > +	struct cmdq_client *cl = (struct cmdq_client *)pkt->cl;
> > +	struct cmdq_sec_thread *thread = ((struct mbox_chan *)(cl-
> > >chan))->con_priv;
> > +	struct cmdq_sec *cmdq = container_of(thread->chan->mbox, struct
> > cmdq_sec, mbox);
> > +	struct cmdq_operand left, right;
> > +	dma_addr_t addr;
> > +
> > +	if (!thread->occupied || !cmdq->shared_mem)
> > +		return -EFAULT;
> > +
> > +	pr_debug("%s %d: pkt:%p thread:%u gce:%#lx",
> 
> All those pr_xxxx shall become dev_debug, dev_err, etc.

OK, I'll change them.

> 
> > +		 __func__, __LINE__, pkt, thread->idx, (unsigned
> > long)cmdq->base_pa);
> > +
> > +	addr = (u32)(cmdq->base_pa + CMDQ_THR_BASE +
> > +		CMDQ_THR_SIZE * thread->idx + CMDQ_THR_EXEC_CNT_PA);
> > +
> > +	cmdq_pkt_assign(pkt, CMDQ_THR_SPR_IDX1, CMDQ_ADDR_HIGH(addr));
> > +	cmdq_pkt_read_s(pkt, CMDQ_THR_SPR_IDX1, CMDQ_ADDR_LOW(addr),
> > CMDQ_THR_SPR_IDX1);
> > +
> > +	left.reg = true;
> > +	left.idx = CMDQ_THR_SPR_IDX1;
> > +	right.reg = false;
> > +	right.value = 1;
> > +	cmdq_pkt_logic_command(pkt, CMDQ_LOGIC_ADD, CMDQ_THR_SPR_IDX1,
> > &left, &right);
> > +
> > +	addr = cmdq->shared_mem->pa + CMDQ_SEC_SHARED_THR_CNT_OFFSET +
> > +		thread->idx * sizeof(u32);
> 
> Fits in one line.

OK. Thanks!

> 
> > +
> > +	cmdq_pkt_assign(pkt, CMDQ_THR_SPR_IDX2, CMDQ_ADDR_HIGH(addr));
> > +	cmdq_pkt_write_s(pkt, CMDQ_THR_SPR_IDX2, CMDQ_ADDR_LOW(addr),
> > CMDQ_THR_SPR_IDX1);
> > +	cmdq_pkt_set_event(pkt, cmdq->cmdq_event);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(cmdq_sec_insert_backup_cookie);
> > +
> > +static int cmdq_sec_realloc_addr_list(struct cmdq_pkt *pkt, const
> > u32 count)
> > +{
> > +	struct cmdq_sec_data *sec_data = (struct cmdq_sec_data *)pkt-
> > >sec_data;
> > +	void *prev = (void *)(unsigned long)sec_data->addrMetadatas,
> > *curr;
> > +
> > +	if (count <= sec_data->addrMetadataMaxCount)
> > +		return 0;
> > +
> > +	curr = kcalloc(count, sizeof(*sec_data), GFP_KERNEL);
> > +	if (!curr)
> > +		return -ENOMEM;
> > +
> > +	if (count && sec_data->addrMetadatas)
> > +		memcpy(curr, prev, sizeof(*sec_data) * sec_data-
> > >addrMetadataMaxCount);
> > +
> > +	kfree(prev);
> > +
> > +	sec_data->addrMetadatas = (uintptr_t)curr;
> > +	sec_data->addrMetadataMaxCount = count;
> > +	return 0;
> > +}
> > +
> > +static int cmdq_sec_check_sec(struct cmdq_pkt *pkt)
> 
> This is not just a check: it is *allocating* memory, so please either
> find a better
> name for this function, or just make sure that the sec_data is
> already allocated
> before calling into functions that are calling this one, as I can
> foresee memory
> leaks because of confusion deriving from this function.

OK, I'll rename it to cmdq_sec_pkt_create_sec_data() and add another
cmdq_sec_pkt_destory_sec_data() to free it.

> 
> > +{
> > +	struct cmdq_sec_data *sec_data;
> > +
> > +	if (pkt->sec_data)
> > +		return 0;
> > +
> > +	sec_data = kzalloc(sizeof(*sec_data), GFP_KERNEL);
> > +	if (!sec_data)
> > +		return -ENOMEM;
> > +
> > +	pkt->sec_data = (void *)sec_data;
> > +
> > +	return 0;
> > +}
> > +
> > +static int cmdq_sec_append_metadata(struct cmdq_pkt *pkt,
> > +				    const enum
> > cmdq_iwc_addr_metadata_type type,
> > +				    const u64 base, const u32 offset,
> > const u32 size,
> > +				    const u32 port)
> > +{
> > +	struct cmdq_sec_data *sec_data;
> > +	struct iwcCmdqAddrMetadata_t *meta;
> 
> No camelcase please.

OK, I'll rename it.

> 
> > +	int idx, max, ret;
> > +
> > +	pr_debug("[%s %d] pkt:%p type:%u base:%#llx offset:%#x size:%#x
> > port:%#x",
> > +		 __func__, __LINE__, pkt, type, base, offset, size,
> > port);
> > +
> > +	ret = cmdq_sec_check_sec(pkt);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	sec_data = (struct cmdq_sec_data *)pkt->sec_data;
> > +	idx = sec_data->addrMetadataCount;
> > +	if (idx >= CMDQ_IWC_MAX_ADDR_LIST_LENGTH) {
> > +		pr_err("idx:%u reach over:%u", idx,
> > CMDQ_IWC_MAX_ADDR_LIST_LENGTH);
> > +		return -EFAULT;
> > +	}
> > +
> > +	if (!sec_data->addrMetadataMaxCount)
> > +		max = ADDR_METADATA_MAX_COUNT_ORIGIN;
> > +	else if (idx >= sec_data->addrMetadataMaxCount)
> > +		max = sec_data->addrMetadataMaxCount * 2;
> > +	else
> > +		max = sec_data->addrMetadataMaxCount;
> > +
> > +	ret = cmdq_sec_realloc_addr_list(pkt, max);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (!sec_data->addrMetadatas) {
> > +		pr_info("addrMetadatas is missing");
> > +
> > +		meta = kzalloc(sizeof(*meta), GFP_KERNEL);
> > +		if (!meta)
> > +			return -ENOMEM;
> > +
> > +		sec_data->addrMetadatas = (uintptr_t)(void *)meta;
> > +	}
> > +	meta = (struct iwcCmdqAddrMetadata_t *)(uintptr_t)sec_data-
> > >addrMetadatas;
> > +
> > +	meta[idx].instrIndex = pkt->cmd_buf_size / CMDQ_INST_SIZE - 1;
> > +	meta[idx].type = type;
> > +	meta[idx].baseHandle = base;
> > +	meta[idx].offset = offset;
> > +	meta[idx].size = size;
> > +	meta[idx].port = port;
> > +	sec_data->addrMetadataCount += 1;
> > +	return 0;
> > +}
> > +
> > +int cmdq_sec_pkt_set_data(struct cmdq_pkt *pkt, const u64
> > dapc_engine,
> > +			  const u64 port_sec_engine, const enum
> > cmdq_sec_scenario scenario)
> > +{
> > +	struct cmdq_sec_data *sec_data;
> > +	int ret;
> > +
> > +	if (!pkt) {
> > +		pr_err("invalid pkt:%p", pkt);
> > +		return -EINVAL;
> > +	}
> > +
> > +	ret = cmdq_sec_check_sec(pkt);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	pr_debug("[%s %d] pkt:%p sec_data:%p dapc:%llu port_sec:%llu
> > scen:%u",
> > +		 __func__, __LINE__, pkt, pkt->sec_data, dapc_engine,
> > port_sec_engine, scenario);
> > +
> > +	sec_data = (struct cmdq_sec_data *)pkt->sec_data;
> > +	sec_data->enginesNeedDAPC |= dapc_engine;
> > +	sec_data->enginesNeedPortSecurity |= port_sec_engine;
> > +	sec_data->scenario = scenario;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(cmdq_sec_pkt_set_data);
> > +
> > +int cmdq_sec_pkt_write(struct cmdq_pkt *pkt, u32 addr, u64 base,
> > +		       const enum cmdq_iwc_addr_metadata_type type,
> > +		       const u32 offset, const u32 size, const u32
> > port)
> > +{
> > +	int ret;
> > +
> > +	ret = cmdq_pkt_assign(pkt, CMDQ_THR_SPR_IDX0, addr);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = cmdq_pkt_write_s_reg_value(pkt, CMDQ_THR_SPR_IDX0,
> > (u32)base);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return cmdq_sec_append_metadata(pkt, type, base, offset, size,
> > port);
> > +}
> > +EXPORT_SYMBOL(cmdq_sec_pkt_write);
> > +
> > +static u32 cmdq_sec_get_cookie(struct cmdq_sec *cmdq, u32 idx)
> > +{
> > +	return *(u32 *)(cmdq->shared_mem->va +
> > +		CMDQ_SEC_SHARED_THR_CNT_OFFSET + idx * sizeof(u32));
> > +}
> > +
> > +static void cmdq_sec_task_done(struct cmdq_sec_task *task, int
> > sta)
> > +{
> > +	struct cmdq_cb_data data;
> > +
> > +	data.sta = sta;
> > +	data.pkt = task->pkt;
> > +
> > +	pr_debug("%s task:%p pkt:%p err:%d",
> > +		 __func__, task, task->pkt, sta);
> > +
> > +	mbox_chan_received_data(task->thread->chan, &data);
> > +
> > +	list_del_init(&task->list_entry);
> > +	kfree(task);
> > +}
> > +
> > +static bool cmdq_sec_irq_handler(struct cmdq_sec_thread *thread,
> > const u32 cookie, const int err)
> > +{
> > +	struct cmdq_sec_task *task, *temp, *cur_task = NULL;
> > +	struct cmdq_sec *cmdq = container_of(thread->chan->mbox, struct
> > cmdq_sec, mbox);
> > +	unsigned long flags;
> > +	int done;
> > +
> > +	spin_lock_irqsave(&thread->chan->lock, flags);
> > +	if (thread->wait_cookie <= cookie)
> > +		done = cookie - thread->wait_cookie + 1;
> > +	else if (thread->wait_cookie == (cookie + 1) %
> > CMDQ_MAX_COOKIE_VALUE)
> > +		done = 0;
> > +	else
> > +		done = CMDQ_MAX_COOKIE_VALUE - thread->wait_cookie + 1
> > +
> > +			cookie + 1;
> > +
> > +	list_for_each_entry_safe(task, temp, &thread->task_list,
> > list_entry) {
> > +		if (!done)
> > +			break;
> > +
> > +		cmdq_sec_task_done(task, err);
> > +
> > +		if (thread->task_cnt)
> > +			thread->task_cnt -= 1;
> > +
> > +		done--;
> > +	}
> > +
> > +	cur_task = list_first_entry_or_null(&thread->task_list,
> > +					    struct cmdq_sec_task,
> > list_entry);
> > +
> > +	if (err && cur_task) {
> > +		spin_unlock_irqrestore(&thread->chan->lock, flags);
> > +
> > +		/* for error task, cancel, callback and done */
> > +		memset(&cmdq->cancel, 0, sizeof(cmdq->cancel));
> > +		cmdq_sec_task_submit(cmdq, cur_task,
> > CMD_CMDQ_IWC_CANCEL_TASK,
> > +				     thread->idx, &cmdq->cancel);
> > +
> > +		cmdq_sec_task_done(task, err);
> > +
> > +		spin_lock_irqsave(&thread->chan->lock, flags);
> > +
> > +		task = list_first_entry_or_null(&thread->task_list,
> > +						struct cmdq_sec_task,
> > list_entry);
> > +		if (cur_task == task)
> > +			cmdq_sec_task_done(cur_task, err);
> > +		else
> > +			pr_err("task list changed");
> > +
> > +		/*
> > +		 * error case stop all task for secure,
> > +		 * since secure tdrv always remove all when cancel
> > +		 */
> > +		while (!list_empty(&thread->task_list)) {
> > +			cur_task = list_first_entry(&thread->task_list,
> > +						    struct
> > cmdq_sec_task, list_entry);
> > +
> > +			cmdq_sec_task_done(cur_task, -ECONNABORTED);
> > +		}
> > +	} else if (err) {
> > +		pr_debug("error but all task done, check notify
> > callback");
> > +	}
> > +
> > +	if (list_empty(&thread->task_list)) {
> > +		thread->wait_cookie = 0;
> > +		thread->next_cookie = 0;
> > +		thread->task_cnt = 0;
> > +		__raw_writel(0, (void __iomem *)cmdq->shared_mem->va +
> > +			     CMDQ_SEC_SHARED_THR_CNT_OFFSET +
> > +			     thread->idx * sizeof(u32));
> > +		spin_unlock_irqrestore(&thread->chan->lock, flags);
> > +		del_timer(&thread->timeout);
> > +		return true;
> > +	}
> > +
> > +	thread->wait_cookie = cookie % CMDQ_MAX_COOKIE_VALUE + 1;
> > +
> > +	mod_timer(&thread->timeout, jiffies + msecs_to_jiffies(thread-
> > >timeout_ms));
> > +	spin_unlock_irqrestore(&thread->chan->lock, flags);
> > +
> > +	return false;
> > +}
> > +
> > +static void cmdq_sec_irq_notify_work(struct work_struct
> > *work_item)
> > +{
> > +	struct cmdq_sec *cmdq = container_of(work_item, struct
> > cmdq_sec, irq_notify_work);
> > +	int i;
> > +
> > +	mutex_lock(&cmdq->exec_lock);
> > +
> > +	for (i = 0; i < CMDQ_MAX_SECURE_THREAD_COUNT; i++) {
> > +		struct cmdq_sec_thread *thread = &cmdq-
> > >thread[CMDQ_MIN_SECURE_THREAD_ID + i];
> > +		u32 cookie = cmdq_sec_get_cookie(cmdq, thread->idx);
> > +
> > +		if (cookie < thread->wait_cookie || !thread->task_cnt)
> > +			continue;
> > +
> > +		cmdq_sec_irq_handler(thread, cookie, 0);
> > +	}
> > +
> > +	mutex_unlock(&cmdq->exec_lock);
> > +}
> > +
> > +static void cmdq_sec_irq_notify_callback(struct mbox_client *cl,
> > void *mssg)
> > +{
> > +	struct cmdq_cb_data *data = (struct cmdq_cb_data *)mssg;
> > +	struct cmdq_sec *cmdq = container_of(((struct cmdq_client
> > *)data->pkt->cl)->chan->mbox,
> > +					     struct cmdq_sec, mbox);
> > +
> 
> 	if (work_pending...) {
> 		dev_dbg
> 		return;
> 	}
> 
> 	queue_work
> 

It's much better. I'll change this. Thanks!

> > +	if (!work_pending(&cmdq->irq_notify_work))
> > +		queue_work(cmdq->notify_wq, &cmdq->irq_notify_work);
> > +	else
> > +		pr_debug("%s last notify callback working", __func__);
> > +}
> > +
> > +static int cmdq_sec_irq_notify_start(struct cmdq_sec *cmdq)
> > +{
> > +	int err;
> > +
> > +	if (cmdq->notify_run)
> > +		return 0;
> > +
> > +	if (!cmdq->clt_pkt) {
> > +		cmdq->clt = cmdq_mbox_create(cmdq->mbox.dev, 0);
> > +		if (!cmdq->clt || IS_ERR(cmdq->clt)) {
> > +			pr_err("clt mbox_create failed clt:%p
> > index:%d",
> > +			       cmdq->clt, CMDQ_SEC_IRQ_THREAD);
> > +			return -EINVAL;
> > +		}
> > +		cmdq->clt->client.rx_callback =
> > cmdq_sec_irq_notify_callback;
> > +
> > +		cmdq->clt_pkt = cmdq_pkt_create(cmdq->clt,
> > CMDQ_BUF_ALLOC_SIZE);
> > +		if (!cmdq->clt_pkt || IS_ERR(cmdq->clt_pkt)) {
> > +			pr_err("clt_pkt cmdq_pkt_create failed pkt:%p
> > index:%d",
> > +			       cmdq->clt_pkt, CMDQ_SEC_IRQ_THREAD);
> > +			return -EINVAL;
> > +		}
> > +
> > +		INIT_WORK(&cmdq->irq_notify_work,
> > cmdq_sec_irq_notify_work);
> > +	}
> > +
> > +	cmdq_pkt_wfe(cmdq->clt_pkt, cmdq->cmdq_event, true);
> > +	cmdq_pkt_finalize_loop(cmdq->clt_pkt);
> > +	dma_sync_single_for_device(cmdq->mbox.dev,
> > +				   cmdq->clt_pkt->pa_base,
> > +				   cmdq->clt_pkt->cmd_buf_size,
> > +				   DMA_TO_DEVICE);
> > +	err = mbox_send_message(cmdq->clt->chan, cmdq->clt_pkt);
> > +	mbox_client_txdone(cmdq->clt->chan, 0);
> > +	if (err < 0) {
> > +		pr_err("%s failed:%d", __func__, err);
> > +		cmdq_pkt_destroy(cmdq->clt_pkt);
> > +		cmdq_mbox_destroy(cmdq->clt);
> 
> 		return err;
> 	}
> 	cmdq->notify_run = true;
> 
> > +	} else {
> > +		cmdq->notify_run = true;
> > +		pr_debug("%s success!", __func__);
> > +	}
> > +
> > +	return err;
> 
> return 0 here
> 

OK, I'll change this. Thanks!

> > +}
> > +
> > +static int cmdq_sec_session_init(struct cmdq_sec_context *context)
> > +{
> > +	int err = 0;
> > +
> > +	if (context->state >= IWC_SES_OPENED) {
> > +		pr_debug("session opened:%u", context->state);
> > +		return err;
> 
> 		return 0;
> 
> > +	}
> > +
> > +	switch (context->state) {
> > +	case IWC_INIT:
> > +		err = cmdq_sec_init_context_base(context);
> > +		if (err)
> 
> 		if (err)
> 			return err;
> 
> > +			break;
> > +		context->state = IWC_CONTEXT_INITED;
> > +	fallthrough;
> > +	case IWC_CONTEXT_INITED:
> > +		if (context->iwc_msg) {
> > +			pr_err("iwcMessage not NULL:%p", context-
> > >iwc_msg);
> > +			err = -EINVAL;
> 
> return -EINVAL;
> 
> > +			break;
> > +		}
> > +
> > +		err = cmdq_sec_allocate_wsm(&context->tee, &context-
> > >iwc_msg,
> > +					    sizeof(struct
> > iwcCmdqMessage_t));
> > +		if (err)
> > +			break;
> 
> return err
> 
> > +
> > +		context->state = IWC_WSM_ALLOCATED;
> > +	fallthrough;
> > +	case IWC_WSM_ALLOCATED:
> > +		err = cmdq_sec_open_session(&context->tee, context-
> > >iwc_msg);
> > +		if (err)
> > +			break;
> 
> return err
> 
> > +
> > +		context->state = IWC_SES_OPENED;
> > +	fallthrough;
> > +	default:
> > +		break;
> > +	}
> 
> 	return 0;
> 

OK' I'll change these return err logic. Thanks!

> > +	return err;
> > +}
> > +
> > +static int cmdq_sec_fill_iwc_msg(struct cmdq_sec_context *context,
> > +				 struct cmdq_sec_task *task, u32
> > thrd_idx)
> > +{
> > +	struct iwcCmdqMessage_t *iwc_msg = NULL;
> > +	struct cmdq_sec_data *data = (struct cmdq_sec_data *)task->pkt-
> > >sec_data;
> > +	u32 size = 0, offset = 0, *instr;
> > +
> > +	iwc_msg = (struct iwcCmdqMessage_t *)context->iwc_msg;
> > +
> > +	if (task->pkt->cmd_buf_size + 4 * CMDQ_INST_SIZE >
> > CMDQ_TZ_CMD_BLOCK_SIZE) {
> > +		pr_err("task:%p size:%zu > %u",
> > +		       task, task->pkt->cmd_buf_size,
> > CMDQ_TZ_CMD_BLOCK_SIZE);
> > +		return -EFAULT;
> > +	}
> > +
> > +	if (thrd_idx == CMDQ_INVALID_THREAD) {
> > +		iwc_msg->command.commandSize = 0;
> > +		iwc_msg->command.metadata.addrListLength = 0;
> > +		return 0;
> 
> if invalid thread, return 0?! Are you sure?!

Oh, There is a cmd will use the invalid thread to send a message.

But I shouldn't just return 0 for all cmd with invalid thread.
I'll refine this. Thanks!

> 
> > +	}
> > +
> 
> 	if (iwc_msg->command.size == 0 && task->pkt->cmd_buf_size == 0)
> {
> 		dev_dbg(...... command size is zero)
> 
> 		return (something);
> 	}
> 
> > +	iwc_msg->command.thread = thrd_idx;
> > +	iwc_msg->command.scenario = task->scenario;
> > +	iwc_msg->command.engineFlag = task->engineFlag;
> > +	size = task->pkt->cmd_buf_size;
> > +	memcpy(iwc_msg->command.pVABase + offset, task->pkt->va_base,
> > size);
> > +	iwc_msg->command.commandSize += size;
> > +	offset += size / 4;
> > +
> > +	if (iwc_msg->command.commandSize == 0) {
> > +		pr_err("%s %d: commandSize is 0\n", __func__,
> > __LINE__);
> 
> So this being zero is valid? Again, are you sure?
> 
Oh, this may be the test code which I forgot to remove.
I'll remove this check. Thanks!

> > +		return 0;
> > +	}
> > +
> > +	instr = &iwc_msg->command.pVABase[iwc_msg->command.commandSize
> > / 4 - 4];
> > +	if (instr[0] == 0x1 && instr[1] == 0x40000000)
> 
> Please define this, I guess that instr[] is a 64-bits instruction, is
> it?
> 
Each GCE instruction is 64-bits, but we usually verify them by each 32-
bits. In this case, I think checking 64-bits is better.

> So this could be... #define SOMETHING 0x140000000ULL
> ...but however you choose to define it is fine, just .. define it
> please :-)
> 

OK, I'll add an EOC(end of command) define for that.

> > +		instr[0] = 0;
> > +	else
> > +		pr_err("%s %d: find EOC failed: %#x %#x",
> > +		       __func__, __LINE__, instr[1], instr[0]);
> > +
> > +	iwc_msg->command.waitCookie = task->waitCookie;
> > +	iwc_msg->command.resetExecCnt = task->resetExecCnt;
> > +
> > +	if (data->addrMetadataCount) {
> > +		iwc_msg->command.metadata.addrListLength = data-
> > >addrMetadataCount;
> > +		memcpy(iwc_msg->command.metadata.addrList,
> > +		       (u32 *)(unsigned long)data->addrMetadatas,
> > +		       data->addrMetadataCount * sizeof(struct
> > iwcCmdqAddrMetadata_t));
> > +	}
> > +
> > +	iwc_msg->command.metadata.enginesNeedDAPC = data-
> > >enginesNeedDAPC;
> > +	iwc_msg->command.metadata.enginesNeedPortSecurity =
> > +		data->enginesNeedPortSecurity;
> > +	iwc_msg->command.hNormalTask = (unsigned long)task->pkt;
> > +
> > +	return 0;
> > +}
> > +
> > +static int cmdq_sec_session_send(struct cmdq_sec_context *context,
> > +				 struct cmdq_sec_task *task, const u32
> > iwc_cmd,
> > +				 const u32 thrd_idx, struct cmdq_sec
> > *cmdq)
> > +{
> > +	int err = 0;
> > +	u64 cost;
> > +	struct iwcCmdqMessage_t *iwc_msg = NULL;
> > +
> > +	iwc_msg = (struct iwcCmdqMessage_t *)context->iwc_msg;
> > +
> > +	memset(iwc_msg, 0, sizeof(*iwc_msg));
> > +	iwc_msg->cmd = iwc_cmd;
> > +	iwc_msg->cmdq_id = cmdq->hwid;
> > +	iwc_msg->command.thread = thrd_idx;
> > +
> > +	switch (iwc_cmd) {
> > +	case CMD_CMDQ_IWC_SUBMIT_TASK:
> > +		err = cmdq_sec_fill_iwc_msg(context, task, thrd_idx);
> > +		if (err)
> > +			return err;
> > +		break;
> > +	case CMD_CMDQ_IWC_CANCEL_TASK:
> > +		iwc_msg->cancelTask.waitCookie = task->waitCookie;
> > +		iwc_msg->cancelTask.thread = thrd_idx;
> > +		break;
> > +	case CMD_CMDQ_IWC_PATH_RES_ALLOCATE:
> > +		if (!cmdq->shared_mem || !cmdq->shared_mem->va) {
> > +			pr_err("%s %d: shared_mem is NULL", __func__,
> > __LINE__);
> > +			return -EFAULT;
> > +		}
> > +		iwc_msg->pathResource.size = cmdq->shared_mem->size;
> > +		iwc_msg->pathResource.shareMemoyPA = cmdq->shared_mem-
> > >pa;
> > +		iwc_msg->pathResource.useNormalIRQ = 1;
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +
> > +	cmdq->sec_invoke = sched_clock();
> > +	pr_debug("%s execute cmdq:%p task:%lx command:%u thread:%u
> > cookie:%d",
> > +		 __func__, cmdq, (unsigned long)task, iwc_cmd,
> > thrd_idx,
> > +		 task ? task->waitCookie : -1);
> > +
> > +	/* send message */
> > +	err = cmdq_sec_execute_session(&context->tee, iwc_cmd,
> > CMDQ_TIMEOUT_DEFAULT);
> > +
> > +	cmdq->sec_done = sched_clock();
> > +	cost = div_u64(cmdq->sec_done - cmdq->sec_invoke, 1000000);
> > +	if (cost >= CMDQ_TIMEOUT_DEFAULT)
> > +		pr_err("%s execute timeout cmdq:%p task:%lx
> > cost:%lluus",
> > +		       __func__, cmdq, (unsigned long)task, cost);
> > +	else
> > +		pr_debug("%s execute done cmdq:%p task:%lx
> > cost:%lluus",
> > +			 __func__, cmdq, (unsigned long)task, cost);
> > +
> 
> 	if (err)
> 		return err;
> 
> 	context->state = IWC_SES_ON_TRANSACTED;
> 	return 0;

OK, I'll change this. Thanks!

> 
> > +	if (err == 0)
> > +		context->state = IWC_SES_ON_TRANSACTED;
> > +
> > +	return err;
> > +}
> > +
> > +static int cmdq_sec_session_reply(const u32 iwc_cmd, struct
> > iwcCmdqMessage_t *iwc_msg,
> > +				  void *data, struct cmdq_sec_task
> > *task)
> > +{
> > +	struct iwcCmdqCancelTask_t *cancel = data;
> > +	struct cmdq_sec_data *sec_data = task->pkt->sec_data;
> > +
> > +	if (iwc_cmd == CMD_CMDQ_IWC_SUBMIT_TASK) {
> 
> 	if (iwc_cmd == CMD_CMDQ_IWC_SUBMIT_TASK && iwc_msg->rsp < 0) {
> 		....
> 	} else if (...) {
> 		....
> 	}
> 
> 	return ...
> 

OK, I'll change this. Thanks!

> > +		if (iwc_msg->rsp < 0) {
> > +			/* submit fail case copy status */
> > +			memcpy(&sec_data->sec_status, &iwc_msg-
> > >secStatus,
> > +			       sizeof(sec_data->sec_status));
> > +			sec_data->response = iwc_msg->rsp;
> > +		}
> > +	} else if (iwc_cmd == CMD_CMDQ_IWC_CANCEL_TASK && cancel) {
> > +		/* cancel case only copy cancel result */
> > +		memcpy(cancel, &iwc_msg->cancelTask, sizeof(*cancel));
> > +	}
> > +
> > +	return iwc_msg->rsp;
> > +}
> > +
> > +static int cmdq_sec_task_submit(struct cmdq_sec *cmdq, struct
> > cmdq_sec_task *task,
> > +				const u32 iwc_cmd, const u32 thrd_idx,
> > void *data)
> > +{
> > +	struct cmdq_sec_context *context;
> > +	int err;
> > +
> > +	do {
> > +		if (!cmdq->context) {
> > +			context = kzalloc(sizeof(*cmdq->context),
> > GFP_ATOMIC);
> > +			if (!context) {
> > +				err = -ENOMEM;
> > +				break;
> > +			}
> > +			cmdq->context = context;
> > +			cmdq->context->state = IWC_INIT;
> > +			cmdq->context->tgid = current->tgid;
> > +		}
> > +
> > +		if (cmdq->context->state == IWC_INIT)
> > +			cmdq_sec_setup_tee_context_base(cmdq->context);
> > +
> > +		err = cmdq_sec_session_init(cmdq->context);
> > +		if (err) {
> > +			pr_err("%s %d: cmdq_sec_session_init fail",
> > __func__, __LINE__);
> > +			break;
> > +		}
> > +
> > +		err = cmdq_sec_irq_notify_start(cmdq);
> > +		if (err < 0) {
> > +			pr_err("%s %d: cmdq_sec_irq_notify_start fail",
> > __func__, __LINE__);
> > +			break;
> > +		}
> > +
> > +		err = cmdq_sec_session_send(cmdq->context, task,
> > iwc_cmd, thrd_idx, cmdq);
> > +		if (err)
> > +			pr_err("%s %d: cmdq_sec_session_send fail:
> > %d\n", __func__, __LINE__, err);
> > +		else
> > +			err = cmdq_sec_session_reply(iwc_cmd, cmdq-
> > >context->iwc_msg, data, task);
> > +	} while (0);
> 
> Seriously? do { ... } while(0) ?!?!?!
> 
> No!!!!
> 

OK, I'll change this like the above checking logic. Thanks!

> > +
> > +	if (err)
> > +		pr_err("%s %d: sec invoke err:%d task:%p thread:%u
> > gce:%#lx",
> > +		       __func__, __LINE__, err, task, thrd_idx,
> > (unsigned long)cmdq->base_pa);
> > +
> > +	return err;
> > +}
> > +
> > +static int cmdq_sec_suspend(struct device *dev)
> > +{
> > +	struct cmdq_sec *cmdq = dev_get_drvdata(dev);
> > +
> > +	clk_bulk_disable_unprepare(cmdq->gce_num, cmdq->clocks);
> > +	return 0;
> > +}
> > +
> > +static int cmdq_sec_resume(struct device *dev)
> > +{
> > +	struct cmdq_sec *cmdq = dev_get_drvdata(dev);
> > +
> > +	WARN_ON(clk_bulk_prepare_enable(cmdq->gce_num, cmdq->clocks));
> > +	return 0;
> > +}
> > +
> > +static const struct dev_pm_ops cmdq_sec_pm_ops = {
> > +	.suspend = cmdq_sec_suspend,
> > +	.resume = cmdq_sec_resume,
> > +};
> > +
> > +static void cmdq_sec_task_exec_work(struct work_struct *work_item)
> > +{
> > +	struct cmdq_sec_task *task = container_of(work_item, struct
> > cmdq_sec_task, exec_work);
> > +	struct cmdq_sec *cmdq = container_of(task->thread->chan->mbox,
> > struct cmdq_sec, mbox);
> > +	struct cmdq_sec_data *data;
> > +	unsigned long flags;
> > +	int err;
> > +
> > +	pr_debug("%s gce:%#lx task:%p pkt:%p thread:%u",
> > +		 __func__, (unsigned long)cmdq->base_pa, task, task-
> > >pkt, task->thread->idx);
> > +
> > +	if (!task->pkt->sec_data) {
> > +		pr_err("pkt:%p without sec_data", task->pkt);
> > +		return;
> > +	}
> > +	data = (struct cmdq_sec_data *)task->pkt->sec_data;
> > +
> > +	mutex_lock(&cmdq->exec_lock);
> > +
> > +	spin_lock_irqsave(&task->thread->chan->lock, flags);
> > +	if (!task->thread->task_cnt) {
> > +		mod_timer(&task->thread->timeout, jiffies +
> > +			msecs_to_jiffies(task->thread->timeout_ms));
> > +		task->thread->wait_cookie = 1;
> > +		task->thread->next_cookie = 1;
> > +		task->thread->task_cnt = 0;
> > +		__raw_writel(0, (void __iomem *)cmdq->shared_mem->va +
> > +			     CMDQ_SEC_SHARED_THR_CNT_OFFSET +
> > +			     task->thread->idx * sizeof(u32));
> > +	}
> > +
> > +	task->resetExecCnt = task->thread->task_cnt ? false : true;
> > +	task->waitCookie = task->thread->next_cookie;
> > +	task->thread->next_cookie = (task->thread->next_cookie + 1) %
> > CMDQ_MAX_COOKIE_VALUE;
> > +	list_add_tail(&task->list_entry, &task->thread->task_list);
> > +	task->thread->task_cnt += 1;
> > +	spin_unlock_irqrestore(&task->thread->chan->lock, flags);
> > +	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_err_callback;
> > +		}
> > +	}
> > +
> > +	if (task->thread->task_cnt > CMDQ_MAX_TASK_IN_SECURE_THREAD) {
> > +		pr_err("task_cnt:%u cannot more than %u task:%p thrd-
> > idx:%u",
> > +		       task->thread->task_cnt,
> > CMDQ_MAX_TASK_IN_SECURE_THREAD,
> > +		       task, task->thread->idx);
> > +		err = -EMSGSIZE;
> > +		goto task_err_callback;
> > +	}
> > +
> > +	err = cmdq_sec_task_submit(cmdq, task,
> > CMD_CMDQ_IWC_SUBMIT_TASK, task->thread->idx, NULL);
> > +	if (err)
> > +		pr_err("task submit CMD_CMDQ_IWC_SUBMIT_TASK failed:%d
> > gce:%#lx task:%p thread:%u",
> > +		       err, (unsigned long)cmdq->base_pa, task, task-
> > >thread->idx);
> > +
> > +task_err_callback:
> 
> end:

OK, I'll change it.

> 
> > +	if (err) {
> > +		struct cmdq_cb_data cb_data;
> > +
> > +		cb_data.sta = err;
> > +		cb_data.pkt = task->pkt;
> > +		mbox_chan_received_data(task->thread->chan, &cb_data);
> > +
> > +		spin_lock_irqsave(&task->thread->chan->lock, flags);
> > +		if (!task->thread->task_cnt)
> > +			pr_err("thread:%u task_cnt:%u cannot below
> > zero",
> > +			       task->thread->idx, task->thread-
> > >task_cnt);
> > +		else
> > +			task->thread->task_cnt -= 1;
> > +
> > +		task->thread->next_cookie = (task->thread->next_cookie
> > - 1 +
> > +			CMDQ_MAX_COOKIE_VALUE) % CMDQ_MAX_COOKIE_VALUE;
> > +		list_del(&task->list_entry);
> > +		pr_debug("gce:%#lx err:%d task:%p pkt:%p thread:%u
> > task_cnt:%u wait_cookie:%u next_cookie:%u",
> > +			 (unsigned long)cmdq->base_pa, err, task, task-
> > >pkt,
> > +			 task->thread->idx, task->thread->task_cnt,
> > +			 task->thread->wait_cookie, task->thread-
> > >next_cookie);
> > +		spin_unlock_irqrestore(&task->thread->chan->lock,
> > flags);
> > +		kfree(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_sec_thread *thread = (struct cmdq_sec_thread
> > *)chan->con_priv;
> > +	struct cmdq_sec_task *task;
> > +
> > +	if (!sec_data)
> > +		return -EINVAL;
> > +
> > +	task = kzalloc(sizeof(*task), GFP_ATOMIC);
> > +	if (!task)
> > +		return -ENOMEM;
> > +
> > +	task->pkt = pkt;
> > +	task->thread = thread;
> > +	task->scenario = sec_data->scenario;
> > +	task->engineFlag = sec_data->enginesNeedDAPC | sec_data-
> > >enginesNeedPortSecurity;
> > +
> > +	INIT_WORK(&task->exec_work, cmdq_sec_task_exec_work);
> > +	queue_work(thread->task_exec_wq, &task->exec_work);
> > +	return 0;
> > +}
> > +
> > +static void cmdq_sec_thread_timeout(struct timer_list *t)
> > +{
> > +	struct cmdq_sec_thread *thread = from_timer(thread, t,
> > timeout);
> > +	struct cmdq_sec *cmdq = container_of(thread->chan->mbox, struct
> > cmdq_sec, mbox);
> > +
> > +	if (!work_pending(&thread->timeout_work))
> > +		queue_work(cmdq->timeout_wq, &thread->timeout_work);
> > +}
> > +
> > +static void cmdq_sec_task_timeout_work(struct work_struct
> > *work_item)
> > +{
> > +	struct cmdq_sec_thread *thread = container_of(work_item, struct
> > cmdq_sec_thread,
> > +						      timeout_work);
> > +	struct cmdq_sec *cmdq = container_of(thread->chan->mbox, struct
> > cmdq_sec, mbox);
> > +	struct cmdq_sec_task *task;
> > +	unsigned long flags;
> > +	u64 duration;
> > +	u32 cookie;
> > +
> > +	mutex_lock(&cmdq->exec_lock);
> > +
> > +	spin_lock_irqsave(&thread->chan->lock, flags);
> > +	if (list_empty(&thread->task_list)) {
> > +		pr_err("thread:%u task_list is empty", thread->idx);
> > +		spin_unlock_irqrestore(&thread->chan->lock, flags);
> > +		goto done;
> > +	}
> > +
> > +	task = list_first_entry(&thread->task_list, struct
> > cmdq_sec_task, list_entry);
> > +	duration = div_u64(sched_clock() - task->trigger, 1000000);
> > +	if (duration < thread->timeout_ms) {
> > +		mod_timer(&thread->timeout, jiffies +
> > +			  msecs_to_jiffies(thread->timeout_ms -
> > duration));
> > +		spin_unlock_irqrestore(&thread->chan->lock, flags);
> > +		goto done;
> > +	}
> > +
> > +	cookie = cmdq_sec_get_cookie(cmdq, thread->idx);
> > +	spin_unlock_irqrestore(&thread->chan->lock, flags);
> > +
> > +	pr_err("%s duration:%llu cookie:%u thread:%u",
> > +	       __func__, duration, cookie, thread->idx);
> > +	cmdq_sec_irq_handler(thread, cookie, -ETIMEDOUT);
> > +
> > +done:
> > +	mutex_unlock(&cmdq->exec_lock);
> > +}
> > +
> > +static int cmdq_sec_mbox_startup(struct mbox_chan *chan)
> > +{
> > +	struct cmdq_sec_thread *thread = (struct cmdq_sec_thread
> > *)chan->con_priv;
> > +	char name[20];
> > +
> > +	timer_setup(&thread->timeout, cmdq_sec_thread_timeout, 0);
> > +
> > +	INIT_WORK(&thread->timeout_work, cmdq_sec_task_timeout_work);
> > +	snprintf(name, sizeof(name), "task_exec_wq_%u", thread->idx);
> > +	thread->task_exec_wq = create_singlethread_workqueue(name);
> > +	thread->occupied = true;
> > +	return 0;
> > +}
> > +
> > +static void cmdq_sec_mbox_shutdown(struct mbox_chan *chan)
> > +{
> > +	struct cmdq_sec_thread *thread = (struct cmdq_sec_thread
> > *)chan->con_priv;
> > +
> > +	thread->occupied = false;
> > +}
> > +
> > +static int cmdq_sec_mbox_flush(struct mbox_chan *chan, unsigned
> > long timeout)
> > +{
> > +	struct cmdq_sec_thread *thread = (struct cmdq_sec_thread
> > *)chan->con_priv;
> > +	struct cmdq_sec *cmdq = container_of(thread->chan->mbox, struct
> > cmdq_sec, mbox);
> > +	int i;
> > +
> > +	mutex_lock(&cmdq->exec_lock);
> > +
> > +	if (list_empty(&thread->task_list))
> 
> if (list_empty..) {
> 	mutex_unlock(...)
> 	return 0;
> }
> 

OK, I'll change this. Thanks!

> > +		goto out;
> > +
> > +	for (i = 0; i < CMDQ_MAX_SECURE_THREAD_COUNT; i++) {
> > +		struct cmdq_sec_thread *thread = &cmdq-
> > >thread[CMDQ_MIN_SECURE_THREAD_ID + i];
> > +		u32 cookie = cmdq_sec_get_cookie(cmdq, thread->idx);
> > +
> > +		if (cookie < thread->wait_cookie || !thread->task_cnt)
> > +			continue;
> > +
> > +		cmdq_sec_irq_handler(thread, cookie, -ECONNABORTED);
> > +	}
> > +
> > +out:
> > +	mutex_unlock(&cmdq->exec_lock);
> > +	return 0;
> > +}
> > +
> > +static bool cmdq_sec_mbox_last_tx_done(struct mbox_chan *chan)
> > +{
> > +	return true;
> 
> Noupe. This is a hack.
> 
> You want to do some check here to return if the last TX is done, you
> can't
> just return true and call it a day.
> 

Since we use TXDONE_BY_ACK, we can remove this function implement.
So I'll remove this in the next version. 

> > +}
> > +
> > +static const struct mbox_chan_ops cmdq_sec_mbox_chan_ops = {
> > +	.send_data = cmdq_sec_mbox_send_data,
> > +	.startup = cmdq_sec_mbox_startup,
> > +	.shutdown = cmdq_sec_mbox_shutdown,
> > +	.flush = cmdq_sec_mbox_flush,
> > +	.last_tx_done = cmdq_sec_mbox_last_tx_done,
> > +};
> > +
> > +static struct mbox_chan *cmdq_sec_mbox_of_xlate(struct
> > mbox_controller *mbox,
> > +						const struct
> > of_phandle_args *sp)
> > +{
> > +	struct cmdq_sec_thread *thread;
> > +	int idx = sp->args[0];
> > +
> > +	if (mbox->num_chans <= idx) {
> > +		pr_err("invalid thrd-idx:%u", idx);
> > +		return ERR_PTR(-EINVAL);
> > +	}
> > +
> > +	thread = (struct cmdq_sec_thread *)mbox->chans[idx].con_priv;
> > +	thread->chan = &mbox->chans[idx];
> > +	thread->timeout_ms = CMDQ_NO_TIMEOUT;
> > +	thread->priority = sp->args[1];
> > +
> > +	return &mbox->chans[idx];
> > +}
> > +
> > +static int cmdq_sec_probe(struct platform_device *pdev)
> > +{
> > +	int i, err;
> > +	struct cmdq_sec *cmdq;
> > +	struct cmdq_sec_plat *plat_data;
> > +	struct device *dev = &pdev->dev;
> > +
> > +	plat_data = (struct cmdq_sec_plat *)pdev->dev.platform_data;
> > +	if (!plat_data) {
> > +		dev_err(dev, "no valid platform data!\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	cmdq = devm_kzalloc(dev, sizeof(*cmdq), GFP_KERNEL);
> > +	if (!cmdq)
> > +		return -ENOMEM;
> > +
> > +	cmdq->base_pa = plat_data->base_pa;
> > +	cmdq->base = plat_data->base;
> > +	cmdq->thread_nr = plat_data->thread_nr;
> > +	cmdq->gce_num = plat_data->gce_num;
> > +	cmdq->clocks = devm_kcalloc(dev, cmdq->gce_num,
> > +				    sizeof(struct clk_bulk_data),
> > GFP_KERNEL);
> > +	if (!cmdq->clocks)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0 ; i < cmdq->gce_num; i++)
> > +		cmdq->clocks[i] = plat_data->clocks[i];
> > +
> > +	cmdq->hwid = plat_data->hwid;
> > +	cmdq->cmdq_event = plat_data->cmdq_event;
> > +
> > +	cmdq->mbox.dev = plat_data->gce_dev;
> > +	cmdq->mbox.chans = devm_kcalloc(dev, cmdq->thread_nr,
> > +					sizeof(*cmdq->mbox.chans),
> > GFP_KERNEL);
> > +	if (!cmdq->mbox.chans)
> > +		return -ENOMEM;
> > +
> > +	cmdq->mbox.ops = &cmdq_sec_mbox_chan_ops;
> > +	cmdq->mbox.num_chans = cmdq->thread_nr;
> > +	cmdq->mbox.of_xlate = cmdq_sec_mbox_of_xlate;
> > +
> > +	/* make use of TXDONE_BY_ACK */
> > +	cmdq->mbox.txdone_irq = false;
> > +	cmdq->mbox.txdone_poll = false;
> > +
> > +	cmdq->thread = devm_kcalloc(dev, cmdq->thread_nr,
> > +				    sizeof(*cmdq->thread), GFP_KERNEL);
> > +	if (!cmdq->thread)
> > +		return -ENOMEM;
> > +
> > +	mutex_init(&cmdq->exec_lock);
> > +	for (i = 0; i < cmdq->thread_nr; i++) {
> > +		cmdq->thread[i].base = cmdq->base + CMDQ_THR_BASE +
> > CMDQ_THR_SIZE * i;
> > +		INIT_LIST_HEAD(&cmdq->thread[i].task_list);
> > +		cmdq->thread[i].idx = i;
> > +		cmdq->thread[i].occupied = false;
> > +		cmdq->mbox.chans[i].con_priv = (void *)&cmdq-
> > >thread[i];
> > +	}
> > +
> > +	cmdq->notify_wq =
> > create_singlethread_workqueue("mtk_cmdq_sec_notify_wq");
> > +	cmdq->timeout_wq =
> > create_singlethread_workqueue("mtk_cmdq_sec_timeout_wq");
> > +	err = devm_mbox_controller_register(dev, &cmdq->mbox);
> > +	if (err)
> > +		return err;
> > +
> > +	cmdq->shared_mem = devm_kzalloc(dev, sizeof(*cmdq->shared_mem), 
> > GFP_KERNEL);
> > +	if (!cmdq->shared_mem)
> > +		return -ENOMEM;
> > +
> > +	cmdq->shared_mem->va = dma_alloc_coherent(dev, PAGE_SIZE,
> > +						  &cmdq->shared_mem-
> > >pa, GFP_KERNEL);
> > +	cmdq->shared_mem->size = PAGE_SIZE;
> > +
> > +	platform_set_drvdata(pdev, cmdq);
> > +	WARN_ON(clk_bulk_prepare_enable(cmdq->gce_num, cmdq->clocks));
> > +
> > +	return 0;
> > +}
> > +
> > +static int cmdq_sec_remove(struct platform_device *pdev)
> > +{
> > +	struct cmdq_sec *cmdq = platform_get_drvdata(pdev);
> > +
> > +	if (cmdq->context)
> > +		cmdq_sec_free_wsm(&cmdq->context->tee, &cmdq->context-
> > >iwc_msg);
> > +
> > +	mbox_controller_unregister(&cmdq->mbox);
> > +
> > +	clk_bulk_disable_unprepare(cmdq->gce_num, cmdq->clocks);
> > +	return 0;
> > +}
> > +
> > +static struct platform_driver cmdq_sec_drv = {
> > +	.probe = cmdq_sec_probe,
> > +	.remove = cmdq_sec_remove,
> > +	.driver = {
> > +		.name = "mtk_cmdq_sec",
> 
> "mtk-cmdq-secure"
> 
OK, I'll change it.

> > +		.pm = &cmdq_sec_pm_ops,
> > +	},
> > +};
> > +
> > +static int __init cmdq_sec_init(void)
> > +{
> > +	int err;
> > +
> > +	err = platform_driver_register(&cmdq_sec_drv);
> > +	if (err)
> > +		pr_err("platform_driver_register failed:%d", err);
> > +	return err;
> > +}
> > +
> > +arch_initcall(cmdq_sec_init);
> 
> Do you *really* need this to be an arch_initcall?! That's a bit
> early, and I don't
> really see any reason for this.

OK, It needs to be later than cmdq mailbox driver (subsys_initcall) and
earlier than user driver (module_initcall).

So I think it should be rootfs_initcall() here to ensure it can be
earlier than all the user drivers.

> 
> > +
> > +MODULE_LICENSE("GPL");
> > diff --git a/drivers/mailbox/mtk-cmdq-sec-tee.c
> > b/drivers/mailbox/mtk-cmdq-sec-tee.c
> > new file mode 100644
> > index 000000000000..e42e9dba343c
> > --- /dev/null
> > +++ b/drivers/mailbox/mtk-cmdq-sec-tee.c
> > @@ -0,0 +1,202 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +//
> > +// Copyright (c) 2019 MediaTek Inc.
> 
> // SPDX...
> /*
>   * Copyright..
>   */
> 

OK, I'll change it. Thanks!

> 
> > +
> > +#include <linux/math64.h>
> > +#include <linux/sched/clock.h>
> > +
> > +#include <linux/mailbox/mtk-cmdq-sec-tee.h>
> > +#include <linux/soc/mediatek/mtk-cmdq.h>
> > +
> > +/* lock to protect atomic secure task execution */
> > +static DEFINE_MUTEX(cmdq_sec_exec_lock);
> > +
> > +static void cmdq_sec_lock_secure_path(void)
> > +{
> > +	mutex_lock(&cmdq_sec_exec_lock);
> > +	smp_mb();	/*memory barrier*/
> 
> I'm sure that you don't need a memory barrier after locking this
> mutex.
> 
> Besides, you don't need this function at all: just call mutex_lock
> and mutex_unlock
> where appropriate.
> 
> > +}
> > +
> > +static void cmdq_sec_unlock_secure_path(void)
> > +{
> > +	mutex_unlock(&cmdq_sec_exec_lock);
> > +}

OK, I'll remove these 2 wrapping functions.

> > +
> > +void cmdq_sec_setup_tee_context(struct cmdq_sec_tee_context *tee)
> > +{
> > +	/* 09010000 0000 0000 0000000000000000 */
> > +	memset(tee->uuid, 0, sizeof(tee->uuid));
> > +	tee->uuid[0] = 0x9;
> > +	tee->uuid[1] = 0x1;
> > +}
> > +EXPORT_SYMBOL(cmdq_sec_setup_tee_context);
> > +
> > +#if IS_ENABLED(CONFIG_TEE)
> 
> Can this driver really work if CONFIG_TEE=n ?!
> 
> I guess it wouldn't, so this is not right at all. The solution is to
> depend on
> CONFIG_TEE to *even compile* this driver....

Yes, I'll remove this and add a dependency to Makefile. Thanks!

> > +static int tee_dev_match(struct tee_ioctl_version_data *t, const
> > void *v)
> > +{
> > +#if IS_ENABLED(CONFIG_OPTEE)
> > +	if (t->impl_id == TEE_IMPL_ID_OPTEE)
> > +		return 1;
> > +#endif
> > +	return 0;
> > +}
> > +#endif
> > +
> 
> ..snip..
> 
> > diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h
> > b/include/linux/mailbox/mtk-cmdq-mailbox.h
> > index fdda995a69ce..997a75c529fb 100644
> > --- a/include/linux/mailbox/mtk-cmdq-mailbox.h
> > +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
> > @@ -7,6 +7,7 @@
> >   #ifndef __MTK_CMDQ_MAILBOX_H__
> >   #define __MTK_CMDQ_MAILBOX_H__
> >   
> > +#include <linux/mailbox_controller.h>
> 
> Why are you adding this header?
> 
> No implicit inclusions in drivers please.
> 
Because I include <mtk-cmdq-mailbox.h> in mtk-cmdq-sec-mailbox.h and
cmdq_get_shift_pa() will use the `struct mbox_chan * chan` as
parameters.

So I add this to avoid the build fail in mtk-cmdq-sec-mailbox.
I think I should split it to another patch.

> >   #include <linux/platform_device.h>
> >   #include <linux/slab.h>
> >   #include <linux/types.h>
> > @@ -77,6 +78,7 @@ struct cmdq_pkt {
> >   	size_t			buf_size; /* real buffer size */
> >   	void			*cl;
> >   	bool			loop;
> > +	void			*sec_data;
> >   };
> >   
> >   struct cmdq_sec_plat {
> > diff --git a/include/linux/mailbox/mtk-cmdq-sec-iwc-common.h
> > b/include/linux/mailbox/mtk-cmdq-sec-iwc-common.h
> > new file mode 100644
> > index 000000000000..addc515df279
> > --- /dev/null
> > +++ b/include/linux/mailbox/mtk-cmdq-sec-iwc-common.h
> > @@ -0,0 +1,293 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2019 MediaTek Inc.
> > + */
> > +
> > +#ifndef __CMDQ_SEC_IWC_COMMON_H__
> > +#define __CMDQ_SEC_IWC_COMMON_H__
> > +
> > +/*
> > + * shared DRAM
> > + * bit x = 1 means thread x raise IRQ
> > + */
> > +#define CMDQ_SEC_SHARED_IRQ_RAISED_OFFSET	0
> > +#define CMDQ_SEC_SHARED_THR_CNT_OFFSET		0x100
> > +#define CMDQ_SEC_SHARED_TASK_VA_OFFSET		0x200
> > +#define CMDQ_SEC_SHARED_OP_OFFSET		0x300
> > +#define CMDQ_SEC_SHARED_SUSPEND_CNT		0x304
> > +#define CMDQ_SEC_SHARED_RESET_CNT		0x308
> > +
> > +/* commanad buffer & metadata */
> > +#define CMDQ_TZ_CMD_BLOCK_SIZE		(20 << 12) /* MDP
> > readback may request 20 pages */
> > +#define CMDQ_IWC_MAX_CMD_LENGTH		(CMDQ_TZ_CMD_BLOCK_SIZE
> > / 4)
> > +
> > +#define CMDQ_IWC_MAX_ADDR_LIST_LENGTH	(30)
> > +
> > +#define CMDQ_IWC_CLIENT_NAME		(16)
> > +
> > +#define CMDQ_SEC_MESSAGE_INST_LEN	(8)
> > +#define CMDQ_SEC_DISPATCH_LEN		(8)
> > +#define CMDQ_MAX_READBACK_ENG		(8)
> > +
> > +/*
> > + * Command IDs for normal world(TLC or linux kernel) to secure
> > world
> > + */
> > +/* submit current task */
> > +#define CMD_CMDQ_IWC_SUBMIT_TASK	(1)
> > +/* (not used)release resource in secure path per session */
> 
> If this is unused, it shouldn't be here at all.
> 
> Also, you don't need those parenthesis, remove.
> 
> > +#define CMD_CMDQ_IWC_RES_RELEASE	(2)

OK, I'll remove this unused definition.

> > +/* cancel current task */
> > +#define CMD_CMDQ_IWC_CANCEL_TASK	(3)
> > +/* create global resource for secure path */
> > +#define CMD_CMDQ_IWC_PATH_RES_ALLOCATE	(4)
> > +/* destroy globacl resource for secure path */
> > +#define CMD_CMDQ_IWC_PATH_RES_RELEASE	(5)
> > +
> > +/*
> > + * ERROR code number (ERRNO)
> > + * note the error result returns negative value, i.e, -(ERRNO)
> > + */
> > +#define CMDQ_ERR_NOMEM					(12) /*
> > out of memory */
> > +#define CMDQ_ERR_FAULT					(14) /*
> > bad address */
> > +
> > +#define CMDQ_ERR_ADDR_CONVERT_HANDLE_2_PA		(1000)
> > +#define CMDQ_ERR_ADDR_CONVERT_ALLOC_MVA			(1100)
> > +#define CMDQ_ERR_ADDR_CONVERT_ALLOC_MVA_N2S		(1101)
> > +#define CMDQ_ERR_ADDR_CONVERT_FREE_MVA			(1200)
> > +#define CMDQ_ERR_PORT_CONFIG				(1300)
> > +
> > +/* param check */
> > +#define CMDQ_ERR_UNKNOWN_ADDR_METADATA_TYPE		(1400)
> > +#define CMDQ_ERR_TOO_MANY_SEC_HANDLE			(1401)
> > +/* security check */
> > +#define CMDQ_ERR_SECURITY_INVALID_INSTR			(1500)
> > +#define CMDQ_ERR_SECURITY_INVALID_SEC_HANDLE		(1501)
> > +#define CMDQ_ERR_SECURITY_INVALID_DAPC_FALG		(1502)
> > +#define CMDQ_ERR_INSERT_DAPC_INSTR_FAILED		(1503)
> > +#define CMDQ_ERR_INSERT_PORT_SECURITY_INSTR_FAILED	(1504)
> > +#define CMDQ_ERR_INVALID_SECURITY_THREAD		(1505)
> > +#define CMDQ_ERR_PATH_RESOURCE_NOT_READY		(1506)
> > +#define CMDQ_ERR_NULL_TASK				(1507)
> > +#define CMDQ_ERR_SECURITY_INVALID_SOP			(1508)
> > +#define CMDQ_ERR_SECURITY_INVALID_SEC_PORT_FALG		(1511)
> > +
> 
> Fully documented enumeration, but no kerneldoc? :-)
> 

I'll refine it by kerneldoc.

> > +enum cmdq_iwc_addr_metadata_type {
> > +	CMDQ_IWC_H_2_PA		= 0, /* sec handle to sec PA */
> > +	CMDQ_IWC_H_2_MVA	= 1, /* sec handle to sec MVA */
> > +	CMDQ_IWC_NMVA_2_MVA	= 2, /* map normal MVA to secure world */
> > +	CMDQ_IWC_PH_2_MVA	= 3, /* protected handle to sec MVA */
> > +};
> > +
> > +enum cmdq_sec_engine_enum {
> > +	/* MDP */
> > +	CMDQ_SEC_MDP_RDMA0		= 0,
> > +	CMDQ_SEC_MDP_RDMA1		= 1,
> > +	CMDQ_SEC_MDP_WDMA		= 2,
> > +	CMDQ_SEC_MDP_RDMA2		= 3,
> > +	CMDQ_SEC_MDP_RDMA3		= 4,
> > +	CMDQ_SEC_MDP_WROT0		= 5,
> > +	CMDQ_SEC_MDP_WROT1		= 6,
> > +	CMDQ_SEC_MDP_WROT2		= 7,
> > +	CMDQ_SEC_MDP_WROT3		= 8,
> > +	CMDQ_SEC_MDP_HDR0		= 9,
> > +	CMDQ_SEC_MDP_HDR1		= 10,
> > +	CMDQ_SEC_MDP_HDR2		= 11,
> > +	CMDQ_SEC_MDP_HDR3		= 12,
> > +	CMDQ_SEC_MDP_AAL0		= 13,
> > +	CMDQ_SEC_MDP_AAL1		= 14,
> > +	CMDQ_SEC_MDP_AAL2		= 15,
> > +	CMDQ_SEC_MDP_AAL3		= 16,
> > +
> > +	/* DISP (VDOSYS0) */
> > +	CMDQ_SEC_DISP_RDMA0		= 17,
> > +	CMDQ_SEC_DISP_RDMA1		= 18,
> > +	CMDQ_SEC_DISP_WDMA0		= 19,
> > +	CMDQ_SEC_DISP_WDMA1		= 20,
> > +	CMDQ_SEC_DISP_OVL0		= 21,
> > +	CMDQ_SEC_DISP_OVL1		= 22,
> > +	CMDQ_SEC_DISP_OVL2		= 23,
> > +	CMDQ_SEC_DISP_2L_OVL0		= 24,
> > +	CMDQ_SEC_DISP_2L_OVL1		= 25,
> > +	CMDQ_SEC_DISP_2L_OVL2		= 26,
> > +
> > +	/* DSIP (VDOSYS1) */
> > +	CMDQ_SEC_VDO1_DISP_RDMA_L0	= 27,
> > +	CMDQ_SEC_VDO1_DISP_RDMA_L1	= 28,
> > +	CMDQ_SEC_VDO1_DISP_RDMA_L2	= 29,
> > +	CMDQ_SEC_VDO1_DISP_RDMA_L3	= 30,
> > +
> > +	/* VENC */
> > +	CMDQ_SEC_VENC_BSDMA		= 31,
> > +	CMDQ_SEC_VENC_CUR_LUMA		= 32,
> > +	CMDQ_SEC_VENC_CUR_CHROMA	= 33,
> > +	CMDQ_SEC_VENC_REF_LUMA		= 34,
> > +	CMDQ_SEC_VENC_REF_CHROMA	= 35,
> > +	CMDQ_SEC_VENC_REC		= 36,
> > +	CMDQ_SEC_VENC_SUB_R_LUMA	= 37,
> > +	CMDQ_SEC_VENC_SUB_W_LUMA	= 38,
> > +	CMDQ_SEC_VENC_SV_COMV		= 39,
> > +	CMDQ_SEC_VENC_RD_COMV		= 40,
> > +	CMDQ_SEC_VENC_NBM_RDMA		= 41,
> > +	CMDQ_SEC_VENC_NBM_WDMA		= 42,
> > +	CMDQ_SEC_VENC_NBM_RDMA_LITE	= 43,
> > +	CMDQ_SEC_VENC_NBM_WDMA_LITE	= 44,
> > +	CMDQ_SEC_VENC_FCS_NBM_RDMA	= 45,
> > +	CMDQ_SEC_VENC_FCS_NBM_WDMA	= 46,
> > +
> > +	CMDQ_SEC_MAX_ENG_COUNT		/* keep in the end */
> > +};
> > +
> > +/*
> > + * IWC message
> 
> kerneldoc please

OK. Thanks!
> 
> > + */
> > +struct iwcCmdqAddrMetadata_t {
> 
> ...and no camelcase.
> 
OK, I'll fix it. Thanks!

> > +	/*
> > +	 * [IN] index of instruction
> > +	 * Update its arg_b value to real PA/MVA in secure world
> > +	 */
> > +	u32 instrIndex;
> > +
> 
> 
> ..snip..
> 
> > diff --git a/include/linux/mailbox/mtk-cmdq-sec-mailbox.h
> > b/include/linux/mailbox/mtk-cmdq-sec-mailbox.h
> > new file mode 100644
> > index 000000000000..17ee0ae646ab
> > --- /dev/null
> > +++ b/include/linux/mailbox/mtk-cmdq-sec-mailbox.h
> > @@ -0,0 +1,83 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2019 MediaTek Inc.
> > + */
> > +
> > +#ifndef __MTK_CMDQ_SEC_MAILBOX_H__
> > +#define __MTK_CMDQ_SEC_MAILBOX_H__
> > +
> > +#include <linux/kernel.h>
> > +
> > +#include <linux/mailbox/mtk-cmdq-mailbox.h>
> > +#include <linux/mailbox/mtk-cmdq-sec-iwc-common.h>
> > +#include <linux/mailbox/mtk-cmdq-sec-tee.h>
> > +#include <linux/soc/mediatek/mtk-cmdq.h>
> > +
> > +#define CMDQ_INVALID_THREAD		(-1)
> > +#define CMDQ_MAX_TASK_IN_SECURE_THREAD	(16)
> > +#define CMDQ_SEC_IRQ_THREAD		(15)
> > +
> > +/* This define should sync with cmdq_sec_def.h in secure world */
> > +#define CMDQ_MIN_SECURE_THREAD_ID	(8)
> 
> As you wrote, this shall be in sync with secure world, so this will
> either break
> at the next SoC, or at the next secure world update.
> 
> This means that those definitions shall be in platform data for this
> driver.

Oh yes! I'll try to use platform data to refine this.
Thanks for your advice.

> 
> > +#define CMDQ_MAX_SECURE_THREAD_ID	(10)
> > +#define CMDQ_MAX_SECURE_THREAD_COUNT	(CMDQ_MAX_SECURE_THREAD
> > _ID - CMDQ_MIN_SECURE_THREAD_ID)
> > +
> > +/* Max value of CMDQ_THR_EXEC_CMD_CNT (value starts from 0) */
> > +#define CMDQ_MAX_COOKIE_VALUE           (0xffff)
> > +
> > +enum cmdq_sec_scenario {
> > +	CMDQ_SEC_PRIMARY_DISP		= 1,
> > +	CMDQ_SEC_SUB_DISP		= 4,
> > +	CMDQ_SEC_PRIMARY_DISP_DISABLE	= 18,
> > +	CMDQ_SEC_SUB_DISP_DISABLE	= 19,
> > +
> > +	CMDQ_MAX_SEC_COUNT	/* keep in the end */
> > +};
> > +
> > +/* Inter-world communication state */
> > +enum cmdq_iwc_state_enum {
> > +	IWC_INIT,
> > +	IWC_CONTEXT_INITED,
> > +	IWC_WSM_ALLOCATED,
> > +	IWC_SES_OPENED,
> > +	IWC_SES_ON_TRANSACTED,
> > +
> > +	IWC_STATE_END_OF_ENUM,
> 
> Usually "END_OF_ENUM" is "MAX", so rename this to IWC_STATE_MAX
> 
OK, I'll change it.

> > +};
> > +
> > +/*
> 
> kerneldoc please.
> 
OK. Thanks!

> > + * Address metadata, used to translate secure buffer PA
> > + * related instruction in secure world.
> > + */
> > +struct cmdq_sec_data {
> > +	u32 addrMetadataCount;	/* [IN] count of element in
> > addrList */
> > +	u64 addrMetadatas;		/* [IN] array of
> > iwcCmdqAddrMetadata_t */
> > +	u32 addrMetadataMaxCount;	/* [Reserved] */
> > +
> > +	enum cmdq_sec_scenario scenario;
> > +
> > +	u64 enginesNeedDAPC;
> > +	u64 enginesNeedPortSecurity;
> > +
> > +	/*
> > +	 * [Reserved]
> > +	 * This is task index in thread's tasklist for CMDQ driver
> > usage.
> > +	 * Not for client. -1 means not in tasklist.
> > +	 */
> > +	s32 waitCookie;
> > +	/* [Reserved] reset HW thread in secure world */
> > +	bool resetExecCnt;
> > +
> > +	/* [OUT] response */
> > +	s32 response;
> > +	struct iwcCmdqSecStatus_t sec_status;
> > +};
> > +
> 
> Please document all those functions that you're exporting with
> kerneldoc.
> 

OK, Thanks!

> > +int cmdq_sec_insert_backup_cookie(struct cmdq_pkt *pkt);
> > +int cmdq_sec_pkt_set_data(struct cmdq_pkt *pkt, const u64
> > dapc_engine,
> > +			  const u64 port_sec_engine, const enum
> > cmdq_sec_scenario scenario);
> > +int cmdq_sec_pkt_write(struct cmdq_pkt *pkt, u32 addr, u64 base,
> > +		       const enum cmdq_iwc_addr_metadata_type type,
> > +		       const u32 offset, const u32 size, const u32
> > port);
> > +
> > +#endif /* __MTK_CMDQ_SEC_MAILBOX_H__ */
> > diff --git a/include/linux/mailbox/mtk-cmdq-sec-tee.h
> > b/include/linux/mailbox/mtk-cmdq-sec-tee.h
> > new file mode 100644
> > index 000000000000..77090921cdf6
> > --- /dev/null
> > +++ b/include/linux/mailbox/mtk-cmdq-sec-tee.h
> > @@ -0,0 +1,31 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2019 MediaTek Inc.
> > + */
> > +
> > +#ifndef __MTK_CMDQ_SEC_TEE_H__
> > +#define __MTK_CMDQ_SEC_TEE_H__
> > +
> > +#include <linux/types.h>
> > +#include <linux/delay.h>
> > +#include <linux/tee_drv.h>
> > +
> > +/* context for tee vendor */
> 
> kerneldoc please.
> 
OK, Thanks!

> > +struct cmdq_sec_tee_context {
> > +	/* Universally Unique Identifier of secure world */
> > +	u8			uuid[TEE_IOCTL_UUID_LEN];
> > +	struct tee_context	*tee_context; /* basic context */
> > +	u32			session; /* session handle */
> > +	struct tee_shm		*shared_mem; /* shared memory */
> > +};
> > +
> 
> Also here, please document those functions.
> 
OK, Thanks!

> > +void cmdq_sec_setup_tee_context(struct cmdq_sec_tee_context *tee);
> > +int cmdq_sec_init_context(struct cmdq_sec_tee_context *tee);
> > +int cmdq_sec_deinit_context(struct cmdq_sec_tee_context *tee);
> > +int cmdq_sec_allocate_wsm(struct cmdq_sec_tee_context *tee, void
> > **wsm_buffer, u32 size);
> > +int cmdq_sec_free_wsm(struct cmdq_sec_tee_context *tee, void
> > **wsm_buffer);
> > +int cmdq_sec_open_session(struct cmdq_sec_tee_context *tee, void
> > *wsm_buffer);
> > +int cmdq_sec_close_session(struct cmdq_sec_tee_context *tee);
> > +int cmdq_sec_execute_session(struct cmdq_sec_tee_context *tee, u32
> > cmd, s32 timeout_ms);
> > +
> > +#endif	/* __MTK_CMDQ_SEC_TEE_H__ */
> 
> 
> 
> Regards,
> Angelo

These patches are porting from the other platform which is not strongly
follow the upstream coding style. And we are asked to send the upstream
patch in a hurry schedule. Although I've remove a lot of unused code
and simplify some code flow, it still has a lot of trivial problems.
I really appreciate you taking the time to review this patch.
Thanks again!

Regards,
Jason-JH.Lin

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ