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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] io: endianness bug when reading/writing FIFO data with readl/writel
- [High] mailbox: missed RX interrupt edge transition when FIFO is completely full
--

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

mailbox: Add AMD PL Mailbox soft IP controller

Adds a mailbox framework controller for the AMD programmable-logic Mailbox soft
IP. The controller exposes separate transmit and receive FIFO channels,
supporting interrupt-driven receive notifications and polling-based transmit
completion.

> diff --git a/drivers/mailbox/amd-pl-mailbox.c b/drivers/mailbox/amd-pl-mailbox.c
> new file mode 100644
> index 0000000000000..bd6595332f533
> --- /dev/null
> +++ b/drivers/mailbox/amd-pl-mailbox.c

[ ... ]

> +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;
> +	size_t offset;
> +
> +	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;
> +
> +	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)
> +			return -EBUSY;
> +
> +		memcpy(&word, &msg->data[offset], count);
> +		writel(word, mb->regs + AMD_PL_MBOX_WRDATA);

[Severity: High]
Will this corrupt the byte order of the stream payload on big-endian
architectures? writel performs implicit CPU-to-device byteswapping.
For bulk FIFO stream data, should this use writesl to preserve the byte
layout of the zynqmp_ipi_message stream when crossing endianness boundaries?

> +	}
> +
> +	return 0;
> +}

[ ... ]

> +static void amd_pl_mbox_rx(struct amd_pl_mbox *mb)
> +{
> +	/* data[] follows a size_t in a kzalloc() buffer: u32 aligned. */
> +	u32 *rx_words = (u32 *)mb->rx_msg->data;
> +	u32 words;
> +
> +	/*
> +	 * The hardware reports neither the peer's message length nor the FIFO
> +	 * occupancy. Read until the FIFO is empty, up to one FIFO depth, and
> +	 * deliver the words as one message.
> +	 */
> +	for (words = 0; words < mb->fifo_len; words++) {
> +		if (readl(mb->regs + AMD_PL_MBOX_STATUS) &
> +		    AMD_PL_MBOX_STS_RX_EMPTY)
> +			break;
> +
> +		rx_words[words] = readl(mb->regs + AMD_PL_MBOX_RDDATA);

[Severity: High]
Similarly, does using readl for stream data cause byte swapping issues on
big-endian systems? Should this use readsl to read the FIFO without unwanted
endianness conversion?

> +	}
> +
> +	if (words) {
> +		mb->rx_msg->len = words * sizeof(u32);
> +		mbox_chan_received_data(&mb->chans[AMD_PL_MBOX_CHAN_RX],
> +					mb->rx_msg);
> +	}
> +}
> +
> +static void amd_pl_mbox_rx_rearm(struct amd_pl_mbox *mb)
> +{
> +	/*
> +	 * The receive event latches only when the FIFO goes from empty to
> +	 * nonempty. A word that arrived after the drain, but before the event
> +	 * was acknowledged, had its event cleared, and words left by the
> +	 * bounded drain never raised one. Raise the threshold and lower it
> +	 * again to latch a new event, so that the interrupt fires for them.
> +	 * This cannot latch an event while the FIFO is full, which happens only
> +	 * if the peer writes a full FIFO depth during one interrupt.
> +	 */
> +	if (!(readl(mb->regs + AMD_PL_MBOX_STATUS) &
> +	      AMD_PL_MBOX_STS_RX_EMPTY)) {
> +		writel(mb->fifo_len - 1, mb->regs + AMD_PL_MBOX_RECV_THRESHOLD);
> +		writel(0, mb->regs + AMD_PL_MBOX_RECV_THRESHOLD);

[Severity: High]
Can this permanently lose the RX interrupt if the FIFO becomes completely full?

If the FIFO has exactly fifo_len words, both occupancy > fifo_len - 1 and
occupancy > 0 evaluate to true. Without a false-to-true edge transition, the
hardware might not latch the new event, leaving data stuck in the FIFO.

> +	}
> +}

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