Re: [PATCH 3/3] mailbox: mtk-cmdq: add mt6572 support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Missing `mminfra_offset` translation in `cmdq_task_handle_error()` causes the GCE hardware to jump to a garbage physical address on error recovery.
- [High] Unbalanced `of_clk_get()` for multi-GCE platforms leads to a leak of clock references in error paths and during unbind.
- [High] Potential NULL pointer dereference in the IRQ handler due to uninitialized `thread->chan`.
--

--- Patch [3]: [PATCH 3/3] mailbox: mtk-cmdq: add mt6572 support ---
commit 125c66f031e1e0c9602b803f39b71e1fb66eb336
Author: Roman Vivchar <rva333@protonmail.com>

mailbox: mtk-cmdq: add mt6572 support

Add legacy_isa field forwarding to the cmdq helper and a platform data
for the mt6572 GCE.

> diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c
> index e523c84b48088..5e7bc52baaa13 100644
> --- a/drivers/mailbox/mtk-cmdq-mailbox.c
> +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> @@ -99,6 +99,7 @@ struct gce_plat {
>  	bool control_by_sw;
>  	bool sw_ddr_en;
>  	bool gce_vm;
> +	bool legacy_isa;
>  	u32 gce_num;
>  };
>  
> @@ -119,6 +120,7 @@ void cmdq_get_mbox_priv(struct mbox_chan *chan, struct cmdq_mbox_priv *priv)
>  	struct cmdq *cmdq = container_of(chan->mbox, struct cmdq, mbox);
>  
>  	priv->shift_pa = cmdq->pdata->shift;
> +	priv->legacy_isa = cmdq->pdata->legacy_isa;
>  	priv->mminfra_offset = cmdq->pdata->mminfra_offset;
>  }
>  EXPORT_SYMBOL(cmdq_get_mbox_priv);
> @@ -787,6 +789,14 @@ static const struct dev_pm_ops cmdq_pm_ops = {
>  			   cmdq_runtime_resume, NULL)
>  };
>  
> +static const struct gce_plat gce_plat_mt6572 = {
> +	.thread_nr = 14,
> +	.shift = 0,
> +	.control_by_sw = false,
> +	.legacy_isa = true,
> +	.gce_num = 1
> +};
> +
>  static const struct gce_plat gce_plat_mt6779 = {
>  	.thread_nr = 24,
>  	.shift = 3,
> @@ -848,6 +858,7 @@ static const struct gce_plat gce_plat_mt8196 = {
>  };
>  
>  static const struct of_device_id cmdq_of_ids[] = {
> +	{.compatible = "mediatek,mt6572-gce", .data = (void *)&gce_plat_mt6572},
>  	{.compatible = "mediatek,mt6779-gce", .data = (void *)&gce_plat_mt6779},
>  	{.compatible = "mediatek,mt8173-gce", .data = (void *)&gce_plat_mt8173},
>  	{.compatible = "mediatek,mt8183-gce", .data = (void *)&gce_plat_mt8183},

[Severity: High]
This isn't a bug introduced by this patch, but there is an unbalanced
of_clk_get() for multi-GCE platforms that leads to a leak of clock references.

When cmdq->pdata->gce_num > 1, additional clocks are fetched in
cmdq_get_clocks():

drivers/mailbox/mtk-cmdq-mailbox.c:cmdq_get_clocks() {
	...
	for_each_child_of_node_scoped(parent, node) {
		...
		clks->clk = of_clk_get(node, 0);
		if (IS_ERR(clks->clk))
			return dev_err_probe(dev, PTR_ERR(clks->clk),
					     "failed to get gce%d clock\n", alias_id);
	}
	...
}

Unlike the first clock which uses devm_clk_get(), of_clk_get() returns an
unmanaged clock reference. If the loop encounters an error on a subsequent
clock, does this return immediately and leak the references for the clocks
already successfully fetched?

Furthermore, during driver unbind, the driver unprepares the clocks but never
calls clk_put() to release them:

drivers/mailbox/mtk-cmdq-mailbox.c:cmdq_remove() {
	...
	clk_bulk_unprepare(cmdq->pdata->gce_num, cmdq->clocks);
}

Should clk_put() be called on these unmanaged clock references in error paths
and during unbind to prevent a permanent resource leak?


[Severity: High]
This is a pre-existing issue, but is there a potential NULL pointer dereference
in the IRQ handler due to an uninitialized thread->chan?

The driver unconditionally registers cmdq_irq_handler() with IRQF_SHARED
during cmdq_probe(). When an interrupt is delivered, the handler iterates over
the active threads and attempts to lock the channel:

drivers/mailbox/mtk-cmdq-mailbox.c:cmdq_irq_handler() {
	...
	for_each_clear_bit(bit, &irq_status, cmdq->pdata->thread_nr) {
		struct cmdq_thread *thread = &cmdq->thread[bit];

		spin_lock_irqsave(&thread->chan->lock, flags);
		cmdq_thread_irq_handler(cmdq, thread);
		spin_unlock_irqrestore(&thread->chan->lock, flags);
	}
	...
}

However, thread->chan is only initialized dynamically when a client requests
the channel via cmdq_xlate():

drivers/mailbox/mtk-cmdq-mailbox.c:cmdq_xlate() {
	...
	thread->chan = &mbox->chans[ind];
	...
}

If a shared interrupt line fires, or a spurious interrupt arrives, or the
bootloader left the hardware in an interrupting state before the client has
requested the mailbox, could the handler dereference a NULL thread->chan here
and cause a kernel oops?


[Severity: Critical]
This isn't a bug introduced by this patch, but does cmdq_task_handle_error()
miss applying the mminfra_offset translation, potentially causing the GCE
hardware to jump to a garbage physical address on error recovery?

When an error occurs and the driver attempts to skip the failed task, it
evaluates the next task's execution address by simply right-shifting its
physical address and writes this directly to the hardware:

drivers/mailbox/mtk-cmdq-mailbox.c:cmdq_task_handle_error() {
	...
	next_task = list_first_entry_or_null(&thread->task_busy_list,
			struct cmdq_task, list_entry);
	if (next_task)
		writel(next_task->pa_base >> cmdq->pdata->shift,
		       thread->base + CMDQ_THR_CURR_ADDR);
	...
}

This ignores the cmdq->pdata->mminfra_offset adjustment which is required for
correct conversion, as handled properly elsewhere in the driver:

drivers/mailbox/mtk-cmdq-mailbox.c:cmdq_convert_gce_addr() {
	...
	return (addr + pdata->mminfra_offset) >> pdata->shift;
}

On SoCs with a non-zero memory infrastructure offset (such as MT8196), will the
hardware receive a severely truncated and incorrect address, leading to memory
corruption or a catastrophic system hang when the thread resumes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728-6572-gce-v1-0-750bba32689e@protonmail.com?part=3