Re: [PATCH v7 2/6] spmi: apple: Validate FIFO state

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Instantaneous check of RX_FIFO_EMPTY randomly aborts valid multi-word reads if the hardware streams data.

Pre-existing issues:
- [High] Missing controller-level lock allows concurrent operations to corrupt SPMI bus transactions and race on prev_fail.
--

--- Patch [2]: [PATCH v7 2/6] spmi: apple: Validate FIFO state ---
commit b638b8c461a5028b5eb344197adecba7ccd922ba
Author: Alba Mendez <me@alba.sh>

spmi: apple: Validate FIFO state

This commit adds checks to validate the FIFO state before reading reply data
and after a command completes. It introduces a prev_fail flag and flushes
the FIFO before executing new commands if previous operations failed.

> diff --git a/drivers/spmi/spmi-apple-controller.c b/drivers/spmi/spmi-apple-controller.c
> --- a/drivers/spmi/spmi-apple-controller.c
> +++ b/drivers/spmi/spmi-apple-controller.c

[ ... ]

> @@ -67,6 +71,11 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
>  	u8 i;
>  	int ret;
>  
> +	if (spmi->prev_fail) {

[Severity: High]
This isn't a bug introduced by this patch, but does this missing
controller-level lock allow concurrent operations to corrupt SPMI bus
transactions?

Since the SPMI subsystem exposes this controller to all its children and
does not natively serialize read_cmd or write_cmd operations, multiple
threads could invoke spmi_read_cmd() simultaneously. This would lead to
concurrent modifications of spmi->prev_fail and shared hardware registers
without mutual exclusion.

> +		writel(SPMI_ACT_FIFO_FLUSH, spmi->regs + SPMI_ACT_REG);
> +		spmi->prev_fail = false;
> +	}
> +
>  	writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);

[ ... ]

> @@ -78,6 +87,12 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
>  
>  	/* Read SPMI data reply */
>  	while (len_read < len) {
> +		if (readl(spmi->regs + SPMI_STATUS_REG) & SPMI_RX_FIFO_EMPTY) {
> +			spmi->prev_fail = true;
> +			dev_err_ratelimited(&ctrl->dev,
> +					    "FIFO lacks reply data, controller stuck?\n");
> +			return -EIO;
> +		}

[Severity: Medium]
Could this instantaneous check of SPMI_RX_FIFO_EMPTY spuriously abort valid
multi-word reads?

In a multi-word read transaction, the CPU might outrun the SPMI bus after
reading the first word. If the hardware streams data and hasn't buffered
the next word yet, the CPU will immediately see SPMI_RX_FIFO_EMPTY and
return -EIO instead of waiting for the next word to arrive.

>  		rsp = readl(spmi->regs + SPMI_RSP_REG);
>  		i = 0;

[ ... ]

> @@ -97,6 +117,11 @@ static int spmi_write_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
>  	size_t i = 0, j;
>  	int ret;
>  
> +	if (spmi->prev_fail) {

[Severity: High]
This is a pre-existing issue, but can concurrent calls to spmi_write_cmd()
race on spmi->prev_fail and hardware state?

Similar to spmi_read_cmd(), this modifies shared hardware state and
spmi->prev_fail without serialization.

> +		writel(SPMI_ACT_FIFO_FLUSH, spmi->regs + SPMI_ACT_REG);
> +		spmi->prev_fail = false;
> +	}
> +
>  	writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818-t603x-spmi-v7-0-dafebe6e7739@chaosmail.tech?part=2