Re: [PATCH v2 7/7] spmi: apple: interrupt controller functionality

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The software IRQ mask cache is updated after the hardware interrupt is unmasked, creating a race window where an interrupt can fire and be dropped.
- [Low] Lockless concurrent access to `irq_mask_cache` lacks `READ_ONCE()` and `WRITE_ONCE()` annotations, leading to a formal C data race.
--

--- Patch [7]: [PATCH v2 7/7] spmi: apple: interrupt controller functionality ---
commit 2c670245829f06c9db4ee3ae30ca2eacf7d1e022
Author: Alba Mendez <me@alba.sh>

spmi: apple: interrupt controller functionality

Add support for interrupts sent by slave devices.

> diff --git a/drivers/spmi/spmi-apple-controller.c b/drivers/spmi/spmi-apple-controller.c
> index 109c5d2c735d4..8f6a0d560da82 100644
> --- a/drivers/spmi/spmi-apple-controller.c
> +++ b/drivers/spmi/spmi-apple-controller.c

[ ... ]

> +static void apple_spmi_irq_unmask(struct irq_data *d)
> +{
> +	struct apple_spmi *spmi = irq_data_get_irq_chip_data(d);
> +	unsigned long flags;
> +
> +	raw_spin_lock_irqsave(&spmi->irq_mask_lock, flags);
> +	apple_spmi_irq_unmask_raw(spmi, d->hwirq);

[Severity: High]
Is there a race window here where a pending interrupt could be lost?

Since the hardware interrupt is unmasked via apple_spmi_irq_unmask_raw()
before the software irq_mask_cache is updated, an interrupt could fire
immediately. If the parent interrupt controller routes it to a CPU and
apple_spmi_irq_handler() executes, it will read the outdated cache mask
(observing a 0 for this IRQ).

The handler would then clear the interrupt from its pending mask and
return without acknowledging the hardware, potentially causing the
interrupt to be permanently lost (if edge-triggered) or resulting in an
interrupt storm (if level-triggered).

Should the irq_mask_cache be updated before the hardware is unmasked?

> +	spmi->irq_mask_cache[d->hwirq / 64] |= BIT_ULL(d->hwirq % 64);

[Severity: Low]
Since irq_mask_cache is read locklessly in apple_spmi_irq_handler(),
should this update use WRITE_ONCE() to prevent potential compiler store
tearing?

> +	raw_spin_unlock_irqrestore(&spmi->irq_mask_lock, flags);
> +}

[ ... ]

> +static irqreturn_t apple_spmi_irq_handler(int irq, void *dev_id)
>  {
>  	struct apple_spmi *spmi = dev_id;
>  	bool handled = false;
> -	u32 val;
> +	u64 val, offset, bit;
>  
>  	val = readl(spmi->regs + SPMI_IRQ_ACK_BASE + SPMI_IRQ_USER_SIZE);
>  	if (val & BIT(SPMI_IRQ_FIFO_RX)) {
> @@ -239,6 +357,23 @@ static irqreturn_t apple_spmi_irq_handler(int irq, void *dev_id)
>  		handled = true;
>  	}
>  
> +	for (offset = 0; offset < SPMI_IRQ_USER_SIZE; offset += sizeof(val)) {
> +		val = readq(spmi->regs + SPMI_IRQ_ACK_BASE + offset);
> +		/**
> +		 * 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]
Since this lockless read happens concurrently with the locked updates in
apple_spmi_irq_mask() and apple_spmi_irq_unmask(), should this use
READ_ONCE() to ensure the compiler doesn't cache the value in a register
or tear the load?

> +		while (val) {
> +			bit = __builtin_ctzll(val);
> +			generic_handle_domain_irq(spmi->irqd, offset * 8 + bit);
> +			handled = true;
> +			val &= ~BIT(bit);
> +		}
> +	}
> +
>  	return handled ? IRQ_HANDLED : IRQ_NONE;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728-t603x-spmi-v2-0-f43e5f10e583@chaosmail.tech?part=7