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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The IRQ teardown function incorrectly unmasks all hardware interrupts instead of masking them, leading to a potential interrupt storm on module removal.
- [High] A race condition during timeout recovery can cause the next command to spuriously fail with `-EIO` due to a late interrupt completion.
- [Medium] The IRQ handler uses `readq()` to read two 32-bit registers at once, which may cause a synchronous external abort (SError) if the peripheral bus strictly limits accesses to 32-bit.
--

--- Patch [6]: [PATCH v5 6/6] spmi: apple: Add interrupt functionality ---
commit cbf4e6ab067495af30bb6cec8a0023d78009efc4
Author: Alba Mendez <[email protected]>

spmi: apple: Add interrupt functionality

This commit adds support for interrupts sent by slave devices on the Apple
SPMI controller. It utilizes an IRQ for the RX FIFO when available, which
reduces the response latency compared to a 10ms polling sleep interval.

> diff --git a/drivers/spmi/spmi-apple-controller.c b/drivers/spmi/spmi-apple-controller.c
> index fabccd25aa0da..1e6cc30da1965 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);

[Severity: High]
Could this reinit_completion() race with a late completion from the IRQ
handler?

If a previous command times out, spmi_raw_cmd() attempts to recover by
flushing the FIFO and calling reinit_completion(). Concurrently, a delayed
hardware interrupt might trigger apple_spmi_irq_handler().

If the handler reads the ACK register before the recovery clears it, but
calls complete() right after this reinit_completion() runs, the new
command's wait_for_completion_timeout() will return immediately. Because
the FIFO is actually empty, this falsely aborts the next valid command with
-EIO.

[ ... ]
> +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: Medium]
Is it safe to use readq() here to read two adjacent 32-bit registers?

Apple Silicon APB peripherals commonly enforce strict 32-bit access widths.
If the peripheral bus rejects 64-bit accesses, this single readq() could
cause a synchronous external abort (SError) resulting in a kernel panic.

Would it be safer to use two 32-bit readl() calls instead?

> +		/**
> +		 * because of other masters in the bus, we're going to get a multitude of
> +		 * interrupts we're not interested in. irq_resolve_mapping isn't very
> +		 * optimized for the nonexistent path, so instead we mask with (a locally
> +		 * cached version of) the IRQ mask
> +		 */
> +		val &= spmi->irq_mask_cache[offset / sizeof(val)];
> +		for_each_set_bit(bit, &val, 64) {
> +			generic_handle_domain_irq(spmi->irqd, offset * 8 + bit);
> +			handled = true;
> +		}
> +	}
> +	if (!handled)
> +		handle_bad_irq(desc);
> +	chained_irq_exit(chip, desc);
> +}
> +
> +static void apple_spmi_teardown_irq(void *data)
> +{
> +	struct apple_spmi *spmi = data;
> +
> +	for (size_t offset = 0; offset < SPMI_NUM_IRQS / 8; offset += 4)
> +		writel(U32_MAX, spmi->regs + SPMI_IRQ_MASK_BASE + offset);

[Severity: High]
Does writing U32_MAX here accidentally unmask all interrupts on module
removal?

In apple_spmi_init_irq(), 0 is written to safely mask everything, which
implies the hardware uses 1 to unmask. Writing U32_MAX here fully unmasks
all 288 peripheral interrupts on the controller just before the chained
handler is removed.

Could this lead to an unhandled interrupt storm that locks up a CPU or
forces the generic IRQ subsystem to permanently disable the shared parent
IRQ line?

> +
> +	synchronize_irq(spmi->irq);
> +	irq_set_chained_handler_and_data(spmi->irq, NULL, NULL);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6