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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] mailbox: apple: lost TX interrupt due to clearing IRQ_FLAG after transmission starts
- [High] mailbox: apple: TOCTOU race in IRQ handler clears newly asserted interrupts
- [Medium] mailbox: apple: interrupt storm when IRQ_TX fires but FIFO is not empty
--

commit 2a04249ed27a338883aa783b0cfe092f13b2057a
Author: Michael Reeves [off-list ref]

mailbox: apple: Add DockChannel FIFO controller

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

[ ... ]

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

[ ... ]

+	dc->tx_buf = msg->data;
+	dc->tx_len = msg->len;
+	dc->tx_pos = 0;
+	dc->tx_active = true;
+
+	apple_dockchannel_write_pending(dc);
+	writel_relaxed(APPLE_DOCKCHANNEL_FIFO_SIZE,
+		       dc->config_base + CONFIG_TX_THRESH);
+	apple_dockchannel_irq_enable(dc, IRQ_TX);

[Severity: High]
Does this create a race condition where a valid completion interrupt
is cleared?

The transmission is initiated by writing to the FIFO in
apple_dockchannel_write_pending(). The hardware can transmit the bytes
and become empty extremely fast. If it does so before the call to
apple_dockchannel_irq_enable() executes, the hardware latches the IRQ_TX
completion flag, and apple_dockchannel_irq_enable() immediately clears
it, potentially permanently hanging the channel.

Could the stale flags be cleared prior to writing the FIFO?

[ ... ]

+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: Medium]
Will this lead to an interrupt storm if the TX interrupt fires but the
FIFO is not empty?

Since IRQ_FLAG is a write-1-to-clear register, removing IRQ_TX from
pending intentionally skips clearing the active hardware interrupt in
this case. The handler returns while the unmasked interrupt is still
asserted, causing the IRQ controller to immediately re-fire the interrupt.

While this may require a spurious hardware interrupt or glitch to trigger,
it would result in an infinite interrupt loop that permanently hangs a
CPU core.

[ ... ]

+out_unlock_none:
+	spin_lock_irqrestore(&dc->lock, irqflags);
+
+	if (flags & (IRQ_TX | IRQ_RX))
+		writel_relaxed(flags & (IRQ_TX | IRQ_RX),
+			       dc->irq_base + IRQ_FLAG);

[Severity: High]
Can this write back a stale flags value, clearing newly asserted interrupts?

The flags value is read outside the lock at the beginning of the handler.
If the handler takes the out_unlock_none path (e.g., due to a concurrent
shutdown making pending == 0), it will write this stale flags value to
IRQ_FLAG locklessly. 

If another CPU executes apple_dockchannel_send_data() and completes a
transmission in the intervening window, this lockless write-1-to-clear
will wipe the newly generated IRQ_TX interrupt, hanging the mailbox channel.

+
+	return IRQ_NONE;
+}

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