[PATCHv2] mailbox: sun6i: modernize probe and convert to fully managed

[PATCHv2] mailbox: sun6i: modernize probe and convert to fully managed

[Rosen Penev]

Replace irq_of_parse_and_map() with platform_get_irq() and move both
IRQ and MMIO resource acquisition to the top of probe, before any
allocations, for early error exit.

Switch from devm_clk_get() + clk_prepare_enable() to
devm_clk_get_enabled(), which combines both operations and registers
devres callbacks for automatic disable/unprepare on unbind. This
eliminates the manual err_disable_unprepare error path and the
remove callback entirely.

Use devm_mbox_controller_register() for devres-managed controller
registration, and drop the remove callback and platform_set_drvdata()
which are no longer needed.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 v2: fix compilation
 drivers/mailbox/sun6i-msgbox.c | 62 ++++++++++------------------------
 1 file changed, 18 insertions(+), 44 deletions(-)

diff --git a/drivers/mailbox/sun6i-msgbox.c b/drivers/mailbox/sun6i-msgbox.c
index 6ba6920f4645..3b838f6c837a 100644
--- a/drivers/mailbox/sun6i-msgbox.c
+++ b/drivers/mailbox/sun6i-msgbox.c
@@ -198,7 +198,17 @@ static int sun6i_msgbox_probe(struct platform_device *pdev)
 	struct mbox_chan *chans;
 	struct reset_control *reset;
 	struct sun6i_msgbox *mbox;
+	void __iomem *regs;
 	int i, ret;
+	int irq;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(regs))
+		return PTR_ERR(regs);
 
 	mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
 	if (!mbox)
@@ -211,24 +221,18 @@ static int sun6i_msgbox_probe(struct platform_device *pdev)
 	for (i = 0; i < NUM_CHANS; ++i)
 		chans[i].con_priv = mbox;
 
-	mbox->clk = devm_clk_get(dev, NULL);
+	mbox->clk = devm_clk_get_enabled(dev, NULL);
 	if (IS_ERR(mbox->clk)) {
 		ret = PTR_ERR(mbox->clk);
 		dev_err(dev, "Failed to get clock: %d\n", ret);
 		return ret;
 	}
 
-	ret = clk_prepare_enable(mbox->clk);
-	if (ret) {
-		dev_err(dev, "Failed to enable clock: %d\n", ret);
-		return ret;
-	}
-
 	reset = devm_reset_control_get_exclusive(dev, NULL);
 	if (IS_ERR(reset)) {
 		ret = PTR_ERR(reset);
 		dev_err(dev, "Failed to get reset control: %d\n", ret);
-		goto err_disable_unprepare;
+		return ret;
 	}
 
 	/*
@@ -242,25 +246,17 @@ static int sun6i_msgbox_probe(struct platform_device *pdev)
 	ret = reset_control_deassert(reset);
 	if (ret) {
 		dev_err(dev, "Failed to deassert reset: %d\n", ret);
-		goto err_disable_unprepare;
+		return ret;
 	}
 
-	mbox->regs = devm_platform_ioremap_resource(pdev, 0);
-	if (IS_ERR(mbox->regs)) {
-		ret = PTR_ERR(mbox->regs);
-		dev_err(dev, "Failed to map MMIO resource: %d\n", ret);
-		goto err_disable_unprepare;
-	}
+	mbox->regs = regs;
 
 	/* Disable all IRQs for this end of the msgbox. */
 	writel(0, mbox->regs + LOCAL_IRQ_EN_REG);
 
-	ret = devm_request_irq(dev, irq_of_parse_and_map(dev->of_node, 0),
-			       sun6i_msgbox_irq, 0, dev_name(dev), mbox);
-	if (ret) {
-		dev_err(dev, "Failed to register IRQ handler: %d\n", ret);
-		goto err_disable_unprepare;
-	}
+	ret = devm_request_irq(dev, irq, sun6i_msgbox_irq, 0, dev_name(dev), mbox);
+	if (ret)
+		return ret;
 
 	mbox->controller.dev           = dev;
 	mbox->controller.ops           = &sun6i_msgbox_chan_ops;
@@ -271,29 +267,8 @@ static int sun6i_msgbox_probe(struct platform_device *pdev)
 	mbox->controller.txpoll_period = 5;
 
 	spin_lock_init(&mbox->lock);
-	platform_set_drvdata(pdev, mbox);
-
-	ret = mbox_controller_register(&mbox->controller);
-	if (ret) {
-		dev_err(dev, "Failed to register controller: %d\n", ret);
-		goto err_disable_unprepare;
-	}
-
-	return 0;
-
-err_disable_unprepare:
-	clk_disable_unprepare(mbox->clk);
-
-	return ret;
-}
-
-static void sun6i_msgbox_remove(struct platform_device *pdev)
-{
-	struct sun6i_msgbox *mbox = platform_get_drvdata(pdev);
 
-	mbox_controller_unregister(&mbox->controller);
-	/* See the comment in sun6i_msgbox_probe about the reset line. */
-	clk_disable_unprepare(mbox->clk);
+	return devm_mbox_controller_register(dev, &mbox->controller);
 }
 
 static const struct of_device_id sun6i_msgbox_of_match[] = {
@@ -308,7 +283,6 @@ static struct platform_driver sun6i_msgbox_driver = {
 		.of_match_table = sun6i_msgbox_of_match,
 	},
 	.probe = sun6i_msgbox_probe,
-	.remove = sun6i_msgbox_remove,
 };
 module_platform_driver(sun6i_msgbox_driver);
 
-- 
2.55.0

Re: [PATCHv2] mailbox: sun6i: modernize probe and convert to fully managed

[sashiko-bot]

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

Pre-existing issues:
- [High] Concurrent execution of the IRQ handler (`sun6i_msgbox_irq`) and the channel shutdown path (`sun6i_msgbox_shutdown`) can lead to a NULL pointer dereference of the mailbox client structure.
--

--- Patch [1]: [PATCHv2] mailbox: sun6i: modernize probe and convert to fully managed ---
commit 434af1afe360881d346c32eafbc6541d4984e655
Author: Rosen Penev <rosenp@gmail.com>

mailbox: sun6i: modernize probe and convert to fully managed

This commit modernizes the probe function by replacing irq_of_parse_and_map
with platform_get_irq, and converting clk, reset, and controller
registration to their fully devres-managed variants. It eliminates the
manual error paths and the remove callback.

> diff --git a/drivers/mailbox/sun6i-msgbox.c b/drivers/mailbox/sun6i-msgbox.c
> --- a/drivers/mailbox/sun6i-msgbox.c
> +++ b/drivers/mailbox/sun6i-msgbox.c
[ ... ]
> @@ -242,25 +246,17 @@ static int sun6i_msgbox_probe(struct platform_device *pdev)
[ ... ]
> -	ret = devm_request_irq(dev, irq_of_parse_and_map(dev->of_node, 0),
> -			       sun6i_msgbox_irq, 0, dev_name(dev), mbox);
> -	if (ret) {
> -		dev_err(dev, "Failed to register IRQ handler: %d\n", ret);
> -		goto err_disable_unprepare;
> -	}
> +	ret = devm_request_irq(dev, irq, sun6i_msgbox_irq, 0, dev_name(dev), mbox);
> +	if (ret)
> +		return ret;

[Severity: High]
This isn't a bug introduced by this patch, but does the interrupt handler
have a race condition with the channel shutdown path?

If CPU0 calls mbox_free_channel() and invokes sun6i_msgbox_shutdown(),
it masks the interrupt in the hardware but does not synchronize with
already-running handlers:

drivers/mailbox/sun6i-msgbox.c:sun6i_msgbox_shutdown() {
    ...
    if (readl(mbox->regs + CTRL_REG(n)) & CTRL_RX(n)) {
        /* Disable the receive IRQ. */
        spin_lock(&mbox->lock);
        writel(readl(mbox->regs + LOCAL_IRQ_EN_REG) & ~RX_IRQ(n),
               mbox->regs + LOCAL_IRQ_EN_REG);
        spin_unlock(&mbox->lock);
    ...
}

If CPU1 is concurrently executing sun6i_msgbox_irq(), it can push data to
the framework without knowing the channel has been shut down:

drivers/mailbox/sun6i-msgbox.c:sun6i_msgbox_irq() {
    ...
    mbox_chan_received_data(chan, &msg);
    ...
}

Meanwhile, CPU0 can return from shutdown and set chan->cl to null in
mbox_clean_and_put_channel(). This causes CPU1 to trigger a null pointer
dereference when the framework locklessly accesses the client:

drivers/mailbox/mailbox.c:mbox_chan_received_data() {
    ...
    if (chan->cl->rx_callback)
        chan->cl->rx_callback(chan->cl, mssg);
    ...
}

Could this sequence actually occur and cause a crash?

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