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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] spmi: apple: dispose of IRQ mappings before domain removal on
  teardown
- [Medium] spmi: apple: race between spmi_raw_cmd() recovery and
  apple_spmi_irq_handler()
--

commit 6198d2cd56367a5447522a621c1df79777ba3ad6
Author: Alba Mendez <me@alba.sh>

spmi: apple: Add interrupt functionality

This commit adds an IRQ domain for SPMI peripheral interrupts and uses the
controller's RX FIFO notification IRQ instead of polling when an interrupt
is available.

> 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);
>  
>  	writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);

[Severity: Medium]
Can a late SPMI_IRQ_NOTIFY interrupt from a timed-out transaction race with
this recovery path and complete spmi->fifo_rx prematurely?

SPMI_IRQ_NOTIFY remains unmasked when wait_for_completion_timeout() times
out in apple_spmi_wait_rx_not_empty() and sets spmi->prev_fail.  When the
next command enters spmi_raw_cmd():

CPU0                              CPU1 (<hardirq>)
spmi_raw_cmd()
  if (spmi->prev_fail)
    writel(SPMI_ACT_FIFO_FLUSH, ...)
                                  apple_spmi_irq_handler()
                                    val = readl(... + SPMI_IRQ_ACK_BASE + ...)
    apple_spmi_irq_ack_raw(...)
  reinit_completion(&spmi->fifo_rx)
                                    complete(&spmi->fifo_rx)
  writel(spmi_cmd, ...)
  apple_spmi_wait_rx_not_empty()
    wait_for_completion_timeout() <-- returns immediately

The new command then wakes before its reply arrives, sees SPMI_RX_FIFO_EMPTY
(or reads a stale reply if the hardware answered just after the flush), and
fails with -EIO.

[ ... ]

> +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);
> +}
> +
> +static int apple_spmi_init_irq(struct platform_device *pdev,
> +			       struct apple_spmi *spmi, int irq)
> +{
> +	int ret;
> +	struct irq_domain_info info = {
> +		.fwnode		= pdev->dev.fwnode,
> +		.hwirq_max	= ~0U,
> +		.ops		= &apple_spmi_irq_domain_ops,
> +		.host_data	= spmi,
> +	};
> +
> +	raw_spin_lock_init(&spmi->irq_mask_lock);
> +
> +	for (size_t offset = 0; offset < SPMI_NUM_IRQS / 8; offset += 4) {
> +		writel(0, spmi->regs + SPMI_IRQ_MASK_BASE + offset);
> +		writel(U32_MAX, spmi->regs + SPMI_IRQ_ACK_BASE + offset);
> +	}
> +
> +	spmi->irqd = devm_irq_domain_instantiate(&pdev->dev, &info);
> +	if (IS_ERR(spmi->irqd))
> +		return PTR_ERR(spmi->irqd);
> +
> +	ret = devm_add_action(&pdev->dev, apple_spmi_teardown_irq, spmi);

[Severity: Medium]
Does apple_spmi_teardown_irq() leave mapped peripheral IRQs in spmi->irqd
when the controller is unbound?

Unregistering child SPMI devices frees their irqactions, but does not call
irq_dispose_mapping().  When devm_irq_domain_remove() runs right after
apple_spmi_teardown_irq(), spmi->irqd->revmap_tree still holds any mapped
hwirqs, which triggers WARN_ON(!radix_tree_empty(&domain->revmap_tree)) in
irq_domain_remove(), leaks the allocated virq descriptors, and leaves their
irq_data pointing to the freed spmi->irqd and spmi structures.

Disposing of existing mappings with irq_dispose_mapping(irq_find_mapping())
across 0..SPMI_NUM_PERIPHERAL_IRQS - 1 in apple_spmi_teardown_irq() (and
setting info.size and info.hwirq_max to SPMI_NUM_PERIPHERAL_IRQS) would
avoid this.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107-t603x-spmi-v11-0-322490215f58@chaosmail.tech?part=6