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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] spmi: race condition in spmi_raw_cmd() error recovery causes premature completion
- [High] spmi: TOCTOU race on irq_mask_cache triggers spurious handle_bad_irq()
- [Medium] spmi: endianness corruption on Big-Endian architectures due to writel() for FIFO
--

commit 30246ba2a054ae67e63f97a3891036e8eeaf0cd4
Author: Alba Mendez <me@alba.sh>

spmi: apple: Add interrupt functionality

This commit adds support for interrupts sent by slave devices and uses the
IRQ for the RX FIFO if possible. This allows the driver to respond as soon
as the reply is available rather than polling with a 10ms sleep interval.

> 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
> @@ -26,6 +32,13 @@
[ ... ]
> +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);
> +	clear_bit(d->hwirq, spmi->irq_mask_cache);
> +	raw_spin_unlock_irqrestore(&spmi->irq_mask_lock, flags);
> +}
[ ... ]
>  static int spmi_raw_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid, u16 param,
>  			const u8 *buf_wr, size_t len_wr, u8 *buf_rd, size_t len_rd)
>  {
[ ... ]
>  	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]
Does this reinit_completion() risk a race condition in spmi_raw_cmd()?

If a previous command timed out and the chained IRQ handler is concurrently
executing on another CPU, it could read the hardware status before it is
acked, but call complete() after this reinit_completion() runs. This would
spuriously mark the next command's wait as done, causing it to fail with
-EIO prematurely.

>  
>  	writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);
>  
>  	while (i < len_wr) {
>  		j = min_t(size_t, sizeof(spmi_cmd), len_wr - i);
>  		spmi_cmd = 0;
>  		memcpy(&spmi_cmd, buf_wr + i, j);
>  		writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);

[Severity: Medium]
Since this driver can be built on 64-bit Big-Endian architectures via
COMPILE_TEST && 64BIT, could using writel() here cause endianness
corruption? The writel() accessor performs byte-swapping which reverses the
memory layout of the stream in 4-byte chunks. Should this use writesl()
exclusively for FIFO stream accesses instead?

>  		i += j;
>  	}
[ ... ]
>  	/* Read SPMI data reply */
>  	i = 0;
>  	while (i < len_rd) {
[ ... ]
>  		rsp = readl(spmi->regs + SPMI_RSP_REG);
>  		j = min_t(size_t, sizeof(spmi_cmd), len_rd - i);
>  		memcpy(buf_rd + i, &rsp, j);

[Severity: Medium]
Similar to the write path above, does using readl() for the FIFO stream read
cause unintended byte-swapping on Big-Endian systems?

>  		i += j;
>  	}
[ ... ]
> +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);
> +		/**
> +		 * 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);
> +}

[Severity: High]
Could this lockless read of irq_mask_cache cause a Time-of-Check to
Time-of-Use race?

If a peripheral interrupt is pending in the hardware status val, but another
thread concurrently calls disable_irq(), it invokes apple_spmi_irq_mask()
which clears the bit in irq_mask_cache.

If irq_mask_cache is read right after the bit is cleared, the pending bit
will be masked out. The handler would then leave the legitimately fired
interrupt unacked, set handled = false, and incorrectly fall through to
handle_bad_irq() on the parent interrupt controller.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001-t603x-spmi-v10-0-ff815c66c73f@chaosmail.tech?part=6