Re: [PATCH 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] Using the 32-bit `BIT()` macro with 64-bit integers causes data truncation and undefined behavior on 32-bit architectures.
- [Low] Missing inclusion of `<linux/io-64-nonatomic-lo-hi.h>` causes build failures on 32-bit architectures under `COMPILE_TEST`.
--

--- Patch [7]: [PATCH 7/7] spmi: apple: interrupt controller functionality ---
commit d1caa99a04e657899090218f53dc529fd1b04548
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 d627187b3ac00..13b48fd66aec4 100644
> --- a/drivers/spmi/spmi-apple-controller.c
> +++ b/drivers/spmi/spmi-apple-controller.c

[ ... ]

> @@ -228,11 +234,123 @@ static void apple_spmi_irq_unmask_raw(struct apple_spmi *spmi, u32 irq)
>  	writel(readl(reg) | BIT(irq % 32), reg);
>  }
>  
> +static void apple_spmi_irq_ack(struct irq_data *d)
> +{
> +	struct apple_spmi *spmi = irq_data_get_irq_chip_data(d);
> +
> +	apple_spmi_irq_ack_raw(spmi, d->hwirq);
> +}
> +
> +static void apple_spmi_irq_mask(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_mask_raw(spmi, d->hwirq);
> +	spmi->irq_mask_cache[d->hwirq / 64] &= ~BIT(d->hwirq % 64);

[Severity: High]
Does using BIT() instead of BIT_ULL() here unintentionally clear the upper
32 bits of the 64-bit irq_mask_cache entry on 32-bit systems?

When d->hwirq % 64 is >= 32, this might also cause undefined behavior on
32-bit architectures, which can be reached via COMPILE_TEST.

> +	raw_spin_unlock_irqrestore(&spmi->irq_mask_lock, flags);
> +}
> +
> +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);
> +	spmi->irq_mask_cache[d->hwirq / 64] |= BIT(d->hwirq % 64);

[Severity: High]
Similarly, could this cause undefined behavior on 32-bit systems when
d->hwirq % 64 is >= 32?

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

[ ... ]

> @@ -241,6 +359,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);

[Severity: Low]
Does calling readq() here cause a build failure on 32-bit architectures
under COMPILE_TEST since <linux/io-64-nonatomic-lo-hi.h> is not included?

> +		/**
> +		 * 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)];
> +		while (val) {
> +			bit = __builtin_ctzll(val);
> +			generic_handle_domain_irq(spmi->irqd, offset * 8 + bit);
> +			handled = true;
> +			val &= ~BIT(bit);

[Severity: High]
Since val is a 64-bit integer, and bit can range from 0 to 63, would
~BIT(bit) on 32-bit systems produce a 32-bit inverted mask that zero-extends
when promoted to 64 bits? This might unintentionally clear the upper 32 bits
of val.

> +		}
> +	}
> +
>  	return handled ? IRQ_HANDLED : IRQ_NONE;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260725-t603x-spmi-v1-0-e1a29fcd2d38@chaosmail.tech?part=7