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

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 <[email protected]>

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/[email protected]?part=1