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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Stale interrupt from a timed-out command can prematurely complete a subsequent command due to lack of synchronization.
- [High] Potential use-after-free of the `spmi` structure due to ineffective synchronization of the chained interrupt handler during device teardown.
- [High] Using a 64-bit `readq()` to read 32-bit peripheral registers is likely to trigger a synchronous external abort (SError) on Apple Silicon.
- [Low] Unprotected lockless read of `spmi->irq_mask_cache` in the interrupt handler.
--

commit 871fe99995b531329b52ffbebcd1520006e84bea
Author: Alba Mendez <me@alba.sh>
Subject: spmi: apple: Add interrupt functionality

This patch adds support for interrupts sent by slave devices and 
utilizes the IRQ for the RX FIFO to reduce polling latency.

> 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);

[Severity: High]
Could a stale interrupt from a previously timed-out command prematurely 
complete the new command here? 

If a prior command times out, the hardware might still assert the IRQ later. 
The IRQ handler will read the status and prepare to call 
complete(&spmi->fifo_rx). If this thread concurrently calls 
reinit_completion(&spmi->fifo_rx) for a new command before the IRQ handler 
calls complete(), the new command's wait_for_completion_timeout() will 
return immediately and likely fail with -EIO. Is there a need for 
synchronization between the hardware status check in the interrupt handler 
and reinit_completion() here?

[ ... ]

> +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);

[Severity: High]
Does this complete() need to be synchronized with the new command setup in 
spmi_raw_cmd()? As mentioned above, a delayed execution here might race with 
reinit_completion() and corrupt the state of a subsequent command.

> +		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]
Will this 64-bit readq() trigger a synchronous external abort (SError) on 
Apple Silicon? 

The IRQ mask and ACK registers appear to be initialized and accessed 
everywhere else as strictly 32-bit registers (e.g., using writel and readl). 
If the peripheral bus only supports 32-bit transactions, emitting a 64-bit 
ldr instruction here could cause a hardware exception and kernel panic.

> +		/**
> +		 * 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)];

[Severity: Low]
Is it safe to read spmi->irq_mask_cache locklessly here? 

The bitmap is modified using clear_bit() and set_bit() in 
apple_spmi_irq_mask() and apple_spmi_irq_unmask() while holding 
spmi->irq_mask_lock. Reading it without READ_ONCE or atomic bitops (like 
test_bit) creates a data race.

> +		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(0, spmi->regs + SPMI_IRQ_MASK_BASE + offset);
> +
> +	synchronize_irq(spmi->irq);
> +	irq_set_chained_handler_and_data(spmi->irq, NULL, NULL);

[Severity: High]
Does this safely synchronize the chained interrupt handler before the devres 
framework frees the device data? 

Since chained interrupt handlers bypass standard flow handlers and do not 
set the IRQD_IRQ_INPROGRESS flag, synchronize_irq() acts as a no-op and 
returns immediately. If apple_spmi_irq_handler() is executing concurrently 
on another CPU, it could continue running while devres frees the spmi 
structure and MMIO mappings, potentially leading to a use-after-free and 
kernel panic upon driver unbind.

> +}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828-t603x-spmi-v8-0-708d4d12e0fd@chaosmail.tech?part=6