linux-kernel - Re: [PATCH v1 10/12] soc: mediatek: cmdq: add loop function

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] [thread-next>] [day] [month] [year] [list]
Message-ID: <1574645703.4712.7.camel@mtksdaap41>
Date:   Mon, 25 Nov 2019 09:35:03 +0800
From:   CK Hu <ck.hu@...iatek.com>
To:     Dennis-YC Hsieh <dennis-yc.hsieh@...iatek.com>
CC:     Rob Herring <robh+dt@...nel.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Jassi Brar <jassisinghbrar@...il.com>,
        <linux-kernel@...r.kernel.org>,
        <linux-mediatek@...ts.infradead.org>, <devicetree@...r.kernel.org>,
        <wsd_upstream@...iatek.com>,
        Bibby Hsieh <bibby.hsieh@...iatek.com>,
        Houlong Wei <houlong.wei@...iatek.com>,
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v1 10/12] soc: mediatek: cmdq: add loop function

Hi, Dennis:

On Fri, 2019-11-22 at 18:29 +0800, Dennis-YC Hsieh wrote:
> Hi CK,
> 
> On Fri, 2019-11-22 at 17:46 +0800, CK Hu wrote:
> > Hi, Dennis:
> > 
> > On Thu, 2019-11-21 at 17:12 +0800, Dennis YC Hsieh wrote:
> > > Add finalize loop function in cmdq helper functions which loop whole pkt
> > > in gce hardware thread without cpu operation.
> > > 
> > > Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@...iatek.com>
> > > ---
> > >  drivers/soc/mediatek/mtk-cmdq-helper.c |   41 ++++++++++++++++++++++++++++++++
> > >  include/linux/soc/mediatek/mtk-cmdq.h  |    8 +++++++
> > >  2 files changed, 49 insertions(+)
> > > 
> > > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > > index 4235cf8..3b10241 100644
> > > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > > @@ -385,12 +385,27 @@ int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value)
> > >  }
> > >  EXPORT_SYMBOL(cmdq_pkt_assign);
> > >  
> > > +static bool cmdq_pkt_finalized(struct cmdq_pkt *pkt)
> > > +{
> > > +	struct cmdq_instruction *inst;
> > > +
> > > +	if (pkt->cmd_buf_size < 2 * CMDQ_INST_SIZE)
> > > +		return false;
> > > +
> > > +	inst = pkt->va_base + pkt->cmd_buf_size - 2 * CMDQ_INST_SIZE;
> > > +	return inst->op == CMDQ_CODE_EOC;
> > > +}
> > > +
> > >  static int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
> > >  {
> > >  	struct cmdq_client *cl = pkt->cl;
> > >  	struct cmdq_instruction inst = { {0} };
> > >  	int err;
> > >  
> > > +	/* do not finalize twice */
> > > +	if (cmdq_pkt_finalized(pkt))
> > > +		return 0;
> > > +
> > >  	/* insert EOC and generate IRQ for each command iteration */
> > >  	inst.op = CMDQ_CODE_EOC;
> > >  	inst.value = CMDQ_EOC_IRQ_EN;
> > > @@ -406,6 +421,32 @@ static int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
> > >  	return err;
> > >  }
> > >  
> > > +int cmdq_pkt_finalize_loop(struct cmdq_pkt *pkt)
> > > +{
> > > +	struct cmdq_client *cl = pkt->cl;
> > > +	struct cmdq_instruction inst = { {0} };
> > > +	int err;
> > > +
> > > +	/* do not finalize twice */
> > > +	if (cmdq_pkt_finalized(pkt))
> > > +		return 0;
> > 
> > Why not just export cmdq_pkt_finalize() for user and do not call
> > cmdq_pkt_finalize() in cmdq_pkt_flush_async(), so you don't need to
> > check this.
> > 
> > I would be more like to export API such as cmdq_pkt_eoc(),
> > cmdq_pkt_jump(), this would provide more flexibility for user to
> > assemble the command it want.
> > 
> > Regards,
> > CK
> 
> Thanks for your comment.
> 
> Should we backward compatible with existing clients? Remove finalize in
> flush will cause existing client flush without IRQ.

The latest kernel (v5.4-rc8) still has no clients which use cmdq landed
on upstream, and we don't need to consider backward compatible. [1] is
the example that iommu would replace the proprietary interface with
standard interface, so it would modify all clients which use the
proprietary interface. So what you should do is to modify client as
well.

[1]
https://patchwork.kernel.org/project/linux-mediatek/list/?series=168801

Regards,
CK

> 
> 
> Regards,
> Dennis
> 
> > 
> > > +
> > > +	/* insert EOC and generate IRQ for each command iteration */
> > > +	inst.op = CMDQ_CODE_EOC;
> > > +	err = cmdq_pkt_append_command(pkt, inst);
> > > +	if (err < 0)
> > > +		return err;
> > > +
> > > +	/* JUMP abaolute to begin */
> > > +	inst.op = CMDQ_CODE_JUMP;
> > > +	inst.offset = 1;
> > > +	inst.value = pkt->pa_base >> cmdq_mbox_shift(cl->chan);
> > > +	err = cmdq_pkt_append_command(pkt, inst);
> > > +
> > > +	return err;
> > > +}
> > > +EXPORT_SYMBOL(cmdq_pkt_finalize_loop);
> > > +
> > >  static void cmdq_pkt_flush_async_cb(struct cmdq_cb_data data)
> > >  {
> > >  	struct cmdq_pkt *pkt = (struct cmdq_pkt *)data.data;
> > > diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> > > index b3474f2..77e8944 100644
> > > --- a/include/linux/soc/mediatek/mtk-cmdq.h
> > > +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> > > @@ -203,6 +203,14 @@ int cmdq_pkt_poll_mask(struct cmdq_pkt *pkt, u8 subsys,
> > >  int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value);
> > >  
> > >  /**
> > > + * cmdq_pkt_finalize_loop() - Append EOC and jump command to loop pkt.
> > > + * @pkt:	the CMDQ packet
> > > + *
> > > + * Return: 0 for success; else the error code is returned
> > > + */
> > > +int cmdq_pkt_finalize_loop(struct cmdq_pkt *pkt);
> > > +
> > > +/**
> > >   * cmdq_pkt_flush_async() - trigger CMDQ to asynchronously execute the CMDQ
> > >   *                          packet and call back at the end of done packet
> > >   * @pkt:	the CMDQ packet
> > 
> > 
> 
>