Re: [PATCH v3 01/10] mailbox: imx: Forward the timeout/ error in imx_mu_generic_tx()

[sashiko-bot]

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

New issues:
- [High] Returning a negative error code (-ETIMEDOUT) from the mailbox send_data callback wedges the TX queue permanently.

Pre-existing issues:
- [High] Using readl_poll_timeout() with a timeout inside an atomic section creates up to 100ms of IRQ-disabled latency and is unsafe.
--

commit 00bd1a49fae92272b2f76397f788a118394fe7ae
Author: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>

mailbox: imx: Forward the timeout/ error in imx_mu_generic_tx()

This commit modifies imx_mu_generic_tx() to return the actual error code
(such as a timeout) instead of silently dropping it and returning 0. This
was intended to properly notify the caller of failures during the busy-wait
polling loop for TXDB_V2 channels.

> diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
> index 246a9a9e39520..0028073be4a71 100644
> --- a/drivers/mailbox/imx-mailbox.c
> +++ b/drivers/mailbox/imx-mailbox.c
> @@ -227,6 +227,7 @@ static int imx_mu_generic_tx(struct imx_mu_priv *priv,
>  	u32 val;
>  	int ret, count;
>  
> +	ret = 0;
>  	switch (cp->type) {

[ ... ]

> @@ -259,7 +260,7 @@ static int imx_mu_generic_tx(struct imx_mu_priv *priv,
>  		return -EINVAL;
>  	}
>  
> -	return 0;
> +	return ret;

[Severity: High]
Does returning a negative error code here stall the TX queue?

The mailbox framework does not propagate errors from the send_data
callback back to the caller. Instead, it treats non-zero return values
as a sign the controller is temporarily busy:

    drivers/mailbox/mailbox.c:msg_submit() {
        ...
        err = chan->mbox->ops->send_data(chan, data);
        if (!err) {
            chan->active_req = data;
            chan->msg_count--;
        }
        ...
    }

Because this now returns -ETIMEDOUT, the framework skips dequeuing the
message (chan->msg_count is not decremented) and does not set
chan->active_req. Because err is non-zero, no retry timer is started.

The failed message remains at the head of the TX queue, blocking all
future messages. Subsequent calls to mbox_send_message() will repeatedly
attempt to send the exact same message.

[Severity: High]
This is a pre-existing issue, but does the busy-wait loop in this function
cause high latency with interrupts disabled?

When handling IMX_MU_TYPE_TXDB_V2, imx_mu_generic_tx() calls
readl_poll_timeout() with a 10ms timeout and retries up to 10 times.
However, the caller holds a spinlock with interrupts disabled:

    drivers/mailbox/mailbox.c:msg_submit() {
        scoped_guard(spinlock_irqsave, &chan->lock) {
            ...
            err = chan->mbox->ops->send_data(chan, data);
            ...
        }
    }

This can result in up to 100ms of busy-waiting with interrupts
disabled, which can trigger watchdogs.

Furthermore, readl_poll_timeout() relies on ktime_get(), which the kernel
documentation explicitly forbids using in atomic contexts if a timeout
is specified.

>  }
>  
>  static int imx_mu_generic_rx(struct imx_mu_priv *priv,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260617-imx_mbox_rproc-v3-0-77948112defc@xxxxxxxxxxxxx?part=1