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

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Using a 64-bit readq() on 32-bit peripheral registers is unsafe and triggers SError panics on ARM64.
- [High] synchronize_irq() fails to synchronize chained interrupt handlers, risking use-after-free of unmapped device memory.
- [Medium] Race condition in the timeout recovery path causes a spurious complete(), instantly failing the next command.
- [Low] Plain lockless read of spmi->irq_mask_cache in the interrupt handler races with atomic bit modifications.
--

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

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

[Severity: Medium]
Is there a race condition between the error recovery path and the interrupt
handler here?

If a previous command times out, and the delayed interrupt fires concurrently,
the lockless interrupt handler might read the IRQ register before it is
cleared here, but call complete() just after reinit_completion() executes.

Could this prematurely complete the new command's wait and spuriously fail it
with -EIO?

>  
>  	writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);
[ ... ]
> @@ -188,6 +271,167 @@ static int spmi_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid)
[ ... ]
> +static void apple_spmi_irq_handler(struct irq_desc *desc)
> +{
[ ... ]
> +	for (offset = 0; offset < SPMI_NUM_PERIPHERAL_IRQS / 8; offset += sizeof(val)) {
> +		val = readq(spmi->regs + SPMI_IRQ_ACK_BASE + offset);

[Severity: High]
Does this 64-bit read on adjacent 32-bit peripheral registers trigger a
Synchronous External Abort (SError) on Apple Silicon?

Using readq() on 32-bit peripheral endpoints is typically unsupported on ARM64
and can lead to a kernel panic during normal hardware operation.

> +		/**
> +		 * 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]
Can this lockless read of the irq_mask_cache array race with atomic bit
modifications applied concurrently on another CPU?

Since set_bit() and clear_bit() are used to modify this array elsewhere,
should this use READ_ONCE() to prevent compiler tearing and KCSAN warnings?

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

[Severity: High]
Does synchronize_irq() successfully wait for chained handlers here?

Since chained handlers don't traverse the standard handle_irq_event path,
they do not set the IRQD_IRQ_INPROGRESS flag, meaning synchronize_irq()
evaluates to a no-op.

If the driver is unbound while an interrupt is actively being handled on
another CPU, could devres proceed to unmap the device memory while the
handler is still running, leading to a use-after-free?

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

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