Re: [PATCH v2 05/10] mailbox: apple: Add DockChannel FIFO controller
flat view
From: sashiko-bot@kernel.org
Date: 2026-09-18 11:19:32
Also in:
linux-devicetree
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 [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. Add a mailbox controller that preallocates RX storage, tracks IRQ enable state in software, and reports TX completion from the TX-empty interrupt.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
+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); +}
[ ... ]
quoted hunk ↗ jump to hunk
+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?
quoted hunk ↗ jump to hunk
+ left--; + block--; + } + } + + dc->tx_pos = dc->tx_len - left; +}
[ ... ]
quoted hunk ↗ jump to hunk
+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?
quoted hunk ↗ jump to hunk
+ + 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