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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Missing hardware mask and synchronization in the driver unbind path causes an interrupt storm and Use-After-Free.
- [High] A time-of-check to time-of-use (TOCTOU) race condition exists between the timeout error recovery path and the interrupt handler, causing spurious failures.

Pre-existing issues:
- [High] The error recovery logic writes the flush command to the wrong hardware register, rendering the FIFO flush ineffective.
--

--- Patch [6]: [PATCH v4 6/6] spmi: apple: Add interrupt functionality ---
commit e606557ef28a4c4b211e2c324dc5cf458fc57741
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 c3bc1f6d57417..7e86d7983fb48 100644
> --- a/drivers/spmi/spmi-apple-controller.c
> +++ b/drivers/spmi/spmi-apple-controller.c

[ ... ]

> @@ -85,8 +166,10 @@ static int spmi_raw_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
>  
>  	if (spmi->prev_fail) {
>  		writel(SPMI_ACT_FIFO_FLUSH, spmi->regs + SPMI_RSP_REG);

[Severity: High]
This is a pre-existing issue, but is this writing the flush command to the
wrong hardware register? 

The code writes SPMI_ACT_FIFO_FLUSH to SPMI_RSP_REG instead of SPMI_ACT_REG.
If a command times out, wouldn't this fail to flush the RX FIFO and cause
subsequent commands to incorrectly read the delayed response?

> +		apple_spmi_irq_ack_raw(spmi, SPMI_IRQ_NOTIFY);
>  		spmi->prev_fail = false;
>  	}
> +	reinit_completion(&spmi->fifo_rx);

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

If a delayed response arrives just as we call reinit_completion() for the
next command:

apple_spmi_irq_handler() {
    ...
    if (val & BIT(SPMI_IRQ_NOTIFY % 32)) {
        apple_spmi_irq_ack_raw(spmi, SPMI_IRQ_NOTIFY);
        complete(&spmi->fifo_rx);
    ...
}

Could the interrupt handler call complete() concurrently, causing the new
command to be falsely signaled as complete and potentially reading corrupted
data from the previous timed-out command?

>  
>  	writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);

[ ... ]

> @@ -188,6 +271,164 @@ static int spmi_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid)

[ ... ]

> +static void remove_chained_handler(void *data)
> +{
> +	unsigned int irq = (unsigned int)(uintptr_t)data;
> +
> +	irq_set_chained_handler_and_data(irq, NULL, NULL);

[Severity: High]
Does this unbind path need to mask the hardware interrupt and synchronize
with in-flight handlers? 

When the driver is unbound, clearing the handler without masking
SPMI_IRQ_NOTIFY in the hardware or calling synchronize_irq() could leave
the interrupt firing concurrently. 

Could this cause an interrupt storm or a use-after-free when the handler
accesses unmapped device memory?

> +}

[ ... ]

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