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?
@@ -332,6 +332,16 @@ config SUN6I_MSGBOX
various Allwinner SoCs. This mailbox is used for communication
between the application CPUs and the power management coprocessor.
+config SUN55I_MSGBOX
+ tristate "Allwinner sun55i/sun60i 4-port Message Box"
+ depends on ARCH_SUNXI || COMPILE_TEST
+ default ARCH_SUNXI
+ help
+ Mailbox implementation for the 4-port hardware message box present in
+ Allwinner A523/A527/T527 (sun55i) and A733 (sun60i) SoCs. This mailbox
+ provides 12 hardware FIFO channels for communication between the
+ Cortex-A55 host, CPUS, HiFi4 DSP, and XuanTie E907 RISC-V coprocessors.
+
config SPRD_MBOX
tristate "Spreadtrum Mailbox"
depends on ARCH_SPRD || COMPILE_TEST
@@ -71,6 +71,8 @@ obj-$(CONFIG_ZYNQMP_IPI_MBOX) += zynqmp-ipi-mailbox.o
obj-$(CONFIG_SUN6I_MSGBOX) += sun6i-msgbox.o
+obj-$(CONFIG_SUN55I_MSGBOX) += sun55i-msgbox.o
+
obj-$(CONFIG_SPRD_MBOX) += sprd-mailbox.o
obj-$(CONFIG_QCOM_CPUCP_MBOX) += qcom-cpucp-mbox.o
new file mode 100644
@@ -0,0 +1,337 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Allwinner sun55i/sun60i 4-Port Hardware Message Box Driver
+ *
+ * Copyright (C) 2026 Tim Michals <tcmichals@gmail.com>
+ * Based on vendor sunxi-msgbox driver by Allwinner Technology Co., Ltd.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/mailbox_controller.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/reset.h>
+#include <linux/spinlock.h>
+
+#define SUN55I_MAX_PROCESSORS 4
+#define SUN55I_CHANS_PER_PROC 4
+#define SUN55I_NUM_CHANS ((SUN55I_MAX_PROCESSORS - 1) * SUN55I_CHANS_PER_PROC)
+#define SUN55I_FIFO_MAX 8
+
+#define SUNXI_MSGBOX_OFFSET(n) (0x100 * (n))
+#define SUNXI_MSGBOX_READ_IRQ_ENABLE(n) (0x020 + SUNXI_MSGBOX_OFFSET(n))
+#define SUNXI_MSGBOX_READ_IRQ_STATUS(n) (0x024 + SUNXI_MSGBOX_OFFSET(n))
+#define SUNXI_MSGBOX_WRITE_IRQ_ENABLE(n) (0x030 + SUNXI_MSGBOX_OFFSET(n))
+#define SUNXI_MSGBOX_WRITE_IRQ_STATUS(n) (0x034 + SUNXI_MSGBOX_OFFSET(n))
+#define SUNXI_MSGBOX_FIFO_STATUS(n, p) (0x050 + SUNXI_MSGBOX_OFFSET(n) + 0x4 * (p))
+#define SUNXI_MSGBOX_MSG_STATUS(n, p) (0x060 + SUNXI_MSGBOX_OFFSET(n) + 0x4 * (p))
+#define SUNXI_MSGBOX_MSG_FIFO(n, p) (0x070 + SUNXI_MSGBOX_OFFSET(n) + 0x4 * (p))
+
+#define RD_IRQ_EN_BIT(p) BIT((p) * 2)
+#define RD_IRQ_PEND_BIT(p) BIT((p) * 2)
+#define MSG_NUM_MASK GENMASK(3, 0)
+
+struct sun55i_route {
+ u8 remote_id;
+ u8 remote_n;
+};
+
+/*
+ * Hardware routing table for Cortex-A55 host (local_id = 0):
+ * local_n = 0 -> CPUS (remote_id = 2, remote_n = 0) -> Channels 0..3
+ * local_n = 1 -> DSP (remote_id = 1, remote_n = 0) -> Channels 4..7
+ * local_n = 2 -> RV (remote_id = 3, remote_n = 2) -> Channels 8..11
+ */
+static const struct sun55i_route arm_routes[3] = {
+ [0] = { .remote_id = 2, .remote_n = 0 },
+ [1] = { .remote_id = 1, .remote_n = 0 },
+ [2] = { .remote_id = 3, .remote_n = 2 },
+};
+
+struct sun55i_msgbox {
+ struct mbox_controller controller;
+ void __iomem *regs[SUN55I_MAX_PROCESSORS];
+ struct clk *clk;
+ struct reset_control *reset;
+ /* Protects concurrent MMIO register access */
+ spinlock_t lock;
+};
+
+static inline struct sun55i_msgbox *to_sun55i_msgbox(struct mbox_chan *chan)
+{
+ return chan->con_priv;
+}
+
+static inline void sun55i_chan_to_route(int chan_idx, int *local_n, int *p,
+ int *remote_id, int *remote_n)
+{
+ *local_n = chan_idx / SUN55I_CHANS_PER_PROC;
+ *p = chan_idx % SUN55I_CHANS_PER_PROC;
+ *remote_id = arm_routes[*local_n].remote_id;
+ *remote_n = arm_routes[*local_n].remote_n;
+}
+
+static irqreturn_t sun55i_msgbox_irq(int irq, void *dev_id)
+{
+ struct sun55i_msgbox *mbox = dev_id;
+ irqreturn_t ret = IRQ_NONE;
+ int local_n, p, chan_idx;
+
+ for (local_n = 0; local_n < 3; local_n++) {
+ void __iomem *local_base = mbox->regs[0];
+ u32 en, stat, pending;
+
+ en = readl(local_base + SUNXI_MSGBOX_READ_IRQ_ENABLE(local_n));
+ stat = readl(local_base + SUNXI_MSGBOX_READ_IRQ_STATUS(local_n));
+ pending = en & stat;
+
+ if (!pending)
+ continue;
+
+ for (p = 0; p < SUN55I_CHANS_PER_PROC; p++) {
+ if (!(pending & RD_IRQ_PEND_BIT(p)))
+ continue;
+
+ 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);
+ }
+
+ writel(RD_IRQ_PEND_BIT(p),
+ local_base + SUNXI_MSGBOX_READ_IRQ_STATUS(local_n));
+ ret = IRQ_HANDLED;
+ }
+ }
+
+ return ret;
+}
+
+static int sun55i_msgbox_send_data(struct mbox_chan *chan, void *data)
+{
+ struct sun55i_msgbox *mbox = to_sun55i_msgbox(chan);
+ int n = chan - mbox->controller.chans;
+ int local_n, p, remote_id, remote_n;
+ u32 msg = data ? *(u32 *)data : 0;
+
+ sun55i_chan_to_route(n, &local_n, &p, &remote_id, &remote_n);
+
+ writel(msg, mbox->regs[remote_id] + SUNXI_MSGBOX_MSG_FIFO(remote_n, p));
+ return 0;
+}
+
+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));
+
+ /* Enable receive IRQ */
+ spin_lock_irqsave(&mbox->lock, flags);
+ val = readl(mbox->regs[0] + SUNXI_MSGBOX_READ_IRQ_ENABLE(local_n));
+ val |= RD_IRQ_EN_BIT(p);
+ writel(val, mbox->regs[0] + SUNXI_MSGBOX_READ_IRQ_ENABLE(local_n));
+ spin_unlock_irqrestore(&mbox->lock, flags);
+
+ return 0;
+}
+
+static void sun55i_msgbox_shutdown(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);
+
+ /* Disable receive IRQ */
+ spin_lock_irqsave(&mbox->lock, flags);
+ val = readl(mbox->regs[0] + SUNXI_MSGBOX_READ_IRQ_ENABLE(local_n));
+ val &= ~RD_IRQ_EN_BIT(p);
+ writel(val, mbox->regs[0] + SUNXI_MSGBOX_READ_IRQ_ENABLE(local_n));
+ spin_unlock_irqrestore(&mbox->lock, flags);
+
+ /* Clear pending status and flush */
+ writel(RD_IRQ_PEND_BIT(p),
+ mbox->regs[0] + SUNXI_MSGBOX_READ_IRQ_STATUS(local_n));
+ 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));
+}
+
+static bool sun55i_msgbox_last_tx_done(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;
+ u32 count;
+
+ sun55i_chan_to_route(n, &local_n, &p, &remote_id, &remote_n);
+
+ count = readl(mbox->regs[remote_id] + SUNXI_MSGBOX_MSG_STATUS(remote_n, p)) & MSG_NUM_MASK;
+ return count < SUN55I_FIFO_MAX;
+}
+
+static bool sun55i_msgbox_peek_data(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;
+ u32 count;
+
+ sun55i_chan_to_route(n, &local_n, &p, &remote_id, &remote_n);
+
+ count = readl(mbox->regs[0] + SUNXI_MSGBOX_MSG_STATUS(local_n, p)) & MSG_NUM_MASK;
+ return count > 0;
+}
+
+static const struct mbox_chan_ops sun55i_msgbox_chan_ops = {
+ .send_data = sun55i_msgbox_send_data,
+ .startup = sun55i_msgbox_startup,
+ .shutdown = sun55i_msgbox_shutdown,
+ .last_tx_done = sun55i_msgbox_last_tx_done,
+ .peek_data = sun55i_msgbox_peek_data,
+};
+
+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 = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
+ if (!mbox)
+ return -ENOMEM;
+
+ spin_lock_init(&mbox->lock);
+
+ for (i = 0; i < SUN55I_MAX_PROCESSORS; i++) {
+ mbox->regs[i] = devm_platform_ioremap_resource(pdev, i);
+ if (IS_ERR(mbox->regs[i]))
+ return dev_err_probe(dev, PTR_ERR(mbox->regs[i]),
+ "failed to map resource %d\n", i);
+ }
+
+ mbox->clk = devm_clk_get(dev, NULL);
+ if (IS_ERR(mbox->clk))
+ return dev_err_probe(dev, PTR_ERR(mbox->clk), "failed to get clock\n");
+
+ ret = clk_prepare_enable(mbox->clk);
+ if (ret)
+ return dev_err_probe(dev, ret, "failed to enable clock\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;
+
+ /* Disable all read IRQs and clear status */
+ for (local_n = 0; local_n < 3; local_n++) {
+ writel(0, mbox->regs[0] + SUNXI_MSGBOX_READ_IRQ_ENABLE(local_n));
+ writel(0xffffffff, mbox->regs[0] + SUNXI_MSGBOX_READ_IRQ_STATUS(local_n));
+ }
+
+ irq_cnt = platform_irq_count(pdev);
+ if (irq_cnt < 0) {
+ ret = irq_cnt;
+ goto err_disable_clk;
+ }
+
+ 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);
+ }
+ }
+
+ chans = devm_kcalloc(dev, SUN55I_NUM_CHANS, sizeof(*chans), GFP_KERNEL);
+ if (!chans) {
+ ret = -ENOMEM;
+ goto err_disable_clk;
+ }
+
+ for (i = 0; i < SUN55I_NUM_CHANS; i++)
+ chans[i].con_priv = mbox;
+
+ mbox->controller.dev = dev;
+ mbox->controller.ops = &sun55i_msgbox_chan_ops;
+ mbox->controller.chans = chans;
+ mbox->controller.num_chans = SUN55I_NUM_CHANS;
+ mbox->controller.txdone_irq = false;
+ mbox->controller.txdone_poll = true;
+ mbox->controller.txpoll_period = 1;
+
+ platform_set_drvdata(pdev, mbox);
+
+ ret = mbox_controller_register(&mbox->controller);
+ if (ret) {
+ dev_err_probe(dev, ret, "failed to register controller\n");
+ goto err_disable_clk;
+ }
+
+ return 0;
+
+err_disable_clk:
+ clk_disable_unprepare(mbox->clk);
+ return ret;
+}
+
+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);
+}
+
+static const struct of_device_id sun55i_msgbox_of_match[] = {
+ { .compatible = "allwinner,sun55i-a523-msgbox" },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, sun55i_msgbox_of_match);
+
+static struct platform_driver sun55i_msgbox_driver = {
+ .driver = {
+ .name = "sun55i-msgbox",
+ .of_match_table = sun55i_msgbox_of_match,
+ },
+ .probe = sun55i_msgbox_probe,
+ .remove = sun55i_msgbox_remove,
+};
+module_platform_driver(sun55i_msgbox_driver);
+
+MODULE_AUTHOR("Tim Michals <tcmichals@gmail.com>");
+MODULE_DESCRIPTION("Allwinner sun55i/sun60i 4-Port Message Box Driver");
+MODULE_LICENSE("GPL");