Re: [PATCH v2 6/7] spmi: apple: use IRQ for RX FIFO if possible

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Data race between `reinit_completion()` and `complete()` due to unhandled hardware timeouts, causing completion state corruption.
- [Low] The static function `apple_spmi_irq_mask_raw` is defined but never used, leading to `-Wunused-function` warnings.
--

--- Patch [6]: [PATCH v2 6/7] spmi: apple: use IRQ for RX FIFO if possible ---
commit fa2cb405f77a7400cdbd89375c67fa8b1bc2c296
Author: Alba Mendez <me@alba.sh>

spmi: apple: use IRQ for RX FIFO if possible

This commit modifies the Apple SPMI driver to use an IRQ for the RX FIFO 
when available, instead of relying on a 10ms sleep interval for polling. 
This reduces the latency to process replies to a few microseconds.

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

[ ... ]

> @@ -77,6 +99,8 @@ static int spmi_raw_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
>  
>  	mutex_lock(&spmi->fifo_lock);
>  
> +	reinit_completion(&spmi->fifo_rx);
> +

[Severity: High]
Does this completion handle late interrupts correctly if a previous command
timed out? 

If wait_for_completion_timeout() times out in apple_spmi_wait_rx_not_empty(),
an interrupt could still arrive later. If a delayed interrupt fires while a
subsequent transaction is calling reinit_completion(), could the late
apple_spmi_irq_handler() call complete() and cause the new transaction to
finish prematurely with incorrect data?

>  	writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);
>  
>  	while (i < len) {

[ ... ]

> @@ -181,10 +205,68 @@ static int spmi_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid)
>  	return -EINVAL;
>  }
>  
> +static void apple_spmi_irq_ack_raw(struct apple_spmi *spmi, u32 irq)
> +{
> +	u32 __iomem *reg = spmi->regs + SPMI_IRQ_ACK_BASE + (irq / 32) * 4;
> +
> +	writel(BIT(irq % 32), reg);
> +}
> +
> +static void apple_spmi_irq_mask_raw(struct apple_spmi *spmi, u32 irq)
> +{
> +	u32 __iomem *reg = spmi->regs + SPMI_IRQ_MASK_BASE + (irq / 32) * 4;
> +
> +	writel(readl(reg) & ~BIT(irq % 32), reg);
> +}

[Severity: Low]
Is this function used anywhere in this patch? It looks like it is defined as
a static function but never called, which might trigger a compiler warning
when building with CONFIG_WERROR.

> +
> +static void apple_spmi_irq_unmask_raw(struct apple_spmi *spmi, u32 irq)
> +{
> +	u32 __iomem *reg = spmi->regs + SPMI_IRQ_MASK_BASE + (irq / 32) * 4;
> +
> +	writel(readl(reg) | BIT(irq % 32), reg);
> +}

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