Re: [PATCH v5 3/8] mailbox: apple: Add DockChannel FIFO controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] mailbox: apple: unhandled TX IRQ storm in
  apple_dockchannel_irq_thread()
- [Medium] mailbox: apple: premature RX IRQ_FLAG clear in
  apple_dockchannel_irq_thread()
- [Medium] mailbox: apple: TX queue stall when FIFO is non-empty with
  tx_active false
--

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

mailbox: apple: Add DockChannel FIFO controller

This commit adds a mailbox controller driver for the Apple DockChannel
hardware FIFO, handling TX completion and RX data in a threaded interrupt
handler.

> diff --git a/drivers/mailbox/apple-dockchannel.c b/drivers/mailbox/apple-dockchannel.c
> new file mode 100644
> index 0000000000000..4400d767f6ad5
> --- /dev/null
> +++ b/drivers/mailbox/apple-dockchannel.c

[ ... ]

> +static int apple_dockchannel_send_data(struct mbox_chan *chan, void *data)
> +{
> +	struct apple_dockchannel *dc = chan->con_priv;
> +	struct apple_dockchannel_msg *msg = data;
> +	unsigned long flags;
> +
> +	if (!msg || !msg->data || !msg->len)
> +		return -EINVAL;
> +
> +	if (msg->len > APPLE_DOCKCHANNEL_FIFO_SIZE)
> +		return -EMSGSIZE;
> +
> +	spin_lock_irqsave(&dc->lock, flags);
> +
> +	if (dc->tx_active || !apple_dockchannel_tx_empty(dc)) {
> +		spin_unlock_irqrestore(&dc->lock, flags);
> +		return -EBUSY;
> +	}

[Severity: Medium]
Can this stall the mailbox TX queue if the hardware TX FIFO is non-empty
while dc->tx_active is false?

When msg_submit() gets -EBUSY from send_data() on a txdone_irq controller,
it leaves the message queued in chan->msg_data and waits for
mbox_chan_txdone() to retry submission.  If residual bytes are in the FIFO
at apple_dockchannel_startup() or after apple_dockchannel_shutdown()
clears dc->tx_active and masks dc->irq_tx, returning -EBUSY here without
arming dc->irq_tx and setting dc->tx_active means no interrupt will fire
when the FIFO drains and mbox_chan_txdone() will never be called.

[ ... ]

> +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;
> +
> +	spin_lock_irqsave(&dc->lock, irqflags);
> +
> +	flags = readl_relaxed(dc->irq_base + IRQ_FLAG);
> +
> +	pending = flags & dc->irq_mask & (dc->irq_tx | dc->irq_rx);
> +	if (!pending)
> +		goto out_unlock_none;
> +
> +	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);
> +
> +	spin_unlock_irqrestore(&dc->lock, irqflags);
> +
> +	if (tx_done)
> +		mbox_chan_txdone(&dc->chan, 0);
> +
> +	if (!(pending & dc->irq_rx))
> +		return pending ? IRQ_HANDLED : IRQ_NONE;

[Severity: High]
Can clearing dc->irq_tx from pending when apple_dockchannel_tx_empty() is
false cause an unhandled IRQ storm?

In apple_dockchannel_send_data(), FIFO writes go to dc->data_base,
CONFIG_TX_THRESH goes to dc->config_base, and apple_dockchannel_irq_enable()
clears IRQ_FLAG and unmasks dc->irq_tx in dc->irq_base using relaxed MMIO
without a readback.  If dc->irq_tx is latched in IRQ_FLAG before
DATA_TX_FREE reaches APPLE_DOCKCHANNEL_FIFO_SIZE, removing dc->irq_tx from
pending skips clearing it in the write-to-clear IRQ_FLAG register while
leaving it unmasked in IRQ_MASK.  When dc->irq_rx is not also set, the
handler returns IRQ_NONE with the level-high interrupt still asserted,
re-entering the handler in a tight loop until genirq disables the IRQ line.

Also, because IRQ_FLAG is written after apple_dockchannel_tx_empty() is
checked, acknowledging IRQ_FLAG before checking DATA_TX_FREE is needed so a
FIFO that finishes draining between the check and the IRQ_FLAG write does
not lose its completion interrupt.

> +
> +	for (;;) {
> +		struct apple_dockchannel_msg msg;
> +		size_t avail;
> +
> +		avail = readl_relaxed(dc->data_base + DATA_RX_COUNT);
> +		if (!avail)
> +			break;
> +
> +		avail = min_t(size_t, avail, APPLE_DOCKCHANNEL_FIFO_SIZE);
> +
> +		apple_dockchannel_read(dc, dc->rx_buf, avail);
> +
> +		msg.data = dc->rx_buf;
> +		msg.len = avail;
> +		mbox_chan_received_data(&dc->chan, &msg);
> +	}
> +
> +	return IRQ_HANDLED;

[Severity: Medium]
Does clearing dc->irq_rx in IRQ_FLAG before draining the RX FIFO cause a
spurious interrupt after every RX transfer?

IRQ_FLAG is a sticky write-to-clear status register, and CONFIG_RX_THRESH
is set to 1.  When IRQ_FLAG is written before reading the FIFO,
DATA_RX_COUNT is still at least 1 (and more bytes may arrive while the loop
drains the FIFO), so dc->irq_rx immediately re-latches in IRQ_FLAG and triggers
an extra wakeup of apple_dockchannel_irq_thread() once the line is unmasked.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261108-b4-apple-mtp-keyboard-v5-0-e2c97b82669a@gmail.com?part=3