Re: [PATCH v4 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] irq: infinite IRQ storm on early/spurious TX interrupt
- [High] mailbox: TOCTOU race loses TX completion interrupt
- [High] mailbox: missed RX interrupt on startup drops pre-existing data
--

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

mailbox: apple: Add DockChannel FIFO controller

This adds a driver for Apple's DockChannel hardware FIFO used by coprocessors.
It exposes the FIFO pair as a Linux mailbox channel and handles interrupts
in a thread.

> diff --git a/drivers/mailbox/apple-dockchannel.c b/drivers/mailbox/apple-dockchannel.c
> new file mode 100644
> --- /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);
> +	apple_dockchannel_irq_update(dc, bits, true);
> +}

[ ... ]

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

[ ... ]

> +	dc->tx_active = true;
> +
> +	apple_dockchannel_write(dc, msg->data, msg->len);
> +	writel_relaxed(APPLE_DOCKCHANNEL_FIFO_SIZE,
> +		       dc->config_base + CONFIG_TX_THRESH);
> +	apple_dockchannel_irq_enable(dc, dc->irq_tx);

[Severity: High]
If the coprocessor is fast enough to drain the FIFO before
apple_dockchannel_irq_enable() executes, the hardware might latch the
TX-empty interrupt flag.

Since apple_dockchannel_irq_enable() unconditionally writes to IRQ_FLAG
to clear latched flags, does this destroy a valid completion interrupt
and stall the channel permanently?

> +
> +	spin_unlock_irqrestore(&dc->lock, flags);
> +
> +	return 0;
> +}
> +
> +static int apple_dockchannel_startup(struct mbox_chan *chan)
> +{
> +	struct apple_dockchannel *dc = chan->con_priv;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&dc->lock, flags);
> +	/*
> +	 * The mailbox framework has no per-client RX threshold. Use byte
> +	 * granularity because UART-style DockChannel clients require it.
> +	 */
> +	writel_relaxed(1, dc->config_base + CONFIG_RX_THRESH);
> +	apple_dockchannel_irq_enable(dc, dc->irq_rx);

[Severity: High]
During startup, apple_dockchannel_irq_enable() unconditionally clears
latched RX interrupts.

If the coprocessor sent data to the FIFO before the mailbox was opened,
does this clear the latched interrupt without reading the data? 

Since the FIFO is already above the threshold, this could strand pre-existing
data by not generating a new edge interrupt.

> +	spin_unlock_irqrestore(&dc->lock, flags);
> +
> +	enable_irq(dc->irq);
> +
> +	return 0;
> +}

[ ... ]

> +static irqreturn_t apple_dockchannel_irq_thread(int irq, void *data)
> +{

[ ... ]

> +	if (pending & dc->irq_tx) {
> +		if (apple_dockchannel_tx_empty(dc)) {
> +			apple_dockchannel_irq_disable(dc, dc->irq_tx);
> +			tx_done = dc->tx_active;
> +			dc->tx_active = false;
> +		} else {
> +			pending &= ~dc->irq_tx;
> +		}
> +	}
> +
> +	writel_relaxed(pending, dc->irq_base + IRQ_FLAG);

[ ... ]

> +	if (!(pending & dc->irq_rx))
> +		return pending ? IRQ_HANDLED : IRQ_NONE;

[Severity: High]
If the hardware triggers an early or spurious TX interrupt before the FIFO
is completely empty, apple_dockchannel_tx_empty() is false, which drops the
TX flag from the pending mask.

When pending is written back to IRQ_FLAG, the unmasked TX interrupt flag
is left uncleared in the hardware register.

If there is no RX interrupt pending, the function returns IRQ_NONE. Because
the interrupt is left asserted in hardware and unmasked at the controller,
will this cause an immediate re-trigger and an infinite IRQ storm?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260925-apple-mtp-keyboard-final-v4-0-304c267518f4@gmail.com?part=5