| Message ID | 20230918192204.32263-1-jason-jh.lin@mediatek.com |
|---|---|
| Headers | show |
| Series | Add CMDQ secure driver for SVP | expand |
Hi, Jason: On Tue, 2023-09-19 at 03:21 +0800, Jason-JH.Lin wrote: > Add cmdq_mbox_stop to disable GCE thread. > > To support the error handling or the stop flow of the GCE loopping > thread, lopping thread user can call cmdq_mbox_stop to disable the > GCE HW thread. > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> > --- > drivers/mailbox/mtk-cmdq-mailbox.c | 6 ++++++ > include/linux/mailbox/mtk-cmdq-mailbox.h | 1 + > 2 files changed, 7 insertions(+) > > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c > b/drivers/mailbox/mtk-cmdq-mailbox.c > index 4d62b07c1411..8bd39fecbf00 100644 > --- a/drivers/mailbox/mtk-cmdq-mailbox.c > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c > @@ -469,6 +469,12 @@ static void cmdq_mbox_shutdown(struct mbox_chan > *chan) > spin_unlock_irqrestore(&thread->chan->lock, flags); > } > > +void cmdq_mbox_stop(struct mbox_chan *chan) > +{ > + cmdq_mbox_shutdown(chan); cmdq_mobx_stop() is equal to cmdq_mbox_shutdown(), so client driver could call mbox_free_channel() to do this and this function is redundant. Regards, CK > +} > +EXPORT_SYMBOL(cmdq_mbox_stop); > + > static int cmdq_mbox_flush(struct mbox_chan *chan, unsigned long > timeout) > { > struct cmdq_thread *thread = (struct cmdq_thread *)chan- > >con_priv; > diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h > b/include/linux/mailbox/mtk-cmdq-mailbox.h > index a8f0070c7aa9..f3e577335acb 100644 > --- a/include/linux/mailbox/mtk-cmdq-mailbox.h > +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h > @@ -79,5 +79,6 @@ struct cmdq_pkt { > }; > > u8 cmdq_get_shift_pa(struct mbox_chan *chan); > +void cmdq_mbox_stop(struct mbox_chan *chan); > > #endif /* __MTK_CMDQ_MAILBOX_H__ */
Hi, Jason: On Tue, 2023-09-19 at 03:21 +0800, Jason-JH.Lin wrote: > Add cmdq_pkt_finalize_loop to CMDQ driver. > > cmdq_pkt_finalize_loop appends end of command(EOC) instruction and > jump to start of command buffer instruction to make the command > buffer loopable. > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> > --- > drivers/soc/mediatek/mtk-cmdq-helper.c | 23 +++++++++++++++++++++++ > include/linux/soc/mediatek/mtk-cmdq.h | 8 ++++++++ > 2 files changed, 31 insertions(+) > > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c > b/drivers/soc/mediatek/mtk-cmdq-helper.c > index 4be2a18a4a02..bbb127620bb3 100644 > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c > @@ -475,6 +475,29 @@ int cmdq_pkt_finalize(struct cmdq_pkt *pkt) > } > EXPORT_SYMBOL(cmdq_pkt_finalize); > > +int cmdq_pkt_finalize_loop(struct cmdq_pkt *pkt) > +{ > + struct cmdq_instruction inst = { {0} }; > + int err; > + > + /* insert EOC and generate IRQ for each command iteration */ > + inst.op = CMDQ_CODE_EOC; > + inst.value = CMDQ_EOC_IRQ_EN; > + err = cmdq_pkt_append_command(pkt, inst); > + if (err < 0) > + return err; > + > + /* JUMP to start of pkt */ > + err = cmdq_pkt_jump(pkt, pkt->pa_base); > + if (err < 0) > + return err; Could you explain the case that a loop thread would trigger an interrupt? In DRM crc function, the loop thread need not to trigger interrupt, so I'm curious about this. Regards, CK > + > + pkt->loop = true; > + > + return err; > +} > +EXPORT_SYMBOL(cmdq_pkt_finalize_loop); > + > int cmdq_pkt_flush_async(struct cmdq_pkt *pkt) > { > int err; > diff --git a/include/linux/soc/mediatek/mtk-cmdq.h > b/include/linux/soc/mediatek/mtk-cmdq.h > index 837ad8656adc..38a8e47da338 100644 > --- a/include/linux/soc/mediatek/mtk-cmdq.h > +++ b/include/linux/soc/mediatek/mtk-cmdq.h > @@ -323,6 +323,14 @@ int cmdq_pkt_jump(struct cmdq_pkt *pkt, > dma_addr_t addr); > */ > int cmdq_pkt_finalize(struct cmdq_pkt *pkt); > > +/** > + * cmdq_pkt_finalize_loop() - Append EOC and jump to start command. > + * @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
Hi, Jason: On Tue, 2023-09-19 at 03:22 +0800, Jason-JH.Lin wrote: > Add cmdq_insert_backup_cookie to append some commands before EOC: > 1. Get GCE HW thread execute count from the GCE HW register. > 2. Add 1 to the execute count and then store into a shared memory. > 3. Set a software event siganl as secure irq to GCE HW. > > Since the value of execute count + 1 is stored in a shared memory, > CMDQ driver in the normal world can use it to handle task done in irq > handler and CMDQ driver in the secure world will use it to schedule > the task slot for each secure thread. > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> > --- > drivers/soc/mediatek/mtk-cmdq-helper.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c > b/drivers/soc/mediatek/mtk-cmdq-helper.c > index bbb127620bb3..7b5392878aba 100644 > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c > @@ -8,6 +8,7 @@ > #include <linux/module.h> > #include <linux/mailbox_controller.h> > #include <linux/of.h> > +#include <linux/mailbox/mtk-cmdq-sec-mailbox.h> > #include <linux/soc/mediatek/mtk-cmdq.h> > > #define CMDQ_WRITE_ENABLE_MASK BIT(0) > @@ -153,7 +154,9 @@ void cmdq_pkt_destroy(struct cmdq_pkt *pkt) > > dma_unmap_single(client->chan->mbox->dev, pkt->pa_base, pkt- > >buf_size, > DMA_TO_DEVICE); > + > kfree(pkt->va_base); > + kfree(pkt->sec_data); > kfree(pkt); > } > EXPORT_SYMBOL(cmdq_pkt_destroy); > @@ -458,6 +461,12 @@ int cmdq_pkt_finalize(struct cmdq_pkt *pkt) > struct cmdq_instruction inst = { {0} }; > int err; > > + if (pkt->sec_data) { > + err = cmdq_sec_insert_backup_cookie(pkt); > + if (err < 0) > + return err; > + } Client driver could directly call cmdq_sec_insert_backup_cookie() before call cmdq_pkt_finalize(). I would like helper provide simple API and client driver would integrate simple API to what they want. Regards, CK > + > /* insert EOC and generate IRQ for each command iteration */ > inst.op = CMDQ_CODE_EOC; > inst.value = CMDQ_EOC_IRQ_EN;
Hi, Jason: On Tue, 2023-09-19 at 03:21 +0800, Jason-JH.Lin wrote: > For the Secure Video Path (SVP) feature, inculding the memory stored > secure video content, the registers of display HW pipeline and the > HW configure operations are required to execute in the secure world. > > So using a CMDQ secure driver to make all display HW registers > configuration secure DRAM access permision settings execute by GCE > secure thread in the secure world. > > We are landing this feature on mt8188 and mt8195 currently. I'm doubt that GCE could be secure enough. Hacker would try any way to hack TEE. If the interface is simple, it's easy to check in the interface entry. But GCE command has enormous combination, how to check it's not hacked? One example is that hacker could use cmdq_pkt_read_s() and cmdq_pkt_write_s() to do memory copy and send this packet to secure GCE. Could this memory copy touch secure memory? Or don't worry about this, once hacker find a way to break this, just find a way to fix it? > > Jason-JH.Lin (15): > dt-bindings: mailbox: Add property for CMDQ secure driver > dt-bindings: gce: mt8195: Add CMDQ_SYNC_TOKEN_SECURE_THR_EOF event > id > soc: mailbox: Add SPR definition for GCE > soc: mailbox: Add cmdq_pkt_logic_command to support math operation > mailbox: mediatek: Add cmdq_pkt_write_s_reg_value to CMDQ driver > mailbox: mediatek: Add cmdq_mbox_stop to disable GCE thread > mailbox: mediatek: Add loop pkt flag and irq handling for loop > command > soc: mediatek: Add cmdq_pkt_finalize_loop to CMDQ driver > mailbox: mediatek: Add secure CMDQ driver support for CMDQ driver > mailbox: mediatek: Add CMDQ secure mailbox driver > soc: mediatek: Add cmdq_insert_backup_cookie before EOC for secure > pkt > mailbox: mediatek: Add CMDQ driver support for mt8188 > mailbox: mediatek: Add mt8188 support for CMDQ secure driver > mailbox: mediatek: Add mt8195 support for CMDQ secure driver > arm64: dts: mediatek: mt8195: Add CMDQ secure driver support for > gce0 > > .../mailbox/mediatek,gce-mailbox.yaml | 30 +- > arch/arm64/boot/dts/mediatek/mt8195.dtsi | 2 + > drivers/mailbox/Makefile | 2 +- > drivers/mailbox/mtk-cmdq-mailbox.c | 67 +- > drivers/mailbox/mtk-cmdq-sec-mailbox.c | 1103 > +++++++++++++++++ > drivers/mailbox/mtk-cmdq-sec-tee.c | 202 +++ > drivers/soc/mediatek/mtk-cmdq-helper.c | 81 ++ > include/dt-bindings/gce/mt8195-gce.h | 6 + > include/linux/mailbox/mtk-cmdq-mailbox.h | 15 + > .../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 + > include/linux/soc/mediatek/mtk-cmdq.h | 65 + > 13 files changed, 1971 insertions(+), 9 deletions(-) > 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 >
Hi CK, Thanks for the reviews. On Tue, 2023-09-19 at 01:24 +0000, CK Hu (胡俊光) wrote: > Hi, Jason: > > On Tue, 2023-09-19 at 03:21 +0800, Jason-JH.Lin wrote: > > Add cmdq_mbox_stop to disable GCE thread. > > > > To support the error handling or the stop flow of the GCE loopping > > thread, lopping thread user can call cmdq_mbox_stop to disable the > > GCE HW thread. > > > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> > > --- > > drivers/mailbox/mtk-cmdq-mailbox.c | 6 ++++++ > > include/linux/mailbox/mtk-cmdq-mailbox.h | 1 + > > 2 files changed, 7 insertions(+) > > > > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c > > b/drivers/mailbox/mtk-cmdq-mailbox.c > > index 4d62b07c1411..8bd39fecbf00 100644 > > --- a/drivers/mailbox/mtk-cmdq-mailbox.c > > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c > > @@ -469,6 +469,12 @@ static void cmdq_mbox_shutdown(struct > > mbox_chan > > *chan) > > spin_unlock_irqrestore(&thread->chan->lock, flags); > > } > > > > +void cmdq_mbox_stop(struct mbox_chan *chan) > > +{ > > + cmdq_mbox_shutdown(chan); > > cmdq_mobx_stop() is equal to cmdq_mbox_shutdown(), so client driver > could call mbox_free_channel() to do this and this function is > redundant. > I'vd tried to use cmdq->mbox.ops->shutdown(cmdq->clt->chan) in mtk- cmdq-sec-mbox.c, but it'll call to cmdq_sec_mbox_shutdown(). If I want to call to the cmdq_mbox_shutdown in mtk-cmdq-sec-mailbox.c, I have to find the way to get the mbox of mtk-cmdq-mailbox.c. So I think open a API is easy solution for this. I have called mbox_free_channel() by calling cmdq_mbox_destroy() after cmdq_mbox_stop() in cmdq_sec_irq_notify_start() in mtk-cmdq-sec- mailbox.c. Regards, Jason-JH.Lin > Regards, > CK > > > +} > > +EXPORT_SYMBOL(cmdq_mbox_stop); > > + > > static int cmdq_mbox_flush(struct mbox_chan *chan, unsigned long > > timeout) > > { > > struct cmdq_thread *thread = (struct cmdq_thread *)chan- > > > con_priv; > > > > diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h > > b/include/linux/mailbox/mtk-cmdq-mailbox.h > > index a8f0070c7aa9..f3e577335acb 100644 > > --- a/include/linux/mailbox/mtk-cmdq-mailbox.h > > +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h > > @@ -79,5 +79,6 @@ struct cmdq_pkt { > > }; > > > > u8 cmdq_get_shift_pa(struct mbox_chan *chan); > > +void cmdq_mbox_stop(struct mbox_chan *chan); > > > > #endif /* __MTK_CMDQ_MAILBOX_H__ */
On Tue, 2023-09-19 at 01:38 +0000, CK Hu (胡俊光) wrote: > Hi, Jason: > > On Tue, 2023-09-19 at 03:21 +0800, Jason-JH.Lin wrote: > > Add cmdq_pkt_finalize_loop to CMDQ driver. > > > > cmdq_pkt_finalize_loop appends end of command(EOC) instruction and > > jump to start of command buffer instruction to make the command > > buffer loopable. > > > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> > > --- > > drivers/soc/mediatek/mtk-cmdq-helper.c | 23 > > +++++++++++++++++++++++ > > include/linux/soc/mediatek/mtk-cmdq.h | 8 ++++++++ > > 2 files changed, 31 insertions(+) > > > > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c > > b/drivers/soc/mediatek/mtk-cmdq-helper.c > > index 4be2a18a4a02..bbb127620bb3 100644 > > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c > > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c > > @@ -475,6 +475,29 @@ int cmdq_pkt_finalize(struct cmdq_pkt *pkt) > > } > > EXPORT_SYMBOL(cmdq_pkt_finalize); > > > > +int cmdq_pkt_finalize_loop(struct cmdq_pkt *pkt) > > +{ > > + struct cmdq_instruction inst = { {0} }; > > + int err; > > + > > + /* insert EOC and generate IRQ for each command iteration */ > > + inst.op = CMDQ_CODE_EOC; > > + inst.value = CMDQ_EOC_IRQ_EN; > > + err = cmdq_pkt_append_command(pkt, inst); > > + if (err < 0) > > + return err; > > + > > + /* JUMP to start of pkt */ > > + err = cmdq_pkt_jump(pkt, pkt->pa_base); > > + if (err < 0) > > + return err; > > Could you explain the case that a loop thread would trigger an > interrupt? In DRM crc function, the loop thread need not to trigger > interrupt, so I'm curious about this. > The looping thread in DRM crc funtion is for update CRC value to DRAM during every vblank event coming. It doesn't need to handle the software flow after the EOC. The looping thread in cmdq_sec_irq_notify_start() is waiting for every CMDQ_SYNC_TOKEN_SEC_THR_EOF being set, that means the GCE in secure world has finished all commands in a command buffer. Then it needs a GCE irq to trigger secure mailbox rx_callback() to handle the task of secure cmdq_pkt done in software. Regards, Jason-JH.Lin > Regards, > CK > > > + > > + pkt->loop = true; > > + > > + return err; > > +} > > +EXPORT_SYMBOL(cmdq_pkt_finalize_loop); > > + > > int cmdq_pkt_flush_async(struct cmdq_pkt *pkt) > > { > > int err; > > diff --git a/include/linux/soc/mediatek/mtk-cmdq.h > > b/include/linux/soc/mediatek/mtk-cmdq.h > > index 837ad8656adc..38a8e47da338 100644 > > --- a/include/linux/soc/mediatek/mtk-cmdq.h > > +++ b/include/linux/soc/mediatek/mtk-cmdq.h > > @@ -323,6 +323,14 @@ int cmdq_pkt_jump(struct cmdq_pkt *pkt, > > dma_addr_t addr); > > */ > > int cmdq_pkt_finalize(struct cmdq_pkt *pkt); > > > > +/** > > + * cmdq_pkt_finalize_loop() - Append EOC and jump to start > > command. > > + * @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
Hi CK, Thanks for the reivews. On Tue, 2023-09-19 at 03:04 +0000, CK Hu (胡俊光) wrote: > Hi, Jason: > > On Tue, 2023-09-19 at 03:22 +0800, Jason-JH.Lin wrote: > > Add cmdq_insert_backup_cookie to append some commands before EOC: > > 1. Get GCE HW thread execute count from the GCE HW register. > > 2. Add 1 to the execute count and then store into a shared memory. > > 3. Set a software event siganl as secure irq to GCE HW. > > > > Since the value of execute count + 1 is stored in a shared memory, > > CMDQ driver in the normal world can use it to handle task done in > > irq > > handler and CMDQ driver in the secure world will use it to schedule > > the task slot for each secure thread. > > > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> > > --- > > drivers/soc/mediatek/mtk-cmdq-helper.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c > > b/drivers/soc/mediatek/mtk-cmdq-helper.c > > index bbb127620bb3..7b5392878aba 100644 > > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c > > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c > > @@ -8,6 +8,7 @@ > > #include <linux/module.h> > > #include <linux/mailbox_controller.h> > > #include <linux/of.h> > > +#include <linux/mailbox/mtk-cmdq-sec-mailbox.h> > > #include <linux/soc/mediatek/mtk-cmdq.h> > > > > #define CMDQ_WRITE_ENABLE_MASK BIT(0) > > @@ -153,7 +154,9 @@ void cmdq_pkt_destroy(struct cmdq_pkt *pkt) > > > > dma_unmap_single(client->chan->mbox->dev, pkt->pa_base, pkt- > > > buf_size, > > > > DMA_TO_DEVICE); > > + > > kfree(pkt->va_base); > > + kfree(pkt->sec_data); > > kfree(pkt); > > } > > EXPORT_SYMBOL(cmdq_pkt_destroy); > > @@ -458,6 +461,12 @@ int cmdq_pkt_finalize(struct cmdq_pkt *pkt) > > struct cmdq_instruction inst = { {0} }; > > int err; > > > > + if (pkt->sec_data) { > > + err = cmdq_sec_insert_backup_cookie(pkt); > > + if (err < 0) > > + return err; > > + } > > Client driver could directly call cmdq_sec_insert_backup_cookie() > before call cmdq_pkt_finalize(). I would like helper provide simple > API > and client driver would integrate simple API to what they want. > OK, I'll move this to the client driver. Thanks. Regards, Jason-JH.Lin > Regards, > CK > > > + > > /* insert EOC and generate IRQ for each command iteration */ > > inst.op = CMDQ_CODE_EOC; > > inst.value = CMDQ_EOC_IRQ_EN;
Hi CK, On Wed, 2023-09-20 at 03:08 +0000, CK Hu (胡俊光) wrote: > Hi, Jason: > > On Tue, 2023-09-19 at 03:21 +0800, Jason-JH.Lin wrote: > > For the Secure Video Path (SVP) feature, inculding the memory > > stored > > secure video content, the registers of display HW pipeline and the > > HW configure operations are required to execute in the secure > > world. > > > > So using a CMDQ secure driver to make all display HW registers > > configuration secure DRAM access permision settings execute by GCE > > secure thread in the secure world. > > > > We are landing this feature on mt8188 and mt8195 currently. > > I'm doubt that GCE could be secure enough. Hacker would try any way > to > hack TEE. If the interface is simple, it's easy to check in the > interface entry. But GCE command has enormous combination, how to > check > it's not hacked? One example is that hacker could use > cmdq_pkt_read_s() > and cmdq_pkt_write_s() to do memory copy and send this packet to > secure > GCE. Could this memory copy touch secure memory? Or don't worry > about > this, once hacker find a way to break this, just find a way to fix > it? We have put a lot protection mechanism in the secure world to avoid the secure video content reveal from our SoC. The hackers may try to add some commands in to cmdq secure pkt, but we'll check all the commands in secure world for every secure pkt and find out the invalid one with whitelist registers checking, DAPC HW protection for write instruction to DRAM with configuring WDMA, larb secure protection for read secure DRAM from normal world, checking read_s instructions for memory copy to an invalid DRAM addr,etc. Maybe hackers would find the way to break the video playback, but they can not find the way to steal the secure video content by adding a commands into secure cmdq pkt. Regards, Jason-JH.Lin > > > > > Jason-JH.Lin (15): > > dt-bindings: mailbox: Add property for CMDQ secure driver > > dt-bindings: gce: mt8195: Add CMDQ_SYNC_TOKEN_SECURE_THR_EOF > > event > > id > > soc: mailbox: Add SPR definition for GCE > > soc: mailbox: Add cmdq_pkt_logic_command to support math > > operation > > mailbox: mediatek: Add cmdq_pkt_write_s_reg_value to CMDQ driver > > mailbox: mediatek: Add cmdq_mbox_stop to disable GCE thread > > mailbox: mediatek: Add loop pkt flag and irq handling for loop > > command > > soc: mediatek: Add cmdq_pkt_finalize_loop to CMDQ driver > > mailbox: mediatek: Add secure CMDQ driver support for CMDQ driver > > mailbox: mediatek: Add CMDQ secure mailbox driver > > soc: mediatek: Add cmdq_insert_backup_cookie before EOC for > > secure > > pkt > > mailbox: mediatek: Add CMDQ driver support for mt8188 > > mailbox: mediatek: Add mt8188 support for CMDQ secure driver > > mailbox: mediatek: Add mt8195 support for CMDQ secure driver > > arm64: dts: mediatek: mt8195: Add CMDQ secure driver support for > > gce0 > > > > .../mailbox/mediatek,gce-mailbox.yaml | 30 +- > > arch/arm64/boot/dts/mediatek/mt8195.dtsi | 2 + > > drivers/mailbox/Makefile | 2 +- > > drivers/mailbox/mtk-cmdq-mailbox.c | 67 +- > > drivers/mailbox/mtk-cmdq-sec-mailbox.c | 1103 > > +++++++++++++++++ > > drivers/mailbox/mtk-cmdq-sec-tee.c | 202 +++ > > drivers/soc/mediatek/mtk-cmdq-helper.c | 81 ++ > > include/dt-bindings/gce/mt8195-gce.h | 6 + > > include/linux/mailbox/mtk-cmdq-mailbox.h | 15 + > > .../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 + > > include/linux/soc/mediatek/mtk-cmdq.h | 65 + > > 13 files changed, 1971 insertions(+), 9 deletions(-) > > 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 > >
On 18/09/2023 21:21, Jason-JH.Lin wrote: > GCE has specific purpose registers, abbreviated as SPR. > Client can us SPR to store data or programs. > > In CMDQ driver, it allows client to STORE or LOAD data into SPR. > The value stored in SPR will be cleared after reset GCE HW thread. > > There are 4 SPR (register index 0 - 3) in every GCE HW thread. > SPR is thread-independent and HW secure protected. > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> > --- > include/linux/soc/mediatek/mtk-cmdq.h | 5 +++++ There is no user of this... Why do you add unused defines? Best regards, Krzysztof
On 18/09/2023 21:21, Jason-JH.Lin wrote: > Add cmdq_mbox_stop to disable GCE thread. > > To support the error handling or the stop flow of the GCE loopping > thread, lopping thread user can call cmdq_mbox_stop to disable the > GCE HW thread. > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> > --- > drivers/mailbox/mtk-cmdq-mailbox.c | 6 ++++++ > include/linux/mailbox/mtk-cmdq-mailbox.h | 1 + > 2 files changed, 7 insertions(+) > > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c > index 4d62b07c1411..8bd39fecbf00 100644 > --- a/drivers/mailbox/mtk-cmdq-mailbox.c > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c > @@ -469,6 +469,12 @@ static void cmdq_mbox_shutdown(struct mbox_chan *chan) > spin_unlock_irqrestore(&thread->chan->lock, flags); > } > > +void cmdq_mbox_stop(struct mbox_chan *chan) > +{ > + cmdq_mbox_shutdown(chan); > +} > +EXPORT_SYMBOL(cmdq_mbox_stop); 1. EXPORT_SYMBOL_GPL 2. Missing kernel doc (full doc) Best regards, Krzysztof
On 18/09/2023 21:21, Jason-JH.Lin wrote: > CMDQ client can use a loop flag for the CMDQ packet to make current > command buffer jumps to the beginning when GCE executes to the end > of commands buffer. > > GCE irq occurs when GCE executes to the end of command instruction. > If the CMDQ packet is a loopping command, GCE irq handler can not > delete the CMDQ task and disable the GCE thread. > > Add cmdq_mbox_stop to support thread disable How or where do you add it? I do not see it in this patch. Your patchset looks randomly organized. Best regards, Krzysztof
On 18/09/2023 21:21, Jason-JH.Lin wrote: > Add cmdq_pkt_finalize_loop to CMDQ driver. > > cmdq_pkt_finalize_loop appends end of command(EOC) instruction and > jump to start of command buffer instruction to make the command > buffer loopable. > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> > --- > drivers/soc/mediatek/mtk-cmdq-helper.c | 23 +++++++++++++++++++++++ > include/linux/soc/mediatek/mtk-cmdq.h | 8 ++++++++ > 2 files changed, 31 insertions(+) > > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c > index 4be2a18a4a02..bbb127620bb3 100644 > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c > @@ -475,6 +475,29 @@ int cmdq_pkt_finalize(struct cmdq_pkt *pkt) > } > EXPORT_SYMBOL(cmdq_pkt_finalize); > > +int cmdq_pkt_finalize_loop(struct cmdq_pkt *pkt) > +{ > + struct cmdq_instruction inst = { {0} }; > + int err; > + > + /* insert EOC and generate IRQ for each command iteration */ > + inst.op = CMDQ_CODE_EOC; > + inst.value = CMDQ_EOC_IRQ_EN; > + err = cmdq_pkt_append_command(pkt, inst); > + if (err < 0) > + return err; > + > + /* JUMP to start of pkt */ > + err = cmdq_pkt_jump(pkt, pkt->pa_base); > + if (err < 0) > + return err; > + > + pkt->loop = true; > + > + return err; > +} > +EXPORT_SYMBOL(cmdq_pkt_finalize_loop); 1. Missing GPL 2. Missing kerneldoc 3. Missing user 4. Are you going to split the patchset into one function per patch? No. Best regards, Krzysztof
On 18/09/2023 21:21, Jason-JH.Lin wrote: > Add cmdq_mbox_stop to disable GCE thread. > > To support the error handling or the stop flow of the GCE loopping > thread, lopping thread user can call cmdq_mbox_stop to disable the > GCE HW thread. > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> > --- > drivers/mailbox/mtk-cmdq-mailbox.c | 6 ++++++ > include/linux/mailbox/mtk-cmdq-mailbox.h | 1 + > 2 files changed, 7 insertions(+) > > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c > index 4d62b07c1411..8bd39fecbf00 100644 > --- a/drivers/mailbox/mtk-cmdq-mailbox.c > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c > @@ -469,6 +469,12 @@ static void cmdq_mbox_shutdown(struct mbox_chan *chan) > spin_unlock_irqrestore(&thread->chan->lock, flags); > } > > +void cmdq_mbox_stop(struct mbox_chan *chan) > +{ > + cmdq_mbox_shutdown(chan); > +} > +EXPORT_SYMBOL(cmdq_mbox_stop); Plus there are no users. NAK. This is not code which should be posted upstream. Best regards, Krzysztof
On 18/09/2023 21:21, Jason-JH.Lin wrote: > Add cmdq_pkt_finalize_loop to CMDQ driver. > > cmdq_pkt_finalize_loop appends end of command(EOC) instruction and > jump to start of command buffer instruction to make the command > buffer loopable. > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> > --- > drivers/soc/mediatek/mtk-cmdq-helper.c | 23 +++++++++++++++++++++++ > include/linux/soc/mediatek/mtk-cmdq.h | 8 ++++++++ > 2 files changed, 31 insertions(+) > > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c > index 4be2a18a4a02..bbb127620bb3 100644 > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c > @@ -475,6 +475,29 @@ int cmdq_pkt_finalize(struct cmdq_pkt *pkt) > } > EXPORT_SYMBOL(cmdq_pkt_finalize); > > +int cmdq_pkt_finalize_loop(struct cmdq_pkt *pkt) > +{ > + struct cmdq_instruction inst = { {0} }; > + int err; > + > + /* insert EOC and generate IRQ for each command iteration */ > + inst.op = CMDQ_CODE_EOC; > + inst.value = CMDQ_EOC_IRQ_EN; > + err = cmdq_pkt_append_command(pkt, inst); > + if (err < 0) > + return err; > + > + /* JUMP to start of pkt */ > + err = cmdq_pkt_jump(pkt, pkt->pa_base); > + if (err < 0) > + return err; > + > + pkt->loop = true; > + > + return err; > +} > +EXPORT_SYMBOL(cmdq_pkt_finalize_loop); NAK. No users (and please carefully think before you answer that your other patch uses it). Best regards, Krzysztof
On 18/09/2023 21:22, Jason-JH.Lin wrote: > Add mt8195 support for CMDQ secure driver. How is it anyhow related to your patch content? > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> > --- > drivers/mailbox/mtk-cmdq-mailbox.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c > index 4e047dc916b9..d27d033c587d 100644 > --- a/drivers/mailbox/mtk-cmdq-mailbox.c > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c > @@ -735,6 +735,7 @@ static const struct gce_plat gce_plat_v6 = { > .thread_nr = 24, > .shift = 3, > .control_by_sw = true, > + .has_sec = true, Really, how? Best regards, Krzysztof
On 18/09/2023 21:22, Jason-JH.Lin wrote: > Add mt8188 support for CMDQ secure driver. > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> > --- > drivers/mailbox/mtk-cmdq-mailbox.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c > index 3940b9f8e774..4e047dc916b9 100644 > --- a/drivers/mailbox/mtk-cmdq-mailbox.c > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c > @@ -750,6 +750,7 @@ static const struct gce_plat gce_plat_v8 = { > .thread_nr = 32, > .shift = 3, > .control_by_sw = true, > + .has_sec = true, No, you just added it patch ago. Do not add broken code and fix it. Are there some KPIs in Mediatek to have patch count? Best regards, Krzysztof
Hi Krzysztof, Thanks for the reviews. On Sat, 2023-09-23 at 20:02 +0200, Krzysztof Kozlowski wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > On 18/09/2023 21:21, Jason-JH.Lin wrote: > > GCE has specific purpose registers, abbreviated as SPR. > > Client can us SPR to store data or programs. > > > > In CMDQ driver, it allows client to STORE or LOAD data into SPR. > > The value stored in SPR will be cleared after reset GCE HW thread. > > > > There are 4 SPR (register index 0 - 3) in every GCE HW thread. > > SPR is thread-independent and HW secure protected. > > > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> > > --- > > include/linux/soc/mediatek/mtk-cmdq.h | 5 +++++ > > There is no user of this... Why do you add unused defines? It'll be used in cmdq_sec_insert_backup_cookie() at [PATCH 10/15]. Should I merge this patch into [PATCH 10/15]? Regards, Jason-JH.Lin > > Best regards, > Krzysztof > >
Hi Krzysztof, Thanks for the review. On Sat, 2023-09-23 at 20:02 +0200, Krzysztof Kozlowski wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > On 18/09/2023 21:21, Jason-JH.Lin wrote: > > Add cmdq_mbox_stop to disable GCE thread. > > > > To support the error handling or the stop flow of the GCE loopping > > thread, lopping thread user can call cmdq_mbox_stop to disable the > > GCE HW thread. > > > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> > > --- > > drivers/mailbox/mtk-cmdq-mailbox.c | 6 ++++++ > > include/linux/mailbox/mtk-cmdq-mailbox.h | 1 + > > 2 files changed, 7 insertions(+) > > > > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c > b/drivers/mailbox/mtk-cmdq-mailbox.c > > index 4d62b07c1411..8bd39fecbf00 100644 > > --- a/drivers/mailbox/mtk-cmdq-mailbox.c > > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c > > @@ -469,6 +469,12 @@ static void cmdq_mbox_shutdown(struct > mbox_chan *chan) > > spin_unlock_irqrestore(&thread->chan->lock, flags); > > } > > > > +void cmdq_mbox_stop(struct mbox_chan *chan) > > +{ > > +cmdq_mbox_shutdown(chan); > > +} > > +EXPORT_SYMBOL(cmdq_mbox_stop); > > 1. EXPORT_SYMBOL_GPL > 2. Missing kernel doc (full doc) > > After reviewing by CK, I think this patch should be dropped. Sorry for bothering you and thanks for the reviews. Regards, Jason-JH.Lin > > Best regards, > Krzysztof > >
Hi Krzysztof, Thanks for the reviews. On Sat, 2023-09-23 at 20:03 +0200, Krzysztof Kozlowski wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > On 18/09/2023 21:21, Jason-JH.Lin wrote: > > CMDQ client can use a loop flag for the CMDQ packet to make current > > command buffer jumps to the beginning when GCE executes to the end > > of commands buffer. > > > > GCE irq occurs when GCE executes to the end of command instruction. > > If the CMDQ packet is a loopping command, GCE irq handler can not > > delete the CMDQ task and disable the GCE thread. > > > > Add cmdq_mbox_stop to support thread disable > > How or where do you add it? I do not see it in this patch. Your > patchset > looks randomly organized. This will be used in cmdq_pkt_finialize_loop() at [PATCH 8/15]. mtk-cmdq-helper.c and mtk-cmdq-mailbox.c are not in the same maintainer's tree, so I separate this to another patch from [PATCH 8/15]. Regards, Jason-JH.Lin > > Best regards, > Krzysztof > >
Hi Krzysztof, Thanks for the reviews. On Sat, 2023-09-23 at 20:07 +0200, Krzysztof Kozlowski wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > On 18/09/2023 21:21, Jason-JH.Lin wrote: > > Add cmdq_mbox_stop to disable GCE thread. > > > > To support the error handling or the stop flow of the GCE loopping > > thread, lopping thread user can call cmdq_mbox_stop to disable the > > GCE HW thread. > > > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> > > --- > > drivers/mailbox/mtk-cmdq-mailbox.c | 6 ++++++ > > include/linux/mailbox/mtk-cmdq-mailbox.h | 1 + > > 2 files changed, 7 insertions(+) > > > > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c > b/drivers/mailbox/mtk-cmdq-mailbox.c > > index 4d62b07c1411..8bd39fecbf00 100644 > > --- a/drivers/mailbox/mtk-cmdq-mailbox.c > > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c > > @@ -469,6 +469,12 @@ static void cmdq_mbox_shutdown(struct > mbox_chan *chan) > > spin_unlock_irqrestore(&thread->chan->lock, flags); > > } > > > > +void cmdq_mbox_stop(struct mbox_chan *chan) > > +{ > > +cmdq_mbox_shutdown(chan); > > +} > > +EXPORT_SYMBOL(cmdq_mbox_stop); > > Plus there are no users. > > NAK. This is not code which should be posted upstream. > It'll be used in cmdq_sec_irq_notify_start() at [PATCH 10/15]. After reviewing by CK, I think I'll try to drop this patch. Regards, Jason-JH.Lin > Best regards, > Krzysztof > >
Hi Krzysztof, Thanks for the reviews. On Sat, 2023-09-23 at 20:08 +0200, Krzysztof Kozlowski wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > On 18/09/2023 21:22, Jason-JH.Lin wrote: > > Add mt8195 support for CMDQ secure driver. > > How is it anyhow related to your patch content? This path is dependent on cmdq_probe() in [PATCH 9/15]. > > > > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> > > --- > > drivers/mailbox/mtk-cmdq-mailbox.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c > b/drivers/mailbox/mtk-cmdq-mailbox.c > > index 4e047dc916b9..d27d033c587d 100644 > > --- a/drivers/mailbox/mtk-cmdq-mailbox.c > > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c > > @@ -735,6 +735,7 @@ static const struct gce_plat gce_plat_v6 = { > > .thread_nr = 24, > > .shift = 3, > > .control_by_sw = true, > > +.has_sec = true, > > Really, how? Within this flag, cmdq_probe() will call platform_device_register_data() to setup a CMDQ secure driver. Regards, Jason-JH.Lin > > Best regards, > Krzysztof > >
Hi Krzysztof, On Sat, 2023-09-23 at 20:09 +0200, Krzysztof Kozlowski wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > On 18/09/2023 21:22, Jason-JH.Lin wrote: > > Add mt8188 support for CMDQ secure driver. > > > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> > > --- > > drivers/mailbox/mtk-cmdq-mailbox.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c > b/drivers/mailbox/mtk-cmdq-mailbox.c > > index 3940b9f8e774..4e047dc916b9 100644 > > --- a/drivers/mailbox/mtk-cmdq-mailbox.c > > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c > > @@ -750,6 +750,7 @@ static const struct gce_plat gce_plat_v8 = { > > .thread_nr = 32, > > .shift = 3, > > .control_by_sw = true, > > +.has_sec = true, > > No, you just added it patch ago. Do not add broken code and fix it. > Are > there some KPIs in Mediatek to have patch count? > This patch is different from [PATCH 14/15] at the gce_plat: [PATCH 13/15] is adding the flag to gce_plat_v8 for mediatek,mt8188-gce [PATCH 14/15] is adding the flag to gce_plat_v6 for mediatek,mt8195-gce I'm sorry about that gce_plat are too similar to cause the confusion. I'vd built the whole series before sending it, so I think it won't break the code and I think there are no KPIs on the patch count. Should I merge [PATCH 13/15] and [PATCH 14/15] in to [PATCH 9/15] to show how it works? Regards, Jason-JH.Lin > Best regards, > Krzysztof > >
Hi Krzysztof, Thanks for the reviews. On Sat, 2023-09-23 at 20:08 +0200, Krzysztof Kozlowski wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > On 18/09/2023 21:21, Jason-JH.Lin wrote: > > Add cmdq_pkt_finalize_loop to CMDQ driver. > > > > cmdq_pkt_finalize_loop appends end of command(EOC) instruction and > > jump to start of command buffer instruction to make the command > > buffer loopable. > > > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> > > --- > > drivers/soc/mediatek/mtk-cmdq-helper.c | 23 > +++++++++++++++++++++++ > > include/linux/soc/mediatek/mtk-cmdq.h | 8 ++++++++ > > 2 files changed, 31 insertions(+) > > > > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c > b/drivers/soc/mediatek/mtk-cmdq-helper.c > > index 4be2a18a4a02..bbb127620bb3 100644 > > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c > > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c > > @@ -475,6 +475,29 @@ int cmdq_pkt_finalize(struct cmdq_pkt *pkt) > > } > > EXPORT_SYMBOL(cmdq_pkt_finalize); > > > > +int cmdq_pkt_finalize_loop(struct cmdq_pkt *pkt) > > +{ > > +struct cmdq_instruction inst = { {0} }; > > +int err; > > + > > +/* insert EOC and generate IRQ for each command iteration */ > > +inst.op = CMDQ_CODE_EOC; > > +inst.value = CMDQ_EOC_IRQ_EN; > > +err = cmdq_pkt_append_command(pkt, inst); > > +if (err < 0) > > +return err; > > + > > +/* JUMP to start of pkt */ > > +err = cmdq_pkt_jump(pkt, pkt->pa_base); > > +if (err < 0) > > +return err; > > + > > +pkt->loop = true; > > + > > +return err; > > +} > > +EXPORT_SYMBOL(cmdq_pkt_finalize_loop); > > NAK. No users (and please carefully think before you answer that your > other patch uses it). > As I know, the API with EXPORT_SYMBOL means it can be used by a dynamic loaded module. Do you means that mtk-cmdq-sec-mailbox.c in [PATCH 10/15] is a built-in module currently, so I should not add EXPORT_SYMBOL to this API? Regards, Jason-JH.Lin > Best regards, > Krzysztof > >
On 25/09/2023 08:04, Jason-JH Lin (林睿祥) wrote: > Hi Krzysztof, > > Thanks for the reviews. > > On Sat, 2023-09-23 at 20:08 +0200, Krzysztof Kozlowski wrote: >> >> External email : Please do not click links or open attachments until >> you have verified the sender or the content. >> On 18/09/2023 21:21, Jason-JH.Lin wrote: >>> Add cmdq_pkt_finalize_loop to CMDQ driver. >>> >>> cmdq_pkt_finalize_loop appends end of command(EOC) instruction and >>> jump to start of command buffer instruction to make the command >>> buffer loopable. >>> >>> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> >>> --- >>> drivers/soc/mediatek/mtk-cmdq-helper.c | 23 >> +++++++++++++++++++++++ >>> include/linux/soc/mediatek/mtk-cmdq.h | 8 ++++++++ >>> 2 files changed, 31 insertions(+) >>> >>> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c >> b/drivers/soc/mediatek/mtk-cmdq-helper.c >>> index 4be2a18a4a02..bbb127620bb3 100644 >>> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c >>> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c >>> @@ -475,6 +475,29 @@ int cmdq_pkt_finalize(struct cmdq_pkt *pkt) >>> } >>> EXPORT_SYMBOL(cmdq_pkt_finalize); >>> >>> +int cmdq_pkt_finalize_loop(struct cmdq_pkt *pkt) >>> +{ >>> +struct cmdq_instruction inst = { {0} }; >>> +int err; >>> + >>> +/* insert EOC and generate IRQ for each command iteration */ >>> +inst.op = CMDQ_CODE_EOC; >>> +inst.value = CMDQ_EOC_IRQ_EN; >>> +err = cmdq_pkt_append_command(pkt, inst); >>> +if (err < 0) >>> +return err; >>> + >>> +/* JUMP to start of pkt */ >>> +err = cmdq_pkt_jump(pkt, pkt->pa_base); >>> +if (err < 0) >>> +return err; >>> + >>> +pkt->loop = true; >>> + >>> +return err; >>> +} >>> +EXPORT_SYMBOL(cmdq_pkt_finalize_loop); >> >> NAK. No users (and please carefully think before you answer that your >> other patch uses it). >> > > As I know, the API with EXPORT_SYMBOL means it can be used by a dynamic > loaded module. > > Do you means that mtk-cmdq-sec-mailbox.c in [PATCH 10/15] is a built-in > module currently, so I should not add EXPORT_SYMBOL to this API? I mean there is no need for this. Please name the name of module where this function will be defined and name of module(s) using it. ".c" is not a module. ".ko" is. Best regards, Krzysztof
On 25/09/2023 07:08, Jason-JH Lin (林睿祥) wrote: > Hi Krzysztof, > > Thanks for the reviews. > > On Sat, 2023-09-23 at 20:02 +0200, Krzysztof Kozlowski wrote: >> >> External email : Please do not click links or open attachments until >> you have verified the sender or the content. >> On 18/09/2023 21:21, Jason-JH.Lin wrote: >>> GCE has specific purpose registers, abbreviated as SPR. >>> Client can us SPR to store data or programs. >>> >>> In CMDQ driver, it allows client to STORE or LOAD data into SPR. >>> The value stored in SPR will be cleared after reset GCE HW thread. >>> >>> There are 4 SPR (register index 0 - 3) in every GCE HW thread. >>> SPR is thread-independent and HW secure protected. >>> >>> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> >>> --- >>> include/linux/soc/mediatek/mtk-cmdq.h | 5 +++++ >> >> There is no user of this... Why do you add unused defines? > > It'll be used in cmdq_sec_insert_backup_cookie() at [PATCH 10/15]. > Should I merge this patch into [PATCH 10/15]? Yes, because what is the purpose of adding unused defines? I asked before and did not get answer... Best regards, Krzysztof
On 25/09/2023 07:21, Jason-JH Lin (林睿祥) wrote: > Hi Krzysztof, > > Thanks for the reviews. > > On Sat, 2023-09-23 at 20:03 +0200, Krzysztof Kozlowski wrote: >> >> External email : Please do not click links or open attachments until >> you have verified the sender or the content. >> On 18/09/2023 21:21, Jason-JH.Lin wrote: >>> CMDQ client can use a loop flag for the CMDQ packet to make current >>> command buffer jumps to the beginning when GCE executes to the end >>> of commands buffer. >>> >>> GCE irq occurs when GCE executes to the end of command instruction. >>> If the CMDQ packet is a loopping command, GCE irq handler can not >>> delete the CMDQ task and disable the GCE thread. >>> >>> Add cmdq_mbox_stop to support thread disable >> >> How or where do you add it? I do not see it in this patch. Your >> patchset >> looks randomly organized. > > This will be used in cmdq_pkt_finialize_loop() at [PATCH 8/15]. > > mtk-cmdq-helper.c and mtk-cmdq-mailbox.c are not in the > same maintainer's tree, so I separate this to another patch from [PATCH > 8/15]. Why? Anyway it has to go through same tree. You have dependencies. Such artificial split makes it only difficult to review and understand. Re-organize your patchset to be correctly split per each logical feature/change. Split per subsystems is not the same. Best regards, Krzysztof
On 25/09/2023 08:01, Jason-JH Lin (林睿祥) wrote: > Hi Krzysztof, > > On Sat, 2023-09-23 at 20:09 +0200, Krzysztof Kozlowski wrote: >> >> External email : Please do not click links or open attachments until >> you have verified the sender or the content. >> On 18/09/2023 21:22, Jason-JH.Lin wrote: >>> Add mt8188 support for CMDQ secure driver. >>> >>> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> >>> --- >>> drivers/mailbox/mtk-cmdq-mailbox.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c >> b/drivers/mailbox/mtk-cmdq-mailbox.c >>> index 3940b9f8e774..4e047dc916b9 100644 >>> --- a/drivers/mailbox/mtk-cmdq-mailbox.c >>> +++ b/drivers/mailbox/mtk-cmdq-mailbox.c >>> @@ -750,6 +750,7 @@ static const struct gce_plat gce_plat_v8 = { >>> .thread_nr = 32, >>> .shift = 3, >>> .control_by_sw = true, >>> +.has_sec = true, >> >> No, you just added it patch ago. Do not add broken code and fix it. >> Are >> there some KPIs in Mediatek to have patch count? >> > > This patch is different from [PATCH 14/15] at the gce_plat: > [PATCH 13/15] is adding the flag to gce_plat_v8 for mediatek,mt8188-gce > [PATCH 14/15] is adding the flag to gce_plat_v6 for mediatek,mt8195-gce ??? I talked about patch 12! Why do you add incomplete code? Best regards, Krzysztof
On Mon, 2023-09-25 at 08:45 +0200, Krzysztof Kozlowski wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > On 25/09/2023 08:01, Jason-JH Lin (林睿祥) wrote: > > Hi Krzysztof, > > > > On Sat, 2023-09-23 at 20:09 +0200, Krzysztof Kozlowski wrote: > >> > >> External email : Please do not click links or open attachments > until > >> you have verified the sender or the content. > >> On 18/09/2023 21:22, Jason-JH.Lin wrote: > >>> Add mt8188 support for CMDQ secure driver. > >>> > >>> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> > >>> --- > >>> drivers/mailbox/mtk-cmdq-mailbox.c | 1 + > >>> 1 file changed, 1 insertion(+) > >>> > >>> diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c > >> b/drivers/mailbox/mtk-cmdq-mailbox.c > >>> index 3940b9f8e774..4e047dc916b9 100644 > >>> --- a/drivers/mailbox/mtk-cmdq-mailbox.c > >>> +++ b/drivers/mailbox/mtk-cmdq-mailbox.c > >>> @@ -750,6 +750,7 @@ static const struct gce_plat gce_plat_v8 = { > >>> .thread_nr = 32, > >>> .shift = 3, > >>> .control_by_sw = true, > >>> +.has_sec = true, > >> > >> No, you just added it patch ago. Do not add broken code and fix > it. > >> Are > >> there some KPIs in Mediatek to have patch count? > >> > > > > This patch is different from [PATCH 14/15] at the gce_plat: > > [PATCH 13/15] is adding the flag to gce_plat_v8 for > mediatek,mt8188-gce > > [PATCH 14/15] is adding the flag to gce_plat_v6 for > mediatek,mt8195-gce > ??? > > I talked about patch 12! Why do you add incomplete code? Do you mean I should merge the patch 12 and 13? I think separating the new supported mt8188 and the new supported secure feature in different patch can make the CMDQ user know how to make a CMDQ mailbox driver support secure feature. Since mt8188 is a new supported IC for mailbox, so merging patch 12 and 13 into the same patch is OK. I'll merge them at the next version. Thanks. Regards, Jason-JH.Lin > > > Best regards, > Krzysztof > >
On Mon, 2023-09-25 at 08:42 +0200, Krzysztof Kozlowski wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > On 25/09/2023 07:08, Jason-JH Lin (林睿祥) wrote: > > Hi Krzysztof, > > > > Thanks for the reviews. > > > > On Sat, 2023-09-23 at 20:02 +0200, Krzysztof Kozlowski wrote: > >> > >> External email : Please do not click links or open attachments > until > >> you have verified the sender or the content. > >> On 18/09/2023 21:21, Jason-JH.Lin wrote: > >>> GCE has specific purpose registers, abbreviated as SPR. > >>> Client can us SPR to store data or programs. > >>> > >>> In CMDQ driver, it allows client to STORE or LOAD data into SPR. > >>> The value stored in SPR will be cleared after reset GCE HW > thread. > >>> > >>> There are 4 SPR (register index 0 - 3) in every GCE HW thread. > >>> SPR is thread-independent and HW secure protected. > >>> > >>> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> > >>> --- > >>> include/linux/soc/mediatek/mtk-cmdq.h | 5 +++++ > >> > >> There is no user of this... Why do you add unused defines? > > > > It'll be used in cmdq_sec_insert_backup_cookie() at [PATCH 10/15]. > > Should I merge this patch into [PATCH 10/15]? > > Yes, because what is the purpose of adding unused defines? I asked > before and did not get answer... > I'm totally agree with merging this patch to the usage parts of mtk- cmdq-sec-mailbox.c. But I have no idea why mtk-cmdq-sec-mailbox.c and mtk-cmdq-mailbox.c are not placed in the same maintainer's tree as mtk-cmdq.h and mtk- cmdq-helper.c, so I just separate them to different patch by tree like the requirement from previous sent series. I will re-organized this series to make the definition and the usage of the code in the same patch. Regards, Jason-JH.Lin > Best regards, > Krzysztof > >
On Mon, 2023-09-25 at 08:44 +0200, Krzysztof Kozlowski wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > On 25/09/2023 07:21, Jason-JH Lin (林睿祥) wrote: > > Hi Krzysztof, > > > > Thanks for the reviews. > > > > On Sat, 2023-09-23 at 20:03 +0200, Krzysztof Kozlowski wrote: > >> > >> External email : Please do not click links or open attachments > until > >> you have verified the sender or the content. > >> On 18/09/2023 21:21, Jason-JH.Lin wrote: > >>> CMDQ client can use a loop flag for the CMDQ packet to make > current > >>> command buffer jumps to the beginning when GCE executes to the > end > >>> of commands buffer. > >>> > >>> GCE irq occurs when GCE executes to the end of command > instruction. > >>> If the CMDQ packet is a loopping command, GCE irq handler can not > >>> delete the CMDQ task and disable the GCE thread. > >>> > >>> Add cmdq_mbox_stop to support thread disable > >> > >> How or where do you add it? I do not see it in this patch. Your > >> patchset > >> looks randomly organized. > > > > This will be used in cmdq_pkt_finialize_loop() at [PATCH 8/15]. > > > > mtk-cmdq-helper.c and mtk-cmdq-mailbox.c are not in the > > same maintainer's tree, so I separate this to another patch from > [PATCH > > 8/15]. > > Why? Anyway it has to go through same tree. You have dependencies. > Such > artificial split makes it only difficult to review and understand. > Re-organize your patchset to be correctly split per each logical > feature/change. Split per subsystems is not the same. > Yes, these related files are in the different maintainer's tree. Refer to https://www.kernel.org/doc/linux/MAINTAINERS MAILBOX API M: Jassi Brar F: drivers/mailbox/ - drivers/mailbox/mtk-cmdq-mailbox.c - drivers/mailbox/mtk-cmdq-sec- mailbox.c ARM/Mediatek SoC support M: Matthias Brugger F: drivers/soc/mediatek/ K: mediatek - drivers/soc/mediatek/mtk-cmdq-helper.c - include/linux/soc/mediatek/mtk-cmdq.h I think we should add a new MAINTAINER label for mediatek CMDQ mailbox and put these files together, such as "MAILBOX ARM MHUv2" and "QUALCOM IPCC MAILBOX DRIVER". But I don't know how to make a request for that. Anyway, I'll squash this logical feature to the same patch, no matter these files are not in the same tree. Regards, Jason-JH.Lin > Best regards, > Krzysztof >
On 26/09/2023 05:20, Jason-JH Lin (林睿祥) wrote: mdq_pkt_finialize_loop() at [PATCH 8/15]. >>> >>> mtk-cmdq-helper.c and mtk-cmdq-mailbox.c are not in the >>> same maintainer's tree, so I separate this to another patch from >> [PATCH >>> 8/15]. >> >> Why? Anyway it has to go through same tree. You have dependencies. >> Such >> artificial split makes it only difficult to review and understand. >> Re-organize your patchset to be correctly split per each logical >> feature/change. Split per subsystems is not the same. >> > > Yes, these related files are in the different maintainer's tree. > Refer to https://www.kernel.org/doc/linux/MAINTAINERS > > MAILBOX API > M: Jassi Brar > F: drivers/mailbox/ > - drivers/mailbox/mtk-cmdq-mailbox.c > - drivers/mailbox/mtk-cmdq-sec- > mailbox.c > > ARM/Mediatek SoC support > M: Matthias Brugger > F: drivers/soc/mediatek/ > K: mediatek > - drivers/soc/mediatek/mtk-cmdq-helper.c > - > include/linux/soc/mediatek/mtk-cmdq.h > > I think we should add a new MAINTAINER label for mediatek CMDQ mailbox > and put these files together, such as "MAILBOX ARM MHUv2" and "QUALCOM > IPCC MAILBOX DRIVER". Why? It's not related to the topic of splitting patchset into patches. There is no problem of patchsets touching multiple subsystems. We already solved this problem many years ago... > But I don't know how to make a request for that. Anyway, you would not be a maintainer taking patches, just a reviewer called "M:" here... > > Anyway, I'll squash this logical feature to the same patch, no matter > these files are not in the same tree. > Best regards, Krzysztof