| 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: <3c97025f-289e-48a3-9b58-3d469cba7366@collabora.com>
Date: Thu, 9 Oct 2025 13:27:16 +0200
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: Jason-JH Lin <jason-jh.lin@...iatek.com>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Jassi Brar <jassisinghbrar@...il.com>,
Chun-Kuang Hu <chunkuang.hu@...nel.org>,
Mauro Carvalho Chehab <mchehab@...nel.org>
Cc: Matthias Brugger <matthias.bgg@...il.com>,
Nicolas Dufresne <nicolas@...fresne.ca>, Nancy Lin <nancy.lin@...iatek.com>,
Singo Chang <singo.chang@...iatek.com>,
Paul-PL Chen <paul-pl.chen@...iatek.com>, Moudy Ho <moudy.ho@...iatek.com>,
Xiandong Wang <xiandong.wang@...iatek.com>,
Sirius Wang <sirius.wang@...iatek.com>, Fei Shao <fshao@...omium.org>,
Chen-yu Tsai <wenst@...omium.org>,
Project_Global_Chrome_Upstream_Group@...iatek.com,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
dri-devel@...ts.freedesktop.org, linux-mediatek@...ts.infradead.org,
linux-arm-kernel@...ts.infradead.org, linux-media@...r.kernel.org
Subject: Re: [PATCH v7 02/20] mailbox: mtk-cmdq: Refine DMA address handling
for the command buffer
Il 27/08/25 13:37, Jason-JH Lin ha scritto:
> GCE can only fetch the command buffer address from a 32-bit register.
> Some SoCs support a 35-bit command buffer address for GCE, which
> requires a right shift of 3 bits before setting the address into
> the 32-bit register. A comment has been added to the header of
> cmdq_get_shift_pa() to explain this requirement.
>
> To prevent the GCE command buffer address from being DMA mapped beyond
> its supported bit range, the DMA bit mask for the device is set during
> initialization.
>
> Additionally, to ensure the correct shift is applied when setting or
> reading the register that stores the GCE command buffer address,
> new APIs, cmdq_reg_shift_addr() and cmdq_reg_revert_addr(), have been
> introduced for consistent operations on this register.
>
> The variable type for the command buffer address has been standardized
> to dma_addr_t to prevent handling issues caused by type mismatches.
>
> Fixes: 0858fde496f8 ("mailbox: cmdq: variablize address shift in platform")
> Signed-off-by: Jason-JH Lin <jason-jh.lin@...iatek.com>
> ---
> drivers/mailbox/mtk-cmdq-mailbox.c | 43 ++++++++++++++++--------
> include/linux/mailbox/mtk-cmdq-mailbox.h | 10 ++++++
> 2 files changed, 39 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c
> index 532929916e99..a60486cbb533 100644
> --- a/drivers/mailbox/mtk-cmdq-mailbox.c
> +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> @@ -92,6 +92,16 @@ struct gce_plat {
> u32 gce_num;
> };
>
> +static inline u32 cmdq_reg_shift_addr(dma_addr_t addr, const struct gce_plat *pdata)
cmdq_format_gce_addr() or cmdq_pa_to_gce_addr()
(or cmdq_va_to_gce_addr() if this is not a physical address)
explains what you're doing here a bit better I think.
> +{
> + return (addr >> pdata->shift);
You don't need parenthesis; just `return addr >> pdata->shift;` is fine
> +}
> +
> +static inline dma_addr_t cmdq_reg_revert_addr(u32 addr, const struct gce_plat *pdata)
cmdq_gce_to_pa_addr() or cmdq_get_pa_addr() perhaps? :-)
(same comments for s/pa/va/g if this is not physical address)
Everything else looks ok.
Cheers,
Angelo
> +{
> + return ((dma_addr_t)addr << pdata->shift);
> +}
> +
> u8 cmdq_get_shift_pa(struct mbox_chan *chan)
> {
> struct cmdq *cmdq = container_of(chan->mbox, struct cmdq, mbox);
> @@ -188,13 +198,12 @@ static void cmdq_task_insert_into_thread(struct cmdq_task *task)
> struct cmdq_task *prev_task = list_last_entry(
> &thread->task_busy_list, typeof(*task), list_entry);
> u64 *prev_task_base = prev_task->pkt->va_base;
> + u32 shift_addr = cmdq_reg_shift_addr(task->pa_base, task->cmdq->pdata);
>
> /* let previous task jump to this task */
> dma_sync_single_for_cpu(dev, prev_task->pa_base,
> prev_task->pkt->cmd_buf_size, DMA_TO_DEVICE);
> - prev_task_base[CMDQ_NUM_CMD(prev_task->pkt) - 1] =
> - (u64)CMDQ_JUMP_BY_PA << 32 |
> - (task->pa_base >> task->cmdq->pdata->shift);
> + prev_task_base[CMDQ_NUM_CMD(prev_task->pkt) - 1] = (u64)CMDQ_JUMP_BY_PA << 32 | shift_addr;
> dma_sync_single_for_device(dev, prev_task->pa_base,
> prev_task->pkt->cmd_buf_size, DMA_TO_DEVICE);
>
> @@ -237,7 +246,8 @@ static void cmdq_thread_irq_handler(struct cmdq *cmdq,
> struct cmdq_thread *thread)
> {
> struct cmdq_task *task, *tmp, *curr_task = NULL;
> - u32 curr_pa, irq_flag, task_end_pa;
> + u32 irq_flag, shift_addr;
> + dma_addr_t curr_pa, task_end_pa;
> bool err;
>
> irq_flag = readl(thread->base + CMDQ_THR_IRQ_STATUS);
> @@ -259,7 +269,8 @@ static void cmdq_thread_irq_handler(struct cmdq *cmdq,
> else
> return;
>
> - curr_pa = readl(thread->base + CMDQ_THR_CURR_ADDR) << cmdq->pdata->shift;
> + shift_addr = readl(thread->base + CMDQ_THR_CURR_ADDR);
> + curr_pa = cmdq_reg_revert_addr(shift_addr, cmdq->pdata);
>
> list_for_each_entry_safe(task, tmp, &thread->task_busy_list,
> list_entry) {
> @@ -378,7 +389,8 @@ static int cmdq_mbox_send_data(struct mbox_chan *chan, void *data)
> struct cmdq_thread *thread = (struct cmdq_thread *)chan->con_priv;
> struct cmdq *cmdq = dev_get_drvdata(chan->mbox->dev);
> struct cmdq_task *task;
> - unsigned long curr_pa, end_pa;
> + u32 shift_addr;
> + dma_addr_t curr_pa, end_pa;
> int ret;
>
> /* Client should not flush new tasks if suspended. */
> @@ -409,20 +421,20 @@ static int cmdq_mbox_send_data(struct mbox_chan *chan, void *data)
> */
> WARN_ON(cmdq_thread_reset(cmdq, thread) < 0);
>
> - writel(task->pa_base >> cmdq->pdata->shift,
> - thread->base + CMDQ_THR_CURR_ADDR);
> - writel((task->pa_base + pkt->cmd_buf_size) >> cmdq->pdata->shift,
> - thread->base + CMDQ_THR_END_ADDR);
> + shift_addr = cmdq_reg_shift_addr(task->pa_base, cmdq->pdata);
> + writel(shift_addr, thread->base + CMDQ_THR_CURR_ADDR);
> + shift_addr = cmdq_reg_shift_addr(task->pa_base + pkt->cmd_buf_size, cmdq->pdata);
> + writel(shift_addr, thread->base + CMDQ_THR_END_ADDR);
>
> writel(thread->priority, thread->base + CMDQ_THR_PRIORITY);
> writel(CMDQ_THR_IRQ_EN, thread->base + CMDQ_THR_IRQ_ENABLE);
> writel(CMDQ_THR_ENABLED, thread->base + CMDQ_THR_ENABLE_TASK);
> } else {
> WARN_ON(cmdq_thread_suspend(cmdq, thread) < 0);
> - curr_pa = readl(thread->base + CMDQ_THR_CURR_ADDR) <<
> - cmdq->pdata->shift;
> - end_pa = readl(thread->base + CMDQ_THR_END_ADDR) <<
> - cmdq->pdata->shift;
> + shift_addr = readl(thread->base + CMDQ_THR_CURR_ADDR);
> + curr_pa = cmdq_reg_revert_addr(shift_addr, cmdq->pdata);
> + shift_addr = readl(thread->base + CMDQ_THR_END_ADDR);
> + end_pa = cmdq_reg_revert_addr(shift_addr, cmdq->pdata);
> /* check boundary */
> if (curr_pa == end_pa - CMDQ_INST_SIZE ||
> curr_pa == end_pa) {
> @@ -656,6 +668,9 @@ static int cmdq_probe(struct platform_device *pdev)
> if (err)
> return err;
>
> + dma_set_coherent_mask(dev,
> + DMA_BIT_MASK(sizeof(u32) * BITS_PER_BYTE + cmdq->pdata->shift));
> +
> cmdq->mbox.dev = dev;
> cmdq->mbox.chans = devm_kcalloc(dev, cmdq->pdata->thread_nr,
> sizeof(*cmdq->mbox.chans), GFP_KERNEL);
> diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h
> index 4c1a91b07de3..e1555e06e7e5 100644
> --- a/include/linux/mailbox/mtk-cmdq-mailbox.h
> +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
> @@ -77,6 +77,16 @@ struct cmdq_pkt {
> size_t buf_size; /* real buffer size */
> };
>
> +/**
> + * cmdq_get_shift_pa() - get the shift bits of physical address
> + * @chan: mailbox channel
> + *
> + * GCE can only fetch the command buffer address from a 32-bit register.
> + * Some SOCs support more than 32-bit command buffer address for GCE, which
> + * requires some shift bits to make the address fit into the 32-bit register.
> + *
> + * Return: the shift bits of physical address
> + */
> u8 cmdq_get_shift_pa(struct mbox_chan *chan);
>
> #endif /* __MTK_CMDQ_MAILBOX_H__ */
Powered by blists - more mailing lists