| 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
| ||
|
Date: Fri, 29 Jun 2018 07:32:19 +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 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
> >
>
> [...]
>
> > +
> > +int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u32 event)
> > +{
> > + u32 arg_b;
> > +
> > + if (event >= CMDQ_MAX_EVENT || event < 0)
>
> The type of event is 'u32', so checking 'event < 0' is redundant.
will fix.
>
> > + return -EINVAL;
> > +
> > + /*
> > + * WFE arg_b
> > + * bit 0-11: wait value
> > + * bit 15: 1 - wait, 0 - no wait
> > + * bit 16-27: update value
> > + * bit 31: 1 - update, 0 - no update
> > + */
> > + arg_b = CMDQ_WFE_UPDATE | CMDQ_WFE_WAIT | CMDQ_WFE_WAIT_VALUE;
> > +
> > + return cmdq_pkt_append_command(pkt, CMDQ_CODE_WFE, event, arg_b);
> > +}
> > +EXPORT_SYMBOL(cmdq_pkt_wfe);
> > +
> > +int cmdq_pkt_clear_event(struct cmdq_pkt *pkt, u32 event)
> > +{
> > + if (event >= CMDQ_MAX_EVENT || event < 0)
>
> The type of event is 'u32', so checking 'event < 0' is redundant.
Will fix.
>
> > + return -EINVAL;
> > +
> > + return cmdq_pkt_append_command(pkt, CMDQ_CODE_WFE, event,
> > + CMDQ_WFE_UPDATE);
> > +}
> > +EXPORT_SYMBOL(cmdq_pkt_clear_event);
> > +
> > +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.
> 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.
>
> 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);
>
> [...]
>
>
Powered by blists - more mailing lists