Re: [PATCH v2 05/10] mailbox: apple: Add DockChannel FIFO controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] mailbox: apple: interrupt storm if TX FIFO not empty during IRQ_TX
- [High] mailbox: apple: data corruption due to asymmetric byte lane mapping on TX
- [High] mailbox: apple: TOCTOU race clearing IRQ_FLAG loses TX/RX events
--

commit 93fab252e833f92aad9624f1eae3f18e4b2d3e72
Author: Michael Reeves <michael.reeves077@gmail.com>

mailbox: apple: Add DockChannel FIFO controller

DockChannel is a hardware FIFO used by Apple coprocessors for
low-latency byte-stream communication with the AP. Add a mailbox
controller that preallocates RX storage, tracks IRQ enable state
in software, and reports TX completion from the TX-empty interrupt.

> diff --git a/drivers/mailbox/apple-dockchannel.c b/drivers/mailbox/apple-dockchannel.c
> index 0000000000000..d0a66e1b786cd
> --- /dev/null
> +++ b/drivers/mailbox/apple-dockchannel.c
[ ... ]
> +static void apple_dockchannel_irq_enable(struct apple_dockchannel *dc, u32 bits)
> +{
> +	/*
> +	 * IRQ_FLAG is write-to-clear. Clear stale latched flags before
> +	 * unmasking so the next interrupt reflects current FIFO state.
> +	 */
> +	writel_relaxed(bits, dc->irq_base + IRQ_FLAG);

[Severity: High]
Does clearing IRQ_FLAG unconditionally before unmasking create a TOCTOU race
that loses events?

For example, in apple_dockchannel_send_data():
1. apple_dockchannel_write_pending() runs and the coprocessor starts draining
   the FIFO.
2. The coprocessor drains the TX FIFO and the hardware latches IRQ_TX.
3. apple_dockchannel_irq_enable(dc, IRQ_TX) unconditionally clears the latched
   IRQ_TX completion flag.

Can this result in permanent loss of TX completion interrupts, or similarly
swallow RX data interrupts during apple_dockchannel_startup(), leading to a
channel deadlock?

> +	apple_dockchannel_irq_update(dc, bits, true);
> +}
[ ... ]
> +static void apple_dockchannel_write_pending(struct apple_dockchannel *dc)
> +{
> +	size_t left = dc->tx_len - dc->tx_pos;
> +	const u8 *p = dc->tx_buf + dc->tx_pos;
> +
> +	while (left) {
> +		size_t avail;
> +		size_t block;
> +
> +		avail = readl_relaxed(dc->data_base + DATA_TX_FREE);
> +		if (!avail)
> +			break;
> +
> +		block = min(left, avail);
> +
> +		while (block >= sizeof(u32)) {
> +			writel_relaxed(get_unaligned_le32(p),
> +				       dc->data_base + DATA_TX32);
> +			p += sizeof(u32);
> +			left -= sizeof(u32);
> +			block -= sizeof(u32);
> +		}
> +
> +		while (block) {
> +			writel_relaxed(*p++, dc->data_base + DATA_TX8);

[Severity: High]
Does this place the remainder byte in the wrong byte lane?

The RX path in apple_dockchannel_read() documents that the hardware uses
bits [15:8] for 8-bit FIFO accesses, shifting the read value by 8:

    *p++ = readl_relaxed(dc->data_base + DATA_RX8) >> 8;

Should the TX path also shift the byte << 8 to align with the hardware's
expected byte lane, avoiding data corruption for unaligned payloads?

> +			left--;
> +			block--;
> +		}
> +	}
> +
> +	dc->tx_pos = dc->tx_len - left;
> +}
[ ... ]
> +static irqreturn_t apple_dockchannel_irq_thread(int irq, void *data)
> +{
> +	struct apple_dockchannel *dc = data;
> +	unsigned long irqflags;
> +	u32 flags;
> +	u32 pending;
> +	bool tx_done = false;
> +
> +	flags = readl_relaxed(dc->irq_base + IRQ_FLAG);
> +
> +	spin_lock_irqsave(&dc->lock, irqflags);
> +
> +	pending = flags & dc->irq_mask & (IRQ_TX | IRQ_RX);
> +	if (!pending)
> +		goto out_unlock_none;
> +
> +	if (pending & IRQ_TX) {
> +		if (apple_dockchannel_tx_empty(dc)) {
> +			apple_dockchannel_irq_disable(dc, IRQ_TX);
> +			tx_done = dc->tx_active;
> +			dc->tx_active = false;
> +		} else {
> +			pending &= ~IRQ_TX;
> +		}
> +	}
> +
> +	writel_relaxed(pending, dc->irq_base + IRQ_FLAG);

[Severity: High]
If IRQ_TX is removed from the pending mask when the FIFO is not empty, does
it prevent the hardware latch from being cleared?

Since IRQ_FLAG is a write-to-clear register, writing back the modified mask
with IRQ_TX cleared will fail to clear the IRQ_TX hardware latch. Will this
cause an infinite interrupt storm upon returning from the handler?

> +
> +	spin_unlock_irqrestore(&dc->lock, irqflags);
> +
> +	if (tx_done)
> +		mbox_chan_txdone(&dc->chan, 0);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260918-apple-mtp-keyboard-final-v2-0-a5c685e50b60@gmail.com?part=5