Re: [PATCH 2/7] mailbox: sun55i: add Allwinner sun55i/sun60i 4-port Message Box driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] mbox: sun55i: leak of shared reset control on remove and probe error paths
- [High] mbox: sun55i: unhandled interrupt storm due to ignored devm_request_irq failure
- [High] mbox: sun55i: unclocked MMIO access panic due to early clock disable
- [High] mbox: sun55i: dropped interrupts due to TOCTOU race in status clearing
- [Medium] mbox: sun55i: CPU hard lockup via unbounded FIFO read loop in hardirq
--

commit 87081d025c7634be651f61e5ab219367cae289d0
Author: Tim Michals <tcmichals@gmail.com>

mailbox: sun55i: add Allwinner sun55i/sun60i 4-port Message Box driver

Add support for the 4-port hardware Message Box present on Allwinner
A523/A527/T527 (sun55i) and A733 (sun60i) SoCs. This hardware block
provides 12 hardware FIFO channels connecting the Cortex-A55 host,
HiFi4 DSP, CPUS, and XuanTie E907 RISC-V coprocessors.

> diff --git a/drivers/mailbox/sun55i-msgbox.c b/drivers/mailbox/sun55i-msgbox.c
> new file mode 100644
> index 0000000000000..b36f26670a461
> --- /dev/null
> +++ b/drivers/mailbox/sun55i-msgbox.c

[ ... ]

> +			chan_idx = local_n * SUN55I_CHANS_PER_PROC + p;
> +			while (readl(local_base +
> +				     SUNXI_MSGBOX_MSG_STATUS(local_n, p)) & MSG_NUM_MASK) {
> +				u32 msg = readl(local_base + SUNXI_MSGBOX_MSG_FIFO(local_n, p));
> +
> +				mbox_chan_received_data(&mbox->controller.chans[chan_idx], &msg);
> +			}

[Severity: Medium]
Does this unbounded loop adequately protect the host CPU against continuous
transmissions from a remote coprocessor?

If a remote coprocessor (e.g., DSP or RISC-V) continuously writes messages to
the FIFO, the loop condition might never evaluate to false, leaving the host
CPU stuck in a hardirq context. This could lead to RCU stalls or watchdog
timeouts.

> +
> +			writel(RD_IRQ_PEND_BIT(p),
> +			       local_base + SUNXI_MSGBOX_READ_IRQ_STATUS(local_n));

[Severity: High]
Does this sequence inadvertently clear the interrupt status for a newly arrived
message without processing it?

If a new message arrives from the coprocessor immediately after the while loop
evaluates to false, but before the writel() clears the pending interrupt, the
newly latched interrupt status could be erroneously cleared. The message would
remain unread in the FIFO and no interrupt would be generated for it.

> +			ret = IRQ_HANDLED;
> +		}
> +	}
> +
> +	return ret;
> +}

[ ... ]

> +static int sun55i_msgbox_startup(struct mbox_chan *chan)
> +{
> +	struct sun55i_msgbox *mbox = to_sun55i_msgbox(chan);
> +	int n = chan - mbox->controller.chans;
> +	int local_n, p, remote_id, remote_n;
> +	unsigned long flags;
> +	u32 val;
> +
> +	sun55i_chan_to_route(n, &local_n, &p, &remote_id, &remote_n);
> +
> +	/* Flush any stale receive data */
> +	while (readl(mbox->regs[0] + SUNXI_MSGBOX_MSG_STATUS(local_n, p)) & MSG_NUM_MASK)
> +		readl(mbox->regs[0] + SUNXI_MSGBOX_MSG_FIFO(local_n, p));
> +
> +	/* Clear pending status */
> +	writel(RD_IRQ_PEND_BIT(p),
> +	       mbox->regs[0] + SUNXI_MSGBOX_READ_IRQ_STATUS(local_n));

[Severity: High]
Is there a race condition here similar to the one in the interrupt handler?

If a message arrives after the while loop finishes but before the pending
status is cleared, the interrupt could be lost.

> +
> +	/* Enable receive IRQ */

[ ... ]

> +static int sun55i_msgbox_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct mbox_chan *chans;
> +	struct sun55i_msgbox *mbox;
> +	int i, ret, irq_cnt, local_n;

[ ... ]

> +	mbox->reset = devm_reset_control_get_optional_shared(dev, NULL);
> +	if (IS_ERR(mbox->reset)) {
> +		ret = PTR_ERR(mbox->reset);
> +		goto err_disable_clk;
> +	}
> +
> +	ret = reset_control_deassert(mbox->reset);
> +	if (ret)
> +		goto err_disable_clk;

[Severity: High]
Will this shared reset control be correctly balanced on failure and removal?

The shared reset control is manually deasserted here, incrementing its usage
count, but it appears there are no corresponding reset_control_assert() calls
in the err_disable_clk error path or in sun55i_msgbox_remove().

This can prevent the physical hardware reset line from ever being asserted
again, affecting other devices sharing the reset line.

> +
> +	/* Disable all read IRQs and clear status */

[ ... ]

> +	for (i = 0; i < irq_cnt; i++) {
> +		int irq = platform_get_irq(pdev, i);
> +
> +		if (irq > 0) {
> +			ret = devm_request_irq(dev, irq, sun55i_msgbox_irq,
> +					       IRQF_SHARED, dev_name(dev), mbox);
> +			if (ret)
> +				dev_warn(dev, "failed to request irq %d: %d\n", irq, ret);

[Severity: High]
If devm_request_irq() fails, could this cause an unhandled interrupt storm?

The driver ignores the failure and only prints a warning. Later,
sun55i_msgbox_startup() unconditionally unmasks the hardware receive interrupt.

If the hardware asserts the IRQ line and there is no registered handler,
the IRQ core will eventually disable the shared IRQ line permanently,
breaking all devices sharing it.

> +		}
> +	}
> +
> +	chans = devm_kcalloc(dev, SUN55I_NUM_CHANS, sizeof(*chans), GFP_KERNEL);

[ ... ]

> +	return 0;
> +
> +err_disable_clk:
> +	clk_disable_unprepare(mbox->clk);
> +	return ret;
> +}

[Severity: High]
Does this error path leak the shared reset control?

Since reset_control_deassert() was called earlier, returning here without
calling reset_control_assert() leaves the usage count unbalanced.

> +
> +static void sun55i_msgbox_remove(struct platform_device *pdev)
> +{
> +	struct sun55i_msgbox *mbox = platform_get_drvdata(pdev);
> +
> +	mbox_controller_unregister(&mbox->controller);
> +	clk_disable_unprepare(mbox->clk);
> +}

[Severity: High]
Could this lead to an unclocked MMIO access panic if an interrupt fires during
module unload?

The shared interrupt is registered using devm_request_irq() in probe, which
means devres will unregister it after this remove function returns.

Because the hardware clock is manually disabled here before the IRQ handler
is unregistered by devres, if another device sharing the IRQ line triggers an
interrupt in this window, sun55i_msgbox_irq() will execute and try to read
from unclocked MMIO registers, resulting in a system crash.

Additionally, because devres cleans up resources in reverse order,
the mbox->controller.chans array will be freed before the IRQ is unregistered,
leading to a use-after-free if the interrupt fires.

Also, does this function need to call reset_control_assert() to balance the
deassert call from probe?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260922034711.190253-1-tcmichals@gmail.com?part=2