linux-kernel - Re: [PATCH v22 4/4] soc: mediatek: Add Mediatek CMDQ helper

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1530665257.24157.7.camel@mhfsdcap03>
Date:   Wed, 4 Jul 2018 08:47:37 +0800
From:   houlong wei <houlong.wei@...iatek.com>
To:     CK Hu <ck.hu@...iatek.com>
CC:     Jassi Brar <jassisinghbrar@...il.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Daniel Kurtz <djkurtz@...omium.org>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-mediatek@...ts.infradead.org" 
        <linux-mediatek@...ts.infradead.org>,
        srv_heupstream <srv_heupstream@...iatek.com>,
        Sascha Hauer <kernel@...gutronix.de>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        "Nicolas Boichat" <drinkcat@...omium.org>,
        Bibby Hsieh (謝濟遠) 
        <Bibby.Hsieh@...iatek.com>,
        YT Shen (沈岳霆) 
        <Yt.Shen@...iatek.com>,
        Daoyuan Huang (黃道原) 
        <Daoyuan.Huang@...iatek.com>,
        Jiaguang Zhang (张加广) 
        <Jiaguang.Zhang@...iatek.com>,
        Dennis-YC Hsieh (謝宇哲) 
        <Dennis-YC.Hsieh@...iatek.com>,
        Monica Wang (王孟婷) 
        <monica.wang@...iatek.com>,
        Hs Liao (廖宏祥) 
        <Hs.Liao@...iatek.com>,
        Ginny Chen (陳治傑) 
        <ginny.chen@...iatek.com>,
        Enzhu Wang (王恩柱) 
        <enzhu.wang@...iatek.com>, <houlong.wei@...iatek.com>
Subject: Re: [PATCH v22 4/4] soc: mediatek: Add Mediatek CMDQ helper

On Fri, 2018-06-29 at 17:22 +0800, CK Hu wrote:
> Hi, Houlong:
> 
> On Fri, 2018-06-29 at 07:32 +0800, houlong wei wrote:
> > On Thu, 2018-06-28 at 18:41 +0800, CK Hu wrote:
> > > Hi, Houlong:
> > > 
> > > Some inline comment.
> > > 
> > > On Wed, 2018-06-27 at 19:16 +0800, Houlong Wei wrote:
> > > > Add Mediatek CMDQ helper to create CMDQ packet and assemble GCE op code.
> > > > 
> > > > Signed-off-by: Houlong Wei <houlong.wei@...iatek.com>
> > > > Signed-off-by: HS Liao <hs.liao@...iatek.com>
> > > > ---
> > > >  drivers/soc/mediatek/Kconfig           |   12 ++
> > > >  drivers/soc/mediatek/Makefile          |    1 +
> > > >  drivers/soc/mediatek/mtk-cmdq-helper.c |  258 ++++++++++++++++++++++++++++++++
> > > >  include/linux/soc/mediatek/mtk-cmdq.h  |  132 ++++++++++++++++
> > > >  4 files changed, 403 insertions(+)
> > > >  create mode 100644 drivers/soc/mediatek/mtk-cmdq-helper.c
> > > >  create mode 100644 include/linux/soc/mediatek/mtk-cmdq.h
> > > > 
> > > > diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig
> > > > index a7d0667..17bd759 100644
> > > > --- a/drivers/soc/mediatek/Kconfig
> > > > +++ b/drivers/soc/mediatek/Kconfig
> > > > @@ -4,6 +4,18 @@
> > > >  menu "MediaTek SoC drivers"
> > > >  	depends on ARCH_MEDIATEK || COMPILE_TEST
> > > >  
> > > 
> 
> [...]
> 
> > > > +
> > > > +static int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
> > > > +{
> > > > +	int err;
> > > > +
> > > > +	if (cmdq_pkt_is_finalized(pkt))
> > > > +		return 0;
> > > > +
> > > > +	/* insert EOC and generate IRQ for each command iteration */
> > > > +	err = cmdq_pkt_append_command(pkt, CMDQ_CODE_EOC, 0, CMDQ_EOC_IRQ_EN);
> > > > +	if (err < 0)
> > > > +		return err;
> > > > +
> > > > +	/* JUMP to end */
> > > > +	err = cmdq_pkt_append_command(pkt, CMDQ_CODE_JUMP, 0, CMDQ_JUMP_PASS);
> > > > +	if (err < 0)
> > > > +		return err;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +int cmdq_pkt_flush_async(struct cmdq_client *client, struct cmdq_pkt *pkt,
> > > > +			 cmdq_async_flush_cb cb, void *data)
> > > > +{
> > > > +	int err;
> > > > +	struct device *dev;
> > > > +	dma_addr_t dma_addr;
> > > > +
> > > > +	err = cmdq_pkt_finalize(pkt);
> > > > +	if (err < 0)
> > > > +		return err;
> > > > +
> > > > +	dev = client->chan->mbox->dev;
> > > > +	dma_addr = dma_map_single(dev, pkt->va_base, pkt->cmd_buf_size,
> > > > +		DMA_TO_DEVICE);
> > > 
> > > You map here, but I could not find unmap, so the unmap should be done in
> > > client driver. I would prefer a symmetric map/unmap which means that
> > > both map and unmap are done in client driver. I think you put map here
> > > because you should map after finalize. 
> > 
> > The unmap is done before callback to client, in function
> > cmdq_task_exec_done, mtk-cmdq-mailbox.c.
> 
> You put unmap in mailbox controller, and map here (here would be mailbox
> client), so this is not symmetric. If the code is asymmetric, it's easy
> to cause bug and not easy to maintain. So I would like move both
> map/unmap to client driver.
> 

Since map/unmap is common code for client drivers, can we move unmap to
CMDQ helper and do not put in client driver?

> > 
> > > Therefore, export
> > > cmdq_pkt_finalize() to client driver and let client do finalize, so
> > > there is no finalize in flush function. This method have a benefit that
> > > if client reuse command buffer, it need not to map/unmap frequently.
> > 
> > If client reuse command buffer or cmdq_pkt(command queue packet), client
> > will add new commands to the cmdq_pkt, so map/unmap are necessary for
> > each cmdq_pkt flush.
> 
> If the buffer size is 4K bytes, client driver could map the whole 4K at
> initialization. Before it write new command, it call
> dma_sync_single_for_cpu(), after it write new command, it call
> dma_sync_single_for_device(). And then it could flush this buffer to
> mailbox controller. So client driver just call dma sync function when it
> reuse the command buffer. dma sync function is much lightweight then dma
> map because map would search the memory area which could be mapped.
> 
> Regards,
> CK

Maybe we can do dma map/unmap/sync in cmdq helper, and make client
driver simple.

> 
> > 
> > > 
> > > Regards,
> > > CK
> > > 
> > > > +	if (dma_mapping_error(dev, dma_addr)) {
> > > > +		dev_err(client->chan->mbox->dev, "dma map failed\n");
> > > > +		return -ENOMEM;
> > > > +	}
> > > > +
> > > > +	pkt->pa_base = dma_addr;
> > > > +	pkt->cb.cb = cb;
> > > > +	pkt->cb.data = data;
> > > > +
> > > > +	mbox_send_message(client->chan, pkt);
> > > > +	/* We can send next packet immediately, so just call txdone. */
> > > > +	mbox_client_txdone(client->chan, 0);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +EXPORT_SYMBOL(cmdq_pkt_flush_async);
> > > > +
> > > > +struct cmdq_flush_completion {
> > > > +	struct completion cmplt;
> > > > +	bool err;
> > > > +};
> > > > +
> > > > +static void cmdq_pkt_flush_cb(struct cmdq_cb_data data)
> > > > +{
> > > > +	struct cmdq_flush_completion *cmplt = data.data;
> > > > +
> > > > +	cmplt->err = data.err;
> > > > +	complete(&cmplt->cmplt);
> > > > +}
> > > > +
> > > > +int cmdq_pkt_flush(struct cmdq_client *client, struct cmdq_pkt *pkt)
> > > > +{
> > > > +	struct cmdq_flush_completion cmplt;
> > > > +	int err;
> > > > +
> > > > +	init_completion(&cmplt.cmplt);
> > > > +	err = cmdq_pkt_flush_async(client, pkt, cmdq_pkt_flush_cb, &cmplt);
> > > > +	if (err < 0)
> > > > +		return err;
> > > > +	wait_for_completion(&cmplt.cmplt);
> > > > +
> > > > +	return cmplt.err ? -EFAULT : 0;
> > > > +}
> > > > +EXPORT_SYMBOL(cmdq_pkt_flush);
> > > 
> > > [...]
> > > 
> > > 
> > 
> > 
> 
>