Re: [PATCH v2 4/6] mailbox: Add AMD PL Mailbox soft IP controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] mailbox: partial FIFO write on TX busy in amd_pl_mbox_send_data()
- [High] mailbox: lost RX threshold interrupt race in amd_pl_mbox_startup()
- [Medium] mailbox: skipped error read in amd_pl_mbox_irq_thread() when ratelimited
--

commit f34934c4693ba6bdc80f192703145aea6291afe7
Author: Ben Levinsky <ben.levinsky@amd.com>

mailbox: Add AMD PL Mailbox soft IP controller

This commit adds a mailbox controller driver for the AMD Programmable Logic
Mailbox soft IP, exposing separate transmit and receive FIFO channels using
zynqmp_ipi_message payloads. It manages interface clock gating across channel
startup and shutdown, polling-based TX completion, and threaded IRQ-driven RX
FIFO draining.

> diff --git a/drivers/mailbox/amd-pl-mailbox.c b/drivers/mailbox/amd-pl-mailbox.c
> new file mode 100644
> index 0000000000000..dc95fe4eff9b6
> --- /dev/null
> +++ b/drivers/mailbox/amd-pl-mailbox.c
> @@ -0,0 +1,378 @@

[ ... ]

> +static int amd_pl_mbox_send_data(struct mbox_chan *chan, void *data)
> +{
> +	struct amd_pl_mbox *mb = chan->con_priv;
> +	struct zynqmp_ipi_message *msg = data;
> +	unsigned long flags;
> +	size_t offset;
> +	int ret = 0;
> +
> +	if (chan != &mb->chans[AMD_PL_MBOX_CHAN_TX])
> +		return -EINVAL;
> +
> +	/* The TX FIFO is empty here, since TX completion waits for it. */
> +	if (!msg || !msg->len || msg->len > mb->fifo_len * sizeof(u32))
> +		return -EINVAL;
> +
> +	/*
> +	 * The mailbox core can submit a queued message after shutdown() has
> +	 * started, while the interface clock is being disabled. Do not access
> +	 * registers then.
> +	 */
> +	spin_lock_irqsave(&mb->tx_lock, flags);
> +	if (!mb->tx_active) {
> +		ret = -ENODEV;
> +		goto unlock;
> +	}
> +
> +	for (offset = 0; offset < msg->len; offset += sizeof(u32)) {
> +		size_t count = min(sizeof(u32), msg->len - offset);
> +		u32 word = 0;
> +
> +		if (readl(mb->regs + AMD_PL_MBOX_STATUS) &
> +		    AMD_PL_MBOX_STS_TX_FULL) {
> +			ret = -EBUSY;
> +			goto unlock;
> +		}
> +
> +		memcpy(&word, &msg->data[offset], count);
> +		writel(word, mb->regs + AMD_PL_MBOX_WRDATA);
> +	}
> +
> +unlock:
> +	spin_unlock_irqrestore(&mb->tx_lock, flags);
> +	return ret;
> +}

[Severity: High]
In drivers/mailbox/amd-pl-mailbox.c, amd_pl_mbox_send_data() assumes the
hardware TX FIFO is empty on entry and checks AMD_PL_MBOX_STS_TX_FULL before
each word inside the loop. Can this corrupt or coalesce messages in the
hardware TX FIFO if a previous transfer timed out or if the FIFO becomes full
mid-message?

In drivers/mailbox/mailbox.c, when a blocking transfer (cl->tx_block) times
out in mbox_send_message(), it calls tx_tick(chan, -ETIME) -> msg_submit():

drivers/mailbox/mailbox.c:mbox_send_message()
	ret = wait_for_completion_timeout(&chan->tx_complete, wait);
	if (ret == 0) {
		t = -ETIME;
		tx_tick(chan, t);
	}

tx_tick() clears chan->active_req and invokes msg_submit(chan) to submit the
next queued message via ops->send_data() even if the remote peer has not yet
drained the timed-out message from the hardware TX FIFO (and similarly if a
client sets cl->knows_txdone).

Because amd_pl_mbox_send_data() does not check that the TX FIFO is empty
(AMD_PL_MBOX_STS_TX_THRESHOLD in AMD_PL_MBOX_STATUS) before writing:

1. If unconsumed words remain in the TX FIFO and space is still available, the
new message's words are written directly behind the old words, coalescing two
messages in the unframed hardware FIFO.

2. If AMD_PL_MBOX_STS_TX_FULL is hit after one or more words have already been
written (offset > 0), amd_pl_mbox_send_data() returns -EBUSY after partially
writing the message to AMD_PL_MBOX_WRDATA. In msg_submit():

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

When send_data() returns -EBUSY, the message is not dequeued and poll_hrt is
not started, leaving the partial words in the hardware FIFO while the full
message remains queued at the head of chan->msg_data[] to be sent again from
offset 0.

Should amd_pl_mbox_send_data() verify that AMD_PL_MBOX_STS_TX_THRESHOLD is set
before writing any words to AMD_PL_MBOX_WRDATA?

[ ... ]

> +static int amd_pl_mbox_startup(struct mbox_chan *chan)
> +{
> +	struct amd_pl_mbox *mb = chan->con_priv;
> +	unsigned long flags;
> +	u32 irq_enable;
> +	int ret;
> +
> +	if (chan != &mb->chans[AMD_PL_MBOX_CHAN_TX] &&
> +	    chan != &mb->chans[AMD_PL_MBOX_CHAN_RX])
> +		return -EINVAL;
> +
> +	ret = clk_prepare_enable(mb->clk);
> +	if (ret)
> +		return ret;
> +
> +	if (chan == &mb->chans[AMD_PL_MBOX_CHAN_TX]) {
> +		writel(AMD_PL_MBOX_CTRL_TX_RESET, mb->regs + AMD_PL_MBOX_CTRL);
> +		writel(0, mb->regs + AMD_PL_MBOX_SEND_THRESHOLD);
> +
> +		spin_lock_irqsave(&mb->tx_lock, flags);
> +		mb->tx_active = true;
> +		spin_unlock_irqrestore(&mb->tx_lock, flags);
> +
> +		return 0;
> +	}
> +
> +	writel(AMD_PL_MBOX_CTRL_RX_RESET, mb->regs + AMD_PL_MBOX_CTRL);
> +	/* Interrupt when the RX FIFO changes from empty to nonempty. */
> +	writel(0, mb->regs + AMD_PL_MBOX_RECV_THRESHOLD);
> +	writel(AMD_PL_MBOX_IRQ_RX_THRESHOLD | AMD_PL_MBOX_IRQ_ERROR,
> +	       mb->regs + AMD_PL_MBOX_IRQ_STATUS);
> +	irq_enable = readl(mb->regs + AMD_PL_MBOX_IRQ_ENABLE);
> +	irq_enable |= AMD_PL_MBOX_IRQ_RX_THRESHOLD | AMD_PL_MBOX_IRQ_ERROR;
> +	writel(irq_enable, mb->regs + AMD_PL_MBOX_IRQ_ENABLE);
> +	enable_irq(mb->irq);
> +
> +	return 0;
> +}

[Severity: High]
In drivers/mailbox/amd-pl-mailbox.c, when amd_pl_mbox_startup() initializes
the RX channel, can a word arriving from the remote peer between the RX FIFO
reset and the AMD_PL_MBOX_IRQ_STATUS clear permanently wedge RX interrupts?

As noted in the driver comments, with AMD_PL_MBOX_RECV_THRESHOLD set to 0,
AMD_PL_MBOX_IRQ_RX_THRESHOLD is edge-latched only when the RX FIFO transitions
from empty to nonempty. In amd_pl_mbox_startup():

	writel(AMD_PL_MBOX_CTRL_RX_RESET, mb->regs + AMD_PL_MBOX_CTRL);
	/* Interrupt when the RX FIFO changes from empty to nonempty. */
	writel(0, mb->regs + AMD_PL_MBOX_RECV_THRESHOLD);
	writel(AMD_PL_MBOX_IRQ_RX_THRESHOLD | AMD_PL_MBOX_IRQ_ERROR,
	       mb->regs + AMD_PL_MBOX_IRQ_STATUS);

If the remote peer writes a word after AMD_PL_MBOX_CTRL_RX_RESET empties the
FIFO, that 0-to-1 occupancy transition latches AMD_PL_MBOX_IRQ_RX_THRESHOLD.
The subsequent writel() to AMD_PL_MBOX_IRQ_STATUS then clears that latched
interrupt status bit while the received word remains unread in the RX FIFO.

Because the RX FIFO is now already nonempty and amd_pl_mbox_startup() does not
reset or drain the FIFO after clearing AMD_PL_MBOX_IRQ_STATUS, neither the
unread word nor any subsequent words written by the peer will cause an
empty-to-nonempty transition, so AMD_PL_MBOX_IRQ_RX_THRESHOLD will never
latch again.

Should AMD_PL_MBOX_RECV_THRESHOLD and AMD_PL_MBOX_IRQ_STATUS be programmed
before issuing AMD_PL_MBOX_CTRL_RX_RESET (or should the RX FIFO be checked
after enabling the interrupt)?

[ ... ]

> +static irqreturn_t amd_pl_mbox_irq_thread(int irq, void *data)
> +{
> +	struct amd_pl_mbox *mb = data;
> +	u32 pending_intr;
> +	u32 irq_enable;
> +	u32 irq_status;
> +
> +	irq_status = readl(mb->regs + AMD_PL_MBOX_IRQ_STATUS);
> +	irq_enable = readl(mb->regs + AMD_PL_MBOX_IRQ_ENABLE);
> +	pending_intr = irq_status & irq_enable;
> +
> +	if (!pending_intr) {
> +		dev_warn_ratelimited(mb->controller.dev,
> +				     "unexpected interrupt: status %#x enable %#x\n",
> +				     irq_status, irq_enable);
> +		return IRQ_NONE;
> +	}
> +
> +	/*
> +	 * Acknowledge before draining. The receive event latches when the RX
> +	 * FIFO goes from empty to nonempty, so a word that arrives after the
> +	 * drain has emptied the FIFO raises a new event.
> +	 */
> +	writel(pending_intr, mb->regs + AMD_PL_MBOX_IRQ_STATUS);
> +
> +	if (pending_intr & AMD_PL_MBOX_IRQ_ERROR)
> +		dev_warn_ratelimited(mb->controller.dev,
> +				     "mailbox FIFO error: %#x\n",
> +				     readl(mb->regs + AMD_PL_MBOX_ERROR));

[Severity: Medium]
In drivers/mailbox/amd-pl-mailbox.c, amd_pl_mbox_irq_thread() passes
readl(mb->regs + AMD_PL_MBOX_ERROR) directly as an argument to
dev_warn_ratelimited().

In include/linux/dev_printk.h, dev_warn_ratelimited() expands to
dev_level_ratelimited(), which only evaluates its variadic arguments when
__ratelimit(&_rs) returns true:

#define dev_level_ratelimited(dev_level, dev, fmt, ...)			\
do {									\
	static DEFINE_RATELIMIT_STATE(_rs,				\
				      DEFAULT_RATELIMIT_INTERVAL,	\
				      DEFAULT_RATELIMIT_BURST);		\
	if (__ratelimit(&_rs))						\
		dev_level(dev, fmt, ##__VA_ARGS__);			\
} while (0)

When AMD_PL_MBOX_IRQ_ERROR fires frequently enough to be rate-limited, will
skipping readl(mb->regs + AMD_PL_MBOX_ERROR) leave sticky error bits uncleared
in the hardware error register?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105051902.1646651-1-ben.levinsky@amd.com?part=4