Re: [PATCH v2 2/2] mailbox: add Axiado AX3005 mailbox driver

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The RX thread blindly reads data from the FIFO if the poll loop breaks early due to an error, leading to hardware underflows and garbage data delivery.
- [High] Returning IRQ_NONE after successfully clearing FIFO errors can trigger the spurious interrupt detector and permanently disable the IRQ.
- [High] TOCTOU race in RX thread between checking the active flag and dereferencing the channel client, leading to a NULL pointer dereference.
- [Medium] The driver uses register I/O accessors (`writel`/`readl`) for FIFO bulk data transfers instead of the mandated stream I/O accessors (`writesl`/`readsl`).
--

--- Patch [2]: [PATCH v2 2/2] mailbox: add Axiado AX3005 mailbox driver ---
commit e614bfa4ee11362f9c00a58fdcc67351b8f3046c
Author: Swark Yang <[email protected]>

mailbox: add Axiado AX3005 mailbox driver

This commit adds a new mailbox controller driver for the Axiado AX3005 SoC.
It supports 8 TX and 8 RX channels using separate register regions, with
TX status via polling and RX via per-channel interrupts.

> diff --git a/drivers/mailbox/axiado-mailbox.c b/drivers/mailbox/axiado-mailbox.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/mailbox/axiado-mailbox.c

[ ... ]

> +static int axiado_mbox_send_data(struct mbox_chan *chan, void *data)
> +{

[ ... ]

> +	for (offset = 0; offset + sizeof(u32) <= msg_len;
> +	     offset += sizeof(u32))
> +		writel(get_unaligned_le32(buf + offset), priv->mbox_reg);

[Severity: Medium]
Since this loop writes bulk data to a FIFO, should it use the stream I/O
accessor writesl() instead of writel()?

The io-accessors subsystem guidelines mandate using stream accessors for
FIFOs to avoid unnecessary per-word memory barriers and redundant endianness
conversions.

> +
> +	tail_len = msg_len - offset;
> +	if (tail_len) {
> +		memcpy(tail, buf + offset, tail_len);
> +		writel(get_unaligned_le32(tail), priv->mbox_reg);
> +	}
> +
> +	dev_dbg(mb->mbox.dev, "%s: Ch-%u sent\n", __func__, idx);
> +
> +	return 0;
> +}
> +
> +static irqreturn_t axiado_rx_thread(int irq, void *dev_id)
> +{
> +	struct axiado_channel_data *priv = dev_id;
> +	struct mbox_chan *chan = priv->chan;
> +	struct axiado_mbox *mb = dev_get_drvdata(chan->mbox->dev);

[ ... ]

> +	/*
> +	 * Clear and log/count any pending errors, but don't discard data
> +	 * on their account alone: a rejected overflow write doesn't
> +	 * corrupt what was already safely queued ahead of it, so let the
> +	 * length-based read below decide whether what's here is usable.
> +	 */
> +	axiado_mbox_clear_fifo_errors(priv);
> +
> +	csr = readl(priv->csr_reg);
> +	if (csr & AXIADO_MBOX_CSR_EMPTY)
> +		return IRQ_NONE;

[Severity: High]
If the interrupt was asserted solely due to a hardware error (like an
underflow), axiado_mbox_clear_fifo_errors() successfully clears the condition.
If the FIFO is empty afterwards, this check returns IRQ_NONE.

Does this risk permanently disabling the IRQ line? Returning IRQ_NONE after
successfully servicing a hardware event can trigger the core spurious
interrupt detector.

> +
> +	/*
> +	 * The first word contains the total message length in bytes,
> +	 * including the length word itself.
> +	 */
> +	word = readl(priv->mbox_reg);
> +	msg_len = word;
> +	put_unaligned_le32(word, buf);
> +
> +	if (msg_len < sizeof(u32) || msg_len > mb->drv_data->msg_size)
> +		goto invalid_message;
> +
> +	num_words = DIV_ROUND_UP(msg_len, sizeof(u32));
> +	remaining = num_words - 1;
> +
> +	/*
> +	 * The not-empty interrupt may occur as soon as the first DW enters
> +	 * the FIFO. Wait until all remaining DWs of this message arrive.
> +	 */
> +	if (remaining) {
> +		ret = readl_poll_timeout(priv->csr_reg, csr,
> +					 (csr & AXIADO_MBOX_CSR_ERRORS) ||
> +					  FIELD_GET(AXIADO_MBOX_CSR_LEVEL, csr) >=
> +					  remaining,

[Severity: High]
The poll condition succeeds if AXIADO_MBOX_CSR_ERRORS is set, terminating
early without verifying the FIFO actually reached the required level.

If the loop exits early due to an error rather than the level check, the code
continues to the for-loop below and blindly reads remaining words from the
FIFO. Will this cause hardware underflows and pass garbage data to the
mailbox client?

> +					  AXIADO_MBOX_RX_POLL_US,
> +					  AXIADO_MBOX_RX_TIMEOUT_US);
> +		if (ret)
> +			goto incomplete_message;
> +
> +		axiado_mbox_clear_fifo_errors(priv);
> +	}
> +
> +	for (i = 1; i < num_words; i++) {
> +		word = readl(priv->mbox_reg);

[Severity: Medium]
Similar to the TX path, does this bulk FIFO read need to use readsl() instead
of readl() to avoid the per-word memory barriers and endianness swaps?

> +		put_unaligned_le32(word, buf + i * sizeof(u32));
> +	}
> +
> +	axiado_mbox_clear_fifo_errors(priv);
> +
> +	if (READ_ONCE(priv->active))
> +		mbox_chan_received_data(chan, priv->rx_buffer);

[Severity: High]
Can mbox_free_channel() nullify the channel client pointer while this thread
is preempted?

If a mailbox client frees the channel, mbox_free_channel() clears chan->cl
under a lock, releases the lock, and calls axiado_mbox_shutdown(). If this
threaded IRQ handler evaluates READ_ONCE(priv->active) as true just before
being preempted, the shutdown routine will block in disable_irq().

When this handler resumes, will mbox_chan_received_data() dereference a NULL
chan->cl pointer?

> +
> +	return IRQ_HANDLED;

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260817-upstream-axiado-ax3005-mailbox-upstream-v2-0-25e713eccff3@axiado.com?part=2