| 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
| ||
|
Message-ID: <480e5b574f8fd0a1d25ad882f76042cb358ebac5.camel@mediatek.com>
Date: Tue, 5 Mar 2024 01:32:56 +0000
From: Jason-JH Lin (林睿祥) <Jason-JH.Lin@...iatek.com>
To: "jassisinghbrar@...il.com" <jassisinghbrar@...il.com>,
"matthias.bgg@...il.com" <matthias.bgg@...il.com>,
"angelogioacchino.delregno@...labora.com"
<angelogioacchino.delregno@...labora.com>, "chunkuang.hu@...nel.org"
<chunkuang.hu@...nel.org>
CC: "linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>,
Singo Chang (張興國) <Singo.Chang@...iatek.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Jason-ch Chen (陳建豪)
<Jason-ch.Chen@...iatek.com>, Shawn Sung (宋孝謙)
<Shawn.Sung@...iatek.com>, Nancy Lin (林欣螢)
<Nancy.Lin@...iatek.com>, Project_Global_Chrome_Upstream_Group
<Project_Global_Chrome_Upstream_Group@...iatek.com>,
"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 3/5] soc: mediatek: mtk-cmdq: Add cmdq_pkt_poll_addr()
function
Hi Angelo,
Thanks for the reviews.
On Mon, 2024-03-04 at 11:05 +0100, AngeloGioacchino Del Regno wrote:
> Il 01/03/24 12:11, Jason-JH.Lin ha scritto:
> > Add cmdq_pkt_poll_addr function to support CMDQ user making
> > an instruction for polling a specific address of hardware rigster
> > to check the value with or without mask.
> >
> > POLL is an old operation in GCE, so it does not support SPR and
>
> POLL is a legacy operation in GCE, so it does not support .....
>
OK, I'll change that.
> > CMDQ_CODE_LOGIC. CMDQ users need to use GPR and CMDQ_CODE_MASK
> > to move polling register address to GPR to make an instruction.
> > This will be done in cmdq_pkt_poll_addr().
> >
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@...iatek.com>
> > Change-Id: I91ff98e06570dc4501187eb29de64aaa65b1cf13
> > ---
> > drivers/soc/mediatek/mtk-cmdq-helper.c | 38
> > ++++++++++++++++++++++++++
> > include/linux/soc/mediatek/mtk-cmdq.h | 16 +++++++++++
> > 2 files changed, 54 insertions(+)
> >
> > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > index 3a1e47ad8a41..2e9fc9bb1183 100644
> > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > @@ -12,6 +12,7 @@
> >
> > #define CMDQ_WRITE_ENABLE_MASK BIT(0)
> > #define CMDQ_POLL_ENABLE_MASK BIT(0)
> > +#define CMDQ_POLL_HIGH_ADDR_GPR (14)
> > #define CMDQ_EOC_IRQ_EN BIT(0)
> > #define CMDQ_REG_TYPE 1
> > #define CMDQ_JUMP_RELATIVE 1
> > @@ -406,6 +407,43 @@ int cmdq_pkt_poll_mask(struct cmdq_pkt *pkt,
> > u8 subsys,
> > }
> > EXPORT_SYMBOL(cmdq_pkt_poll_mask);
> >
> > +int cmdq_pkt_poll_addr(struct cmdq_pkt *pkt, dma_addr_t addr, u32
> > value, u32 mask)
> > +{
> > + struct cmdq_instruction inst = { {0} };
> > + int err;
> > + u8 use_mask = 0;
> > +
> > + if (mask != U32_MAX) {
>
> inst.op = CMDQ_CODE_MASK;
>
> /* Describe what you're doing here as a comment please */
> if (mask < GENMASK(31, 0)) {
> inst.mask = ~mask;
> err = cmdq_pkt_append_command ...
> use_mask ...
> }
OK, I'll change the U32_MAX to GENMASK(31, 0).
>
> /* POLL is a legacy operation in GCE and ... etc etc */
> inst.mask = addr;
> inst.dst_t = ...
> inst.sop = ...
> err = cmdq_pkt_append... etc etc etc
>
OK, I'll change like this and I'll use inst.value = addr;
because addr is not mask.
>
> > + inst.op = CMDQ_CODE_MASK;
> > + inst.mask = ~mask;
> > + err = cmdq_pkt_append_command(pkt, inst);
> > + if (err != 0)
> > + return err;
> > + use_mask = CMDQ_POLL_ENABLE_MASK;
> > + }
> > +
> > + /*
> > + * POLL is an old operation in GCE and it does not support SPR
> > and CMDQ_CODE_LOGIC,
> > + * so it can not use cmdq_pkt_assign to keep polling register
> > address to SPR.
> > + * It needs to use GPR and CMDQ_CODE_MASK to move polling
> > register address to GPR.
> > + */
> > + inst.op = CMDQ_CODE_MASK;
> > + inst.dst_t = CMDQ_REG_TYPE;
> > + inst.sop = CMDQ_POLL_HIGH_ADDR_GPR;
> > + inst.mask = addr;
> > + err = cmdq_pkt_append_command(pkt, inst);
> > + if (err < 0)
> > + return err;
> > +
> > + inst.op = CMDQ_CODE_POLL;
> > + inst.dst_t = CMDQ_REG_TYPE;
> > + inst.sop = CMDQ_POLL_HIGH_ADDR_GPR;
>
> This is a reassignment of the same value. You can do it like
>
> /* dst_t and sop must be CMDQ_REG_TYPE, CMDQ_POLL_HIGH_ADDR_GPR */
> inst.op = CMDQ_CODE_POLL;
> inst.offset ...
> inst.value ....
>
inst is used to generate an instruction for GCE, so it will be
reassigned every time after calling cmdq_pkt_append_command(pkt, inst);
to append a generated inst into the cmd buffer.
> (but you're also inheriting the same `inst.mask`, was that intended?)
>
The definition of cmdq_instruction is:
struct cmdq_instruction {
union {
u32 value;
u32 mask;
struct {
u16 arg_c;
u16 src_reg;
};
};
union {
u16 offset;
u16 event;
u16 reg_dst;
};
...
};
so inst.mask and inst.value are the same u32 in inst.
That means inst.mask has been reassigned as the inst.value here.
> err = cmdq_pkt_append ....
> if (err < 0)
> return err;
>
> return 0
> }
OK, I'll change it to return 0.
>
> > + inst.offset = use_mask;
> > + inst.value = value;
> > + return cmdq_pkt_append_command(pkt, inst);
> > +}
> > +EXPORT_SYMBOL(cmdq_pkt_poll_addr);
> > +
> > int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value)
> > {
> > struct cmdq_instruction inst = {};
> > diff --git a/include/linux/soc/mediatek/mtk-cmdq.h
> > b/include/linux/soc/mediatek/mtk-cmdq.h
> > index b6dbe2d8f16a..2fe9be240fbc 100644
> > --- a/include/linux/soc/mediatek/mtk-cmdq.h
> > +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> > @@ -253,6 +253,22 @@ int cmdq_pkt_poll(struct cmdq_pkt *pkt, u8
> > subsys,
> > int cmdq_pkt_poll_mask(struct cmdq_pkt *pkt, u8 subsys,
> > u16 offset, u32 value, u32 mask);
> >
> > +/**
> > + * cmdq_pkt_poll_addr() - Append polling command to the CMDQ
> > packet, ask GCE to
> > + * execute an instruction that wait for a
> > specified
> > + * address of hardware register to check for the
> > value
> > + * w/ or w/o mask.
> > + * All GCE hardware threads will be blocked by
> > this
> > + * instruction.
>
> /**
> * cmdq_pkt_poll_addr() - Append blocking POLL command to CMDQ
> packet
> * @pkt: the CMDQ packet
> * @addr: the hardware register address
> * @value: the specified target register value
> * @mask: the specified target register mask
> *
> * Appends a polling (POLL) command to the CMDQ packet and asks the
> GCE
> * to execute an instruction that checks for the specified `value`
> (with
> * or without `mask`) to appear in the specified hardware register
> `addr`.
> * All GCE threads will be blocked by this instruction.
> *
> * Return: 0 for success or negative error code
> */
>
> That's better, isn't it? :-)
>
OK, I'll change that. Thanks for making the entire sample.
Regards,
Jason-JH.Lin
> Cheers,
> Angelo
>
>
Powered by blists - more mailing lists