Re: [PATCH v7 6/6] spmi: apple: Add interrupt functionality

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Spurious completions during timeout recovery cause subsequent commands to fail with -EIO.
- [High] Using a 64-bit `readq` on a 32-bit peripheral register array causes a bus fault (SError) on Apple SoCs.
- [Medium] Incorrect use of register accessors (`writel`/`readl`) with `memcpy()` for FIFO access causes byte reversal on big-endian architectures.
--

--- Patch [6]: [PATCH v7 6/6] spmi: apple: Add interrupt functionality ---
commit ed584cbe528c0915b9974fbcae3310e9b639b962
Author: Alba Mendez <me@alba.sh>

spmi: apple: Add interrupt functionality

Add support for interrupts sent by slave devices and use IRQ for
RX FIFO if possible, as that IRQ fires as soon as the reply is
available, which is usually takes a few us instead of the 10ms sleep
interval for polling.

> diff --git a/drivers/spmi/spmi-apple-controller.c b/drivers/spmi/spmi-apple-controller.c
> index fabccd25aa0da..2390174452ab4 100644
> --- a/drivers/spmi/spmi-apple-controller.c
> +++ b/drivers/spmi/spmi-apple-controller.c

[ ... ]

> @@ -84,8 +165,10 @@ static int spmi_raw_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid, u16 param,
>  
>  	if (spmi->prev_fail) {
>  		writel(SPMI_ACT_FIFO_FLUSH, spmi->regs + SPMI_ACT_REG);
> +		apple_spmi_irq_ack_raw(spmi, SPMI_IRQ_NOTIFY);
>  		spmi->prev_fail = false;
>  	}
> +	reinit_completion(&spmi->fifo_rx);
>  
>  	writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);

[Severity: High]
Can a delayed interrupt cause spurious completions here?

If a previous command times out, apple_spmi_wait_rx_not_empty() returns
-ETIMEDOUT and sets prev_fail = true.

On the next command, spmi_raw_cmd() executes the recovery path and then calls
reinit_completion(). If a delayed interrupt arrives concurrently and the
IRQ handler calls complete() after reinit_completion() has executed, the next
command's wait_for_completion_timeout() could return immediately before the
actual reply is ready. This would lead to an -EIO failure.

Does the recovery path need to synchronize with the IRQ handler to prevent
this?

[Severity: Medium]
Does using writel() and readl() with memcpy() later in this function cause
byte reversal on big-endian architectures?

When spmi_raw_cmd() handles payload data:

drivers/spmi/spmi-apple-controller.c:spmi_raw_cmd() {
    ...
    while (i < len_wr) {
        j = min_t(size_t, sizeof(spmi_cmd), len_wr - i);
        spmi_cmd = 0;
        memcpy(&spmi_cmd, buf_wr + i, j);
        writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);
        i += j;
    }
    ...
}

Using memcpy() to copy data to or from a u32 variable followed by writel()
or readl() causes implicit byte-swapping on big-endian CPUs. While Apple
SoCs run in little-endian mode, this could corrupt data during SPMI transfers
if the driver is compiled and run on a big-endian system (e.g., during
COMPILE_TEST).

Should this use stream-oriented accessors like writesl() and readsl() to
ensure consistent endianness?

[ ... ]

> +static void apple_spmi_irq_handler(struct irq_desc *desc)
> +{
> +	struct apple_spmi *spmi = irq_desc_get_handler_data(desc);
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	bool handled = false;
> +	unsigned long val, offset, bit;
> +
> +	chained_irq_enter(chip, desc);
> +	val = readl(spmi->regs + SPMI_IRQ_ACK_BASE + (SPMI_IRQ_NOTIFY / 32) * 4);
> +	if (val & BIT(SPMI_IRQ_NOTIFY % 32)) {
> +		apple_spmi_irq_ack_raw(spmi, SPMI_IRQ_NOTIFY);
> +		complete(&spmi->fifo_rx);
> +		handled = true;
> +	}
> +
> +	for (offset = 0; offset < SPMI_NUM_PERIPHERAL_IRQS / 8; offset += sizeof(val)) {
> +		val = readq(spmi->regs + SPMI_IRQ_ACK_BASE + offset);

[Severity: High]
Does reading a 32-bit peripheral register using a 64-bit readq() cause a bus
fault on Apple SoCs?

Apple SoC peripheral fabrics strictly enforce MMIO access widths. Issuing a
64-bit read to the 32-bit SPMI_IRQ_ACK_BASE registers generates a Synchronous
External Abort (SError), resulting in a kernel panic when a peripheral
interrupt fires.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818-t603x-spmi-v7-0-dafebe6e7739@chaosmail.tech?part=6