Re: [PATCH v3 05/10] mailbox: imx: Use channel index instead of zero in imx_mu_specific_rx()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unsafe hardware polling inside atomic context leads to deadlocks.
- [High] Error paths in `imx_mu_specific_rx()` leave the RX interrupt permanently masked.
- [High] Error paths in `imx_mu_seco_rxdb()` skip clearing GIP and leave the RXDB interrupt permanently masked.
- [High] PM runtime usage counter leak in `imx_mu_startup()` error path.
--

commit ab09c59a56c0e6ea31b7f95dd9b506c460b17d31
Author: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

mailbox: imx: Use channel index instead of zero in imx_mu_specific_rx()

imx_mu_specific_rx() masks channel 0 and unmasks it again at the end of
the function. Given that at startup the channel index got unmasked it
should do the right job.

> diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
> index a8d4e970fb786..408cd083c64ee 100644
> --- a/drivers/mailbox/imx-mailbox.c
> +++ b/drivers/mailbox/imx-mailbox.c
> @@ -381,7 +381,7 @@ static int imx_mu_specific_rx(struct imx_mu_priv *priv, struct imx_mu_con_priv *
>  
>  	data = (u32 *)priv->msg;
>  
> -	imx_mu_xcr_rmw(priv, IMX_MU_RCR, 0, IMX_MU_xCR_RIEn(priv->dcfg->type, 0));
> +	imx_mu_xcr_rmw(priv, IMX_MU_RCR, 0, IMX_MU_xCR_RIEn(priv->dcfg->type, cp->idx));
>  	*data++ = imx_mu_read(priv, priv->dcfg->xRR);
>  
>  	if (priv->dcfg->type & IMX_MU_V2_S4) {

[Severity: High]
This is a pre-existing issue, but the error paths in imx_mu_specific_rx() skip
the corresponding unmask operation.

If size exceeds max_size / 4 or if readl_poll_timeout() times out:

drivers/mailbox/imx-mailbox.c:imx_mu_specific_rx() {
...
	if (size > max_size / 4) {
		dev_err(priv->dev, "Maximal message size (%u bytes) exceeded on RX; got: %i bytes\n", max_size, size << 2);
		return -EINVAL;
	}
...
	for (i = 1; i < size; i++) {
		ret = readl_poll_timeout(...);
		if (ret) {
			dev_err(priv->dev, "timeout read idx %d\n", i);
			return ret;
		}
...
}

Does returning an error here skip unmasking the RIEn interrupt?

In the threaded IRQ handler imx_mu_isr_th(), the unmask is only performed if
this function returns 0. Could this leave the RX interrupt permanently masked
and cause a denial of service for the channel?

[ ... ]

> @@ -408,7 +408,7 @@ static int imx_mu_specific_rx(struct imx_mu_priv *priv, struct imx_mu_con_priv *
>  		*data++ = imx_mu_read(priv, priv->dcfg->xRR + (i % num_rr) * 4);
>  	}
>  
> -	imx_mu_xcr_set_act(priv, cp, IMX_MU_RCR, IMX_MU_xCR_RIEn(priv->dcfg->type, 0));
> +	imx_mu_xcr_set_act(priv, cp, IMX_MU_RCR, IMX_MU_xCR_RIEn(priv->dcfg->type, cp->idx));
>  	mbox_chan_received_data(cp->chan, (void *)priv->msg);
>  
>  	return 0;

[Severity: High]
This isn't a bug introduced by this patch, but are there unsafe hardware polling
loops inside atomic contexts elsewhere in this file?

The imx_mu_send_data() callback executes in atomic context (with chan->lock held
and interrupts disabled), but the underlying TX functions wait using mechanisms
that are unsafe in atomic context.

For example, imx_mu_tx_waiting_write():

drivers/mailbox/imx-mailbox.c:imx_mu_tx_waiting_write() {
...
	} while (!can_write && time_is_after_jiffies64(timeout_time));
...
}

There are also readl_poll_timeout() calls in imx_mu_generic_tx() and
imx_mu_specific_tx():

drivers/mailbox/imx-mailbox.c:imx_mu_specific_tx() {
...
		ret = readl_poll_timeout(priv->base + priv->dcfg->xSR[IMX_MU_TSR], xsr, xsr & IMX_MU_xSR_TEn(priv->dcfg->type, i % num_tr), 0, 5 * USEC_PER_SEC);
...
}

Can polling jiffies or using readl_poll_timeout() inside a spinlock with IRQs
disabled cause hard deadlocks, especially if the local CPU is responsible for
advancing the timer tick? Should readl_poll_timeout_atomic() be used here
instead?


[Severity: High]
This is a pre-existing issue, but do error paths in imx_mu_seco_rxdb() skip
clearing the GIP interrupt status?

drivers/mailbox/imx-mailbox.c:imx_mu_seco_rxdb() {
...
	if (byte_size > sizeof(*msg)) {
...
		err = -EINVAL;
		goto error;
	}
...
	for (i = 1; i < msg->hdr.size; i++) {
		err = imx_mu_rx_waiting_read(priv, arg++, i);
		if (err) {
			dev_err(priv->dev, "timeout read idx %d\n", i);
			goto error;
		}
	}
...
error:
	return err;
}

Because these paths jump directly to the error label, they skip the GSR
register write that clears the GIPn interrupt status.

Furthermore, returning an error skips the GIEn unmasking in imx_mu_isr_th().
Could this leave the RXDB interrupt permanently masked and cause spurious
interrupts if the channel is later closed and reopened?


[Severity: High]
This is a pre-existing issue, but does imx_mu_startup() leak the PM runtime
usage counter on error?

drivers/mailbox/imx-mailbox.c:imx_mu_startup() {
...
	ret = pm_runtime_get_sync(priv->dev);
...
	ret = request_threaded_irq(priv->irq[cp->type],
				   imx_mu_isr,
				   imx_mu_isr_th,
				   IRQF_SHARED,
				   cp->irq_desc, cp);
	if (ret) {
		dev_err(priv->dev, "Unable to acquire IRQ %d\n", priv->irq[cp->type]);
		return ret;
	}
...
}

If request_threaded_irq() fails, the function returns immediately without
calling pm_runtime_put_sync().

Since the mailbox core does not call shutdown() if startup() fails, does this
permanently prevent the device and its power domain from suspending?

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